Simplify transaction's constructor and EIP155 handling #153

alcuadrado · 2019-05-17T18:06:02Z

This PR implements the changes discussed in the TS migration PR and in fixes #149.

A summary of the changes:

chainId can't be provided in the data object.
Only opts.common, or opts.chain and opts.hardfork can be used.
The default hardfork is now 'petersburg'.
The chain id is always obtained from the internal common object now. It isn't computed in the constructor.
The constructor throws if the v value is present, indicates that EIP155 was enabled, and the chain id it indicates doesn't match the one of the internal common object.
No default v is set. If a transaction isn't signed, it would be an empty buffer.
Transactions are always signed with EIP155 after Spurios Dragon.
If v is changed after construction, ~~and it doesn't match the one of the internal common object, we throw a clear message in tx.verifySignature() instead of just letting it return false.~~ its value is validated in its setter.

I think (7) and (8) require some extra comments:

(7) This is different from the previous implementation, where by default EIP155 wasn't used. We'd need an extra param to implement that. That would also make the logic to decide when to use EIP155 would be more complex, and it's already fairly complex. IMO it's better to use EIP155 by default, giving the users replay-protection by default. This EIP has been active for a long time now.

(8) ~~Maybe this should be done in the v setter. What do you think? It would require a call to Object.defineProperty in the constructor, as anything in the prototype is shadowed by defineProperties.~~ Update: See next comment

alcuadrado · 2019-05-18T15:43:38Z

(8) Maybe this should be done in the v setter. What do you think? It would require a call to Object.defineProperty in the constructor, as anything in the prototype is shadowed by defineProperties.

I just updated this PR with an implementation of this logic. I think it's better to validate v in its setter than throwing a possible unexpected error in a boolean function.

To implement this I had to change the hash method because it was temporarily modifying the transaction with invalid values. Now it's a pure function.

In doing the modification to hash, I noticed that all the fields have allowLess: true except v. I'm not sure where to check if this is correct or just an error, but I'm pretty sure it's wrong. Also, as we have already discussed in #151, this is RLP-related logic, which IMO shouldn't be part of the internal representation of these objects.

alcuadrado · 2019-05-18T19:57:44Z

Update: I fixed a bug when signing already signed txs. If it was previously signed without EIP155, tx.hash(false) would not generate an EIP155 hash, but sign would assume it did.

holgerd77 · 2019-05-19T12:42:31Z

Thanks for the super solid work and the great summary, I would generally go along with all changes proposed/implemented, just as some first estimation. 😄

holgerd77

One change request on a comment, another question on v setting, otherwise to be continued...

src/transaction.ts

alcuadrado · 2019-05-19T15:36:26Z

Update: I fixed a bug when signing already signed txs. If it was previously signed without EIP155, tx.hash(false) would not generate an EIP155 hash, but sign would assume it did.

Woke up thinking that this change isn't right. Signing signed and unsigned transactions should have the same logic to decide whether to use EIP155 or not, but _implementsEIP155 works differently for signed and unsigned transactions.

I see two ways of fixing this:

Split the hash method in three: one to get the tx id, one to get the message to sign, and finally one to get the message to verify the signature.
Remove any previous signature before signing.

I'll make a commit with the second alternative, as the first one needs further discussion, and can be addressed as part of #151.

UPDATE: Done.
UPDATE2: I implemented (1) in #154 to get an idea of how it would look.

alcuadrado · 2019-05-19T15:58:41Z

I'll make a commit with the second alternative, as the first one needs further discussion, and can be addressed as part of #151.

While testing this I realized that toJSON wasn't included in the TS migration, so I added it back.

I didn't reimplement its behavior, as defineProperties will overwrite it anyway. I'm not happy with that though, but duplicating its logic didn't seem right either. I think I just don't like defineProperties 😅

holgerd77 · 2019-05-20T09:13:45Z

Signing signed and unsigned transactions should have the same logic to decide whether to use EIP155 or not

Would agree here, taking 2. should be a valid fix for now.

toJSON wasn't included in the TS migration, so I added it back

Ok.

holgerd77

@alcuadrado Ok, I think I can give this a go now, thanks for the great work! 😄

Please nevertheless answer the one outstanding question.

holgerd77 · 2019-05-20T09:26:18Z

