Update official transaction tests #131

danjm · 2018-12-04T17:31:25Z

Opening this PR instead of #126 as this branch is in the main repo.

fixes #115

Updates the transactionRunner.js tests as per the changes and instructions described here: ethereum/tests#373

Each test is run for each of the hardforks. If the tx is defined but has an invalid signature after creation, the test passes if the test data for the given hardfork is empty. If the tx is undefined after creation, the test passes if the test data for the given hardfork is empty. If the tx has a sender and hash that matches that in the test data for the given hard fork, the test passes.

This PR also adds the ability to run a single test by passing an exact filename match. For example:
npm run test:node -- -t --file=TransactionWithHighGas | tap-spec

danjm · 2018-12-04T17:33:15Z

@holgerd77 This might be ready to go, depending on whether we want to resolve the following:

This implemenation of transactionRunner.js will flag the transactions this._homestead property as false when running the hardfork tests other than "Homestead". However, there is some behaviour of ethereumjs-tx that should hold true for Homestead and all future hard forks.

For instance, that All transaction signatures whose s-value is greater than secp256k1n/2 are considered invalid has been true since Homestead. In our codebase, this condition only invalidates a tx if this._homestead is true. As such, tests for other hardforks that expect invalidity due to violation of this condition will not currently pass.

To see an example of this, run npm run test:node -- -t --file=TransactionWithHighGas | tap-spec

this._homestead defaults to true. Is it assumed that it will only be set to false if ethereumjs-tx is being used prior to the Homestead hardfork?

If not, we need to explicitly handle these other forks in some way.

holgerd77 · 2018-12-04T19:27:09Z

Hi Dan, thanks for the update! This whole this._homestead thing is totally legacy and should be replaced. Can you cross-coordinate with this #130 PR currently in progress? I think this would be a perfect match. We should optimally make sure that all hardfork specific logic is supported, or minimally then explicitly limit (using the common library) to that set of hardforks that ARE supported.

Would probably be optimal to have this PR merged and then work here on top of the hardfork and chain integration work.

danjm · 2019-01-04T15:15:33Z

I put "blocked" in the title as the latest commit makes this PR dependent on #130

Once that is merged, I will rebase here

…eumjs-testing

… to be undefined.

