new(tests): Blockhash at 255/256/257 (ethereum/execution-specs#374) #988
Conversation
|
For this one I sneakily grabbed the hashes off of the outputs for the tests when I ran them. You won't be able to do that with future forks right? Is there a way that it could be computed on the fly? |
| Note: Hardcoded blockhashes to get us started. Will look for a better | ||
| way of doing this in future |
There was a problem hiding this comment.
Definitely, we would use such a solution for other tests also. There are some tests that use up to 8000+ blocks and significantly slow down the fixture generation process for us, so having a way of pre-generating the tests would be very beneficial.
| post = { | ||
| account: Account( | ||
| storage={"0x0": expected}, | ||
| ), | ||
| } |
There was a problem hiding this comment.
Perhaps outside of the scope of this PR, but I think we could maybe enhance the way we generate tests to make post more dynamic.
I'm thinking that maybe the post object could be a callable/function that receives all the generated blocks and returns the post dictionary?
We would need to update the framework for something like this to work since this has never been done before, but it might benefit other tests too, potentially EIP-2935 tests.
cc @danceratopz to comment on this idea.
There was a problem hiding this comment.
I'll wait for your cc to reply before taking action on this. Interesting idea!
| @@ -0,0 +1,70 @@ | |||
| """ | |||
| abstract: Test the chain synchronization feature. | |||
There was a problem hiding this comment.
In this specific case, for us to test the actual sync we would need to enable the verify_sync flag for the BlockchainTest object (blockchain_test in line 65), which is kind of an obscure feature and only gets really used when the test is consumed in hive, but I think this kind of test is perfect for it.
| env = Environment() | ||
| sender = pre.fund_eoa() | ||
| blockhash_code = Op.SSTORE( | ||
| 0, |
There was a problem hiding this comment.
We could save this value to a key derived from the offset (or the offset itself) instead of zero, because I think we can save up test generation time by testing all offsets in one-shot instead of parametrizing its value (normally we prefer parametrization but in this case we are generating a ton of blocks).
There was a problem hiding this comment.
Hmmm, that's a good point. Okay I'll rewrite that part.
As it is right now, there's no need to update the docs, they should be updated automatically.
See my comment in the review. I think this approach is ok for now, but if you want to adventure into modifying the framework to implement the dynamic-post feature I mention, maybe in a separate PR, I'll gladly guide you through it! |
…on name Co-authored-by: Mario Vega <[email protected]>
The nonce being equal to 0 is implicit and does not need to be state explicitly Co-authored-by: Mario Vega <[email protected]>
🗒️ Description
This adds tests designed to test the BLOCKHASH opcode for when there are 255, 256, and 257 blocks in the chain.
Filled with https://github.com/ethereum/execution-specs#9b95554a88d2a8485f8180254d0f6a493a593fda
🔗 Related Issues
This is actually issue (ethereum/execution-specs#374) in the execution-specs
✅ Checklist
mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.I will add the CHANGELOG update before this gets merged after review.
Do I need to update the docs in the codebase for this one too?