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 }
triedb/pathdb/database.go
Outdated
| 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()
triedb/pathdb/database.go
Outdated
| // 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"` |
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
Improvements
Bug Fixes
Dependency Updates