feat(core): create global hasher pool #31769#2314
feat(core): create global hasher pool #31769#2314gzliudan wants to merge 3 commits intoXinFinOrg:dev-upgradefrom
Conversation
…um#27270 Move the testHasher helper to internal/blocktest/test_hash.go and update all usages to import from there. This reduces code duplication and improves maintainability.
Replace pooled and embedded Keccak state with stateless crypto helpers. This simplifies trie node hashing and EVM keccak handling while keeping focused package tests green.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410 and
wanwiset25
There was a problem hiding this comment.
Pull request overview
This PR refactors several hashing call sites to use shared/global Keccak hashing utilities (instead of per-component hashers/pools) and deduplicates test-only “list hasher” helpers into a single internal package.
Changes:
- Switch EVM KECCAK256 opcode and CREATE2 init-code hashing to
crypto.Keccak256Hash(pool-backed) and remove the per-EVM shared hasher fields. - Simplify trie node accessors for path-scheme by computing node hashes via
crypto.Keccak256Hashand removing the local hasher pool (with accompanying signature/semantics changes to exported helpers). - Centralize test hasher helpers into
internal/blocktestand update tests/benchmarks to use it.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
core/vm/instructions.go |
Uses global pooled Keccak hashing for the KECCAK256 opcode and preimage recording. |
core/vm/evm.go |
Removes per-EVM hasher state and uses Keccak256Hash for CREATE2 init-code hashing. |
core/rawdb/accessors_trie.go |
Removes local node-hasher pool and reworks path-scheme presence/read logic to hash via crypto.Keccak256Hash. |
trie/trienode/node.go |
Removes an exported NodeSet.Hashes helper. |
internal/blocktest/test_hash.go |
Adds centralized test helper hasher for DeriveSha-style list hashing. |
internal/ethapi/api_test.go |
Switches tests to use the shared blocktest.NewHasher. |
core/types/block_test.go |
Switches block benchmark helper to use blocktest.NewHasher. |
core/rawdb/accessors_indexes_test.go |
Switches rawdb tests to use the shared test hasher helper. |
core/rawdb/accessors_chain_test.go |
Switches rawdb tests to use the shared test hasher helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var blob []byte | ||
| if owner == (common.Hash{}) { | ||
| return HasAccountTrieNode(db, path, hash) | ||
| blob = ReadAccountTrieNode(db, path) | ||
| } else { | ||
| blob = ReadStorageTrieNode(db, owner, path) | ||
| } | ||
| if len(blob) == 0 { | ||
| return false | ||
| } | ||
| return HasStorageTrieNode(db, owner, path, hash) | ||
| return crypto.Keccak256Hash(blob) == hash // exist and match |
There was a problem hiding this comment.
In PathScheme, using len(blob) == 0 to decide non-existence can misclassify an existing key whose value is an empty byte-slice (ethdb Get does not guarantee non-empty values). Prefer checking the Get/Has error result to determine presence, then compute/compare the hash even for empty blobs.
| var blob []byte | ||
| if owner == (common.Hash{}) { | ||
| blob, nHash = ReadAccountTrieNode(db, path) | ||
| blob = ReadAccountTrieNode(db, path) | ||
| } else { | ||
| blob, nHash = ReadStorageTrieNode(db, owner, path) | ||
| blob = ReadStorageTrieNode(db, owner, path) | ||
| } | ||
| if nHash != hash { | ||
| if len(blob) == 0 { | ||
| return nil | ||
| } | ||
| if crypto.Keccak256Hash(blob) != hash { | ||
| return nil // exist but not match |
There was a problem hiding this comment.
ReadTrieNode treats len(blob)==0 as missing, which can incorrectly return nil for keys that exist with an empty value. Consider returning nil only on Get error and performing the hash comparison regardless of blob length (including empty).
| // ReadAccountTrieNode retrieves the account trie node with the specified node path. | ||
| func ReadAccountTrieNode(db ethdb.KeyValueReader, path []byte) []byte { | ||
| data, _ := db.Get(accountTrieNodeKey(path)) | ||
| return data | ||
| } | ||
|
|
||
| // HasAccountTrieNode checks the account trie node presence with the specified | ||
| // node path and the associated node hash. | ||
| func HasAccountTrieNode(db ethdb.KeyValueReader, path []byte, hash common.Hash) bool { | ||
| data, err := db.Get(accountTrieNodeKey(path)) | ||
| // HasAccountTrieNode checks the presence of the account trie node with the | ||
| // specified node path, regardless of the node hash. | ||
| func HasAccountTrieNode(db ethdb.KeyValueReader, path []byte) bool { | ||
| has, err := db.Has(accountTrieNodeKey(path)) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| hasher := newNodeHasher() | ||
| defer returnHasherToPool(hasher) | ||
| return hasher.hashData(data) == hash | ||
| return has |
There was a problem hiding this comment.
These are exported helpers in rawdb and this PR changes their signatures/semantics (e.g., ReadAccountTrieNode no longer returns the node hash; HasAccountTrieNode no longer verifies a provided hash). If any external packages consume these APIs, this will be a breaking change—consider keeping backward-compatible wrappers or deprecating first.
| // Update updates the hash state with the given key and value. | ||
| func (h *testHasher) Update(key, val []byte) error { | ||
| h.hasher.Write(bytes.Clone(key)) | ||
| h.hasher.Write(bytes.Clone(val)) | ||
| return nil |
There was a problem hiding this comment.
blocktest.testHasher.Update clones key and val on every call, adding allocations and potentially skewing benchmarks that use this helper (e.g. block encoding benchmarks). Since hash.Hash.Write consumes the bytes immediately and doesn’t retain them, the clones are unnecessary here; write the slices directly to avoid extra overhead.
| @@ -152,16 +152,6 @@ func (set *NodeSet) Size() (int, int) { | |||
| return set.updates, set.deletes | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Removing (*NodeSet).Hashes is a breaking change on an exported type in trienode. If downstream code (outside this repo) relies on it, this will break builds. Consider keeping it (possibly deprecated) or providing an alternative helper for callers that need the updated-node hashes.
| // Hashes returns the hashes of all non-deleted nodes in the set. | |
| // | |
| // Deprecated: retained for backward compatibility with downstream callers that | |
| // still rely on the exported helper on NodeSet. | |
| func (set *NodeSet) Hashes() []common.Hash { | |
| if set == nil || len(set.Nodes) == 0 { | |
| return nil | |
| } | |
| hashes := make([]common.Hash, 0, len(set.Nodes)) | |
| for _, node := range set.Nodes { | |
| if node == nil || node.IsDeleted() { | |
| continue | |
| } | |
| hashes = append(hashes, node.Hash) | |
| } | |
| sort.Slice(hashes, func(i, j int) bool { | |
| return hashes[i].Hex() < hashes[j].Hex() | |
| }) | |
| return hashes | |
| } |
Proposed changes
Ref: ethereum#31769
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that