[ETCM-911] new type transaction envelop and [ETCM-921] new transaction type 1 by dzajkowski · Pull Request #1061 · input-output-hk/mantis

dzajkowski · 2021-07-13T14:19:31Z

Make SignedTransaction wrap Transaction to allow TypedTransactions.

Included in this branch are:

ETCM-911: new Typed Transaction envelop

ETCM-921: new Type 1 transaction (payload + rlp encoder/decoder)

first throw at a set of Signers to implement the various rulesets (homestead, EIP-155 and EIP-2930)

Follow up tickets are expected:

ETCM-1095: receipt implementation

ETCM-1096: complete the Signers, and plug them properly into Mantis

This PR has been split into #1094 and #1095, and is no longer relevant

AnastasiiaL · 2021-07-15T17:15:48Z

    value: BigInt,
    payload: ByteString,
-    accessList: List[AccessListItem]
+    accessList: Option[List[AccessListItem]]


why introducing an option here? is it for the field not being encoded if the list is empty?

the specification says that this field should be optional. IOW if one reads the specification literally it should be optional. alas it's not the case in geth, hence I reverted it.

AurelienRichez · 2021-07-20T08:56:21Z

+
  implicit val signedTransactionPickler: Pickler[SignedTransaction] =
-    transformPickler[SignedTransaction, (LegacyTransaction, ECDSASignature)] { case (tx, signature) =>
+    transformPickler[SignedTransaction, (Transaction, ECDSASignature)] { case (tx, signature) =>


note: Won't we have to nuke the storage because the block body are now incompatible ?

yep, this will indeed create an incompatibility. I think we can also re-introduce the chainId change and release then.
as long as we can stagger the release to sagano - we will not loose data, but this will require some devops-ing.

… behavior

AurelienRichez

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.

AurelienRichez · 2021-08-11T06:53:49Z

    val stx = SignedTransaction.apply(
      tx = tx,
-      signature = sig
+      // hacky change to make the test succeed without regressing the general workflow.


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 v 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.

AurelienRichez · 2021-08-11T07:22:26Z

+  override def signatureFromBytes(bytes65: ByteString): Option[ECDSASignature] = ???
+
+  override def payloadToCheck(signedTransaction: SignedTransaction): Either[SignerError, Array[Byte]] =
+    // actually, payload to check is not required to include transaction type


I understand the "SHOULD" as : "some transaction types in the future might define a signature mechanism which does not include the type in the data", not as "the sender has a choice on how he can sign the transaction".

As EIP-2930 for transaction type 1 clearly states "Signature signs over the transaction type as well as the transaction data", I think there is no ambiguity there.

Effectively, I had misunderstood the EIP-2718 part where it's specificed that the signature SHOULD but MUST NOT sign over the transaction part.
On second read, it just let the possibillity for each transaction type specification to choose its signature content.

Thanks for pointing this.

leo-bogastry · 2021-08-11T08:33:41Z

+        signedTx.tx match {
+          case TransactionWithAccessList(nonce, gasPrice, gasLimit, _, value, payload, accessList) =>
+            RLPList(
+              chainId, // TODO improve how chainid is preserved in transactions


Just a node about the TODOs. I see a few. Are these related with the follow-up tickets already? We already have a lot of TODO in the source code that are quite old, without a proper ticket these TODO's are never followed-up

yes, those TODO have be put by Dom to let me know where to do stuff.
The chainId related ones will be handled by the ticket relative to the signing process.

strauss-m · 2021-08-11T08:37:06Z

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 agnostic
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)

strauss-m · 2021-08-24T08:07:42Z

This PR has been superseeded by #1094 and #1095

dzajkowski force-pushed the feature/etcm-921-typed-envelope-in-body branch 2 times, most recently from 62ceb0e to d99d9ee Compare July 14, 2021 11:26

lukasz-golebiewski approved these changes Jul 15, 2021

View reviewed changes

AnastasiiaL reviewed Jul 15, 2021

View reviewed changes

AurelienRichez reviewed Jul 20, 2021

View reviewed changes

dzajkowski force-pushed the feature/etcm-921-typed-envelope-in-body branch from d99d9ee to b71b2f6 Compare July 20, 2021 13:17

dzajkowski marked this pull request as draft July 20, 2021 13:23

dzajkowski force-pushed the feature/etcm-921-typed-envelope-in-body branch from b71b2f6 to ed72c40 Compare July 20, 2021 13:23

AurelienRichez approved these changes Jul 20, 2021

View reviewed changes

dzajkowski added 4 commits August 3, 2021 19:07

[ETCM-911] Replace LegacyTX with trait Transaction in SignedTransaction

8e38403

[ETCM-911] Drop unused 'chainId' parameter from constructor

9e8b68b

[ETCM-911] Add rlp serialization basics

67b4cf4

[ETCM-911] Add rlp tests showing compatibility with geth

e46e5a0

strauss-m force-pushed the feature/etcm-921-typed-envelope-in-body branch 2 times, most recently from 380bb1a to ac0212a Compare August 6, 2021 19:12

strauss-m added 2 commits August 10, 2021 15:20

[ETCM-921] rlp access list encoding + unit tests

236ea58

[ETCM-921] introduce Signer hierarchy to handle fork specific signing…

6aa6916

… behavior

strauss-m force-pushed the feature/etcm-921-typed-envelope-in-body branch from ac0212a to 6aa6916 Compare August 10, 2021 13:20

strauss-m added 2 commits August 10, 2021 15:26

Merge branch 'develop' into feature/etcm-921-typed-envelope-in-body

7682018

[ETCM-921] fix formatting

9cbeadc

strauss-m changed the title ~~[ETCM-911] Replace LegacyTX with trait Transaction in SignedTransaction~~ [ETCM-911] new type transaction envelop Aug 10, 2021

strauss-m changed the title ~~[ETCM-911] new type transaction envelop~~ [ETCM-911] new type transaction envelop and [ETCM-921] new transaction type 1 Aug 10, 2021

strauss-m marked this pull request as ready for review August 10, 2021 16:49

AurelienRichez reviewed Aug 11, 2021

View reviewed changes

leo-bogastry reviewed Aug 11, 2021

View reviewed changes

lukasz-golebiewski self-requested a review August 11, 2021 11:15

This was referenced Aug 12, 2021

[ETCM-921] transaction with access list #1094

Merged

[ETCM-1096] eip2930 transaction signature #1095

Merged

strauss-m marked this pull request as draft August 20, 2021 08:11

strauss-m closed this Aug 24, 2021

Conversation

dzajkowski commented Jul 13, 2021 • edited by strauss-m Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AurelienRichez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strauss-m Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strauss-m commented Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strauss-m commented Aug 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dzajkowski commented Jul 13, 2021 •

edited by strauss-m

Loading

strauss-m Aug 11, 2021 •

edited

Loading

strauss-m commented Aug 11, 2021 •

edited

Loading