…eumjs-common (PR #130)

…correctly.

… common and util

danjm · 2019-03-01T07:13:09Z

@holgerd77 This is ready for final review and merge. I'll confirm tomorrow, but the latest changes likely make #132 redundant.

coveralls · 2019-03-01T07:32:25Z

Coverage increased (+1.2%) to 98.936% when pulling 4d75731 on update-official-transaction-tests into 471a13e on master.

holgerd77 · 2019-03-01T16:37:30Z

Great! 👍

holgerd77

We should have a closer look at this chainId redundancy, one minor formatting suggestions, otherwise looks good! Thanks Dan! 😄

holgerd77 · 2019-03-05T21:36:57Z

index.js

@@ -151,7 +151,7 @@ class Transaction {
    if (chainId < 0) chainId = 0

    // set chainId
-    this._chainId = chainId || data.chainId || 0
+    this._chainId = opts.chainId || chainId || data.chainId || 0


The Common object is set with a specific chain and can be used to access the chain ID, can we use this instead of adding yet another option here?

I would also tend to remove the date.chainId parameter and also set this from Common after instantiation.

Good suggestions, thanks. I also overlooked how the opts.chain param was added with #130.

So we might also be able to get rid of the code on lines 150, 151 and 152. No need to calculate the chainId from the signature. Either an opts.chain is passed or we assume it is mainnet. Then Common can do the rest.

holgerd77 · 2019-03-05T21:54:54Z

index.js

@@ -179,7 +179,8 @@ class Transaction {
    if (includeSignature) {
      items = this.raw
    } else {
-      if (this._chainId > 0) {
+      const v = ethUtil.bufferToInt(this.v)
+      if ((v === this._chainId * 2 + 35 || v === this._chainId * 2 + 36) && this._common.gteHardfork('spuriousDragon')) {


I would prefer to have this separated into two if clauses so that this get's more readable. So first the hardfork check on spuriousDragon to indicate the HF dependent logic and then a separate v === this._chainId * 2 + 35 || v === this._chainId * 2 + 36 check. For the second check a short comment what's happening would also be helpful for readability.

Changes made in 812c99d

I didn't quite use two if statements, probably because of stylistic preference. It should be much clearer. I'm fine to go with two if statements explicitly if that is your preference :)

holgerd77 · 2019-03-05T22:01:31Z

index.js

-      this._senderPubKey = ethUtil.ecrecover(msgHash, v, this.r, this.s)
+      const v = ethUtil.bufferToInt(this.v)
+      const useChainIdWhileRecoveringPubKey = v >= this._chainId * 2 + 35 && this._common.gteHardfork('spuriousDragon')
+      this._senderPubKey = ethUtil.ecrecover(msgHash, v, this.r, this.s, useChainIdWhileRecoveringPubKey && this._chainId)


holgerd77 · 2019-03-05T22:05:00Z

test/transactionRunner.js

+  .then(() => {
+    t.end()
+  })
+})


Looks good, clean and nice and easy to read test setup! 👍

holgerd77 · 2019-03-05T22:07:02Z

Also: can you still confirm regarding #132?

danjm · 2019-03-06T17:34:32Z

@holgerd77 I've closed #132, this PR covers the important pieces of it

danjm · 2019-03-06T18:01:50Z

In addition to the above changes, I am going to give myself one more task before I merge:

QA to verify that this does not break anything on ropsten or rinkeby

danjm · 2019-03-07T12:24:32Z

Oops, my latest update broke the build. I will fix it.

holgerd77 · 2019-03-07T13:42:36Z

Sounds good, let me know once this is ready for re-review!

holgerd77 · 2019-03-15T16:12:47Z

What's the status of this?

danjm · 2019-03-22T04:09:53Z

~~@holgerd77 This is now complete, but there is a caveat that may mean another change is necessary: this PR have introduces three breaking changes with regards to how this._chaindId is set.~~

Previously chainId was calculated this way:

    // calculate chainId from signature
    let sigV = ethUtil.bufferToInt(this.v)
    let chainId = Math.floor((sigV - 35) / 2)
    if (chainId < 0) chainId = 0

    // set chainId
    this._chainId = chainId || data.chainId || 0

~~After this PR it is just: this._chainId = this._common.chainId() (where this._common.chainId() depends on passed opts.chain params)~~

~~So, the caller is explicitly required to set the opts.chainproperty if they want the chainId to be something other than the default associated with mainnet (i.e. 1).~~

This means two breaking changes:
(1) this._chainId will no longer be calculated from the v value included in the data param passed to the constructor.
(2) a specific chain id cannot be included in the data param passed to the constructor
(3) The chainId will now default to 1 instead of 0 if no other info is specified

This is in line with this comment #131 (comment), but goes a step further to include the removal of calculation of chainId from v and the defaulting to 0 (which seems to be more aligned with the specification https://github.com/ethereum/EIPs/blob/master/EIPS/eip-155.md#specification)

…gh 'chain' and 'common' opts.

…mjs common updates.

danjm · 2019-03-31T01:49:47Z

@holgerd77 I have revised this PR so that my previous comment is no longer true: there are no longer breaking changes.

Instead, this PR updates the codebase to support the new way of setting chainId and the legacy approach:

    if (opts.chain || opts.common) {
      this._chainId = this._common.chainId()
    } else {
      this._chainId = chainId || data.chainId || 0
    }

Developers that us opts.chain or opts.common will be able to explicitly set chainId to a value of their choice. Developers that do not set it will have it set by default according to the existing rules:

    // calculate chainId from signature
    let sigV = ethUtil.bufferToInt(this.v)
    let chainId = Math.floor((sigV - 35) / 2)
    if (chainId < 0) chainId = 0

I've decided to leave support for the use of data.chainId (despite your comment) as I think it would be better to give notice of the deprecation before breaking the functionality. Of course, if you think it is okay given this will be a major version upgrade, then it is easy to remove this entirely.

Meanwhile, this PR includes an update to run all transaction tests on mainnet, which seems to be an assumption baked into those tests.

Also not that #143 is based off this PR. Among other things, it addresses an outstanding issue with this PR.

I believe this PR is now complete and ready to merge, notwithstanding the a possible decision on the removal of data.chainId from the constructor, as described above.

holgerd77 · 2019-04-02T08:54:11Z

Trusting your choices here, will try to do a short-term final review. I am thinking a bit if you guys want to completely take over responsibility for the continued work and directly do cross-reviews with e.g. @chikeichan and eventually new contributors from Nomic, I think you are understanding this library at least as good as me anyhow. And I would rather watch from the sideline and try to make sure it fits into overall strategy, modernization efforts and technical guidelines.

So Jacky e.g., if you want to do a review on this and then merge, feel free (but generally: please take reviews seriously, you can very well take 1-2 hours for this and also put this in the timesheet, this is as important as the work itself).

Best
Holger

holgerd77 · 2019-04-02T09:09:28Z

Or @s1na could eventually also do a review on this

alcuadrado · 2019-04-03T00:24:10Z

I reviewed this PR today, and it looks good. Note that this is my first review here, so I may be missing something. I didn't make any comment on style either, as I'm still getting used to the ethereumjs's standards.

The only thing that raised my attention is that after this PR the function hash doesn't include the _chainId info when being used to sign new transactions. I don't know if this is intentional, as there's #143, but I haven't seen it yet. I belive this may be a bug, as sign is encoding the v like EIP155 dictates anyway.

I created this test which can be added to test/api.js as an example of the issue raised in the previous paragraph. It is failing in this branch and passing in master.

  t.test('EIP155 hashing when singing', function (st) {
    txFixtures.slice(0, 3).forEach(function (txData) {
      const tx = new Transaction(txData.raw.slice(0, 6), {chain: 1})

      var privKey = new Buffer(txData.privateKey, 'hex')
      tx.sign(privKey)

      st.equal(
        tx.getSenderAddress().toString("hex"),
        txData.sendersAddress,
        'computed sender address should equal the fixture\'s one'
      )
    });

    st.end()
  })

chikeichan · 2019-04-03T01:29:41Z

index.js

+
+      const onEIP155BlockOrLater = this._common.gteHardfork('spuriousDragon')
+      const v = ethUtil.bufferToInt(this.v)
+      const vAndChainIdMeetEIP155Conditions = v === this._chainId * 2 + 35 || v === this._chainId * 2 + 36


chikeichan · 2019-04-03T01:37:02Z

reviewed coding style and structure - LGTM! I also agree with the decision to not introduce breaking changes without a waring period.

And thanks for the detailed review @alcuadrado - @danjm can you please take a look at the hash function tests @alcuadrado added?

chikeichan · 2019-04-03T01:39:59Z

index.js

+    if (opts.chain || opts.common) {
+      this._chainId = this._common.chainId()
+    } else {
+      this._chainId = chainId || data.chainId || 0


The discussion on here is pretty useful - can we add an inline comment to summarize (or paste the link to comment)? This would help future devs to gather context quickly.

danjm · 2019-04-04T13:26:07Z

Great catch @alcuadrado. That bug has been addressed on #143

I just pushed the test you wrote to that branch. It passed for me locally.

holgerd77

Seems we are ready, thanks Dan, feel free to merge! 😀

holgerd77 · 2019-04-14T19:34:34Z

Ok, will now merge here myself so that we can move on on top of the work here.

danjm mentioned this pull request Dec 4, 2018

Update official transaction tests to run with latest ethereumjs-testing (WIP) #126

Closed

danjm changed the title ~~Update official transaction tests~~ Update official transaction tests (Blocked) Jan 4, 2019

danjm mentioned this pull request Jan 4, 2019

Support spurious dragon and later #132

Closed

holgerd77 mentioned this pull request Jan 10, 2019

Drop ethereumjs-testing dep and fix bug in branch value update ethereumjs/merkle-patricia-tree#69

Merged

danjm added 7 commits February 8, 2019 12:08

Update official transaction tests to run with latest version of ether…

93911c6

…eumjs-testing

Fix testRunner to ensure that all tests run and fail correctly

5ada62d

Run one assertion per test and properly handle txs which are expected…

d9c00e8

… to be undefined.

Allow running of a single test according to an exact filename match.

249672e

Refactor transactionRunner for compatability with use of latest ether…

e7b1a57

…eumjs-common (PR #130)

Ensure transaction runner handles all forks, valid and invalid tests …

26b53bf

…correctly.

ethereumjs-tx support for spurious dragon, as well latest versions of…

b39d17c

… common and util

danjm force-pushed the update-official-transaction-tests branch from 50dddbd to 857c685 Compare March 1, 2019 07:10

danjm changed the title ~~Update official transaction tests (Blocked)~~ Update official transaction tests Mar 1, 2019

chikeichan mentioned this pull request Mar 5, 2019

Update Travis to run on Node v8 and v10; Add tests for inherited FakeTransaction functions #138

Merged

holgerd77 suggested changes Mar 5, 2019

View reviewed changes

danjm force-pushed the update-official-transaction-tests branch from c011704 to 8ea361a Compare March 22, 2019 03:46

Support legacy chainId setting alongside new setting of chainId throu…

e85d975

…gh 'chain' and 'common' opts.

Fix lint and fake.js tests in response to transaction test and ethere…

c15edfb

…mjs common updates.

danjm force-pushed the update-official-transaction-tests branch 2 times, most recently from 8773c7b to d7c98ea Compare March 31, 2019 00:14

danjm mentioned this pull request Mar 31, 2019

Correctly support signing and serializing of EIP155 valid transaction #143

Merged

Clarify application of EIP155 rules in the hash() method

4d75731

danjm force-pushed the update-official-transaction-tests branch from d7c98ea to 4d75731 Compare March 31, 2019 01:30

danjm mentioned this pull request Mar 31, 2019

Should throw error if chainId not specified #137

Closed

chikeichan reviewed Apr 3, 2019

View reviewed changes

holgerd77 approved these changes Apr 4, 2019

View reviewed changes

alcuadrado mentioned this pull request Apr 13, 2019

TypeScript migration #145

Merged

holgerd77 merged commit 8e425ec into master Apr 14, 2019

alcuadrado mentioned this pull request Apr 29, 2019

Simplify Transaction constructor's options #149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update official transaction tests #131

Update official transaction tests #131

danjm commented Dec 4, 2018

danjm commented Dec 4, 2018

holgerd77 commented Dec 4, 2018

danjm commented Jan 4, 2019

danjm commented Mar 1, 2019

coveralls commented Mar 1, 2019 •

edited

Loading

holgerd77 commented Mar 1, 2019

holgerd77 left a comment

holgerd77 Mar 5, 2019

danjm Mar 6, 2019

holgerd77 Mar 5, 2019

danjm Mar 7, 2019

holgerd77 Mar 5, 2019

holgerd77 Mar 5, 2019

holgerd77 commented Mar 5, 2019

danjm commented Mar 6, 2019

danjm commented Mar 6, 2019 •

edited

Loading

danjm commented Mar 7, 2019

holgerd77 commented Mar 7, 2019

holgerd77 commented Mar 15, 2019

danjm commented Mar 22, 2019 •

edited

Loading

danjm commented Mar 31, 2019

holgerd77 commented Apr 2, 2019

holgerd77 commented Apr 2, 2019

alcuadrado commented Apr 3, 2019 •

edited

Loading

chikeichan Apr 3, 2019

chikeichan commented Apr 3, 2019

chikeichan Apr 3, 2019

danjm commented Apr 4, 2019

holgerd77 left a comment

holgerd77 commented Apr 14, 2019

Update official transaction tests #131

Update official transaction tests #131

Conversation

danjm commented Dec 4, 2018

danjm commented Dec 4, 2018

holgerd77 commented Dec 4, 2018

danjm commented Jan 4, 2019

danjm commented Mar 1, 2019

coveralls commented Mar 1, 2019 • edited Loading

holgerd77 commented Mar 1, 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

holgerd77 commented Mar 5, 2019

danjm commented Mar 6, 2019

danjm commented Mar 6, 2019 • edited Loading

danjm commented Mar 7, 2019

holgerd77 commented Mar 7, 2019

holgerd77 commented Mar 15, 2019

danjm commented Mar 22, 2019 • edited Loading

danjm commented Mar 31, 2019

holgerd77 commented Apr 2, 2019

holgerd77 commented Apr 2, 2019

alcuadrado commented Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

chikeichan commented Apr 3, 2019

Choose a reason for hiding this comment

danjm commented Apr 4, 2019

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 commented Apr 14, 2019

coveralls commented Mar 1, 2019 •

edited

Loading

danjm commented Mar 6, 2019 •

edited

Loading

danjm commented Mar 22, 2019 •

edited

Loading

alcuadrado commented Apr 3, 2019 •

edited

Loading