fix: serve blockhash from state #1224
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant opBlockhashPostFeynman
participant StateDB
Caller->>opBlockhashPostFeynman: Invoke with block number
opBlockhashPostFeynman->>opBlockhashPostFeynman: Compute ring index (blockNum % window)
opBlockhashPostFeynman->>opBlockhashPostFeynman: Encode key (last 8 bytes of 32-byte hash)
opBlockhashPostFeynman->>StateDB: Get block hash from history storage using key
StateDB-->>opBlockhashPostFeynman: Return block hash
opBlockhashPostFeynman->>Caller: Set block hash on stack value (or clear if out of range)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the blockhash opcode implementation in the post-Feynman transition to serve blockhash values directly from state storage instead of using the context's GetHash method, ensuring consistency with scroll-revm behavior.
- Changes the
opBlockhashPostFeynmanfunction to retrieve blockhash values from the history storage system contract - Updates version patch number from 70 to 71 for deployment tracking
- Removes witness-related code and simplifies the blockhash retrieval logic
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| params/version.go | Updates patch version from 70 to 71 |
| core/vm/instructions.go | Modifies blockhash opcode to serve values from state storage instead of context |
Comments suppressed due to low confidence (1)
core/vm/instructions.go:492
- The change from using interpreter.evm.Context.GetHash() to StateDB.GetState() represents a significant behavioral change that should have comprehensive test coverage to ensure the state storage contains the expected blockhash values.
res := interpreter.evm.StateDB.GetState(params.HistoryStorageAddress, key)
greged93
left a comment
greged93
left a comment
There was a problem hiding this comment.
I'm trying to see how the L1 handled this but can't find anything rn on revm
Revm does not serve |
Makes sense! |
Thegaram
dismissed stale reviews from georgehao, greged93, and colinlyguo
via
69bbb41
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
core/state_processor_test.go (3)
524-533: Consider adding the Solidity source code for better maintainability.While the bytecode is documented with the Solidity version, including the actual source code would improve maintainability and make it easier to verify the bytecode.
Add the complete Solidity source code as a comment:
// pragma solidity =0.8.24; // // contract GetBlockHash { + // fallback() external payable {} // function get(uint256 blockNumber) external view returns (bytes32) { // return blockhash(blockNumber); // } // }
547-547: Consider reducing the gas limit for better test efficiency.The gas limit of 30,000,000 seems excessive for a simple
blockhashquery. A more reasonable limit would improve test efficiency.- 30_000_000, // gasLimit + 100_000, // gasLimitAnd update line 558 accordingly:
- ret, _, _ := evm.Call(vm.AccountRef(msg.From()), *msg.To(), msg.Data(), 30_000_000, common.Big0, nil) + ret, _, _ := evm.Call(vm.AccountRef(msg.From()), *msg.To(), msg.Data(), 100_000, common.Big0, nil)Also applies to: 558-558
651-651: Add type assertion check for safety.The type assertion could panic if the implementation changes.
- hasher := sha3.NewLegacyKeccak256().(keccakState) + hasher, ok := sha3.NewLegacyKeccak256().(keccakState) + if !ok { + panic("sha3.NewLegacyKeccak256() does not implement keccakState") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/state_processor_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
core/state_processor_test.go (2)
22-22: LGTM!The
hashimport is correctly added and necessary for thekeccakStateinterface.
640-643: LGTM!The
keccakStateinterface correctly combineshash.HashwithReadmethod for type assertion of the Keccak256 hasher.
1. Purpose or design rationale of this PR
Ensure behavior of
blockhashopcode in blocks shortly after the Feynman transition is consistent with scroll-revm.2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Bug Fixes
Chores
Tests