-
Notifications
You must be signed in to change notification settings - Fork 54
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
test: add debug_traceTransaction
with opcode logger tests
#786
test: add debug_traceTransaction
with opcode logger tests
#786
Conversation
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
1e40103
to
94cb33e
Compare
94cb33e
to
ed75d98
Compare
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG but mostly questions
with: | ||
version: nightly | ||
|
||
- name: Run besu node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: why do we need to run besu again.
Is it that the @OpcodeLogger
tests rely on comparing outputs from besu and local node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of them (tagged with 'besu comparison') - yes, and that's the reason. Another possible question could be:
Why just don't hardcode the besu's responses in a JSON and compare against it?
Every time we change a solidity's compiler version in the hardhat.config.js
, we will have to locally run these tests against besu to generate a new hardcoded JSON which will be used for further comparison. That should be needed because let's get for example changes from solidity 0.8.23 and 0.8.24. Contracts compiled with the older version will not include EIP-5656 (for mcopy opcode) and EIP-1153 (for tstore and tload opcodes) and debug_traceTransaction
will return opcodes based on the contract's bytecode. When we change a solidity version to 0.8.24 in hardhat.config.js
, contracts will be precompiled and the new opcodes (from EIP-5656 and EIP-1153) will be introduced in the contracts bytecodes, so when we run the tests and compare debug_traceTransaction
responses with the hardcoded ones (generated with contracts compiled with solidity 0.8.23) they will differ.
Based on the info above, we have two possible approaches:
- skip the besu flow in the ci, generate a hardcoded json with responses from the besu beforehand, and rely on developers to keep the hardcoded besu responses up-to-date with the solidity version defined in
hardhat.config.js
- stick to this CI where on each run we generate a json for comparison based on the current solidity version in
hardhat.config.js
I prefer the second approach because it's not binding to the developers and we shouldn't take extra care if the solidity version is bumped.
Hope that clears some of the things out. I'm open to changes if you find something disrupting or not right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. I'd like this called out in some doc somewhere so people understand the flow
package.json
Outdated
@@ -28,7 +28,8 @@ | |||
"hh:test": "hardhat test", | |||
"hedera:start": "npx @hashgraph/hedera-local start --limits=false --dev=true --balance=10000000", | |||
"hedera:stop": "npx @hashgraph/hedera-local stop", | |||
"prepare": "husky install" | |||
"prepare": "husky install", | |||
"besu:start": "docker run -d -v ./utils/besu-configs:/var/lib/besu/ -p 8540:8545 -p 8541:8546 hyperledger/besu:24.6.0 --config-file=/var/lib/besu/customConfigFile.toml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Is there a README that should be updated and explains how besu is being used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed adding a README, it's my mistake. Maybe a better idea is to remove this from the npm scripts section and just move the docker run
command into the opcode-logger-testing.yml
only? In that way, we'll not confuse the developers with this specific besu testing flow. Or it's better to add a descriptive README for besu:start
npm script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a README to be more descriptive for this flow.
Approach makes sense, just explain it somewhere so it's an easy reference
ab56fbc
to
542d158
Compare
Signed-off-by: nikolay <[email protected]>
…ogger-tests # Conflicts: # package.json
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Description:
Recently, the mirror node web3 added a debug opcodes endpoint which is utilized in the relay as well but there is no testing coverage yet.
Related issue(s):
Fixes #783
Notes for reviewer:
Checklist