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

Changes to allow evmtool t8n-server to work with execution-spec-tests #5701

Merged
merged 5 commits into from
Jul 18, 2023

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented Jul 13, 2023

An omnibus of minor changes needed for t8n-server to work with the EFs new execution-spec-tests framework

  • Reduce logging output
  • Fix json library mismatch between t8n and t8n-server
  • Add hook to enumerate supported forks
  • temporarily map Shanghai+6780 to Cancun
  • add to main distro under 'evmtool' name

PR description

Fixed Issue(s)

An omnibus of minor changes needed for t8n-server to work with the EFs
new execution-spec-tests framework

* Reduce logging output
* Fix json library mismatch between t8n and t8n-server
* Add hook to enumerate supported forks
* temporarily map Shanghai+6780 to Cancun
* add to main distro under 'evmtool' name

Signed-off-by: Danno Ferrin <[email protected]>
@github-actions
Copy link

github-actions bot commented Jul 13, 2023

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@shemnon shemnon added the doc-change-required Indicates an issue or PR that requires doc to be updated label Jul 13, 2023
@shemnon
Copy link
Contributor Author

shemnon commented Jul 13, 2023

Docs change would include this section: https://besu.hyperledger.org/stable/public-networks/reference/evm-tool

  • point out the tool is now standard to the distribution
  • the name is now evmtool to not conflict with Geth's evm command.
    • We do aim to be CLI compatible with Geth's evm command so bugs pointing out incompatibilities are welcome.
  • For the sub-commands (block-builder/b11r, code-validate, state-test, transition/t8n, and `t8n-server) I don't think extensive documentation is needed. Point to https://ethereum-tests.readthedocs.io/en/develop/t8ntool-ref.html and https://ethereum.github.io/execution-spec-tests/ . These sub commands exist solely to serve those testing code bases and are not meant for typical user interactions.

Signed-off-by: Danno Ferrin <[email protected]>
@danceratopz
Copy link

I can confirm this is working nicely with ethereum/execution-spec-tests/pull/206 apart from some tests failing to fill. There seems to be 2 sources of fails, one is due to the incorrect chain id being specified and the other is to do with tx playback protection. I think both of these can (and perhaps even should) be remedied on our side, will let you know early next week once I've taken a closer look.

@danceratopz
Copy link

So, on our side, I've fixed whether playback protection is enabled/disabled in some of the test cases. I pushed these to ethereum/execution-spec-tests/pull/206.

However, as we don't set the protected field in transactions any more, it would be helpful, if we could check for playback protection using v. I made a suggestion in shemnon/pull/14.

ethereum/execution-spec-tests/pull/206/ (ethereum/execution-spec-tests@739173c) in combination with shemnon/pull/14 fills all test cases up and including Shanghai successfully with:

fill --evm-bin=/path/to/besu/ethereum/evmtool/build/install/evmtool/bin/evm

Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

Thanks for the descriptive PR!
The changes themselves look good.

I would only question why we are adding the evm bin to the standard distribution?
It won't be needed for the vast majority of users AFAICT and I'm concerned we may inconvenience more people than we help with this (cue the Discord questions about which binary to use and why things don't match the install guides etc)

@shemnon
Copy link
Contributor Author

shemnon commented Jul 18, 2023

Size wise it's a script and a small jar.

But the reason is to improve user experience. Running execution-spec-tests should't require installing raw source and executing it. It's like stripping javac from the java distro, Oracle stopped doing that 7 years ago when they sunset the JRE.

Note that Geth ships a whole lot more with their homebrew formula and linux packages. Notably adding clef, devp2p, abigen, bootnode, evm, rlpdump and puppeth. People have no problem identifying that 'geth' runs geth, similarly 'besu' runs besu.

We probably should have added it years ago.

@shemnon
Copy link
Contributor Author

shemnon commented Jul 18, 2023

However, as we don't set the protected field in transactions any more, it would be helpful, if we could check for playback protection using v. I made a suggestion in shemnon#14.

Merged.

…-if-tx-is-playback-protected

Use v to determine if the transaction is protected

Signed-off-by: Danno Ferrin <[email protected]>
Copy link
Contributor

@diega diega left a comment

Choose a reason for hiding this comment

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

👍

@siladu
Copy link
Contributor

siladu commented Jul 18, 2023

Note that Geth ships a whole lot more with their homebrew formula and linux packages.

Homebrew, yes, but if I download the linux package from https://geth.ethereum.org/downloads then it's just a single geth binary.

People have no problem identifying that 'geth' runs geth, similarly 'besu' runs besu.

I do agree that it should be easy enough for users to figure it out and looking at the common setups guides the instructions will still work, so I have no objection based on impaired usability for less savvy users:

Signed-off-by: Danno Ferrin <[email protected]>
@shemnon shemnon enabled auto-merge (squash) July 18, 2023 23:17
@shemnon shemnon merged commit 03ff688 into hyperledger:main Jul 18, 2023
davidkngo pushed a commit to liquichain/besu that referenced this pull request Jul 21, 2023
…hyperledger#5701)

An omnibus of minor changes needed for t8n-server to work with the EFs
new execution-spec-tests framework

* Reduce logging output
* Fix json library mismatch between t8n and t8n-server
* Add hook to enumerate supported forks
* temporarily map Shanghai+6780 to Cancun
* add to main distro under 'evmtool' name
* No longer support the "protected" attribute in TXes

Signed-off-by: Danno Ferrin <[email protected]>
davidkngo added a commit to liquichain/besu that referenced this pull request Jul 21, 2023
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Aug 14, 2023
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
…hyperledger#5701)

An omnibus of minor changes needed for t8n-server to work with the EFs
new execution-spec-tests framework

* Reduce logging output
* Fix json library mismatch between t8n and t8n-server
* Add hook to enumerate supported forks
* temporarily map Shanghai+6780 to Cancun
* add to main distro under 'evmtool' name
* No longer support the "protected" attribute in TXes

Signed-off-by: Danno Ferrin <[email protected]>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
…hyperledger#5701)

An omnibus of minor changes needed for t8n-server to work with the EFs
new execution-spec-tests framework

* Reduce logging output
* Fix json library mismatch between t8n and t8n-server
* Add hook to enumerate supported forks
* temporarily map Shanghai+6780 to Cancun
* add to main distro under 'evmtool' name
* No longer support the "protected" attribute in TXes

Signed-off-by: Danno Ferrin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants