Add support for L1 MPT #1104
Add support for L1 MPT #1104
Conversation
WalkthroughThe pull request introduces enhancements to the Ethereum client's state management, focusing on the integration of the Scroll network and stateless execution capabilities. Key changes include the addition of a new flag for configuring Merkle Patricia Trie (MPT) options, modifications to state validation logic, and the introduction of methods for managing disk state roots. The API is expanded to support execution witnesses, and new witness tracking mechanisms are implemented. These changes aim to improve state validation, enhance blockchain configuration flexibility, and provide insights into state transitions. Changes
Sequence DiagramsequenceDiagram
participant Client
participant BlockChain
participant StateDB
participant Witness
participant API
Client->>BlockChain: Request Block
BlockChain->>StateDB: Start Prefetcher
StateDB->>Witness: Create Witness
Witness-->>StateDB: Capture State Nodes
StateDB->>BlockChain: Process Block
Client->>API: Request Execution Witness
API->>BlockChain: Retrieve Block
API->>Witness: Generate Witness
Witness-->>API: Return Witness Details
API->>Client: Provide Execution Witness
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
omerfirmak
force-pushed
the
omerfirmak/mpt
branch
from
9716dfa to
bc9dc5d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (27)
core/stateless/witness.go (2)
49-69: Check parent header retrieval logicWhen
chainis not nil, the code retrieves the parent header (parent := chain.GetHeader(...)). Ifparentis nil for any reason (database inconsistency or missing data), an error is returned. Consider logging the block hash or other context for easier debugging, especially in long-running systems.
83-88: Check memory usage for code collectionStoring code in a map keyed by the string representation (
w.Codes[string(code)]) is simple, but storing large byte slices in string form can result in heightened memory usage or possible collisions. If large codes are anticipated, you might want to store hashes or references instead.core/state/database.go (2)
108-110: Document potential performance impact of returning large setsThe new interface method
Witness() map[string]struct{}may produce large sets if the trie is big. Document usage expectations or provide a streaming alternative if performance or memory is a concern.
142-144: Log or handle errors encountered while reading disk rootCurrently, if
rawdb.ReadDiskStateRoot(...)returns an error, it is silently ignored and the existingrootremains. Adding debug logs or metrics in theerr != nilcase can help troubleshoot issues (e.g., partial corruption or I/O problems).trie/zk_trie_test.go (1)
289-338: Global state usage in tests may cause concurrency pitfallsGlobal vars (e.g.,
accountsLeft) can lead to race conditions if tests run in parallel. Wrap them inside the test struct or use test-level counters to avoid altering shared state, ensuring thread-safe test environments.trie/trie.go (3)
65-67: Document or enforce concurrency restrictions ontracerThe
tracerfield withinTrieindicates added instrumentation for tracking node access. If the trie is used concurrently, clarify thatt.traceris thread-safe or enforce single-thread usage of the trie to prevent race conditions.
320-324: Manage tracer calls during node insertions/deletionsCalls to
t.tracer.onInsert(...)andt.tracer.onDelete(...)are scattered across the insertion and deletion paths. Ensure consistent handling of partial updates (e.g., if an insert fails midway). Consider a rollback or snapshot approach for robust error handling.Also applies to: 339-343, 396-400, 413-416, 478-482
621-631: Assess memory footprint of witness generation
func (t *Trie) Witness() map[string]struct{}accumulates accessed trie nodes int.tracer.accessList. For large tries, this might be significant. If memory usage becomes problematic, consider a streaming approach or partial flushing of tracked nodes.eth/api.go (2)
326-337: Consider returning nil witness when errors occur.Currently, if
generateWitnessreturns a partially-built witness along with an error, the subsequent return passes the non-nil witness toToExecutionWitness. Depending on the calling context, partial data might be confusing or misleading if there's an error. Consider returningnilin case of errors to avoid partial witness representations.
365-394: Add more context about potential partial failures.The function
testWitnessproperly checks if thediskRootexists and reassignsstateRootorpostStateRoot. The error strings are general, but you might consider including additional context (e.g., mismatch details or expected vs. computed root) to help troubleshoot witness failures.core/state/statedb.go (3)
300-302: Avoid double retrieval of code inGetCode.The code snippet calls
stateObject.Code(s.db)both when adding to the witness and again when returning. For large contracts, this might induce overhead. A small refactor to store it in a local variable is recommended:code := stateObject.Code(s.db) if s.witness != nil { - s.witness.AddCode(stateObject.Code(s.db)) + s.witness.AddCode(code) } return code
311-314: Consider caching code inGetCodeSize.Although the function fetches only the code size, calling
stateObject.Code(s.db)inAddCodecan be expensive. If the logic truly needs the code bytes, caching it or confirming necessity may be worthwhile.
933-939: Repeated addition of storage trie witnesses.We see the
s.witness.AddState(obj.trie.Witness())call repeated before and afterobj.updateRoot(...). Possibly unify or clarify if both calls are indeed necessary, or combine them if feasible.params/config.go (1)
899-902: Doc mismatch inIsEuclid.The comment says “returns whether num is … the Darwin fork block,” but the implementation references
EuclidTime. Update doc comment to match the Euclid fork context to avoid confusion.core/stateless/database.go (2)
47-55: Improve readability with direct iteration over codes map values.
Instead of iterating overmap[string]struct{}keys and converting them to[]byte, it might be more expressive to store the code as[]bytein the map (assuming the data is originally in[]byteformat). This eliminates the need to convert fromstringto[]byte.
57-65: Avoid potential collisions in hashing.
While Keccak-256 collisions are extremely unlikely, ensure no mismatch occurs if a future enhancement tries to unify code and state node writes. Using distinct storage keys for codes vs. nodes (like a prefix) is prudent.trie/tracer.go (2)
45-48: Consider more descriptive field names or doc comments.
Whileinserts,deletes, andaccessListare straightforward, additional docstrings clarifying their role might help maintainers.
88-93: Align tracer lifecycle with other components.
After callingreset, ensure other references tot.insertsort.deleteswon't inadvertently reintroduce stale data.cmd/geth/usage.go (1)
53-53: Add usage information forScrollMPTFlag.The new flag is introduced without explicit usage text. Consider adding a brief description of its effect on the MPT state mechanism to guide users.
trie/zk_trie.go (3)
237-243: Validate error handling when retrieving the ZkTrie root.
panic("CountLeaves cannot get root")may cause a crash in production if the root retrieval fails. Consider returning an error instead to fail gracefully in edge cases (e.g., partial database corruption).
245-260: Avoid panics for recoverable I/O errors.
panic("countLeaves cannot get rootNode")may terminate the node. Consider translating I/O errors into typed errors or logging them for safer recoverability.
262-265: Implement theWitnessmethod or provide a TODO comment.Leaving this method as a
panic("not implemented")can hamper the broader witness functionality. If you plan to add it soon, consider adding a TODO clarifying the intended approach.cmd/geth/main.go (1)
153-153: Add usage examples or clarifications.
utils.ScrollMPTFlagis added here. Consider documenting the effect of enabling it in help messages or logs to guide users, e.g., “Enables L1 MPT features.”core/state/state_object.go (2)
503-514: Error handling improvement inCodeSize.
- The approach gracefully handles the scenario where
KeccakCodeHashis empty.- However, storing the error in
dbErrcan mask subsequent errors, since only the first is recorded. Consider logging or ensuring it doesn’t impede diagnosing multiple retrieval issues.
547-547: Document the panic reason.Replacing the previous implementation with
panic("shouldn't be called")is valid if truly unreachable. Provide a descriptive explanation or reference in code comments to clarify why it's never expected to be invoked.miner/scroll_worker.go (1)
812-815: Validate pre-fork and post-fork states.The condition
canCommitState := w.chainConfig.Scroll.UseZktrie != w.chainConfig.IsEuclid(w.current.header.Time)ensures the block commit is skipped unless Zktrie usage aligns with the Euclid fork timing. Verify that off-by-one scenarios and minor clock discrepancies do not cause indefinite skipping, potentially blocking the chain.cmd/utils/flags.go (1)
1905-1912: Consider refactoring duplicate configuration logic.The Zktrie configuration logic is identical for both Scroll Sepolia and mainnet networks. Consider extracting this into a helper function to reduce code duplication and improve maintainability.
+func configureZktrie(ctx *cli.Context, cfg *ethconfig.Config) { + cfg.Genesis.Config.Scroll.UseZktrie = !ctx.GlobalBool(ScrollMPTFlag.Name) + if cfg.Genesis.Config.Scroll.UseZktrie { + // disable pruning + if ctx.GlobalString(GCModeFlag.Name) != GCModeArchive { + log.Crit("Must use --gcmode=archive") + } + log.Info("Pruning disabled") + cfg.NoPruning = true + } +}Then use it in both places:
case ctx.GlobalBool(ScrollSepoliaFlag.Name): // ... existing code ... - cfg.Genesis.Config.Scroll.UseZktrie = !ctx.GlobalBool(ScrollMPTFlag.Name) - if cfg.Genesis.Config.Scroll.UseZktrie { - // disable pruning - if ctx.GlobalString(GCModeFlag.Name) != GCModeArchive { - log.Crit("Must use --gcmode=archive") - } - log.Info("Pruning disabled") - cfg.NoPruning = true - } + configureZktrie(ctx, cfg) case ctx.GlobalBool(ScrollFlag.Name): // ... existing code ... - cfg.Genesis.Config.Scroll.UseZktrie = !ctx.GlobalBool(ScrollMPTFlag.Name) - if cfg.Genesis.Config.Scroll.UseZktrie { - // disable pruning - if ctx.GlobalString(GCModeFlag.Name) != GCModeArchive { - log.Crit("Must use --gcmode=archive") - } - log.Info("Pruning disabled") - cfg.NoPruning = true - } + configureZktrie(ctx, cfg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
cmd/geth/main.go(1 hunks)cmd/geth/usage.go(1 hunks)cmd/utils/flags.go(3 hunks)core/block_validator.go(1 hunks)core/blockchain.go(3 hunks)core/genesis.go(1 hunks)core/rawdb/accessors_state.go(1 hunks)core/rawdb/schema.go(2 hunks)core/state/database.go(2 hunks)core/state/state_object.go(2 hunks)core/state/statedb.go(8 hunks)core/stateless/database.go(1 hunks)core/stateless/encoding.go(1 hunks)core/stateless/witness.go(1 hunks)core/types/state_account.go(1 hunks)eth/api.go(3 hunks)internal/web3ext/web3ext.go(2 hunks)light/trie.go(1 hunks)miner/scroll_worker.go(1 hunks)params/config.go(8 hunks)rollup/pipeline/pipeline.go(1 hunks)trie/database.go(2 hunks)trie/secure_trie.go(2 hunks)trie/tracer.go(1 hunks)trie/trie.go(9 hunks)trie/zk_trie.go(1 hunks)trie/zk_trie_test.go(2 hunks)
🔇 Additional comments (47)
core/stateless/witness.go (4)
75-80: Validate retrieved headers in the loop
This loop appends headers by repeatedly calling w.chain.GetHeader(...) without checking for a nil return. If the chain database is missing headers for any reason, this code will panic on the next iteration. Suggest verifying that each retrieved header is non-nil and returning an error or handling the missing header appropriately.
90-99: Concurrent insertion is well-protected
The function uses a mutex (w.lock) to protect against concurrent writes to w.State, which is good. The approach of using maps.Copy is straightforward and correct, ensuring no partial updates occur if multiple goroutines call AddState.
101-114: Deep copy approach is correct and coherent
Cloning slices and maps through slices.Clone and maps.Clone is a clean approach to guaranteeing an independent copy of the witness state. This is especially beneficial if you need an immutable snapshot of the witness at any given state.
120-122: Ensure non-empty Headers slice before indexing
return w.Headers[0].Root indexes the first header in the slice. If w.Headers happens to be empty (for example, if chain was unavailable), this will panic. Consider adding a precondition check (e.g., if len(w.Headers) == 0 { ... }) to handle unexpected states more gracefully.
trie/zk_trie_test.go (2)
29-38: Import changes look fine
The additional require import from github.com/stretchr/testify/require and other minor import changes are standard for improved test assertions and do not introduce any apparent issues.
272-287: Avoid embedding absolute paths in test code
The test function TestEquivalence references specific local paths such as /Users/omer/Documents/go-ethereum/l2geth-datadir/geth/chaindata. This limits portability and may break in CI environments. Consider parameterizing these paths or using temp directories.
rollup/pipeline/pipeline.go (1)
231-231: Prefetcher invocation looks consistent
Invoking p.state.StartPrefetcher("miner", nil) with the extra parameter is consistent with the approach in other parts of the code. This matches the method signature changes observed in related files.
trie/trie.go (1)
618-619: Tracer reset for subsequent usage
t.tracer.reset() is invoked in t.Reset(). This is fine if the intention is to clear all previous tracking data. Just confirm that no external references rely on the old tracer state once the reset is done.
eth/api.go (7)
37-39: No issues with the new imports.
The import of "github.com/scroll-tech/go-ethereum/core/stateless" appears straightforward and does not introduce any obvious conflicts or redundancies.
339-363: Implementation of generateWitness looks good.
The error handling at each step (creating the witness, retrieving the parent state, processing, and validating) is consistent. Returning (witness, testWitness(...)) ensures immediate cross-validation.
396-403: New ExecutionWitness type is straightforward.
This struct properly exposes headers, codes, and state in an easy-to-serialize format. No issues spotted.
405-413: transformMap is cleanly implemented.
Using the keccak hash of the item as the map key and storing the hex-encoded node as the value is a clear approach.
415-424: ToExecutionWitness accurately transforms the internal witness representation.
The function provides a concise mapping of w.Codes and w.State into the final JSON representation. This is efficient for cross-client compatibility.
965-968: DiskAndHeaderRoot struct.
Straightforward struct addition, properly typed fields.
970-990: DiskRoot method design is straightforward.
The method checks for a stored disk root and falls back to the header root if none is found. Consider logging or returning the encountered error from rawdb.ReadDiskStateRoot if needed for debugging or further handling.
core/state/statedb.go (5)
32-32: New import of github.com/scroll-tech/go-ethereum/core/stateless.
No immediate concerns. This aligns with the new witness functionality.
110-112: Confirm concurrency safety for the new witness field.
If multiple goroutines may call StartPrefetcher or manipulate witness, ensure synchronization or clarify single-threaded usage to avoid potential data races.
166-174: StartPrefetcher changes approved.
Warmly closing any previously running prefetcher and then setting s.witness is clear. The logic for prefetcher = newTriePrefetcher(...) remains consistent.
950-959: Approved additional witness logic.
Gathering the storage trie for objects that have originStorage is consistent with the approach. Implementation looks solid.
991-997: Final witness addition in IntermediateRoot.
Appending the account trie witness here is appropriately placed. The code is straightforward and ensures the final root’s witness is included.
params/config.go (6)
32-41: Genesis hash and state additions look correct.
These newly defined constants for the Scroll networks do not raise any conflict.
345-345: GenesisStateRoot assignment for Scroll Sepolia.
No issues. The reference to ScrollSepoliaGenesisState is consistent with the new config fields.
386-386: GenesisStateRoot assignment for Scroll Mainnet.
Same approach as Sepolia, logically consistent.
640-640: New EuclidTime field.
Adding the Euclid time-based fork is in line with existing time-based forks. No immediate concerns.
669-671: GenesisStateRoot *common.Hash in ScrollConfig.
Well-defined pointer for capturing the MPT-based genesis state root. Suits the new requirements.
1116-1116: IsEuclid integration into Rules.
Adding the boolean is consistent with the new time-based fork logic.
core/types/state_account.go (1)
34-35: RLP Ignoring PoseidonCodeHash and CodeSize.
Marking these fields with rlp:"-" prevents them from being included in the main account encoding. This is correct if they’re externally derived or not part of the consensus object.
core/stateless/encoding.go (1)
1-76: New file for witness encoding/decoding.
- License & Package Declarations (lines 1-18): No issues. The license header and package declaration are standard.
- Imports (lines 19-24): Straightforward; includes
rlpandtypes. toExtWitness/fromExtWitness(lines 26-40, 42-55): Good separation of the external vs. internal representation. The approach of converting maps to slices and vice versa is clean.- RLP Encode/Decode (lines 57-61, 62-69): Implementation is standard. RLP into an
extWitnessintermediate, then converted to/from the internalWitness. This fosters a simpler code path for serialization. extWitnessDefinition (lines 71-76): Straightforward structure for RLP transport.
Overall, the new file cleanly implements witness serialization with no evident performance or correctness issues.
core/stateless/database.go (2)
19-24: Consider grouping imports logically, or adding minimal usage comments.
Imports are quite straightforward here. Just ensure each imported package is used consistently throughout the file and keep them minimal if possible.
36-67: Ensure consistent error checking when writing to the ephemeral DB.
Currently, the code writes headers, codes, and node data without checking for potential errors from the rawdb layer. If any write fails, an error would remain silent. Consider either:
- Handling the returned errors from
WriteHeader,WriteCode, andWriteTrieNode, or - Logging them if complete error handling is not required.
core/rawdb/accessors_state.go (1)
98-110: Handle partial database writes more gracefully.
WriteDiskStateRoot and ReadDiskStateRoot rely on db.Put/db.Get but do not consider partial writes or concurrent usage. If concurrency is expected, ensure your underlying DB can handle it, or protect these calls with appropriate synchronization.
trie/tracer.go (3)
66-75: Handle reinserted and resurrected nodes with caution.
The logic for removing an item from deletes if it is resurrected seems correct. Just ensure no unexpected concurrency issues occur if multiple insert/delete calls overlap.
95-106: Deep copy overhead and memory usage.
When copy() is called, a full deep copy is performed, which could be expensive for large tries. Verify that copy() won't degrade performance in large-scale usage or that usage patterns remain minimal.
108-122: Confirm filtering in deletedNodes if embedded nodes never appear in accessList.
Some embedded node deletions might not be recognized as “formerly accessible.” Confirm the behavior is correct when partial expansions occur and not all nodes have been read.
trie/secure_trie.go (2)
193-193: Ensure deep copying of the tracer.
Copying the tracer is beneficial for preserving the original SecureTrie's state. Verify that t.trie.tracer.copy() performs a deep copy to avoid data races if shared among multiple SecureTrie instances.
226-229: Document concurrency implications for the new Witness method.
Returning a map of accessed nodes is useful for debugging or proof generation. However, consider clarifying concurrency recommendations in the method’s docstring to ensure thread-safety or potential usage constraints.
core/block_validator.go (1)
229-230: Verify correctness of the state root validation toggle.
shouldValidateStateRoot := v.config.Scroll.UseZktrie != v.config.IsEuclid(header.Time) introduces a conditional skip of state root checks. Confirm that skipping validation is intentional, as it may allow inconsistent states when certain flags or time conditions are met.
core/rawdb/schema.go (2)
130-130: Consider prefix collisions.
While diskStateRootPrefix is short and descriptive, ensure no existing prefix collisions occur. Double-check that keys using this prefix won’t inadvertently overlap with others.
318-320: Function usage verification.
diskStateRootKey cleanly appends diskStateRootPrefix and headerRoot. Verify that all call sites (for example, WriteDiskStateRoot, ReadDiskStateRoot) handle errors or empty roots consistently.
internal/web3ext/web3ext.go (2)
485-491: Validate input for executionWitness.
Adding the new debug_executionWitness method is helpful. Consider clarifying input usage or adding front-end checks, since inputFormatter: [null] does not enforce specific validations.
952-958: Check block number range in diskRoot.
diskRoot uses inputDefaultBlockNumberFormatter, which presumably handles typical edge cases. Validate that negative or extremely large values are handled gracefully downstream.
trie/database.go (1)
670-672: Validate behavior on read error.
Here, if rawdb.ReadDiskStateRoot returns an error, the logic silently falls back to the original node without any logs. This may be intentional, but consider adding debug logs upon errors to diagnose potential mismatches between the in-memory and disk state roots.
core/blockchain.go (2)
1683-1683: Check for consistent usage of the additional parameter.
Calling statedb.StartPrefetcher("chain", nil) clarifies that no witness data is supplied. Ensure all code references expecting a non-nil pointer handle this case gracefully to avoid runtime errors or unexpected prefetch behavior.
[approve]
1820-1820: Confirm witness parameter usage.
Similarly, statedb.StartPrefetcher("l1sync", nil) is consistent with the prior pattern. Verify that no code path assumes a valid, non-nil witness, preventing possible nil pointer dereferences if the parameter is used in the future.
core/genesis.go (1)
325-328: Ensure consistency of configured genesis state root.
If GenesisStateRoot is not nil but does not match the computed root, there may be confusion about which root is canonical. Double-check that advanced scenarios involving custom or pre-computed state roots handle discrepancies correctly, and consider logging or validating that the stored root matches the actual state root.
cmd/utils/flags.go (2)
186-189: LGTM: Clear and well-defined flag.
The new flag follows the existing pattern and clearly describes its purpose.
1886-1893: LGTM: Proper configuration for Scroll Sepolia network.
The code correctly:
- Sets UseZktrie based on the inverse of ScrollMPTFlag
- Enforces archive mode when Zktrie is used
- Provides clear error messages and logging
omerfirmak
force-pushed
the
omerfirmak/mpt
branch
from
bc9dc5d to
bef84a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
core/state/state_test.go (2)
158-159: Use consistent naming for clarity
The variablesstateDbandstatemay lead to confusion when used interchangeably with other references to “database” vs. “state” in the codebase. Consider renaming them for clarity, for instance,dbandstateDB.
233-234: Consider preemptive code caching
CallingCodeSize(db)repeatedly could introduce overhead if done in bigger loops. For tests, it’s likely negligible, but in production usage, you might consider caching or storing code sizes once fetched.core/stateless/database.go (1)
36-67: Add error handling or guard rails for empty or nil witness
MakeHashDB()proceeds assumingw.Headers,w.Codes, andw.Stateare valid and non-nil. Ifwis partially initialized or empty, the code might silently succeed or produce unexpected results. Consider adding safeguards or returning an error if the witness is incomplete.core/rawdb/accessors_state.go (1)
98-110: Check for potential collisions or overwrites
When writing disk state roots using the parent header’s hash as the key, ensure that no collisions with other processes or future expansions in the DB scheme can occur. Also, consider handling partial writes or error states more gracefully instead of callinglog.Crit.core/stateless/witness.go (1)
17-47: Document concurrency rules
TheWitnessstruct includes async.Mutexfor concurrency. Document which methods are safe to call concurrently (e.g., “All public methods are concurrency-safe after initialization.”) to avoid confusion for other maintainers.core/state/database.go (1)
142-144: Consider explicit error handlingWhile the error handling works, consider being more explicit about the expected error types and logging relevant information for debugging purposes.
-if diskRoot, err := rawdb.ReadDiskStateRoot(db.db.DiskDB(), root); err == nil { +diskRoot, err := rawdb.ReadDiskStateRoot(db.db.DiskDB(), root) +if err != nil { + log.Debug("No disk state root found", "root", root, "err", err) +} else { root = diskRoot }trie/zk_trie_test.go (2)
296-316: Refactor test helper for better maintainabilityThe
checkTrieEqualityfunction has multiple responsibilities and could be split into smaller, more focused functions.Consider breaking it down:
- A function to initialize tries
- A function to compare tries
- A function to track progress
This would improve readability and make the code easier to maintain.
318-330: Add error cases to account equality checksThe account equality check only verifies the happy path. Consider adding test cases for:
- Malformed account data
- Missing accounts
- Different account types
eth/api.go (2)
339-394: Consider adding memory management for large blocks.The witness generation and testing process could consume significant memory for blocks with many state changes. Consider adding a warning log when processing large blocks.
func generateWitness(blockchain *core.BlockChain, block *types.Block) (*stateless.Witness, error) { + if block.Transactions().Len() > 1000 { + log.Warn("Generating witness for large block", "number", block.Number(), "txs", block.Transactions().Len()) + } witness, err := stateless.NewWitness(block.Header(), blockchain)
396-424: Consider adding version field to ExecutionWitness struct.Adding a version field would help with future compatibility and migrations.
type ExecutionWitness struct { + Version uint64 `json:"version"` Headers []*types.Header `json:"headers"` Codes map[string]string `json:"codes"` State map[string]string `json:"state"` }core/state/statedb.go (1)
166-174: Consider adding witness cleanup in StopPrefetcher.The witness field should be cleared when stopping the prefetcher to prevent memory leaks.
func (s *StateDB) StopPrefetcher() { if s.prefetcher != nil { s.prefetcher.close() s.prefetcher = nil } + s.witness = nil }cmd/utils/flags.go (1)
1905-1912: Consider refactoring the duplicated Zktrie configuration logic.The Zktrie configuration logic is duplicated between Scroll Sepolia and Scroll mainnet. Consider extracting this into a helper function to improve maintainability.
+func configureZktrie(ctx *cli.Context, cfg *ethconfig.Config) { + cfg.Genesis.Config.Scroll.UseZktrie = !ctx.GlobalBool(ScrollMPTFlag.Name) + if cfg.Genesis.Config.Scroll.UseZktrie { + // disable pruning + if ctx.GlobalString(GCModeFlag.Name) != GCModeArchive { + log.Crit("Must use --gcmode=archive") + } + log.Info("Pruning disabled") + cfg.NoPruning = true + } +}Then use it in both network configurations:
case ctx.GlobalBool(ScrollSepoliaFlag.Name): // ... existing configuration ... - cfg.Genesis.Config.Scroll.UseZktrie = !ctx.GlobalBool(ScrollMPTFlag.Name) - if cfg.Genesis.Config.Scroll.UseZktrie { - // disable pruning - if ctx.GlobalString(GCModeFlag.Name) != GCModeArchive { - log.Crit("Must use --gcmode=archive") - } - log.Info("Pruning disabled") - cfg.NoPruning = true - } + configureZktrie(ctx, cfg) case ctx.GlobalBool(ScrollFlag.Name): // ... existing configuration ... - cfg.Genesis.Config.Scroll.UseZktrie = !ctx.GlobalBool(ScrollMPTFlag.Name) - if cfg.Genesis.Config.Scroll.UseZktrie { - // disable pruning - if ctx.GlobalString(GCModeFlag.Name) != GCModeArchive { - log.Crit("Must use --gcmode=archive") - } - log.Info("Pruning disabled") - cfg.NoPruning = true - } + configureZktrie(ctx, cfg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
cmd/geth/main.go(1 hunks)cmd/geth/usage.go(1 hunks)cmd/utils/flags.go(3 hunks)core/block_validator.go(1 hunks)core/blockchain.go(3 hunks)core/genesis.go(1 hunks)core/rawdb/accessors_state.go(1 hunks)core/rawdb/schema.go(2 hunks)core/state/database.go(2 hunks)core/state/state_object.go(2 hunks)core/state/state_test.go(4 hunks)core/state/statedb.go(8 hunks)core/stateless/database.go(1 hunks)core/stateless/encoding.go(1 hunks)core/stateless/witness.go(1 hunks)core/types/state_account.go(1 hunks)eth/api.go(3 hunks)internal/web3ext/web3ext.go(2 hunks)light/trie.go(1 hunks)miner/scroll_worker.go(1 hunks)params/config.go(8 hunks)rollup/pipeline/pipeline.go(1 hunks)trie/database.go(2 hunks)trie/secure_trie.go(2 hunks)trie/tracer.go(1 hunks)trie/trie.go(9 hunks)trie/zk_trie.go(1 hunks)trie/zk_trie_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- cmd/geth/usage.go
- cmd/geth/main.go
- core/types/state_account.go
- miner/scroll_worker.go
- core/rawdb/schema.go
- core/block_validator.go
- light/trie.go
- trie/zk_trie.go
- core/genesis.go
- trie/database.go
- trie/trie.go
- trie/tracer.go
- core/stateless/encoding.go
- core/blockchain.go
🔇 Additional comments (19)
core/state/state_test.go (2)
205-205: Validate return errors when calling helper functions
Ensure that compareStateObjects(...) provides meaningful diagnostics on error scenarios. If it must fail the test, consider returning an error or capturing additional context about the failing data.
214-214: Check for concurrency
compareStateObjects might be shared among parallel tests. Ensure no concurrency hazards exist (e.g., shared state or external references) that might cause inconsistent test behavior.
core/stateless/witness.go (2)
49-69: Validate parent header retrieval
NewWitness relies on retrieving the parent header from the chain. If the chain’s data is missing or corrupted, you return an error. Ensure you handle additional corner cases, such as the case when context.Number is zero (genesis block).
90-99: Add concurrency tests
AddState uses a lock to combine MPT nodes. It would be beneficial to add targeted concurrency tests to confirm that different goroutines adding states simultaneously do not cause race conditions or inconsistencies.
core/state/database.go (1)
108-110: LGTM: Clean interface extension
The new Witness() method is a well-defined addition to the Trie interface, providing a clear contract for implementations to expose accessed trie nodes.
trie/secure_trie.go (2)
193-193: LGTM: Essential tracer state preservation
Correctly preserves the tracer state during trie copying, which is essential for maintaining witness tracking functionality.
226-229: LGTM: Clean delegation pattern
The Witness implementation correctly delegates to the underlying trie's implementation, maintaining the abstraction hierarchy.
trie/zk_trie_test.go (1)
332-338: LGTM: Clean storage equality check
The storage equality check is well-implemented and handles the RLP encoding correctly.
rollup/pipeline/pipeline.go (1)
231-231: LGTM: StartPrefetcher parameter update
The addition of the nil parameter aligns with the witness parameter addition to StartPrefetcher. Since this is in a mining context, not collecting witness data is appropriate.
core/state/state_object.go (1)
503-514: LGTM: Improved CodeSize implementation
The changes improve the implementation by:
- Adding proper database dependency injection
- Following a clear size calculation hierarchy
- Enhancing error handling with detailed messages
internal/web3ext/web3ext.go (2)
485-491: LGTM: Added debug_executionWitness method
The new method properly extends the debug namespace with execution witness functionality. The use of null input formatter is appropriate for raw block identifier input.
952-958: LGTM: Added scroll_diskRoot method
The new method properly extends the scroll namespace with disk root retrieval functionality. The use of inputDefaultBlockNumberFormatter is appropriate for block number parameter.
eth/api.go (1)
326-337: LGTM: ExecutionWitness implementation is robust and well-structured.
The function properly handles error cases and provides clear error messages.
core/state/statedb.go (1)
110-112: LGTM: Clean witness field addition.
The witness field is properly documented and integrated into the StateDB struct.
params/config.go (3)
40-41: LGTM: Genesis state constants are properly defined.
The genesis state hashes are clearly defined as constants.
899-902: LGTM: Euclid fork timing implementation follows established patterns.
The IsEuclid implementation matches other time-based fork checks.
669-671: Consider adding validation for GenesisStateRoot.
The GenesisStateRoot field should be validated when used in chain initialization.
cmd/utils/flags.go (2)
186-189: LGTM! The flag definition is clear and well-structured.
The new ScrollMPTFlag follows the established pattern for boolean flags and provides clear usage documentation.
1886-1893: LGTM! The Scroll Sepolia network configuration is correct.
The code properly configures Zktrie usage and enforces the required archive mode when Zktrie is enabled. Note that UseZktrie is set to the inverse of ScrollMPTFlag, which is logical as the flag controls MPT usage, not Zktrie usage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rollup/ccc/async_checker.go (1)
101-105: Add debug-level logging to clarify skipping of Euclid blocks.Currently, the CCC check is silently skipped for blocks identified as Euclid, which can obscure debugging if issues arise after this fork transition. Consider adding a log statement to more clearly indicate that the block is being skipped.
params/config.go (1)
899-903: Fix doc comment to reference Euclid instead of Darwin.The doc block incorrectly refers to Darwin blocks, but the function checks EuclidTime. Update the comment for clarity and correctness.
-// IsEuclid returns whether num is either equal to the Darwin fork block or greater. +// IsEuclid returns whether the current timestamp is after the Euclid fork time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
miner/scroll_worker.go(3 hunks)params/config.go(8 hunks)rollup/ccc/async_checker.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- miner/scroll_worker.go
🔇 Additional comments (6)
params/config.go (6)
32-41: No concerns about newly introduced genesis hashes.The definitions for additional genesis hashes and states appear correct. Ensure they are tracked accurately in any required documentation or test coverage.
345-345: Valid pointer assignment for ScrollSepoliaChainConfig.This reference to ScrollSepoliaGenesisState looks correct. No further concerns.
386-386: Valid pointer assignment for ScrollMainnetChainConfig.This reference to ScrollMainnetGenesisState looks correct. No further concerns.
669-671: GenesisStateRoot field looks properly declared.This field is consistent with other Scroll config parameters. No immediate concerns about correctness or usage.
Line range hint
1116-1142: New “IsEuclid” Rules field is properly added.The logic for setting and returning
IsEuclidmirrors existing patterns for previous forks. Looks good.
640-640: Verify alignment with other time-based forks.Introducing the EuclidTime field aligns well with other time-based forks, but ensure it’s consistently checked across the codebase (e.g., anywhere Darwin is used) to avoid potential misconfigurations.
✅ Verification successful
Let me verify the usage of fork times further by checking for any fork-specific logic implementations.
Fork time implementation is consistent and properly integrated
The verification shows that
EuclidTimeis well-aligned with other time-based forks in the codebase:
- The implementation follows the same pattern as other forks (Darwin, DarwinV2) with consistent helper methods (
IsEuclid,IsDarwin, etc.)- The fork time is properly integrated into the chain rules and used in critical components:
rollup/ccc/async_checker.go: Handles Euclid-specific MPT logicminer/scroll_worker.go: Manages state commitment based on Euclid activationcore/block_validator.go: Validates state roots considering Euclid's requirementscore/vm/interpreter.go: Fork-specific instruction sets are properly handledThe implementation maintains consistency with existing fork-time patterns and is correctly utilized across relevant components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # This script searches for references to DarwinTime or EuclidTime throughout the repository # to verify consistent usage of fork time logic. rg -A 3 "DarwinTime|EuclidTime"Length of output: 3585
Script:
#!/bin/bash # Search for fork-specific rules or implementations ast-grep --pattern 'func $_($$) bool { return isForked$$$ }' # Also check for any direct comparisons or checks of these times rg "IsDarwin|IsEuclid" -A 3Length of output: 2351
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
eth/api.go (2)
340-367: Consider optimizing witness generation performanceThe
generateWitnessfunction includes proper error handling and validation. However, consider the following improvements:
- Add context parameter for cancellation support
- Consider caching the witness to avoid regeneration
- Document the special handling of
WithdrawTrieRootSlotfor maintainability-func generateWitness(blockchain *core.BlockChain, block *types.Block) (*stateless.Witness, error) { +func generateWitness(ctx context.Context, blockchain *core.BlockChain, block *types.Block) (*stateless.Witness, error) { witness, err := stateless.NewWitness(block.Header(), blockchain) if err != nil { return nil, fmt.Errorf("failed to create witness: %w", err) } + // Special handling: Collect storage for withdrawal processing statedb.GetState(rcfg.L2MessageQueueAddress, rcfg.WithdrawTrieRootSlot)
969-972: Add documentation for DiskAndHeaderRoot structureConsider adding documentation explaining:
- The purpose of each field
- When they might differ
- Usage scenarios
+// DiskAndHeaderRoot contains both the disk state root and header state root. +// They may differ when the state is stored in a different format on disk +// compared to how it's represented in the block header. type DiskAndHeaderRoot struct { + // DiskRoot is the root hash of the state as stored on disk DiskRoot common.Hash `json:"diskRoot"` + // HeaderRoot is the root hash as stored in the block header HeaderRoot common.Hash `json:"headerRoot"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/api.go(3 hunks)
🔇 Additional comments (5)
eth/api.go (5)
37-37: LGTM: Required imports added for new functionalityThe new imports for
statelessandrcfgpackages are necessary for the implementation of execution witness and disk root functionality.Also applies to: 44-44
327-338: LGTM: Well-structured API method with proper error handlingThe
ExecutionWitnessmethod properly handles block retrieval and error cases. The error messages are descriptive and helpful for debugging.
400-428: LGTM: Well-designed witness data structuresThe
ExecutionWitnessstructure and conversion utilities are well-designed:
- Clear JSON field tags
- Efficient map transformations
- Good documentation explaining future considerations
974-994: LGTM: Well-implemented root retrievalThe
DiskRootmethod properly handles:
- Block retrieval errors
- Missing block cases
- Fallback to header root when disk root is not available
369-398: Verify witness testing implementationThe
testWitnessfunction performs a complete reprocessing of the block to validate the witness. This could be computationally expensive for large blocks.Consider implementing a more efficient validation strategy for production environments, such as:
- Sampling-based validation
- Periodic validation instead of per-block
- Configurable validation depth
1. Purpose or design rationale of this PR
best reviewed commit by commit.
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
Based on the comprehensive summary, here are the updated release notes:
New Features
Performance Improvements
Configuration Updates
Debugging Capabilities
Blockchain Compatibility
These release notes capture the high-level changes without revealing implementation details, focusing on user-facing improvements and new capabilities.