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

Support spurious dragon and later #132

Closed danjm closed 5 years ago

danjm commented 5 years ago

This PR builds off the changes in #130 and is needed to get a number of tests passing after the test rewrite in #131

Before this can be merged, we will need to merged those and rebase this on top of it. I am posting this PR now as the code changes were useful to me in verifying the work of #130 and #131

It should be noted that there are still failures from 12 test files after this PR. All of these failures are cases where the transaction should be invalid on 'Byzantium', 'Constantinople', and 'EIP158'/spuriousDragon. However I have spent an significant amount of time investigating those failures and cannot determine there cause. In fact, the transactions from at least two of these test cases are valid when creating them from their test file data in geth (https://github.com/ethereum/go-ethereum/compare/master...danjm:demo-WrongVRSTestVEqual39-validity?expand=1) even though the test file asserts that they should be invalid.

I will continue to investigate these issues, however we may wish to merge this PR before the final test cases are resolved, as this PR increase our overall coverage/support for 'Byzantium', 'Constantinople', and 'EIP158'/spuriousDragon

holgerd77 commented 5 years ago

Hi @danjm, thanks for all this work!

You don't have to take for granted that the test files (respectively all test cases in it) are valid, these may very well be an issue with the tests itself. In doubt open an issue over on the tests repository.

holgerd77 commented 5 years ago

Ok, merged #130. 😄

danjm commented 5 years ago

Closing as relevant changes are now included in https://github.com/ethereumjs/ethereumjs-tx/pull/131