pbss implemention #161
pbss implemention #161
Conversation
core/state/state_object.go
Outdated
| addrHash := s.addrHash | ||
| if db.TrieDB().IsMorphZk() { | ||
| addr_s, _ := zkt.ToSecureKey(s.address.Bytes()) | ||
| addrHash = common.BigToHash(addr_s) |
There was a problem hiding this comment.
If addrHash is only for storage, use s.addrHash directly without reassigning.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
trie/database.go (4)
79-80: Add doc comments for clarity
Consider adding brief documentation explaining the usage of bothZktrieandPathZkTriefields for maintainability.
299-299: Global variable usage
Using a top-level variable likeGenesisStateInPathZkTriecan sometimes lead to confusion. Consider placing it in the config or a struct if you anticipate modifications or concurrency concerns.
526-534: Typographical issue
There's a misspelling in the error message: “Databse” should be “Database.”- log.Error("Databse not support reference") + log.Error("Database does not support reference")
565-574: Typographical issue
Similarly, correct the spelling of “Databse” in the “not support dereference” log.- log.Error("Databse not support dereference") + log.Error("Database does not support dereference")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
trie/database.go(14 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
trie/database.go (6)
trie/path_zk_trie.go (1)
PathZkTrie(35-38)triedb/hashdb/zk_trie_database.go (3)
Config(19-22)NewZkDatabaseWithConfig(43-62)ZktrieDatabase(33-41)core/state/database.go (1)
Database(43-61)core/state/statedb.go (1)
New(129-157)core/rawdb/accessors_trie.go (1)
HashScheme(40-40)core/rawdb/schema.go (1)
CompactStorageTrieNodeKey(349-356)
🔇 Additional comments (24)
trie/database.go (24)
23-23: Import for big.Int usage
No concerns here; the addition of"math/big"is appropriate for handling large integer operations.
37-40: New library imports
These imports forhashdb,pathdb, andzktrieappear to align well with the added ZkTrie functionality. No issues identified.
103-103: New backend field
No immediate concerns. This field enables the choice of different trie database backends.
289-298: Expanded configuration structure
The additional fields and references toHashDBandPathDBlook good. Configuration usage should be straightforward.
301-307: Reader interface
Introducing a dedicatedReaderinterface for node retrieval is a clear abstraction. No issues found.
309-331: backend interface
Defining a consistent interface for multiple storage schemes (hash-based vs path-based) is a clean approach.
340-342: Convenience constructor
NewZkDatabasecorrectly delegates toNewDatabaseWithConfig(...)enablingZktrie.
347-370: Zktrie initialization
ModifyingNewDatabaseWithConfigto handle ZkTrie options and path-based configuration is straightforward. No logical issues detected.
371-383: Configuration conflict check
Raising a critical error if bothHashDBandPathDBare set prevents misconfiguration. Implementation is appropriate.
650-663: Delegated Cap method
The logic for delegating the Cap operation to a ZkTrie backend is properly structured. No concerns.
757-766: Delegated commit logic
Custom logic for zktrie commit is properly separated. No immediate concerns.
1001-1011: Backend-based Reader dispatch
Switching on the backend type to return a custom Reader is correctly implemented.
1013-1022: CommitState delegation
Method defers state commits to the underlying backend. Implementation is consistent.
1024-1030: Scheme retrieval
Falls back nicely torawdb.HashSchemeif no backend is set. No issues found.
1032-1044: Close method
Flushing preimages and then closing the backend is appropriate for clean resource release.
1046-1054: Conditional preimage retrieval
Implementation aligns with the presence or absence of a preimage store. No issues.
1055-1068: pathdb Journal
The specializedJournallogic is path-based only. The fallback error is a good safeguard.
1070-1072: ZkTrie indicators
Helper methods are straightforward for checking ZkTrie usage.
1073-1084: Concurrent UpdatePreimage
This mirrors the previous feedback on potential concurrency issues.
1085-1091: Put method
Delegates storage operations to the backend. No particular concerns noted.
1092-1101: Get method
Properly delegates retrieval to the Zktrie backend or returns “not supported” for others.
1103-1128: GetFrom method
Processes reversed root logic for pathdb usage and handles missing keys with azktrie.ErrKeyNotFound. Implementation looks consistent.
743-749:❓ Verification inconclusive
CommitGenesis usage
Callsdb.backend.CommitGenesisbefore final commit. Ensure path-based databases also implement this gracefully or handle any failure scenario.
🏁 Script executed:
#!/bin/bash # Verify CommitGenesis implementation in pathdb fd --hidden --type f --exec rg '^func\s+\(\s*\*Database\s*\)\s+CommitGenesis\s*\('Length of output: 84
Action: Verify graceful handling of CommitGenesis in path‐based databases
The
CommitGenesismethod intrie/database.gocallsdb.backend.CommitGenesis(root)before proceeding with the final commit. Our initial script did not locate any explicit implementations of this method—especially in contexts that might relate to path‑based databases—which raises concerns about whether such backends handle errors or missing implementations gracefully.Please manually verify that:
- If a path‑based database is used as the backend, it either implements its own version of
CommitGenesis(with proper error handling) or provides a safe fallback (such as a no‑op) so that failures do not cascade.- The overall commit flow in
db.CommitGenesisproperly manages any errors returned fromdb.backend.CommitGenesis(root).
505-508:❓ Verification inconclusive
Nodes() method unsupported with backend
Since this call panics whendb.backendis defined, please verify that no upstream calls attempt to invokeNodes()under those conditions.
🏁 Script executed:
#!/bin/bash # Searches for any usage of the Nodes() method to verify no code calls it when db.backend != nil. fd --hidden --type f --exec rg 'Nodes\s*\('Length of output: 11174
Action Required: Confirm Safe Invocation of
Nodes()Method When a Backend Is ConfiguredThe
Nodes()method intentionally panics ifdb.backendis non-nil. Our automated search revealed many instances whereNodes()is called, but the context doesn’t conclusively show that these calls occur only whendb.backendis nil. Please manually verify that all upstream invocations ofNodes()are correctly guarded so that none of them execute under conditions where a backend is active.Key areas to review:
- The safeguard in
trie/database.go(lines 505–508).- All call sites in both production and test code that invoke
Nodes(), ensuring they do not trigger the panic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eth/backend.go (1)
143-146: Consider adding documentation about GenesisStateInPathZkTrie.While the code correctly sets the flag based on the StateScheme, there's no documentation explaining what this flag does or its implications. Consider adding a comment explaining its purpose and effects.
// Set path trie tag before using trie db if config.StateScheme == rawdb.PathScheme { + // Enable path-based trie for genesis state when using PathScheme trie.GenesisStateInPathZkTrie = true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
eth/backend.go(3 hunks)
🔇 Additional comments (5)
eth/backend.go (5)
57-57: Verify the trie package import is used appropriately.The addition of the trie package import supports the new feature for setting the
GenesisStateInPathZkTrieflag when using the path scheme. This aligns with the changes introduced below.
126-128: Default value configuration is properly implemented.Setting a default value for
JournalFileNameensures the configuration is always valid, which is a good practice for handling optional configuration parameters.
138-141: StateScheme parsing with proper error handling.The implementation correctly parses the StateScheme from the database and handles potential errors. This ensures the system will fail gracefully if there are issues with state scheme configuration.
207-209: New cache configuration fields properly populated.The additional CacheConfig fields are correctly populated from the configuration object, allowing the system to use these settings for state management operations.
125-147:✅ Verification successful
Verify all components utilizing StateScheme are updated.
These configuration changes around StateScheme, JournalFileName, and PathSyncFlush appear to be part of a larger feature implementation. Ensure all components that interact with these settings are updated appropriately.
🏁 Script executed:
#!/bin/bash # Search for components that might need to be aware of StateScheme echo "Searching for StateScheme usages:" rg "StateScheme" --type go echo -e "\nSearching for JournalFileName usages:" rg "JournalFileName" --type go echo -e "\nSearching for PathSyncFlush usages:" rg "PathSyncFlush" --type go echo -e "\nSearching for GenesisStateInPathZkTrie usages:" rg "GenesisStateInPathZkTrie" --type goLength of output: 3825
Review Approved: Consistent Updates for StateScheme and Related Settings
The search confirmed that updates to StateScheme, JournalFileName, and PathSyncFlush—as well as the conditional assignment of
trie.GenesisStateInPathZkTriewhen usingrawdb.PathScheme—are properly reflected across the codebase. Specifically:
- StateScheme: Verified usage in
core/blockchain.go,core/rawdb/accessors_trie.go,eth/ethconfig/config.go, and CLI flag handling in the cmd utilities.- JournalFileName: Found in both
eth/backend.goandeth/ethconfig/config.go, ensuring defaults are correctly set.- PathSyncFlush: Consistently applied in
eth/backend.go,eth/ethconfig/config.go, andcore/blockchain.go.- GenesisStateInPathZkTrie: Properly set in
eth/backend.goand confirmed incore/genesis.goand elsewhere.No additional issues were detected, so the changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eth/backend.go (1)
144-146: Consider alternatives to global flag usage.The code uses a global flag
trie.GenesisStateInPathZkTrieto indicate the path trie scheme is active. While functional, global flags can lead to issues with testing and concurrent use in different contexts.Consider refactoring to pass this configuration as a parameter to functions that need it, rather than setting a global flag. This would make the code more modular and easier to test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (6)
cmd/geth/chaincmd.go(2 hunks)cmd/geth/main.go(1 hunks)cmd/utils/flags.go(6 hunks)core/genesis.go(3 hunks)eth/backend.go(5 hunks)eth/ethconfig/config.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmd/geth/main.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/geth/chaincmd.go
- eth/ethconfig/config.go
- core/genesis.go
- cmd/utils/flags.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
eth/backend.go (3)
eth/ethconfig/config.go (1)
Defaults(63-95)core/rawdb/accessors_trie.go (2)
ParseStateScheme(157-181)PathScheme(48-48)trie/database.go (1)
GenesisStateInPathZkTrie(299-299)
🔇 Additional comments (4)
eth/backend.go (4)
57-57: Import addition looks good.Adding the trie package import is necessary to access the
GenesisStateInPathZkTrieflag that will be used later in the file.
126-128: Good defensive programming.Setting a default value for
JournalFileNamewhen it's empty ensures that the application will have a valid journal file path even if the configuration is incomplete.
138-141: State scheme validation looks good.The code correctly uses
rawdb.ParseStateSchemeto validate and set the state scheme, with proper error handling if the parsing fails. This ensures that only valid schemes are used and maintains compatibility with the database.
212-214: Cache configuration extension looks good.The addition of
PathSyncFlush,StateScheme, andJournalFilePathto the cache configuration appropriately extends the configuration structure to support the new state scheme functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/blockchain_l2.go (1)
110-200: Consider adding error handling for edge cases in time-based flushingThe time-based flushing mechanism introduces potential edge cases that aren't fully addressed:
- There's no validation that flushInterval is a reasonable value (not zero or negative)
- The block time comparison doesn't handle out-of-order block timestamps
These edge cases could lead to unexpected flushing behavior in certain scenarios.
- flushInterval := time.Duration(bc.flushInterval.Load()) + flushInterval := time.Duration(bc.flushInterval.Load()) + // Ensure we have a reasonable flush interval + if flushInterval <= 0 { + flushInterval = 30 * time.Second // Default reasonable interval + log.Warn("Invalid flush interval detected, using default", "default", flushInterval) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (3)
core/blockchain.go(7 hunks)core/blockchain_l2.go(3 hunks)eth/ethconfig/config.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- eth/ethconfig/config.go
- core/blockchain.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/blockchain_l2.go (3)
accounts/scwallet/hub.go (1)
Scheme(53-53)core/rawdb/accessors_trie.go (1)
PathScheme(48-48)core/blockchain.go (1)
TriesInMemory(98-98)
🔇 Additional comments (5)
core/blockchain_l2.go (5)
111-113: New variables for block trackingAdded variables to track the current block number and origin root, which will be used with the path-based state storage scheme.
121-122: Time-based flush interval retrievalThe code now loads the flush interval from an atomic value, which allows for runtime configuration of when state changes are flushed to disk.
172-173: Updated flush time allowance conditionThe code now uses the configurable flush interval instead of a fixed value to determine when to flush state to disk for non-archive nodes.
181-182: Updated logging for memory usage warningThe warning log message for excessive memory usage now references the configurable flush interval instead of a hardcoded value.
125-149:✅ Verification successful
Path scheme support for archive nodes
The implementation introduces support for the Path-Based State Storage (PBSS) scheme with a time-based flushing mechanism rather than the immediate flush used for hash-based schemes.
The time-based flushing logic correctly tracks when the last flush occurred and only triggers a new flush when sufficient block time has elapsed, with appropriate logging to indicate when this happens.
🏁 Script executed:
#!/bin/bash # Check if the flushInterval and lastFlushTime fields are properly initialized elsewhere in the codebase # Look for initialization of flushInterval echo "Searching for flushInterval initialization..." rg -A 3 "flushInterval.*Store" --type go # Look for related configuration parameters echo "Searching for flush interval configuration..." rg "PathSyncFlush" --type goLength of output: 814
PBSS flush logic initialization verified
Verified that the time‐based flushing parameters are correctly initialized and wired:
- In core/blockchain.go,
bc.flushInterval.Store(int64(cacheConfig.TrieTimeLimit))sets the flush interval.- In the same file,
bc.lastFlushTime.Store(0)initializes the last‐flush timestamp.- The
PathSyncFlushflag (default true) in eth/ethconfig/config.go and passed through backend.go is correctly applied.No further changes needed.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
triedb/pathdb/database.go (1)
260-271: Underlying DB handle never closedSame as previously flagged:
Close()setsreadOnlybut does not closedb.diskdb, leaking file descriptors.
🧹 Nitpick comments (3)
triedb/pathdb/asyncnodebuffer.go (1)
131-141: No back‑off on repeated flush errorsIf
nc.flushkeeps failing, the goroutine immediately retries in a tight loop.
Addtime.Sleepor exponential back‑off to avoid hammering the storage backend and burning CPU.triedb/pathdb/database.go (1)
290-299: Unused parametergenesisRoot
Initialized(genesisRoot common.Hash)never usesgenesisRoot.
Either utilise it (e.g. compare against disk root) or drop the argument to avoid dead code and confusion.triedb/pathdb/disklayer.go (1)
228-238: Lock upgrade pattern blocks writers
setBufferSizeuses an RLock but calls intobuffer.setSize, which may perform IO and potentially block for a long time.
Consider switching to regularLock()here or doing the heavy work outside the critical section to avoid starving readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (6)
triedb/pathdb/asyncnodebuffer.go(1 hunks)triedb/pathdb/database.go(1 hunks)triedb/pathdb/difflayer.go(1 hunks)triedb/pathdb/disklayer.go(1 hunks)triedb/pathdb/layertree.go(1 hunks)triedb/pathdb/nodebuffer.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- triedb/pathdb/layertree.go
🚧 Files skipped from review as they are similar to previous changes (2)
- triedb/pathdb/difflayer.go
- triedb/pathdb/nodebuffer.go
🔇 Additional comments (1)
triedb/pathdb/asyncnodebuffer.go (1)
74-77: Unsupported API silently discarded
setSizealways returns"not supported"but callers may ignore the error (several paths do). Either implement size‑adjustment or make the error a hard failure (e.g.log.Crit) so mis‑configurations don’t go unnoticed.
| atomic.StoreUint64(&a.current.immutable, 1) | ||
| a.current, a.background = a.background, a.current | ||
|
|
||
| a.isFlushing.Store(true) | ||
| go func(persistID uint64) { | ||
| defer a.isFlushing.Store(false) | ||
| for { | ||
| err := a.background.flush(db, clean, persistID) | ||
| if err == nil { | ||
| log.Debug("Succeed to flush background nodecache to disk", "state_id", persistID) | ||
| return | ||
| } | ||
| log.Error("Failed to flush background nodecache to disk", "state_id", persistID, "error", err) | ||
| } | ||
| }(id) | ||
| return nil |
There was a problem hiding this comment.
Race: goroutine uses mutable a.background reference
After swapping the caches you start a goroutine:
go func(persistID uint64) {
...
err := a.background.flush(...)a.background may change again if another flush is triggered before this goroutine finishes, causing it to flush the new background instead of the intended one, and leaving the first cache un‑flushed.
Capture the pointer you really want to persist:
- atomic.StoreUint64(&a.current.immutable, 1)
- a.current, a.background = a.background, a.current
+ atomic.StoreUint64(&a.current.immutable, 1)
+ a.current, a.background = a.background, a.current
+ bg := a.background // snapshot
- go func(persistID uint64) {
+ go func(nc *nodecache, persistID uint64) {
defer a.isFlushing.Store(false)
for {
- err := a.background.flush(db, clean, persistID)
+ err := nc.flush(db, clean, persistID)
@@
- }
- }(id)
+ }
+ }(bg, id)This eliminates the race and guarantees each goroutine flushes exactly the cache it was spawned for.
| if force { | ||
| for { | ||
| if atomic.LoadUint64(&a.background.immutable) == 1 { | ||
| time.Sleep(time.Duration(DefaultBackgroundFlushInterval) * time.Second) | ||
| log.Info("Waiting background memory table flushed into disk for forcing flush node buffer") | ||
| continue | ||
| } | ||
| atomic.StoreUint64(&a.current.immutable, 1) | ||
| return a.current.flush(db, clean, id) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Busy‑wait inside locked critical section can freeze writers
flush(force=true) spins in a for { … time.Sleep … } loop while holding a.mux.
If the background cache needs more time, every other writer (including commit, node, etc.) is blocked, effectively pausing the whole database.
Consider releasing the lock while waiting or at least adding a bounded timeout / back‑off so the system remains responsive under heavy load.
| // only 1 entity, state have no changes | ||
| // some block maybe has no txns, so state do not change | ||
| if root == parentRoot && len(db.dirties) == 1 { | ||
| if flush { | ||
| db.timeFlush.Store(flush) | ||
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Logic skips commits when len(dirties)==1, risking state loss
The early‑return assumes “no state change” when root==parentRoot && len(dirties)==1.
However a single dirty entry can still contain actual changes (e.g. account nonce bump).
Skipping tree.add in that case loses the mutation.
Please remove the shortcut or base it on len(dirties)==0.
- if root == parentRoot && len(db.dirties) == 1 {
+ if root == parentRoot && len(db.dirties) == 0 {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // only 1 entity, state have no changes | |
| // some block maybe has no txns, so state do not change | |
| if root == parentRoot && len(db.dirties) == 1 { | |
| if flush { | |
| db.timeFlush.Store(flush) | |
| } | |
| return nil | |
| } | |
| // only 1 entity, state have no changes | |
| // some block maybe has no txns, so state do not change | |
| - if root == parentRoot && len(db.dirties) == 1 { | |
| + if root == parentRoot && len(db.dirties) == 0 { | |
| if flush { | |
| db.timeFlush.Store(flush) | |
| } | |
| return nil | |
| } |
| func (dl *diskLayer) commit(bottom *diffLayer, force, timeFlush bool) (*diskLayer, error) { | ||
| dl.lock.Lock() | ||
| defer dl.lock.Unlock() | ||
|
|
||
| // Construct and store the state history first. If crash happens after storing | ||
| // the state history but without flushing the corresponding states(journal), | ||
| // the stored state history will be truncated from head in the next restart. | ||
|
|
||
| // Mark the diskLayer as stale before applying any mutations on top. | ||
| dl.stale = true | ||
|
|
||
| // Store the root->id lookup afterwards. All stored lookups are identified | ||
| // by the **unique** state root. It's impossible that in the same chain | ||
| // blocks are not adjacent but have the same root. | ||
| if dl.id == 0 { | ||
| rawdb.WriteStateID(dl.db.diskdb, dl.root, 0) | ||
| } | ||
| rawdb.WriteStateID(dl.db.diskdb, bottom.rootHash(), bottom.stateID()) | ||
|
|
||
| // Construct a new disk layer by merging the nodes from the provided diff | ||
| // layer, and flush the content in disk layer if there are too many nodes | ||
| // cached. The clean cache is inherited from the original disk layer. | ||
|
|
||
| ndl := newDiskLayer(bottom.root, bottom.stateID(), dl.db, dl.cleans, dl.buffer.commit(bottom.nodes)) | ||
|
|
||
| if err := ndl.buffer.flush(ndl.db.diskdb, ndl.cleans, ndl.id, force, timeFlush); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Stale flag set before flush – inconsistent state on failure
commit marks the layer as stale before flushing buffered nodes.
If the subsequent flush fails, the layer is unusable yet the data is not persisted; additionally the root→id mapping has already been written, leading to on‑disk inconsistencies.
Move dl.stale = true (or markStale()) only after a successful flush, or revert it on error.
| key := path | ||
| if dl.cleans != nil { | ||
| if blob := dl.cleans.Get(nil, key); len(blob) > 0 { | ||
| cleanHitMeter.Mark(1) | ||
| cleanReadMeter.Mark(int64(len(blob))) | ||
| return blob, nil | ||
| } | ||
| cleanMissMeter.Mark(1) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Aliasing caller memory in clean cache may cause corruption
You store path directly as the cache key:
key := path
dl.cleans.Set(key, n)path is a slice owned by the caller; if the caller reuses the backing array, the key mutates inside the cache, breaking look‑ups and possibly violating fastcache’s invariants.
Clone the slices before use:
- key := path
+ key := append([]byte(nil), path...)
...
- dl.cleans.Set(key, n)
+ dl.cleans.Set(key, append([]byte(nil), n...))
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
triedb/pathdb/database.go (2)
226-236:⚠️ Potential issueEarly‑return still risks discarding real state changes
(Already mentioned in a previous review.)A single dirty entry can hide real mutations (e.g. an account nonce bump). Skipping
tree.addwhenlen(db.dirties)==1can therefore lose state.- if root == parentRoot && len(db.dirties) == 1 { + if root == parentRoot && len(db.dirties) == 0 { ... }
262-276: 🛠️ Refactor suggestionClose the underlying on‑disk DB to prevent FD leaks
(Same issue flagged earlier.)
Close()only marks the DB read‑only and purges caches, but never closesdb.diskdb.
Long‑running processes that repeatedly open pathdb instances will leak file handles.func (db *Database) Close() error { @@ - db.tree.bottom().resetCache() - + db.tree.bottom().resetCache() + + if closer, ok := db.diskdb.(io.Closer); ok { + if err := closer.Close(); err != nil { + return err + } + } return nil }
| func (db *Database) CommitGenesis(root common.Hash) error { | ||
| log.Info("pathdb write genesis state to disk", "root", root.Hex()) | ||
| batch := db.diskdb.NewBatch() | ||
| for _, v := range db.dirties { | ||
| batch.Put(v.K, v.V) | ||
| } | ||
| for k := range db.dirties { | ||
| delete(db.dirties, k) | ||
| } | ||
| if err := batch.Write(); err != nil { | ||
| return err | ||
| } | ||
| batch.Reset() | ||
|
|
||
| // Update stateID | ||
| rawdb.WriteStateID(db.diskdb, root, 0) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Guard CommitGenesis with the same write‑lock & read‑only check used elsewhere
CommitGenesis mutates shared state (db.dirties, writes to db.diskdb) without taking db.lock.
If a concurrent Put / CommitState call is in flight the map ‑ or even Pebble’s batch – can be corrupted and data may be lost.
The method also bypasses modifyAllowed, so it will still write even after the DB was switched to read‑only during shutdown.
func (db *Database) CommitGenesis(root common.Hash) error {
- log.Info("pathdb write genesis state to disk", "root", root.Hex())
+ db.lock.Lock()
+ defer db.lock.Unlock()
+
+ if err := db.modifyAllowed(); err != nil {
+ return err
+ }
+
+ log.Info("pathdb write genesis state to disk", "root", root.Hex())
batch := db.diskdb.NewBatch()
@@
- for k := range db.dirties {
- delete(db.dirties, k)
- }
+ // Replace the entire map instead of deleting in‑place; this is cheaper
+ // and avoids iterator invalidation.
+ db.dirties = make(dbtypes.KvMap)
@@
rawdb.WriteStateID(db.diskdb, root, 0)
return nil
}| // DetermineJournalTypeForReader is used when loading the journal. It loads based on whether JournalKV or JournalFile currently exists. | ||
| func (db *Database) DetermineJournalTypeForReader() JournalType { | ||
| if journal := rawdb.ReadTrieJournal(db.diskdb); len(journal) != 0 { | ||
| return JournalKVType | ||
| } | ||
|
|
||
| if fileInfo, stateErr := os.Stat(db.config.JournalFilePath); stateErr == nil && !fileInfo.IsDir() { | ||
| return JournalFileType | ||
| } | ||
|
|
||
| return JournalKVType | ||
| } | ||
|
|
||
| func (db *Database) DeleteTrieJournal(writer ethdb.KeyValueWriter) error { | ||
| // To prevent any remnants of old journals after converting from JournalKV to JournalFile or vice versa, all deletions must be completed. | ||
| rawdb.DeleteTrieJournal(writer) | ||
|
|
||
| // delete from journal file, may not exist | ||
| filePath := db.config.JournalFilePath | ||
| if _, err := os.Stat(filePath); os.IsNotExist(err) { | ||
| return nil | ||
| } | ||
| errRemove := os.Remove(filePath) | ||
| if errRemove != nil { | ||
| log.Crit("Failed to remove tries journal", "journal path", filePath, "err", errRemove) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Empty JournalFilePath may cause false positives & unintended deletions
Both DetermineJournalTypeForReader and DeleteTrieJournal treat an empty JournalFilePath ("") as current directory.
os.Stat("") reports the working dir as a file exists, leading to:
- Selecting
JournalFileTypeeven though no journal file is configured. - Attempting to delete the working directory on cleanup.
Add an explicit non‑empty check:
- if fileInfo, stateErr := os.Stat(db.config.JournalFilePath); stateErr == nil && !fileInfo.IsDir() {
+ if db.config.JournalFilePath != "" {
+ if fileInfo, stateErr := os.Stat(db.config.JournalFilePath); stateErr == nil && !fileInfo.IsDir() {
return JournalFileType
+ }
}and
- filePath := db.config.JournalFilePath
+ filePath := db.config.JournalFilePath
+ if filePath == "" {
+ return nil
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // DetermineJournalTypeForReader is used when loading the journal. It loads based on whether JournalKV or JournalFile currently exists. | |
| func (db *Database) DetermineJournalTypeForReader() JournalType { | |
| if journal := rawdb.ReadTrieJournal(db.diskdb); len(journal) != 0 { | |
| return JournalKVType | |
| } | |
| if fileInfo, stateErr := os.Stat(db.config.JournalFilePath); stateErr == nil && !fileInfo.IsDir() { | |
| return JournalFileType | |
| } | |
| return JournalKVType | |
| } | |
| func (db *Database) DeleteTrieJournal(writer ethdb.KeyValueWriter) error { | |
| // To prevent any remnants of old journals after converting from JournalKV to JournalFile or vice versa, all deletions must be completed. | |
| rawdb.DeleteTrieJournal(writer) | |
| // delete from journal file, may not exist | |
| filePath := db.config.JournalFilePath | |
| if _, err := os.Stat(filePath); os.IsNotExist(err) { | |
| return nil | |
| } | |
| errRemove := os.Remove(filePath) | |
| if errRemove != nil { | |
| log.Crit("Failed to remove tries journal", "journal path", filePath, "err", errRemove) | |
| } | |
| return nil | |
| } | |
| func (db *Database) DetermineJournalTypeForReader() JournalType { | |
| if journal := rawdb.ReadTrieJournal(db.diskdb); len(journal) != 0 { | |
| return JournalKVType | |
| } | |
| if db.config.JournalFilePath != "" { | |
| if fileInfo, stateErr := os.Stat(db.config.JournalFilePath); stateErr == nil && !fileInfo.IsDir() { | |
| return JournalFileType | |
| } | |
| } | |
| return JournalKVType | |
| } | |
| func (db *Database) DeleteTrieJournal(writer ethdb.KeyValueWriter) error { | |
| // To prevent any remnants of old journals after converting from JournalKV to JournalFile or vice versa, all deletions must be completed. | |
| rawdb.DeleteTrieJournal(writer) | |
| // delete from journal file, may not exist | |
| filePath := db.config.JournalFilePath | |
| if filePath == "" { | |
| return nil | |
| } | |
| if _, err := os.Stat(filePath); os.IsNotExist(err) { | |
| return nil | |
| } | |
| errRemove := os.Remove(filePath) | |
| if errRemove != nil { | |
| log.Crit("Failed to remove tries journal", "journal path", filePath, "err", errRemove) | |
| } | |
| return nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
triedb/pathdb/database.go (5)
194-211:CommitGenesisstill unprotected & ignores read‑only flag
This was raised in a previous review and remains unresolved.
Please acquiredb.lock, checkmodifyAllowed, and close the batch gracefully to prevent concurrent corruption and illegal writes after shutdown.
232-240: Skipping commit whenlen(dirties)==1can drop real state changesA single dirty entry may contain an actual mutation (e.g. nonce bump).
The early‑return risks state loss; uselen(db.dirties)==0instead, or remove the shortcut.Issue previously flagged and still present.
261-274:Closeshould close the underlyingdiskdbto avoid resource leaksRe‑iterating the prior suggestion: attempt a type‑assert to
io.Closerand callClose().
Otherwise file handles remain open.
368-376: EmptyJournalFilePathtreated as current directoryIf
JournalFilePathis"",os.Stat("")will return the working directory, causing false positives.
Add an explicit non‑empty check beforeos.Stat.Previously reported; still pending.
380-394:DeleteTrieJournalmay delete/operate on working directory when path is emptyReturn early when
db.config.JournalFilePath == ""to avoid accidental deletions.
Issue mirrors the reader logic and was highlighted before.
| // Size returns the current storage size of the memory cache in front of the | ||
| // persistent database layer. | ||
| func (db *Database) Size() (diffs common.StorageSize, nodes common.StorageSize, immutableNodes common.StorageSize) { | ||
| db.tree.forEach(func(layer layer) { | ||
| if diff, ok := layer.(*diffLayer); ok { | ||
| diffs += common.StorageSize(diff.memory) | ||
| } | ||
| if disk, ok := layer.(*diskLayer); ok { | ||
| nodes, immutableNodes = disk.size() | ||
| } | ||
| }) | ||
| return diffs, nodes, immutableNodes | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard Size() with a read‑lock
Size() iterates over the layer tree while other goroutines may mutate it, leading to race conditions under the race detector.
Same locking pattern as Reader is recommended:
db.lock.RLock()
defer db.lock.RUnlock()| // Reader retrieves a layer belonging to the given state root. | ||
| func (db *Database) Reader(root common.Hash) (layer, error) { | ||
| l := db.tree.get(root) | ||
| if l == nil { | ||
| return nil, fmt.Errorf("state %#x is not available", root) | ||
| } | ||
| return l, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add read‑lock around Reader to avoid concurrent map/tree hazards
db.tree.get() walks the mutable layer tree without any synchronization.
If a concurrent CommitState, SetBufferSize, Close, etc. mutates the tree while Reader is traversing it, it can lead to data races or panics.
func (db *Database) Reader(root common.Hash) (layer, error) {
- l := db.tree.get(root)
+ db.lock.RLock()
+ defer db.lock.RUnlock()
+
+ l := db.tree.get(root)
if l == nil {
return nil, fmt.Errorf("state %#x is not available", root)
}
return l, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Reader retrieves a layer belonging to the given state root. | |
| func (db *Database) Reader(root common.Hash) (layer, error) { | |
| l := db.tree.get(root) | |
| if l == nil { | |
| return nil, fmt.Errorf("state %#x is not available", root) | |
| } | |
| return l, nil | |
| } | |
| // Reader retrieves a layer belonging to the given state root. | |
| func (db *Database) Reader(root common.Hash) (layer, error) { | |
| db.lock.RLock() | |
| defer db.lock.RUnlock() | |
| l := db.tree.get(root) | |
| if l == nil { | |
| return nil, fmt.Errorf("state %#x is not available", root) | |
| } | |
| return l, nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
triedb/pathdb/database.go (5)
186-192: Add read‑lock aroundReaderto avoid concurrent map/tree races
Readertraversesdb.treewithout any synchronization while other goroutines may mutate the tree (e.g.CommitState,SetBufferSize,Close). This was already pointed out in a previous review and is still unresolved. Re‑use the same locking pattern you applied elsewhere (RLock/RUnlock) to silence the race detector.
282-291: GuardSize()with a read‑lock
Size()walks all layers while other goroutines may mutate them. Add the same read lock used byHead()to prevent races under-race.
236-243:⚠️ Potential issueEarly‑return still skips real state changes when only one dirty entry exists
The shortcutroot == parentRoot && len(db.dirties) == 1treats “single dirty entry” as “no change”. One entry can still hold a valid mutation (e.g. nonce bump). The earlier review suggested basing the check onlen(db.dirties)==0; please apply that change to prevent silent state loss.- if root == parentRoot && len(db.dirties) == 1 { + if root == parentRoot && len(db.dirties) == 0 {
266-278: 🛠️ Refactor suggestion
Closestill leaks file handles – close the underlyingdiskdb
db.Close()switches to read‑only but never callsClose()ondb.diskdb. Any Pebble/LMDB instance keeps OS resources open, which can break tests and long‑running services. Re‑apply the earlier suggestion:func (db *Database) Close() error { @@ - db.tree.bottom().resetCache() + db.tree.bottom().resetCache() + + if closer, ok := db.diskdb.(io.Closer); ok { + if err := closer.Close(); err != nil { + return err + } + } return nil }
388-396:⚠️ Potential issueEmpty
JournalFilePathmay delete the working directory
DeleteTrieJournalstill callsos.Stat/os.Removewith an empty path, which the OS interprets as the current directory. This can lead to catastrophic data loss. Bail out early whenJournalFilePath == "".- filePath := db.config.JournalFilePath + filePath := db.config.JournalFilePath + if filePath == "" { + return nil + }
🧹 Nitpick comments (1)
triedb/pathdb/database.go (1)
194-214: Resetdb.dirtiesinstead of deleting keys one‑by‑one
CommitGenesisloops overdb.dirtiestwice: once to write, once to delete. Replacing the map is cheaper and avoids iterator invalidation pitfalls.batch := db.diskdb.NewBatch() for _, v := range db.dirties { batch.Put(v.K, v.V) } - for k := range db.dirties { - delete(db.dirties, k) - } + db.dirties = make(dbtypes.KvMap)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
triedb/pathdb/database.go (7)
281-292:⚠️ Potential issueGuard
Size()with a read lock
Size()iterates over the layer tree while other goroutines may mutate it, leading to data races.func (db *Database) Size() (diffs common.StorageSize, nodes common.StorageSize, immutableNodes common.StorageSize) { - db.tree.forEach(func(layer layer) { + db.lock.RLock() + defer db.lock.RUnlock() + + db.tree.forEach(func(layer layer) { ... }) return diffs, nodes, immutableNodes }
185-192:⚠️ Potential issueAdd read‑lock around
Readerto avoid data races
db.tree.get()walks the (mutable) layer tree while other goroutines may mutate it viaCommitState,SetBufferSize,Close, etc.
Without a lock this can trigger the Go race detector and produce undefined behaviour in production.func (db *Database) Reader(root common.Hash) (layer, error) { - l := db.tree.get(root) + db.lock.RLock() + defer db.lock.RUnlock() + + l := db.tree.get(root) if l == nil { return nil, fmt.Errorf("state %#x is not available", root) } return l, nil }
389-396:⚠️ Potential issueProtect against accidental deletion when
JournalFilePathis empty
os.Remove("")would attempt to delete “.” (current dir). Add an explicit non‑empty check.- filePath := db.config.JournalFilePath - if _, err := os.Stat(filePath); os.IsNotExist(err) { + filePath := db.config.JournalFilePath + if filePath == "" { + return nil + } + if _, err := os.Stat(filePath); os.IsNotExist(err) { return nil }
236-243:⚠️ Potential issueIncorrect shortcut may drop real state changes
A single dirty entry can still represent an actual state change (e.g. account nonce bump).
Basing the early‑return onlen(db.dirties)==1skips committing such changes and risks state corruption.
The shortcut should only trigger when there are no dirties.- if root == parentRoot && len(db.dirties) == 1 { + if root == parentRoot && len(db.dirties) == 0 {
266-277:⚠️ Potential issueClose underlying
diskdbto prevent file‑handle leaks
Close()setsreadOnlybut never closesdb.diskdb.
Long‑running processes may exhaust file descriptors.db.readOnly = true // Release the memory held by clean cache. db.tree.bottom().resetCache() + if closer, ok := db.diskdb.(io.Closer); ok { + if err := closer.Close(); err != nil { + return err + } + } return nil }
377-380:⚠️ Potential issueEmpty
JournalFilePathtreated as working directory
os.Stat("")resolves to the current directory, so an empty path makes the method believe a journal file exists and may later try to delete the working dir.- if fileInfo, stateErr := os.Stat(db.config.JournalFilePath); stateErr == nil && !fileInfo.IsDir() { + if db.config.JournalFilePath != "" { + if fileInfo, stateErr := os.Stat(db.config.JournalFilePath); stateErr == nil && !fileInfo.IsDir() { return JournalFileType + } }
194-215: 🛠️ Refactor suggestion
CommitGenesisstill ignores read‑only mode and wastes CPU
- It bypasses
modifyAllowed(), so the DB will happily write afterClose()has switched it to read‑only.- Deleting every key individually from
db.dirtiesis O(n) and keeps the backing array allocated. Replacing the map is cheaper.func (db *Database) CommitGenesis(root common.Hash) error { - log.Info("pathdb write genesis state to disk", "root", root.Hex()) + db.lock.Lock() + defer db.lock.Unlock() + + if err := db.modifyAllowed(); err != nil { + return err + } + + log.Info("pathdb write genesis state to disk", "root", root.Hex()) - batch := db.diskdb.NewBatch() + batch := db.diskdb.NewBatch() @@ - for k := range db.dirties { - delete(db.dirties, k) - } + // Replace the whole map – faster and releases memory + db.dirties = make(dbtypes.KvMap)
| func (db *Database) Put(k, v []byte) error { | ||
| db.lock.Lock() | ||
| defer db.lock.Unlock() | ||
|
|
||
| key := rawdb.CompactStorageTrieNodeKey(k[:]) | ||
| db.dirties.Put(key, v) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Respect read‑only mode in Put
Put mutates db.dirties but does not check db.modifyAllowed(), allowing writes after Close().
func (db *Database) Put(k, v []byte) error {
db.lock.Lock()
defer db.lock.Unlock()
+ if err := db.modifyAllowed(); err != nil {
+ return err
+ }
+
key := rawdb.CompactStorageTrieNodeKey(k[:])
db.dirties.Put(key, v)
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (db *Database) Put(k, v []byte) error { | |
| db.lock.Lock() | |
| defer db.lock.Unlock() | |
| key := rawdb.CompactStorageTrieNodeKey(k[:]) | |
| db.dirties.Put(key, v) | |
| return nil | |
| } | |
| func (db *Database) Put(k, v []byte) error { | |
| db.lock.Lock() | |
| defer db.lock.Unlock() | |
| if err := db.modifyAllowed(); err != nil { | |
| return err | |
| } | |
| key := rawdb.CompactStorageTrieNodeKey(k[:]) | |
| db.dirties.Put(key, v) | |
| return nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
triedb/pathdb/database.go (6)
239-246:⚠️ Potential issueShortcut still skips legitimate single-entry state changes
The early-return keeps the previous logic
len(db.dirties)==1, which was already flagged as unsafe: a single dirty item can contain real changes (e.g. nonce bump).
Change the guard tolen(db.dirties)==0or drop it entirely.- if root == parentRoot && len(db.dirties) == 1 { + if root == parentRoot && len(db.dirties) == 0 {
269-281: 🛠️ Refactor suggestionDon’t leak file handles – close the underlying disk DB
Close()enters read-only mode and frees caches but never closesdb.diskdb.
If the pebble/leveldb implementation implementsio.Closer, the handles stay open until GC.db.tree.bottom().resetCache() - return nil + if closer, ok := db.diskdb.(io.Closer); ok { + if err := closer.Close(); err != nil { + return err + } + } + return nil
285-295:⚠️ Potential issue
Size()needs a read-lock to avoid races while traversing the layer treeAnother reviewer already pointed this out. Iterating without a lock while other goroutines mutate the tree can trip the race detector.
+ db.lock.RLock() + defer db.lock.RUnlock()
380-383:⚠️ Potential issueEmpty
JournalFilePathstill treated as “current directory”Calling
os.Stat("")reports the working dir as “exists”, causing false positives and even accidental deletions.
Guard against an empty path first:- if fileInfo, stateErr := os.Stat(db.config.JournalFilePath); stateErr == nil && !fileInfo.IsDir() { - return JournalFileType - } + if db.config.JournalFilePath != "" { + if fileInfo, stateErr := os.Stat(db.config.JournalFilePath); stateErr == nil && !fileInfo.IsDir() { + return JournalFileType + } + }
197-214:⚠️ Potential issue
CommitGenesisbypasses read-only check and needlessly deletes in-place
- Mutations after
Close()are still possible becausemodifyAllowed()isn’t invoked.- Deleting each key in-place is slower than swapping the map.
db.lock.Lock() defer db.lock.Unlock() + if err := db.modifyAllowed(); err != nil { + return err + } + batch := db.diskdb.NewBatch() @@ - for k := range db.dirties { - delete(db.dirties, k) - } + db.dirties = make(dbtypes.KvMap)
404-410:⚠️ Potential issueWrite operations ignore read-only mode
Putmutatesdb.dirtieswithout checkingmodifyAllowed(), so writes are still accepted after the DB is closed.db.lock.Lock() defer db.lock.Unlock() + if err := db.modifyAllowed(); err != nil { + return err + } - key := rawdb.CompactStorageTrieNodeKey(k[:]) + key := rawdb.CompactStorageTrieNodeKey(k) db.dirties.Put(key, v) return nil
🧹 Nitpick comments (1)
triedb/pathdb/database.go (1)
186-195: Use read-lock for non-mutatingReaderto improve concurrency
Readeronly performs look-ups, yet it acquires an exclusive lock which serialises all reads behind writers.
Switching to anRLockis enough to protect against concurrent mutations while allowing multiple readers in parallel.- db.lock.Lock() - defer db.lock.Unlock() + db.lock.RLock() + defer db.lock.RUnlock()
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
core/types/tx_setcode.go (5)
36-42: Avoid magic numbers – derive the delegation slice length instead of hard-coding23.Hard-coding the expected length makes future maintenance harder (e.g. if the address size ever changes).
Consider deriving it from the prefix length andcommon.AddressLength:-if len(b) != 23 || !bytes.HasPrefix(b, DelegationPrefix) { +expected := len(DelegationPrefix) + common.AddressLength +if len(b) != expected || !bytes.HasPrefix(b, DelegationPrefix) {
45-47: Potential slice aliasing when usingappendwith the global prefix.
append(DelegationPrefix, …)may (in theory) re-use the backing array ofDelegationPrefixif it ever has spare capacity, mutating the global var in-place.
A safer pattern is:out := make([]byte, 0, len(DelegationPrefix)+len(addr)) out = append(out, DelegationPrefix...) out = append(out, addr.Bytes()...) return outThis guarantees the global prefix is never mutated.
90-106: Validate signature before returning the authorization object.
crypto.Signshould already return a canonical signature, yet defensive validation (e.g.ValidateSignatureValues) prior to constructing the struct would help catch malleable or otherwise invalid signatures early.Additionally,
decodeSignature’s returnedvvalue is ignored – if it ever differs fromsig[64], the mismatch would be silently lost. Suggest checking that the two match or removing the redundant call.
108-115:sigHash()prefix constant0x05is undocumented.Please add a short comment explaining what the
0x05domain-separator represents (e.g. “// 0x05 = SetCodeAuthorization type ID”).
This aids reviewers and future maintainers.
116-138: Tighten error handling & clarify failure causes inAuthority().
- Distinguish between recoverable errors (e.g. bad signature) and systemic failures (e.g. invalid public key length) with wrapped errors.
- Consider bubbling up
ValidateSignatureValues’s failure reason to the caller for easier debugging.Not blocking, but improves diagnostics.
core/types/transaction.go (2)
208-210: Decoder path missing size sanity check forSetCodeTx.
inner.decodewill happily succeed on an empty payload (tx.decodeTypedalready guaranteeslen(b) > 1but not that the RLP contains mandatory fields).
Consider validatinginner.(*SetCodeTx)afterwards (similar to other typed-tx validations) to catch malformed transactions early.
481-502: Deduplication logic is fine but order is undocumented & map makes it implicit.Because a map is used for tracking, the resulting slice preserves first-seen order. Add a comment stating this, or sort deterministically before returning, to avoid surprises in callers relying on order.
common/hexutil/json.go (1)
261-266: Consider enforcing the same prefix rules inU256.UnmarshalText
UnmarshalTextforwards directly toSetFromHex, which happily accepts strings without the0xprefix.
If the intention is to keep all public text-based encodings strictly0x-prefixed (as with theBighelper), reuse the logic above here as well.This is a minor consistency tweak, but it avoids surprises for downstream tooling that relies on the prefix to detect hex strings.
core/types/transaction_marshalling.go (1)
512-516: EmptyauthorizationListpasses validationA zero-length slice is not
nil, hencedec.AuthorizationList == nilpasses when the field is present but empty.
If at least one authorization is required, validate the length explicitly:if dec.AuthorizationList == nil || len(dec.AuthorizationList) == 0 { return errors.New("authorizationList must contain at least one entry") }Otherwise, drop the strict nil-check to allow the field to be legitimately omitted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (7)
common/hexutil/json.go(2 hunks)core/types/gen_authorization.go(1 hunks)core/types/transaction.go(3 hunks)core/types/transaction_marshalling.go(3 hunks)core/types/tx_setcode.go(1 hunks)core/types/tx_setcode_test.go(1 hunks)params/version.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- params/version.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
core/types/gen_authorization.go (2)
core/types/tx_setcode.go (1)
SetCodeAuthorization(72-79)common/hexutil/json.go (2)
U256(233-233)Uint64(275-275)
core/types/transaction.go (2)
core/types/tx_setcode.go (2)
SetCodeTx(51-67)SetCodeAuthorization(72-79)core/types/gen_authorization.go (2)
SetCodeAuthorization(18-25)SetCodeAuthorization(38-45)
core/types/tx_setcode.go (6)
common/types.go (1)
BytesToAddress(206-210)common/hexutil/json.go (3)
Bytes(40-40)U256(233-233)Uint64(275-275)core/types/gen_authorization.go (2)
SetCodeAuthorization(18-25)SetCodeAuthorization(38-45)crypto/crypto.go (1)
ValidateSignatureValues(264-275)core/types/transaction.go (3)
ErrInvalidSig(36-36)TxData(80-100)SetCodeTxType(54-54)rlp/decode.go (1)
DecodeBytes(92-106)
🔇 Additional comments (2)
core/types/transaction.go (1)
54-55: Confirm that0x04is unassigned in all deployed networks.Before hard-coding the new typed-transaction ID, double-check that 0x04 is not reserved by another EIP or by any L2 you inter-operate with, otherwise interchangeability may break.
core/types/transaction_marshalling.go (1)
485-489:tofield mandated forSetCodeTx– double-check against the spec
SetCodeTxcurrently rejects JSON that omits thetoaddress, whereas the draft EIP-7702 states the target address can be derived from the signer (similar to contract-creation txs).
Please verify the latest specification; iftois optional you might want to:-if dec.To == nil { - return errors.New("missing required field 'to' in transaction") -} -itx.To = *dec.To +if dec.To != nil { + itx.To = *dec.To +}Failing to align with the spec will make the client incompatible with other implementations.
| // copy creates a deep copy of the transaction data and initializes all fields. | ||
| func (tx *SetCodeTx) copy() TxData { | ||
| cpy := &SetCodeTx{ | ||
| Nonce: tx.Nonce, | ||
| To: tx.To, | ||
| Data: common.CopyBytes(tx.Data), | ||
| Gas: tx.Gas, | ||
| // These are copied below. | ||
| AccessList: make(AccessList, len(tx.AccessList)), | ||
| AuthList: make([]SetCodeAuthorization, len(tx.AuthList)), | ||
| Value: new(uint256.Int), | ||
| ChainID: new(uint256.Int), | ||
| GasTipCap: new(uint256.Int), | ||
| GasFeeCap: new(uint256.Int), | ||
| V: new(uint256.Int), | ||
| R: new(uint256.Int), | ||
| S: new(uint256.Int), | ||
| } | ||
| copy(cpy.AccessList, tx.AccessList) | ||
| copy(cpy.AuthList, tx.AuthList) | ||
| if tx.Value != nil { | ||
| cpy.Value.Set(tx.Value) | ||
| } | ||
| if tx.ChainID != nil { | ||
| cpy.ChainID.Set(tx.ChainID) | ||
| } | ||
| if tx.GasTipCap != nil { | ||
| cpy.GasTipCap.Set(tx.GasTipCap) | ||
| } | ||
| if tx.GasFeeCap != nil { | ||
| cpy.GasFeeCap.Set(tx.GasFeeCap) | ||
| } | ||
| if tx.V != nil { | ||
| cpy.V.Set(tx.V) | ||
| } | ||
| if tx.R != nil { | ||
| cpy.R.Set(tx.R) | ||
| } | ||
| if tx.S != nil { | ||
| cpy.S.Set(tx.S) | ||
| } | ||
| return cpy | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
copy() allocates zeroed uint256.Int even when the source field is nil.
For fields such as Value, GasTipCap, … a nil pointer semantically means “unset”. Creating a zeroed placeholder changes that semantics and can slightly inflate the encoded transaction.
Pattern used elsewhere in geth copies the value but keeps nil when the source is nil:
-if tx.Value != nil {
- cpy.Value.Set(tx.Value)
-}
+if tx.Value != nil {
+ cpy.Value.Set(tx.Value)
+} else {
+ cpy.Value = nil
+}Please replicate that for all pointer fields.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // copy creates a deep copy of the transaction data and initializes all fields. | |
| func (tx *SetCodeTx) copy() TxData { | |
| cpy := &SetCodeTx{ | |
| Nonce: tx.Nonce, | |
| To: tx.To, | |
| Data: common.CopyBytes(tx.Data), | |
| Gas: tx.Gas, | |
| // These are copied below. | |
| AccessList: make(AccessList, len(tx.AccessList)), | |
| AuthList: make([]SetCodeAuthorization, len(tx.AuthList)), | |
| Value: new(uint256.Int), | |
| ChainID: new(uint256.Int), | |
| GasTipCap: new(uint256.Int), | |
| GasFeeCap: new(uint256.Int), | |
| V: new(uint256.Int), | |
| R: new(uint256.Int), | |
| S: new(uint256.Int), | |
| } | |
| copy(cpy.AccessList, tx.AccessList) | |
| copy(cpy.AuthList, tx.AuthList) | |
| if tx.Value != nil { | |
| cpy.Value.Set(tx.Value) | |
| } | |
| if tx.ChainID != nil { | |
| cpy.ChainID.Set(tx.ChainID) | |
| } | |
| if tx.GasTipCap != nil { | |
| cpy.GasTipCap.Set(tx.GasTipCap) | |
| } | |
| if tx.GasFeeCap != nil { | |
| cpy.GasFeeCap.Set(tx.GasFeeCap) | |
| } | |
| if tx.V != nil { | |
| cpy.V.Set(tx.V) | |
| } | |
| if tx.R != nil { | |
| cpy.R.Set(tx.R) | |
| } | |
| if tx.S != nil { | |
| cpy.S.Set(tx.S) | |
| } | |
| return cpy | |
| } | |
| // copy creates a deep copy of the transaction data and initializes all fields. | |
| func (tx *SetCodeTx) copy() TxData { | |
| cpy := &SetCodeTx{ | |
| Nonce: tx.Nonce, | |
| To: tx.To, | |
| Data: common.CopyBytes(tx.Data), | |
| Gas: tx.Gas, | |
| // These are copied below. | |
| AccessList: make(AccessList, len(tx.AccessList)), | |
| AuthList: make([]SetCodeAuthorization, len(tx.AuthList)), | |
| Value: new(uint256.Int), | |
| ChainID: new(uint256.Int), | |
| GasTipCap: new(uint256.Int), | |
| GasFeeCap: new(uint256.Int), | |
| V: new(uint256.Int), | |
| R: new(uint256.Int), | |
| S: new(uint256.Int), | |
| } | |
| copy(cpy.AccessList, tx.AccessList) | |
| copy(cpy.AuthList, tx.AuthList) | |
| if tx.Value != nil { | |
| cpy.Value.Set(tx.Value) | |
| } else { | |
| cpy.Value = nil | |
| } | |
| if tx.ChainID != nil { | |
| cpy.ChainID.Set(tx.ChainID) | |
| } | |
| if tx.GasTipCap != nil { | |
| cpy.GasTipCap.Set(tx.GasTipCap) | |
| } | |
| if tx.GasFeeCap != nil { | |
| cpy.GasFeeCap.Set(tx.GasFeeCap) | |
| } | |
| if tx.V != nil { | |
| cpy.V.Set(tx.V) | |
| } | |
| if tx.R != nil { | |
| cpy.R.Set(tx.R) | |
| } | |
| if tx.S != nil { | |
| cpy.S.Set(tx.S) | |
| } | |
| return cpy | |
| } |
| // TestParseDelegation tests a few possible delegation designator values and | ||
| // ensures they are parsed correctly. | ||
| func TestParseDelegation(t *testing.T) { | ||
| addr := common.Address{0x42} | ||
| for _, tt := range []struct { | ||
| val []byte | ||
| want *common.Address | ||
| }{ | ||
| { // simple correct delegation | ||
| val: append(DelegationPrefix, addr.Bytes()...), | ||
| want: &addr, | ||
| }, | ||
| { // wrong address size | ||
| val: append(DelegationPrefix, addr.Bytes()[0:19]...), | ||
| }, | ||
| { // short address | ||
| val: append(DelegationPrefix, 0x42), | ||
| }, | ||
| { // long address | ||
| val: append(append(DelegationPrefix, addr.Bytes()...), 0x42), | ||
| }, | ||
| { // wrong prefix size | ||
| val: append(DelegationPrefix[:2], addr.Bytes()...), | ||
| }, | ||
| { // wrong prefix | ||
| val: append([]byte{0xef, 0x01, 0x01}, addr.Bytes()...), | ||
| }, | ||
| { // wrong prefix | ||
| val: append([]byte{0xef, 0x00, 0x00}, addr.Bytes()...), | ||
| }, | ||
| { // no prefix | ||
| val: addr.Bytes(), | ||
| }, | ||
| { // no address | ||
| val: DelegationPrefix, | ||
| }, | ||
| } { | ||
| got, ok := ParseDelegation(tt.val) | ||
| if ok && tt.want == nil { | ||
| t.Fatalf("expected fail, got %s", got.Hex()) | ||
| } | ||
| if !ok && tt.want != nil { | ||
| t.Fatalf("failed to parse, want %s", tt.want.Hex()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Test only checks success/failure – it should also assert the parsed address.
Currently, if ParseDelegation returns ok == true but an unexpected address, the test passes.
Add an equality assertion:
if ok && tt.want != nil && got != *tt.want {
t.Fatalf("parsed address mismatch: got %s, want %s", got.Hex(), tt.want.Hex())
}This guards against false positives.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TestParseDelegation tests a few possible delegation designator values and | |
| // ensures they are parsed correctly. | |
| func TestParseDelegation(t *testing.T) { | |
| addr := common.Address{0x42} | |
| for _, tt := range []struct { | |
| val []byte | |
| want *common.Address | |
| }{ | |
| { // simple correct delegation | |
| val: append(DelegationPrefix, addr.Bytes()...), | |
| want: &addr, | |
| }, | |
| { // wrong address size | |
| val: append(DelegationPrefix, addr.Bytes()[0:19]...), | |
| }, | |
| { // short address | |
| val: append(DelegationPrefix, 0x42), | |
| }, | |
| { // long address | |
| val: append(append(DelegationPrefix, addr.Bytes()...), 0x42), | |
| }, | |
| { // wrong prefix size | |
| val: append(DelegationPrefix[:2], addr.Bytes()...), | |
| }, | |
| { // wrong prefix | |
| val: append([]byte{0xef, 0x01, 0x01}, addr.Bytes()...), | |
| }, | |
| { // wrong prefix | |
| val: append([]byte{0xef, 0x00, 0x00}, addr.Bytes()...), | |
| }, | |
| { // no prefix | |
| val: addr.Bytes(), | |
| }, | |
| { // no address | |
| val: DelegationPrefix, | |
| }, | |
| } { | |
| got, ok := ParseDelegation(tt.val) | |
| if ok && tt.want == nil { | |
| t.Fatalf("expected fail, got %s", got.Hex()) | |
| } | |
| if !ok && tt.want != nil { | |
| t.Fatalf("failed to parse, want %s", tt.want.Hex()) | |
| } | |
| } | |
| } | |
| func TestParseDelegation(t *testing.T) { | |
| addr := common.Address{0x42} | |
| for _, tt := range []struct { | |
| val []byte | |
| want *common.Address | |
| }{ | |
| // … test cases … | |
| } { | |
| got, ok := ParseDelegation(tt.val) | |
| if ok && tt.want == nil { | |
| t.Fatalf("expected fail, got %s", got.Hex()) | |
| } | |
| if !ok && tt.want != nil { | |
| t.Fatalf("failed to parse, want %s", tt.want.Hex()) | |
| } | |
| // new check: ensure the returned address matches expectation | |
| if ok && tt.want != nil && got != *tt.want { | |
| t.Fatalf("parsed address mismatch: got %s, want %s", got.Hex(), tt.want.Hex()) | |
| } | |
| } | |
| } |
| // SetCodeAuthorizations returns the authorizations list of the transaction. | ||
| func (tx *Transaction) SetCodeAuthorizations() []SetCodeAuthorization { | ||
| setcodetx, ok := tx.inner.(*SetCodeTx) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| return setcodetx.AuthList | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose a copy to protect internal slice from mutation.
Returning the internal AuthList allows callers to modify it, breaking transaction immutability assumptions elsewhere.
Either document that mutation is forbidden or return a defensive copy:
-return setcodetx.AuthList
+out := make([]types.SetCodeAuthorization, len(setcodetx.AuthList))
+copy(out, setcodetx.AuthList)
+return outCommittable suggestion skipped: line range outside the PR's diff.
| // UnmarshalJSON implements json.Unmarshaler. | ||
| func (b *U256) UnmarshalJSON(input []byte) error { | ||
| // The uint256.Int.UnmarshalJSON method accepts "dec", "0xhex"; we must be | ||
| // more strict, hence we check string and invoke SetFromHex directly. | ||
| if !isString(input) { | ||
| return errNonString(u256T) | ||
| } | ||
| // The hex decoder needs to accept empty string ("") as '0', which uint256.Int | ||
| // would reject. | ||
| if len(input) == 2 { | ||
| (*uint256.Int)(b).Clear() | ||
| return nil | ||
| } | ||
| err := (*uint256.Int)(b).SetFromHex(string(input[1 : len(input)-1])) | ||
| if err != nil { | ||
| return &json.UnmarshalTypeError{Value: err.Error(), Type: u256T} | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
U256.UnmarshalJSON accepts non-prefixed and malformed hex strings
U256.UnmarshalJSON currently skips the prefix/length checks implemented for the other numeric helpers (Uint64, Big).
As a consequence the following inputs are silently accepted, despite being invalid according to the rest of the API:
"123abc" // missing 0x-prefix
"0x" // empty number → ends up as “invalid syntax” from uint256To keep the semantics consistent and fail early we can reuse checkNumberText, handle the "0x" edge-case and propagate the error through wrapTypeError, e.g.:
-// The hex decoder needs to accept empty string ("") as '0', which uint256.Int
-// would reject.
-if len(input) == 2 {
- (*uint256.Int)(b).Clear()
- return nil
-}
-err := (*uint256.Int)(b).SetFromHex(string(input[1 : len(input)-1]))
+raw, err := checkNumberText(input[1:len(input)-1])
+if err != nil {
+ return wrapTypeError(err, u256T)
+}
+if len(raw) == 0 { // "0x" → zero
+ (*uint256.Int)(b).Clear()
+ return nil
+}
+err = (*uint256.Int)(b).SetFromHex("0x" + string(raw))
if err != nil {
return &json.UnmarshalTypeError{Value: err.Error(), Type: u256T}
}This mirrors Big.UnmarshalText, guarantees the 0x prefix, forbids leading zeroes and yields uniform error messages across helpers.
| s.Nonce = uint64(*dec.Nonce) | ||
| if dec.V == nil { | ||
| return errors.New("missing required field 'yParity' for SetCodeAuthorization") | ||
| } | ||
| s.V = uint8(*dec.V) | ||
| if dec.R == nil { |
There was a problem hiding this comment.
yParity value is not range-checked before narrowing to uint8
The ECDSA “Y parity” must be either 0 or 1.
Casting *dec.V straight to uint8 silently accepts any 64-bit value (0x2 → 2, 0xff → 255), which would break signature recovery.
- s.V = uint8(*dec.V)
+ val := uint64(*dec.V)
+ if val != 0 && val != 1 {
+ return errors.New("invalid yParity: must be 0 or 1")
+ }
+ s.V = uint8(val)Adding this check keeps the generated type robust and prevents invalid transactions from propagating further into the pipeline.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s.Nonce = uint64(*dec.Nonce) | |
| if dec.V == nil { | |
| return errors.New("missing required field 'yParity' for SetCodeAuthorization") | |
| } | |
| s.V = uint8(*dec.V) | |
| if dec.R == nil { | |
| s.Nonce = uint64(*dec.Nonce) | |
| if dec.V == nil { | |
| return errors.New("missing required field 'yParity' for SetCodeAuthorization") | |
| } | |
| val := uint64(*dec.V) | |
| if val != 0 && val != 1 { | |
| return errors.New("invalid yParity: must be 0 or 1") | |
| } | |
| s.V = uint8(val) | |
| if dec.R == nil { |
| ChainID *hexutil.Big `json:"chainId,omitempty"` | ||
| Nonce *hexutil.Uint64 `json:"nonce"` | ||
| To *common.Address `json:"to"` | ||
| Gas *hexutil.Uint64 `json:"gas"` | ||
| GasPrice *hexutil.Big `json:"gasPrice"` | ||
| MaxPriorityFeePerGas *hexutil.Big `json:"maxPriorityFeePerGas"` | ||
| MaxFeePerGas *hexutil.Big `json:"maxFeePerGas"` | ||
| MaxFeePerBlobGas *hexutil.Big `json:"maxFeePerBlobGas,omitempty"` | ||
| Value *hexutil.Big `json:"value"` | ||
| Input *hexutil.Bytes `json:"input"` | ||
| AccessList *AccessList `json:"accessList,omitempty"` | ||
| BlobVersionedHashes []common.Hash `json:"blobVersionedHashes,omitempty"` | ||
| AuthorizationList []SetCodeAuthorization `json:"authorizationList,omitempty"` | ||
| V *hexutil.Big `json:"v"` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
authorizationList marked omitempty although required for SetCodeTx
During unmarshalling we throw an error when authorizationList is absent (nil).
With omitempty, however, a caller that crafts a SetCodeTx with an intentionally empty list will marshal it back without the key, yielding JSON that will later be rejected by your own decoder.
If the list must be present (even when empty) consider removing omitempty, or switch the nil-check in the decoder to len(list) == 0 if an empty slice is acceptable.
🏁 Script executed:
#!/bin/bash
set -e
# Locate transaction_marshalling.go
echo "Locating transaction_marshalling.go:"
fd transaction_marshalling.go || true
echo
# Print the first 200 lines for context
echo "Printing first 200 lines of core/types/transaction_marshalling.go:"
sed -n '1,200p' core/types/transaction_marshalling.go || true
echo
# Search for SetCodeTx definitions and usages
echo "Searching for SetCodeTx in core/types:"
rg -n "SetCodeTx" -n core/types || true
echo
# Search for any custom JSON unmarshalling or nil-check on authorizationList
echo "Searching for authorizationList checks or UnmarshalJSON implementations:"
rg -n "authorizationList" -n core/types || true
rg -n "UnmarshalJSON" -n core/types || trueLength of output: 11917
🏁 Script executed:
#!/bin/bash
set -e
# Print lines 480-550 for UnmarshalJSON case of SetCodeTx
echo "Printing lines 480-550 of core/types/transaction_marshalling.go:"
sed -n '480,550p' core/types/transaction_marshalling.go || trueLength of output: 2348
Remove omitempty from authorizationList in txJSON
The authorizationList field is required by the UnmarshalJSON logic (it errors if dec.AuthorizationList == nil), but marking it omitempty causes empty or nil slices to be dropped during MarshalJSON, leading to JSON that will be rejected on decode.
Suggested fix in core/types/transaction_marshalling.go:
- // in type txJSON
- AuthorizationList []SetCodeAuthorization `json:"authorizationList,omitempty"`
+ // ensure the empty list is always emitted
+ AuthorizationList []SetCodeAuthorization `json:"authorizationList"`Alternatively, if truly an empty list should be acceptable, change the decoder check to:
- if dec.AuthorizationList == nil {
- return errors.New("missing required field 'authorizationList' in transaction")
- }
+ if len(dec.AuthorizationList) == 0 {
+ return errors.New("authorizationList must contain at least one entry")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ChainID *hexutil.Big `json:"chainId,omitempty"` | |
| Nonce *hexutil.Uint64 `json:"nonce"` | |
| To *common.Address `json:"to"` | |
| Gas *hexutil.Uint64 `json:"gas"` | |
| GasPrice *hexutil.Big `json:"gasPrice"` | |
| MaxPriorityFeePerGas *hexutil.Big `json:"maxPriorityFeePerGas"` | |
| MaxFeePerGas *hexutil.Big `json:"maxFeePerGas"` | |
| MaxFeePerBlobGas *hexutil.Big `json:"maxFeePerBlobGas,omitempty"` | |
| Value *hexutil.Big `json:"value"` | |
| Input *hexutil.Bytes `json:"input"` | |
| AccessList *AccessList `json:"accessList,omitempty"` | |
| BlobVersionedHashes []common.Hash `json:"blobVersionedHashes,omitempty"` | |
| AuthorizationList []SetCodeAuthorization `json:"authorizationList,omitempty"` | |
| V *hexutil.Big `json:"v"` | |
| ChainID *hexutil.Big `json:"chainId,omitempty"` | |
| Nonce *hexutil.Uint64 `json:"nonce"` | |
| To *common.Address `json:"to"` | |
| Gas *hexutil.Uint64 `json:"gas"` | |
| GasPrice *hexutil.Big `json:"gasPrice"` | |
| MaxPriorityFeePerGas *hexutil.Big `json:"maxPriorityFeePerGas"` | |
| MaxFeePerGas *hexutil.Big `json:"maxFeePerGas"` | |
| MaxFeePerBlobGas *hexutil.Big `json:"maxFeePerBlobGas,omitempty"` | |
| Value *hexutil.Big `json:"value"` | |
| Input *hexutil.Bytes `json:"input"` | |
| AccessList *AccessList `json:"accessList,omitempty"` | |
| BlobVersionedHashes []common.Hash `json:"blobVersionedHashes,omitempty"` | |
| // ensure the empty list is always emitted | |
| AuthorizationList []SetCodeAuthorization `json:"authorizationList"` | |
| V *hexutil.Big `json:"v"` |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
cmd/utils/flags.go (1)
1756-1756: Consider avoiding global variable mutation.Setting
trie.GenesisStateInPathZkTrie = truemutates a package-level global variable, which can lead to unexpected behavior in tests or when multiple configurations are used. Consider passing this as a configuration parameter instead.If refactoring is not feasible in this PR, add a comment explaining why global mutation is necessary:
+ // Set global flag for genesis state handling in path-based zk-trie. + // This must be set before any trie operations during node initialization. if cfg.StateScheme == rawdb.PathScheme { trie.GenesisStateInPathZkTrie = true }core/genesis.go (1)
340-343: Consider more robust disk database detection.The
toDiskDbflag assumes any non-nil database is disk-backed. While this works for the current usage pattern (nil triggers creation of a memory database), it could be fragile ifrawdb.NewMemoryDatabase()is passed directly.Consider adding an explicit interface or type check:
toDiskDb := true if db == nil { toDiskDb = false db = rawdb.NewMemoryDatabase() } else if _, ok := db.(*memorydb.Database); ok { toDiskDb = false }Note: This requires importing the memorydb package and may be overkill for the current use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/geth/chaincmd.go(1 hunks)cmd/geth/dbcmd.go(3 hunks)cmd/geth/main.go(1 hunks)cmd/geth/snapshot.go(1 hunks)cmd/geth/usage.go(1 hunks)cmd/utils/flags.go(7 hunks)core/genesis.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/geth/main.go
- cmd/geth/usage.go
- cmd/geth/chaincmd.go
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/geth/snapshot.go (1)
core/rawdb/accessors_trie.go (2)
ReadStateScheme(112-135)HashScheme(40-40)
cmd/geth/dbcmd.go (4)
cmd/utils/flags.go (3)
SyncModeFlag(239-243)AncientFlag(128-131)MakeChainDatabase(2073-2092)trie/hbss2pbss.go (1)
NewHbss2Pbss(30-52)core/rawdb/accessors_state.go (1)
ReadPersistentStateID(184-190)core/rawdb/database.go (1)
PruneHashTrieNodeInDataBase(472-498)
cmd/utils/flags.go (4)
cmd/utils/cmd.go (1)
Fatalf(55-70)core/rawdb/accessors_trie.go (2)
PathScheme(48-48)ValidateStateScheme(139-144)trie/database.go (2)
GenesisStateInPathZkTrie(299-299)Config(288-297)eth/ethconfig/config.go (1)
Config(120-226)
core/genesis.go (5)
eth/backend.go (2)
Config(62-62)New(104-317)trie/database.go (4)
Config(288-297)GenesisStateInPathZkTrie(299-299)NewDatabaseWithConfig(347-385)Database(75-104)core/rawdb/accessors_state.go (1)
ExistsStateID(225-231)core/state/database.go (2)
NewDatabaseWithConfig(121-130)Database(43-61)core/rawdb/database.go (1)
NewMemoryDatabase(234-236)
🔇 Additional comments (7)
cmd/geth/snapshot.go (1)
232-234: LGTM!The safeguard correctly prevents offline pruning for path-based schemes. The critical exit is appropriate since pruning logic differs between hash and path schemes.
cmd/utils/flags.go (2)
864-872: LGTM!The flag definitions are clear and follow the existing patterns in the codebase. The usage text accurately describes their purpose.
2221-2238: LGTM!The function correctly handles CLI and config state scheme resolution with appropriate validation and logging. The conflict detection logic is sound.
core/genesis.go (4)
225-225: PathZkTrie configuration looks correct.The addition of
PathZkTrie: trie.GenesisStateInPathZkTrieto the trie config aligns with the pattern seen ineth/backend.go, where the global is set based on the state scheme selection before genesis setup.Also applies to: 228-228
347-347: PathZkTrie configuration consistent with SetupGenesisBlockWithOverride.The addition of
PathZkTrie: trie.GenesisStateInPathZkTriematches the pattern used inSetupGenesisBlockWithOverride(lines 225, 228).
237-255: Control flow change is correct: the early return for uninitialized chains preserves the original fallback behavior and aligns with all call sites; no further action required.
231-236: Fix typo and ensure safe genesis skip.
- Line 232: change comment to “override” instead of “overide”.
- Confirm that
ExistsStateIDcan’t return true on a partial or corrupted state (e.g. add a deeper integrity check or clear stale markers before skipping genesis).
| dbHbss2PbssCmd = cli.Command{ | ||
| Action: hbss2pbss, | ||
| Name: "hbss-to-pbss", | ||
| ArgsUsage: "<jobnum (optional)>", | ||
| Flags: []cli.Flag{ | ||
| utils.DataDirFlag, | ||
| utils.SyncModeFlag, | ||
| utils.AncientFlag, | ||
| }, | ||
| Usage: "Convert Hash-Base to Path-Base trie node.", | ||
| Description: `This command iterates the entire trie node database and convert the hash-base node to path-base node.`, | ||
| } |
There was a problem hiding this comment.
Fix argument handling or update usage text.
The command definition specifies <jobnum (optional)> in ArgsUsage, but the implementation at lines 743-746 rejects any arguments with "Too many arguments given". This inconsistency will confuse users.
Either update the implementation to accept and use the optional jobnum argument:
- if ctx.NArg() > 0 {
+ if ctx.NArg() > 1 {
log.Error("Too many arguments given")
return errors.New("too many arguments")
}
+
+ jobnum := 0
+ if ctx.NArg() == 1 {
+ var err error
+ jobnum, err = strconv.Atoi(ctx.Args().Get(0))
+ if err != nil {
+ log.Error("Invalid jobnum argument", "err", err)
+ return err
+ }
+ // TODO: Use jobnum parameter in conversion
+ }Or remove the unused argument from the command definition:
- ArgsUsage: "<jobnum (optional)>",
+ ArgsUsage: "",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dbHbss2PbssCmd = cli.Command{ | |
| Action: hbss2pbss, | |
| Name: "hbss-to-pbss", | |
| ArgsUsage: "<jobnum (optional)>", | |
| Flags: []cli.Flag{ | |
| utils.DataDirFlag, | |
| utils.SyncModeFlag, | |
| utils.AncientFlag, | |
| }, | |
| Usage: "Convert Hash-Base to Path-Base trie node.", | |
| Description: `This command iterates the entire trie node database and convert the hash-base node to path-base node.`, | |
| } | |
| dbHbss2PbssCmd = cli.Command{ | |
| Action: hbss2pbss, | |
| Name: "hbss-to-pbss", | |
| ArgsUsage: "", | |
| Flags: []cli.Flag{ | |
| utils.DataDirFlag, | |
| utils.SyncModeFlag, | |
| utils.AncientFlag, | |
| }, | |
| Usage: "Convert Hash-Base to Path-Base trie node.", | |
| Description: `This command iterates the entire trie node database and convert the hash-base node to path-base node.`, | |
| } |
🤖 Prompt for AI Agents
In cmd/geth/dbcmd.go around lines 268-279 the command's ArgsUsage advertises an
optional "<jobnum (optional)>" argument but the implementation at lines 743-746
rejects any arguments; either make the handler accept a single optional jobnum
or remove the argument from the command definition. To fix, either (A) update
the handler at ~743-746 to allow 0 or 1 args, validate and parse the single arg
as an integer and use it when running hbss2pbss (return an error if more than 1
arg), and update usage/error messages accordingly; or (B) remove the "<jobnum
(optional)>" text from ArgsUsage and any related help text so the command does
not advertise an unused argument.
| lastStateID := rawdb.ReadPersistentStateID(chaindb) | ||
| if lastStateID == 0 { | ||
| h2p.Run() | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add logging for skipped conversion.
The conversion only runs when lastStateID == 0 (lines 749-751), but there's no else branch to inform users why conversion was skipped. This could lead to confusion when the command appears to succeed but doesn't perform the expected conversion.
Add an informative log message:
lastStateID := rawdb.ReadPersistentStateID(chaindb)
if lastStateID == 0 {
h2p.Run()
+ } else {
+ log.Info("Skipping conversion: path-based state already exists", "stateID", lastStateID)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| lastStateID := rawdb.ReadPersistentStateID(chaindb) | |
| if lastStateID == 0 { | |
| h2p.Run() | |
| } | |
| lastStateID := rawdb.ReadPersistentStateID(chaindb) | |
| if lastStateID == 0 { | |
| h2p.Run() | |
| } else { | |
| log.Info("Skipping conversion: path-based state already exists", "stateID", lastStateID) | |
| } |
🤖 Prompt for AI Agents
In cmd/geth/dbcmd.go around lines 748-752, the code only runs h2p.Run() when
lastStateID == 0 and doesn't inform the user when conversion is skipped; add an
else branch that logs an informative message (using the same logging facility
used elsewhere in this file) stating that the conversion was skipped and include
the current lastStateID value so users know why the conversion did not run.
| log.Info("Prefetch disabled") | ||
| cfg.NoPrefetch = true | ||
|
|
||
| // cheked for path zktrie |
There was a problem hiding this comment.
Fix typo: "cheked" → "checked".
The comment contains a typo that appears three times in the file (lines 1807, 1827, and 1847).
Apply this diff at line 1807:
- // cheked for path zktrie
+ // checked for path zktrie
if cfg.StateScheme == rawdb.PathScheme && !cfg.Genesis.Config.Morph.ZktrieEnabled() {Repeat the same fix at lines 1827 and 1847.
Also applies to: 1827-1827, 1847-1847
🤖 Prompt for AI Agents
In cmd/utils/flags.go at lines 1807, 1827, and 1847, there is a misspelled
comment "cheked for path zktrie" (typo: "cheked"); update each occurrence to
"checked for path zktrie" so all three comments are corrected.
| if toDiskDb { | ||
| statedb.Database().TrieDB().CommitGenesis(root) | ||
| } else { | ||
| statedb.Database().TrieDB().Commit(root, true, nil) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify CommitGenesis semantics for genesis state persistence.
The switch from Commit(root, true, nil) to CommitGenesis(root) for disk-backed databases introduces new behavior. Ensure CommitGenesis correctly handles genesis state persistence, including:
- Writing state ID markers (checked in
SetupGenesisBlockWithOverride) - Flushing all trie nodes to disk
- Any PathZkTrie-specific initialization
Run the following script to examine the CommitGenesis implementation:
🏁 Script executed:
#!/bin/bash
# Description: Find and examine CommitGenesis implementation
# Find CommitGenesis method definitions
ast-grep --pattern 'func ($$$) CommitGenesis($$$) { $$$ }'
# Find CommitGenesis interface declarations
ast-grep --pattern 'CommitGenesis($$$)'
# Compare with regular Commit method
ast-grep --pattern 'func ($$$) Commit($_, $_, $$$) { $$$ }'
# Check for state ID writes in CommitGenesis
rg -nP --type=go -A10 'func.*CommitGenesis' | rg 'WriteStateID'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any occurrences of CommitGenesis
rg -n 'CommitGenesis' -C3Length of output: 1923
🏁 Script executed:
#!/bin/bash
# Extract Database.Commit implementation
rg -nP '^func \(db \*Database\) Commit\(' -A50 trie/database.go
# Extract PathDB CommitGenesis implementation
sed -n '180,260p' triedb/pathdb/database.goLength of output: 3731
Ensure CommitGenesis flushes trie state and writes the state ID
The ZktrieDatabase’s CommitGenesis(root) calls CommitState(..., flush=false) and then Database.CommitGenesis calls Commit(root, true, nil) which also uses flush=false, so trie nodes aren’t flushed to disk at genesis.
• In triedb/hashdb/zk_trie_database.go, pass flush=true to CommitState or otherwise force a full flush.
• In core/genesis.go, confirm rawdb.WriteStateID (as in PathDB) is invoked for ZktrieDB.
🤖 Prompt for AI Agents
In core/genesis.go around lines 390 to 394, the current ZktrieDatabase commit
path uses CommitGenesis/Commit with flush=false so trie nodes aren’t flushed and
the state ID may not be persisted; update triedb/hashdb/zk_trie_database.go so
CommitState is called with flush=true (or otherwise force a full flush) during
genesis commits, and modify core/genesis.go to ensure rawdb.WriteStateID is
invoked for ZktrieDB after the commit (same behavior as PathDB) so the state
root ID is written to disk.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
core/genesis.go (1)
394-400: Verify CommitGenesis semantics (flush + state ID persisted)Switching to TrieDB.CommitGenesis(root) for on-disk genesis requires that it:
- Forces a full flush of genesis trie nodes.
- Persists state ID (for path scheme) similarly to WriteStateID.
This was previously flagged; please confirm the implementation.
Run:
#!/bin/bash # Locate CommitGenesis implementations and check for flush/state-id writes rg -nP --type=go 'func\s+\(.*\)\s+CommitGenesis\(' -C3 rg -nP --type=go -A6 'CommitGenesis\(' triedb/ trie/ # Look for WriteStateID in CommitGenesis paths rg -nP --type=go 'WriteStateID|stateID' -n triedb/ trie/ core/rawdbcmd/utils/flags.go (1)
1811-1811: Fix typo: "cheked" → "checked".The comment "cheked for path zktrie" contains a typo. It should be "checked for path zktrie" at lines 1811, 1831, and 1851.
This issue was already flagged in a previous review.
Also applies to: 1831-1831, 1851-1851
🧹 Nitpick comments (2)
core/state/state_object.go (1)
175-181: Consider using the field directly to avoid reassignment.The code creates a local variable
addrHashand conditionally reassigns it. While this works correctly, a past review comment suggested using the field directly (e.g.,s.addrHashors.addrPoseidonHash) to improve clarity.Based on past review comment.
Alternative approach:
if s.trie == nil { var err error - addrHash := s.addrHash - if db.TrieDB().IsPathZkTrie() { - addrHash = s.addrPoseidonHash - } - s.trie, err = db.OpenStorageTrie(addrHash, s.data.Root, s.db.originalRoot) + var addrHash common.Hash + if db.TrieDB().IsPathZkTrie() { + addrHash = s.addrPoseidonHash + } else { + addrHash = s.addrHash + } + s.trie, err = db.OpenStorageTrie(addrHash, s.data.Root, s.db.originalRoot) if err != nil { s.trie, _ = db.OpenStorageTrie(addrHash, common.Hash{}, s.db.originalRoot)This makes it explicit that we're choosing one or the other, not modifying a default.
core/blockchain.go (1)
844-887: Stop-path journaling for path scheme: OK; log level nitPath scheme journaling before exit is appropriate. Consider using log.Error on Journal failure instead of Info to surface issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/geth/chaincmd.go(1 hunks)cmd/geth/main.go(1 hunks)cmd/utils/flags.go(7 hunks)core/blockchain.go(7 hunks)core/blockchain_l2.go(3 hunks)core/genesis.go(3 hunks)core/state/state_object.go(4 hunks)core/state/statedb.go(5 hunks)eth/backend.go(5 hunks)eth/ethconfig/config.go(2 hunks)rollup/tracing/tracing.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/geth/main.go
- eth/ethconfig/config.go
🧰 Additional context used
🧬 Code graph analysis (8)
cmd/geth/chaincmd.go (1)
cmd/utils/flags.go (1)
StateSchemeFlag(868-871)
core/genesis.go (5)
eth/backend.go (2)
Config(62-62)New(104-320)trie/database.go (4)
Config(288-297)GenesisStateInPathZkTrie(299-299)NewDatabaseWithConfig(347-385)Database(75-104)core/rawdb/accessors_state.go (1)
ExistsStateID(225-231)core/state/database.go (2)
NewDatabaseWithConfig(121-130)Database(43-61)core/rawdb/database.go (1)
NewMemoryDatabase(234-236)
core/state/state_object.go (3)
common/types.go (3)
Hash(50-50)Hash(172-172)BigToHash(62-62)core/state/statedb.go (1)
StateDB(63-127)crypto/crypto.go (1)
Keccak256Hash(90-97)
core/state/statedb.go (1)
core/vm/interface.go (1)
StateDB(29-89)
core/blockchain_l2.go (2)
core/rawdb/accessors_trie.go (1)
PathScheme(48-48)core/blockchain.go (1)
TriesInMemory(98-98)
eth/backend.go (3)
eth/ethconfig/config.go (1)
Defaults(63-94)core/rawdb/accessors_trie.go (2)
ParseStateScheme(157-181)PathScheme(48-48)trie/database.go (1)
GenesisStateInPathZkTrie(299-299)
core/blockchain.go (6)
eth/ethconfig/config.go (1)
Config(120-233)eth/backend.go (2)
Config(62-62)New(104-320)triedb/pathdb/database.go (3)
Config(104-113)Database(147-159)New(164-183)trie/database.go (3)
Config(288-297)Database(75-104)NewDatabaseWithConfig(347-385)core/rawdb/accessors_trie.go (1)
PathScheme(48-48)core/state/database.go (2)
Database(43-61)NewDatabaseWithConfig(121-130)
cmd/utils/flags.go (4)
cmd/utils/cmd.go (1)
Fatalf(55-70)core/rawdb/accessors_trie.go (2)
PathScheme(48-48)ValidateStateScheme(139-144)trie/database.go (2)
GenesisStateInPathZkTrie(299-299)Config(288-297)eth/ethconfig/config.go (1)
Config(120-233)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
core/state/statedb.go (5)
160-162: LGTM!The new
GetOriginRoot()method provides access to the pre-state root, which is useful for operations that need to reference the original state before modifications.
197-203: LGTM!The method renaming from
IsZktrie()toIsZkTrie()improves casing consistency, and the newIsPathZkTrie()method properly delegates to the underlying TrieDB to determine path-based zk-trie usage.
341-343: LGTM!The variable rename from
addr_stoaddressKeyimproves readability, and the updated call toIsZkTrie()is consistent with the method rename elsewhere in the file.
744-744: LGTM!Explicitly copying
originalRootin theCopy()method is essential for maintaining correct state semantics, especially when the copy needs to reference the pre-state root independently of the source StateDB.
627-627: LGTM!The updated call to
IsZkTrie()is consistent with the method rename and correctly determines the decoding strategy for on-disk state objects.eth/backend.go (3)
126-128: Defaulting JournalFileName: LGTMGood fallback to ethconfig.Defaults when empty.
138-142: State scheme parsing: LGTMDelegating to rawdb.ParseStateScheme with chainDb is correct and returns early on incompatibility.
205-218: Propagating cache/journal/path flags: LGTMPassing PathSyncFlush, StateScheme, and resolved JournalFilePath into core.CacheConfig is correct and consistent with downstream usage.
core/blockchain.go (3)
141-144: New CacheConfig fields: LGTMNaming and types align with downstream triedb/pathdb config.
201-207: Flush timers: LGTMAdding flushInterval/lastFlushTime is fine and used by L2 path.
277-284: State cache and timers initialization: LGTMUsing cacheConfig.triedbConfig(chainConfig.Morph.ZktrieEnabled()) centralizes backend wiring; timers initialized properly.
Also applies to: 298-300
core/blockchain_l2.go (1)
154-200: Non-archive path: LGTMUsing flushInterval instead of TrieTimeLimit and retaining GC/Cap behavior is correct.
core/genesis.go (1)
230-234: PathZkTrie init guard: LGTMUsing rawdb.ExistsStateID to skip reinit in path mode is sound; fallback to commit on failure is correct.
Also applies to: 236-261
cmd/utils/flags.go (1)
2225-2242: LGTM! Well-structured state scheme resolution logic.The function correctly handles all cases:
- Falls back to config when CLI is not specified
- Validates CLI input before use
- Detects and reports incompatible CLI/config combinations
- Provides clear logging for transparency
The logic is sound and the error messages are informative.
| utils.StateSchemeFlag, | ||
| utils.OverrideMorph203TimeFlag, | ||
| utils.OverrideViridianTimeFlag, | ||
| }, |
There was a problem hiding this comment.
state.scheme flag is not wired into init; geth init ignores it
initGenesis never uses the flag to select path/hash scheme, so genesis creation won’t enable PathZkTrie. Wire it before SetupGenesisBlockWithOverride.
Apply this diff:
@@
- Flags: []cli.Flag{
+ Flags: []cli.Flag{
utils.DataDirFlag,
utils.StateSchemeFlag,
utils.OverrideMorph203TimeFlag,
utils.OverrideViridianTimeFlag,
},
@@
func initGenesis(ctx *cli.Context) error {
@@
- for _, name := range []string{"chaindata", "lightchaindata"} {
+ for _, name := range []string{"chaindata", "lightchaindata"} {
chaindb, err := stack.OpenDatabase(name, 0, 0, "", false)
if err != nil {
utils.Fatalf("Failed to open database: %v", err)
}
+ // Resolve and enforce state scheme for genesis initialization
+ scheme, err := rawdb.ParseStateScheme(ctx.String(utils.StateSchemeFlag.Name), chaindb)
+ if err != nil {
+ utils.Fatalf("Invalid state scheme: %v", err)
+ }
+ trie.GenesisStateInPathZkTrie = (scheme == rawdb.PathScheme)
_, hash, err := core.SetupGenesisBlockWithOverride(chaindb, genesis, &overrides)
if err != nil {
utils.Fatalf("Failed to write genesis block: %v", err)
}
chaindb.Close()
log.Info("Successfully wrote genesis state", "database", name, "hash", hash)
}Add import if missing:
@@
- "github.com/morph-l2/go-ethereum/core/state"
+ "github.com/morph-l2/go-ethereum/core/state"
"github.com/morph-l2/go-ethereum/core/types"
+ "github.com/morph-l2/go-ethereum/trie"As per ParseStateScheme in core/rawdb/accessors_trie.go and the global toggle used by core/genesis.go. [Based on learnings]
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/geth/chaincmd.go around lines 53–56, the state.scheme CLI flag is parsed
but never applied during init, so initGenesis ignores it and PathZkTrie won't be
enabled; before calling SetupGenesisBlockWithOverride, parse the flag value with
the ParseStateScheme function (importing the package where ParseStateScheme
lives, e.g., core/rawdb/accessors_trie or the appropriate package that exposes
it) and assign the result to the global state-scheme toggle used by
core/genesis.go (the same global the genesis code reads). Ensure you add the
necessary import if missing and perform this wiring right before
SetupGenesisBlockWithOverride so the chosen scheme is honored during genesis
creation.
| PathDBSyncFlag = cli.BoolFlag{ | ||
| Name: "pathdb.sync", | ||
| Usage: "Sync flush nodes cache to disk in path schema", | ||
| } |
There was a problem hiding this comment.
Fix terminology: "schema" → "scheme".
The usage text says "path schema" but should be "path scheme" for consistency with the state scheme terminology used throughout the codebase (e.g., StateScheme, rawdb.PathScheme, rawdb.HashScheme).
Apply this diff:
PathDBSyncFlag = cli.BoolFlag{
Name: "pathdb.sync",
- Usage: "Sync flush nodes cache to disk in path schema",
+ Usage: "Sync flush nodes cache to disk in path scheme",
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PathDBSyncFlag = cli.BoolFlag{ | |
| Name: "pathdb.sync", | |
| Usage: "Sync flush nodes cache to disk in path schema", | |
| } | |
| PathDBSyncFlag = cli.BoolFlag{ | |
| Name: "pathdb.sync", | |
| Usage: "Sync flush nodes cache to disk in path scheme", | |
| } |
🤖 Prompt for AI Agents
In cmd/utils/flags.go around lines 873 to 876, the Usage string for
PathDBSyncFlag uses the term "path schema" which is inconsistent with the
codebase; change "schema" to "scheme" so the Usage reads "Sync flush nodes cache
to disk in path scheme" to match terminology like StateScheme/rawdb.PathScheme.
| cfg.PathSyncFlush = true | ||
| } | ||
|
|
||
| scheme, err := ParseCLIAndConfigStateScheme(ctx.String(StateSchemeFlag.Name), cfg.StateScheme) |
There was a problem hiding this comment.
Use ctx.GlobalString instead of ctx.String for flag access.
Line 1754 uses ctx.String(StateSchemeFlag.Name) while the rest of the file consistently uses ctx.GlobalString() for reading global flags. This inconsistency could cause the flag value to not be read correctly.
Apply this diff:
- scheme, err := ParseCLIAndConfigStateScheme(ctx.String(StateSchemeFlag.Name), cfg.StateScheme)
+ scheme, err := ParseCLIAndConfigStateScheme(ctx.GlobalString(StateSchemeFlag.Name), cfg.StateScheme)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| scheme, err := ParseCLIAndConfigStateScheme(ctx.String(StateSchemeFlag.Name), cfg.StateScheme) | |
| scheme, err := ParseCLIAndConfigStateScheme(ctx.GlobalString(StateSchemeFlag.Name), cfg.StateScheme) |
🤖 Prompt for AI Agents
In cmd/utils/flags.go around line 1754, replace the local flag access
ctx.String(StateSchemeFlag.Name) with the global flag accessor
ctx.GlobalString(StateSchemeFlag.Name) so the code consistently reads the global
flag value (keeping the same parameter names and error handling); update that
call only to use ctx.GlobalString and ensure any surrounding code that expects
the returned string remains unchanged.
| if cfg.StateScheme == rawdb.PathScheme { | ||
| trie.GenesisStateInPathZkTrie = true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Setting global variable as a side effect may cause issues.
Setting trie.GenesisStateInPathZkTrie = true as a side effect during config setup creates hidden global state mutation. This could cause problems:
- In tests if
SetEthConfigis called multiple times - Makes the system harder to reason about
- Could lead to unexpected behavior if config is reloaded
Consider whether this global should be set elsewhere (e.g., during trie initialization) or if it should be passed as a parameter rather than mutating global state.
Run the following script to check where else this global variable is accessed or modified:
🏁 Script executed:
#!/bin/bash
# Description: Find all references to GenesisStateInPathZkTrie to understand its usage pattern
rg -n "GenesisStateInPathZkTrie" --type go -C 3Length of output: 1975
Remove redundant global state mutation from config setup.
The assignment at lines 1759-1761 duplicates identical logic already present in eth/backend.go:144-146. Config setup should not mutate global state; this responsibility belongs in backend initialization where it's already handled. Remove this assignment from cmd/utils/flags.go to eliminate the side effect.
🤖 Prompt for AI Agents
In cmd/utils/flags.go around lines 1759 to 1761, remove the redundant global
state mutation setting trie.GenesisStateInPathZkTrie = true when cfg.StateScheme
== rawdb.PathScheme; this duplicates logic already performed in eth/backend.go
(lines ~144-146) and mutates global state during config setup. Edit the file to
delete those lines so the config setup no longer changes global variables,
leaving initialization of GenesisStateInPathZkTrie exclusively in backend
initialization.
| triedb := bc.stateCache.TrieDB() | ||
| flushInterval := time.Duration(bc.flushInterval.Load()) | ||
|
|
||
| // If we're running an archive node, always flush | ||
| if bc.cacheConfig.TrieDirtyDisabled { | ||
| if triedb.Scheme() == rawdb.PathScheme { | ||
| flush := false | ||
| if bc.lastFlushTime.Load() == 0 { | ||
| bc.lastFlushTime.Store(int64(block.Time())) | ||
| } else { | ||
| // Blocktime distance is not a good indicator of the time | ||
| // elapsed since the last commit, so we need to check | ||
| // the block time to see if we need to flush. | ||
| if int64(block.Time()) > bc.lastFlushTime.Load() { | ||
| distance := time.Duration(int64(block.Time())-bc.lastFlushTime.Load()) * time.Second | ||
|
|
||
| // If the distance is greater than the flush interval, we need to flush | ||
| if distance > flushInterval { | ||
| flush = true | ||
| log.Info("State in memory for too long, committing", "distance", distance, "allowance", flushInterval, "last flush time", bc.lastFlushTime.Load(), "blocktime", block.Time()) | ||
|
|
||
| // Reset the last flush time | ||
| bc.lastFlushTime.Store(0) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Time-based flush bookkeeping: set lastFlushTime to current block time on flush
Resetting bc.lastFlushTime to 0 after a flush loses the flush timestamp and causes the next block to reinitialize it ambiguously. Store the current block time instead for clearer semantics and stable intervals.
Apply this diff:
- // Reset the last flush time
- bc.lastFlushTime.Store(0)
+ // Record flush time at this block
+ bc.lastFlushTime.Store(int64(block.Time()))Optional: initialize lastFlushTime to the first observed block time and always compare against it.
Also applies to: 147-151
🤖 Prompt for AI Agents
In core/blockchain_l2.go around lines 120-146 (also applies to 147-151): the
code resets bc.lastFlushTime to 0 after performing a flush which loses the
timestamp and causes the next block to reinitialize it ambiguously; instead set
bc.lastFlushTime to the current block time (int64(block.Time())) when you flush
so future distance calculations compare against the actual last flush time
(optionally ensure bc.lastFlushTime is initialized to the first observed block
time when previously zero).
| // triedbConfig derives the configures for trie database. | ||
| func (c *CacheConfig) triedbConfig(zktrie bool) *trie.Config { | ||
| config := &trie.Config{ | ||
| Cache: c.TrieCleanLimit, | ||
| Journal: c.TrieCleanJournal, | ||
| Preimages: c.Preimages, | ||
| Zktrie: zktrie, | ||
| PathZkTrie: zktrie && c.StateScheme == rawdb.PathScheme, | ||
| } | ||
| if config.PathZkTrie { | ||
| config.PathDB = &pathdb.Config{ | ||
| SyncFlush: c.PathSyncFlush, | ||
| CleanCacheSize: c.TrieCleanLimit * 1024 * 1024, | ||
| DirtyCacheSize: c.TrieCleanLimit * 1024 * 1024, | ||
| JournalFilePath: c.JournalFilePath, | ||
| } | ||
| } else { | ||
| if config.Zktrie { | ||
| config.HashDB = &hashdb.Config{ | ||
| Cache: c.TrieCleanLimit, | ||
| Journal: c.TrieCleanJournal, | ||
| } | ||
| } | ||
| } | ||
| return config | ||
| } |
There was a problem hiding this comment.
DirtyCacheSize uses TrieCleanLimit — likely incorrect
In triedbConfig for PathDB, DirtyCacheSize is derived from TrieCleanLimit instead of TrieDirtyLimit. This misallocates memory and may hinder flushing/GC behavior.
Apply this diff:
- CleanCacheSize: c.TrieCleanLimit * 1024 * 1024,
- DirtyCacheSize: c.TrieCleanLimit * 1024 * 1024,
+ CleanCacheSize: c.TrieCleanLimit * 1024 * 1024,
+ DirtyCacheSize: c.TrieDirtyLimit * 1024 * 1024,🤖 Prompt for AI Agents
In core/blockchain.go around lines 156 to 181, the PathDB configuration
incorrectly sets DirtyCacheSize using TrieCleanLimit; change DirtyCacheSize to
derive from TrieDirtyLimit (i.e., DirtyCacheSize: c.TrieDirtyLimit * 1024 *
1024) while keeping CleanCacheSize as TrieCleanLimit, so the dirty cache uses
the intended limit and memory is allocated correctly.
| var addrPoseidonHash common.Hash | ||
| if db.db.TrieDB().IsPathZkTrie() { | ||
| addressKey, _ := zkt.ToSecureKey(address.Bytes()) | ||
| addrPoseidonHash = common.BigToHash(addressKey) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Handle the error from zkt.ToSecureKey.
The error returned by zkt.ToSecureKey is silently ignored. If this conversion fails, addrPoseidonHash will be set to an incorrect zero hash, which could lead to storage trie corruption or incorrect proof generation for path-based zk-tries.
Apply this diff to handle the error:
var addrPoseidonHash common.Hash
if db.db.TrieDB().IsPathZkTrie() {
- addressKey, _ := zkt.ToSecureKey(address.Bytes())
+ addressKey, err := zkt.ToSecureKey(address.Bytes())
+ if err != nil {
+ // This should never happen for valid addresses, but handle defensively
+ log.Crit("Failed to compute Poseidon hash for address", "address", address, "error", err)
+ }
addrPoseidonHash = common.BigToHash(addressKey)
}Verify whether zkt.ToSecureKey can actually fail for valid Ethereum addresses:
🏁 Script executed:
#!/bin/bash
# Search for ToSecureKey implementation and error cases
rg -nP -A10 'func\s+ToSecureKey' --type=goLength of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Broader search for ToSecureKey without pattern constraints
rg -nP 'ToSecureKey' --type=go -C5
Length of output: 8366
🏁 Script executed:
#!/bin/bash
# Find where zkt package is imported and used
rg -nP 'import.*zkt|from.*zkt' --type=go -C3Length of output: 497
🏁 Script executed:
#!/bin/bash
# Search for the zkt package definition
fd --type=f -name '*.go' | xargs grep -l 'package.*zkt\|ToSecureKey.*func' 2>/dev/null | head -20Length of output: 230
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for zkt import statements to identify package source
rg -nP 'import\s+\(' --type=go -A20 | grep -A20 'state_object\|state_prove\|statedb' | head -50
Length of output: 2365
🏁 Script executed:
#!/bin/bash
# Find where zkt package comes from
rg -nP '^import' --type=go -B2 -A15 core/state/state_object.go | head -40Length of output: 429
🏁 Script executed:
#!/bin/bash
# Check for documentation or comments around the ignored errors
rg -nP 'addressKey.*ToSecureKey|key_s.*ToSecureKeyBytes' --type=go -B3 -A3Length of output: 1788
🏁 Script executed:
#!/bin/bash
# Check git history to see if there's any context around these error-ignoring patterns
git log -p --all -S 'addressKey, _' -- 'core/state/state_object.go' | head -100Length of output: 59
🏁 Script executed:
#!/bin/bash
# Search for any README or documentation about zktrie error handling
rg -i 'tosecurekey|error handling|zktrie' --type=md --type=txt 2>/dev/null | head -20Length of output: 531
Handle the error from zkt.ToSecureKey to prevent storage trie corruption.
The error returned by zkt.ToSecureKey is silently ignored in core/state/state_object.go:122. This same external function is properly handled in other parts of the codebase (trie/zk_trie.go:211, trie/path_zk_trie.go:204, core/state/state_prove.go:58), confirming it can fail. If conversion fails, addrPoseidonHash will be zero, leading to storage trie corruption or incorrect proof generation.
Additionally, this error-ignoring pattern exists in multiple files within core/state/ (state_prove.go:25, state_prove.go:91, statedb.go:342), suggesting a broader refactoring should address all instances.
Apply the suggested diff to state_object.go:122, and consider applying the same fix to the other identified locations in the core/state/ package.
🤖 Prompt for AI Agents
In core/state/state_object.go around lines 120-124, the call to zkt.ToSecureKey
ignores its error which can produce a zero addrPoseidonHash; change to capture
the error (addressKey, err := zkt.ToSecureKey(...)) and if err != nil propagate
it (return or wrap the error from the current function) instead of proceeding,
otherwise set addrPoseidonHash = common.BigToHash(addressKey). Apply the same
error-handling pattern to the other occurrences in core/state (state_prove.go,
statedb.go) so the functions consistently return or propagate the conversion
error rather than using a zero hash.
| // Set path trie tag before using trie db | ||
| if config.StateScheme == rawdb.PathScheme { | ||
| trie.GenesisStateInPathZkTrie = true | ||
| } | ||
|
|
There was a problem hiding this comment.
Global toggle may leak across runs; set explicitly for both branches
trie.GenesisStateInPathZkTrie is set only when scheme == path and never reset otherwise; in multi-init/test scenarios within one process it may remain true unintentionally.
Apply this diff to make it explicit:
- // Set path trie tag before using trie db
- if config.StateScheme == rawdb.PathScheme {
- trie.GenesisStateInPathZkTrie = true
- }
+ // Set path trie tag before using trie db
+ trie.GenesisStateInPathZkTrie = (config.StateScheme == rawdb.PathScheme)🤖 Prompt for AI Agents
In eth/backend.go around lines 143 to 147, trie.GenesisStateInPathZkTrie is only
set to true when config.StateScheme == rawdb.PathScheme which can leak across
runs; explicitly set the flag in both branches by assigning
trie.GenesisStateInPathZkTrie = true when scheme is PathScheme and
trie.GenesisStateInPathZkTrie = false in the else branch so the global state is
deterministic for repeated in-process initializations.
| if statedb.IsPathZkTrie() { | ||
| return nil, fmt.Errorf("path trie not implemention, block=%d", block.NumberU64()) | ||
| } |
There was a problem hiding this comment.
Fix typo in error message.
The error message contains "not implemention" which should be "not implemented".
Apply this diff:
- return nil, fmt.Errorf("path trie not implemention, block=%d", block.NumberU64())
+ return nil, fmt.Errorf("path trie not implemented, block=%d", block.NumberU64())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if statedb.IsPathZkTrie() { | |
| return nil, fmt.Errorf("path trie not implemention, block=%d", block.NumberU64()) | |
| } | |
| if statedb.IsPathZkTrie() { | |
| return nil, fmt.Errorf("path trie not implemented, block=%d", block.NumberU64()) | |
| } |
🤖 Prompt for AI Agents
In rollup/tracing/tracing.go around lines 52 to 54, the error string "path trie
not implemention" contains a typo; update the message to "path trie not
implemented" so the returned error becomes fmt.Errorf("path trie not
implemented, block=%d", block.NumberU64()) preserving the existing formatting
and block number interpolation.
2 participants
1. Purpose or design rationale of this PR
...
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
New Features
Performance
CLI
Tests
Chores