test/api.ts

    st.equal(
      pt.serialize().toString('hex'),
-      'ec098504a817c800825208943535353535353535353535353535353535353535880de0b6b3a764000080018080',
+      'ec098504a817c800825208943535353535353535353535353535353535353535880de0b6b3a764000080808080',


I likely don't have the complete picture yet, but I am not getting atm why the serialization now differs from the example from the EIP page linked above in the comment.

Ah, is this the RLP empty buffer case?

Yes, it's because of that.

Note that what the EIP calls "signing data" isn't the output of serialize(), it's the preimage of the hash returned by hash(false). We don't have a getter for such value in this class.

I added an explanation about this in the test

holgerd77 · 2019-05-21T08:23:28Z

test/fake.ts

@@ -137,7 +136,7 @@ tape('[FakeTransaction]: Basic functions', function(t) {
  })

  t.test('should sign', st => {
-    const tx = new FakeTransaction(txData)
+    const tx = new FakeTransaction(txData, { hardfork: 'tangerineWhistle' })
    tx.sign(Buffer.from('164122e5d39e9814ca723a749253663bafb07f6af91704d9754c361eb315f0c1', 'hex'))
    st.plan(3)
    st.equal(


New test cases sufficiently cover the changed EIP-155 and v value setting behavior, good.

holgerd77 · 2019-05-21T08:26:45Z

src/transaction.ts

      this._senderPubKey = ecrecover(
        msgHash,
        v,
        this.r,
        this.s,
-        useChainIdWhileRecoveringPubKey ? this._chainId : undefined,
+        useChainIdWhileRecoveringPubKey ? this.getChainId() : undefined,


All verifySignature() changes unproblematic, ok.

holgerd77 · 2019-05-21T08:33:25Z

src/transaction.ts

-      const unsigned = !this.r.length && !this.s.length
-      const seeksReplayProtection = this._chainId > 0
-
-      if ((unsigned && seeksReplayProtection) || (!unsigned && meetsAllEIP155Conditions)) {


This is the heaviest code logic change in the PR and I get that this upper part has been moved into this._implementsEIP155() but I am not completely sure how to read this conditional simplification.

Could you give a short reasoning for the two cases in the if clause? That has likely already been done implicitly in your other explanations on the overall changes, but just want to make sure we are not missing anything here.

Won't make this a blocker request though.

Actually, they aren't completely equivalent. Not for unsigned transactions.

The decision to use EIP155 or not when signing was previously based on the _chainId field, and its complex initialization logic in the constructor. It's simpler now: if the hardfork is spurious dragon or later, we use EIP155.

If a transaction is already signed and we are validating it, the logic is the same. It should be in spurious dragon or later and have an EIP155-encoded chain id as v to enable EIP155 validation.

holgerd77 · 2019-05-21T08:34:26Z

src/transaction.ts

    }
+
    Object.assign(this, sig)
  }


Changes in sign() are ok.

holgerd77 · 2019-05-21T08:41:35Z

@s1na @alcuadrado After this has been merged (@alcuadrado feel free to merge) I think we are in a good working state on the library. I would very much tend to do now do a major release on this, I think the changes already done are heavy enough and further refactoring/simplification work started and discussed by you guys is still a bit far off respectively shouldn't be overrushed and we should take our time here and do a subsequent likely major release on this once ready.

Are you going along with this?

alcuadrado · 2019-05-21T15:16:06Z

@s1na @alcuadrado After this has been merged (@alcuadrado feel free to merge) I think we are in a good working state on the library. I would very much tend to do now do a major release on this, I think the changes already done are heavy enough and further refactoring/simplification work started and discussed by you guys is still a bit far off respectively shouldn't be overrushed and we should take our time here and do a subsequent likely major release on this once ready.

Are you going along with this?

Completely agree. I created #154 as a place to explore different ideas, not to be merged and released now.

holgerd77

Looks good, thanks for the explanations.

alcuadrado added 2 commits May 17, 2019 14:50

Simplify EIP155 handling.

37d43fc

Update tests

d0c7375

alcuadrado mentioned this pull request May 17, 2019

EIP-155 support/no support use case questions #152

Closed

Validate v in its setter

48e4f57

alcuadrado added the PR state: needs review label May 18, 2019

Fix bug when signing an already signed tx

1159315

holgerd77 suggested changes May 19, 2019

View reviewed changes

src/transaction.ts Show resolved Hide resolved

src/transaction.ts Show resolved Hide resolved

src/transaction.ts Show resolved Hide resolved

src/transaction.ts Show resolved Hide resolved

src/transaction.ts Show resolved Hide resolved

Remove outdated comment

27b8876

alcuadrado added 4 commits May 19, 2019 12:54

Restore the Transaction's toJSON method

7f08267

Remove any previous signature before signing again

1698f75

Update fake's tests

12f5de3

Add a tests for the new behaviour

ed4b695

holgerd77 previously approved these changes May 21, 2019

View reviewed changes

alcuadrado mentioned this pull request May 21, 2019

A possible simplification of this library #154

Closed

Add an explanation to an EIP155 test.

bba15cb

alcuadrado dismissed holgerd77’s stale review via bba15cb May 21, 2019 15:43

holgerd77 approved these changes May 21, 2019

View reviewed changes

alcuadrado merged commit b564c15 into master May 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify transaction's constructor and EIP155 handling #153

Simplify transaction's constructor and EIP155 handling #153

alcuadrado commented May 17, 2019 •

edited

Loading

alcuadrado commented May 18, 2019 •

edited

Loading

alcuadrado commented May 18, 2019

holgerd77 commented May 19, 2019

holgerd77 left a comment

alcuadrado commented May 19, 2019 •

edited

Loading

alcuadrado commented May 19, 2019

holgerd77 commented May 20, 2019

holgerd77 left a comment

holgerd77 May 20, 2019

holgerd77 May 20, 2019

alcuadrado May 21, 2019

alcuadrado May 21, 2019

holgerd77 May 21, 2019

holgerd77 May 21, 2019

holgerd77 May 21, 2019

alcuadrado May 21, 2019

holgerd77 May 21, 2019

holgerd77 commented May 21, 2019

alcuadrado commented May 21, 2019

holgerd77 left a comment

Simplify transaction's constructor and EIP155 handling #153

Simplify transaction's constructor and EIP155 handling #153

Conversation

alcuadrado commented May 17, 2019 • edited Loading

alcuadrado commented May 18, 2019 • edited Loading

alcuadrado commented May 18, 2019

holgerd77 commented May 19, 2019

holgerd77 left a comment

Choose a reason for hiding this comment

alcuadrado commented May 19, 2019 • edited Loading

alcuadrado commented May 19, 2019

holgerd77 commented May 20, 2019

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented May 21, 2019

alcuadrado commented May 21, 2019

holgerd77 left a comment

Choose a reason for hiding this comment

alcuadrado commented May 17, 2019 •

edited

Loading

alcuadrado commented May 18, 2019 •

edited

Loading

alcuadrado commented May 19, 2019 •

edited

Loading