[ETCM-1096] eip2930 transaction signature

[strauss-m]

Description

Transaction with optional access list (added by EIP 2930) requires a specific rule for signing / validation.
There is currently no framework to handle different rulesets for such a purpose.

This PR is split from #1061

Proposed Solution

Implement the specific ruleset for new transaction type 1

This may need a signature process refactoring. (Signature refactoring has been parked into the branch https://github.com/input-output-hk/mantis/tree/feature/eip2930%2Fsigner_hierarchy)

[dzajkowski]

can be sealed

[strauss-m]

Taken from the splitted PR.

AurelienRichez 9 days ago Member

Is there a ticket for this ? If yes, it would be good to mention it I think.

at the time of coding, there wasn't , but the signer stuff will remove the need for the hack, since there should be a method to import the signature according to a given ruleset.

This behavior is also caused by the fact that, internally, Mantis uses the concept of ECDSASignature for two things:

the actual crypto signature, which is unambiguous, and independant of ethereum/mantis
the field r, s and v in the signed transaction (a signed transaction is the union of a Transaction and a ECDSASignature).

I'm wondering if we shouldn't introduce a EthererumEncodedSignature to properly differentiate between the two, since the field value is converted between the crypto signature (possible value 0/1) and the rlp encoding (for exemple, before eip-155, it's 27/28, but after eip-155, it's 0/1 + chainId * 2 + 35, and for transaction type 1, it's back to 0/1)

Another possibility would be to only keep the unmodified ECDSASignature and doing the computation on the fly when rlp encoding/decoding. However it could possibly lead to the introduction of additional fields (encoding version, chainId, etc) , which would be dependant on the signature ruleset.

Could you explain how the Signer will be used ?
Particularly, why is payload to check different than payload to sign ? I mean that we cannot check what was not signed anyway so I don't see why they can be different in practice.

The Signer will provide implementation for the different rulesets (homestead, eip-155, eip-2930).

Main differences from each of them are:

• which transaction's fields to use when building the signature / checking the signature
• how to encode the signature V field when doing the rlp encoding (but actually it's more a side effect of the fact that we pre-build the v field when computing the signature instead of when doing the rlp encoding / decoding)
• how to import a binary encoded signature (likewise, due to the fact that the signed transaction is including a pre-computed signature)

The main difference between payloadToSign an payloadToCheck is that it's possible to have a choice in the way a client can encode a payload. It's currently only the case for EIP-155, where a transaction can be chainId-protected or not.
When signing, the client can arbitrarely decide which method to use, but when checking, the client has to find out which one was used by the emitter.

In our case, the EIP155Signer payloadToSign is alwasy chainId-protecting the transaction, but the payloadToCheck has to guess if the emitter has protected or not the transaction. In this particular case, they can be different. For all the other cases, they are not.

Initially, I had a default implementation payloadToCheck = payloadToSign, but it was not working properly when delegating to previous signer, since the call hierarchy would become:
SignerB.payloadToCheck -> SignerB.payloadToSign -> SignerA.payloadToSign
instead of:
SignerB.payloadToCheck -> SignerA.payloadToCheck

But I'm open to proposal to reduce the footprint of the signer.

Likewise, I'm contemplating an implementation where we just change the meaning of ECDSASignature, and we restrict it to the pure crypto values.

Potential benefits are:

• v field is fork agnosti
• no more need for the importFromByte and the getRawSignature
• no ambiguity when reading the code (are we using the rlp v value or the crypto v value ?)
• cleanup some crypto part when checking the signature (crypto shouldn't be aware of the chainId content)

Potential drawbacks (TBD):

• we loss the knowledge of which convention was used by the emitters for received transaction
• we may need additional fields to keep this context into the transaction.

The signers should then be plugged in the Mantis client. It add a dependency to two things: the block number and the blockchain config, to know which rulesetto use. The blockchain config could be made available thanks to the implicit pattern already used, but the block number would need some refactor.

When receiving a broadcast signed transaction, it seems ok to add this dependency, since this transaction is inserted into a block (but if the block is not emitted, Mantis will have to recheck for every pending transactions if the Signer has changed).

When receiving a complet block, there is not ambiguity, the block number is known.

Additionally, it may be possible to keep the Signer in the SignedTransaction. It would notably help with the verification part, since the primitive SignedTransaction.getSender would not depend anymore on an external block number.

So one possiblity (TBD) could be going from:
SignedTransaction(Transaction, context sensitive ECDSASignature)
to
SignedTransaction(Transaction, raw ECDSASignature, Signer)

[AurelienRichez]

I made a small unit tests suggestion but other than that OK

[AurelienRichez]

We should probably add a test to

• make the round trip and check that the serialization is the same with the padded zeros.
• check that we get the right sender from a given transaction (because it was wrong when we forgot to add the padded zeros)

[lukasz-golebiewski]

Transaction is only extended by LegacyTransaction and TransactionWithAccessList so can this case ever occur?

[strauss-m]

With the current code base, it shouldn't.
New transaction types are expected to be introduced on a regular basis, so this will change.
Currently, Transaction is a sealed trait, so by removing the case _ match, any new transaction not explicitely handled should be caught by the compiler, but since warnings are not generating errors, this won't be visible, until the running client raises a match exception.
The IllegalArgumentException somehow tries to provided a better error.