Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade ethereumjs components for VM v5 #1337

Merged
merged 59 commits into from
Mar 31, 2021

Conversation

cgewecke
Copy link
Contributor

@cgewecke cgewecke commented Mar 17, 2021

This is some initial work migrating hardhat to the new ethereumjs stack. Very WIP 😄 .

Main areas of (real) test failure are forking related, also some TxPool and mining stuff. However, atm almost all provider tests fail due to issue with proposed fix at ethereumjs/ethereumjs-monorepo#1153.

(If you want to see what the actual state of this PR is, you'd need to implement that change locally in your node_modules.)


Summary:

  • Replaces ethereumjs vm, tx, block, util, merkle-patricia-tree, and account with their latest versions.
    • ethereumjs-account no longer exists, has been merged into ethereumjs-util
  • Replaces PStatemanager with ethereumjs vm's statemanager
  • Implements various data type changes - notably addresses have their own type and are no longer Buffer
  • Adds a FakeTransaction class (this no longer exists in ethereumjs/tx)
  • Most packages in the new stack use static generators instead of new to create objects, and their internal properties are immutable. Have left comments in the code where this is presenting difficulties. One place is here.

Resources

TODO

  • Remove extraneous types
    • hardhat-network/provider/types/StateManager
    • hardhat-network/provider/types/PStateManager
    • config/typescripts/types/ethereumjs-block
  • Resolve [email protected] incompatibility w/ nomiclabs/trufflev4/v5 packages. (See lint error in CI)
  • Bump vm / tx / block packages on ethereumjs Berlin release (est. March 23)
  • Investigate / understand callbackified Blockchain (changes there are v. likely wrong)
  • Integrate EIP selection into network config, so users can toggle EIPs on/off at will. (We won't do this right now)

Major areas of test failure

  • node#mineBlock (transaction is not signed)
  • TxPool (transaction is not signed)
  • ForkedBlockchain (6 failing )
  • ForkedProvider: ( 1 failing )
  • All stack traces tests (This transaction is not signed)

NB: There are scattered single failures in other parts of the test suite (usually just one or two). FSM tests all pass.

Have left some comments in the code describing rationale for misc changes - apologies for doing this directly in the files. I wanted to keep notes as I went through rather than forget to leave notes in GH when opening the PR. Most of these will need to be removed.

Obviously there's still a lot to do (plus some things are done incorrectly) Am just hoping to get the "chore" part of this over the fence so the trickier parts can be fixed by smart people here :)

Please lmk/advise at your leisure about how to proceed and if you have questions.

Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just posting the progress so far

@cgewecke
Copy link
Contributor Author

cgewecke commented Mar 24, 2021

Leaving this for tonight... there are still 3 test failures that am unsure about the cause of or remedy for:

  • This, which seems pretty bad...
1) Local accounts provider
        Should, given two identical tx, return send the same raw tansaction:
 
 |       AssertionError: 
 |       + expected - actual
 | 
 |       -0xf86280830a5c008094b5bc06d4548a3ac17d72b372ae1e416bf65b8ead018082011aa052ab42ab3e2e74ca24df12dbd3539197028316216087c1a333c971377b3cc3dba068a5be657ade9e4476f4c54c3f4984e894d82548df4405a024a8f7c37bbc6de6
 |       +0xf86480830a5c0082520894b5bc06d4548a3ac17d72b372ae1e416bf65b8ead018082011aa0614471b82c6ffedd4722ca5faa7f9b309a923661a4b2adc1a53a3ebe8c4d1f0aa06aebf2fbbe82703e5075965c65c776a9caeeff4b637f203d65383e1ed2e22654
  • ...and this, on the forked provider ...
1) Forked provider
       Using Alchemy
         eth_getTransactionByHash
           supports remote transactions:

      AssertionError: expected '0x2d3b7f324aca71436c5c43ccd615bdbe86661c6f' to equal '0x4e87582f5e48f3e505b7d3b544972399ad9f2e5f'
      + expected - actual

      -0x2d3b7f324aca71436c5c43ccd615bdbe86661c6f
      +0x4e87582f5e48f3e505b7d3b544972399ad9f2e5f
      
      at Context.<anonymous> (test/internal/hardhat-network/provider/forked-provider.ts:298:18)
      at process._tickCallback (internal/process/next_tick.js:68:7)

Something's not right.

@cgewecke cgewecke marked this pull request as ready for review March 24, 2021 00:39
Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll continue this later today

Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I forgot to submit these comments yesterday

@alcuadrado alcuadrado merged commit 767f68d into NomicFoundation:master Mar 31, 2021
@alcuadrado alcuadrado mentioned this pull request Mar 31, 2021
@cgewecke
Copy link
Contributor Author

cgewecke commented Apr 1, 2021

Thanks so much @fvictorio for all those fixes!!! (Sorry this needed so much additional work)

Thanks @alcuadrado 😄

@cgewecke cgewecke deleted the ethereumjs-v5 branch April 1, 2021 01:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants