-
Notifications
You must be signed in to change notification settings - Fork 839
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
Add an acceptance test to ensure eth_getBlockByNumber and eth_getBlockByHash return withdrawals correctly for shanghai #5146
Conversation
…kByHash return withdrawals correctly for shanghai Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
@@ -0,0 +1,35 @@ | |||
{ |
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.
Why does the first test start from 02?
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.
haha, I knew you'd pick up on that ;)
I copied the test cases over from replayBlock ATs and I was too lazy to rename all the files after deleting the unnecessary first 01_prepare_payload.
I will rename :p
...e-tests/tests/src/test/resources/jsonrpc/eth/getBlockBy/test-cases/04_update_forkchoice.json
Show resolved
Hide resolved
...ests/tests/src/test/resources/jsonrpc/eth/getBlockBy/test-cases/06_get_last_paris_block.json
Show resolved
Hide resolved
"uncles" : [ ], | ||
"transactions" : [ ], | ||
"withdrawalsRoot" : "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421", | ||
"withdrawals": [ ] |
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.
Can we also test this block with the empty withdrawals case using eth_getBlockByHash
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.
yeh sure. I know it's using the same code underneath so took a short cut to minimise the test execution cost, but maybe I should go whole hog and test last paris block too?
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.
don't we need to test the last Paris block, just testing the shanghai block would be enough
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
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.
LGTM
…yperledger#5146) Add an acceptance test to ensure eth_getBlockByNumber and eth_getBlockByHash return withdrawals correctly for shanghai Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
…yperledger#5146) Add an acceptance test to ensure eth_getBlockByNumber and eth_getBlockByHash return withdrawals correctly for shanghai Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
Fixes #4808
I spent a good amount of time trying to add a test similar to https://github.com/hyperledger/besu/blob/main/ethereum/api/src/integration-test/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/fork/frontier/EthGetBlockByNumberIntegrationTest.java
...but was ultimately thwarted by some post-merge genesis protocol schedule issues.
I think this AT-style is actually easier to understand and work with, and also tests the JSON serialization as a bonus.