search

ethereumjs / ethereumjs-tx

Project is in active development and has been moved to the EthereumJS VM monorepo.
https://github.com/ethereumjs/ethereumjs-vm/tree/master/packages/tx
Mozilla Public License 2.0
779 stars 235 forks source link

Update official transaction tests #131

Closed danjm closed 5 years ago

danjm commented 5 years ago

Opening this PR instead of https://github.com/ethereumjs/ethereumjs-tx/pull/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 commented 5 years ago

@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 commented 5 years ago

Hi Dan, thanks for the update! This whole this._homestead thing is totally legacy and should be replaced. Can you cross-coordinate with this https://github.com/ethereumjs/ethereumjs-tx/pull/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 commented 5 years ago

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

Once that is merged, I will rebase here

danjm commented 5 years ago

@holgerd77 This is ready for final review and merge. I'll confirm tomorrow, but the latest changes likely make https://github.com/ethereumjs/ethereumjs-tx/pull/132 redundant.

coveralls commented 5 years ago

Coverage Status

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

holgerd77 commented 5 years ago

Great! 👍

holgerd77 commented 5 years ago

Also: can you still confirm regarding #132?

danjm commented 5 years ago

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

danjm commented 5 years ago

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

danjm commented 5 years ago

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

holgerd77 commented 5 years ago

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

holgerd77 commented 5 years ago

What's the status of this?

danjm commented 5 years ago

~@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 https://github.com/ethereumjs/ethereumjs-tx/pull/131#discussion_r262700210, 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)~

danjm commented 5 years ago

@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 https://github.com/ethereumjs/ethereumjs-tx/pull/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 commented 5 years ago

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 commented 5 years ago

Or @s1na could eventually also do a review on this

alcuadrado commented 5 years ago

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 commented 5 years ago

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?

danjm commented 5 years ago

Great catch @alcuadrado. That bug has been addressed on https://github.com/ethereumjs/ethereumjs-tx/pull/143

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

holgerd77 commented 5 years ago

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