refactor(core/rawdb): improve db APIs for accessing trie nodes #29362 by gzliudan · Pull Request #2313 · XinFinOrg/XDPoSChain

gzliudan · 2026-04-17T05:57:17Z

Proposed changes

Ref: ethereum#29362

Types of changes

What types of changes does your code introduce to XDC network?
Put an ✅ in the boxes that apply

Impacted Components

Which parts of the codebase does this PR touch?
Put an ✅ in the boxes that apply

Checklist

Put an ✅ in the boxes once you have confirmed below actions (or provide reasons on not doing so) that

This PR has sufficient test coverage (unit/integration test) OR I have provided reason in the PR description for not having test coverage
Tested on a private network from the genesis block and monitored the chain operating correctly for multiple epochs.
Provide an end-to-end test plan in the PR description on how to manually test it on the devnet/testnet.
Tested the backwards compatibility.
Tested with XDC nodes running this version co-exist with those running the previous version.
Relevant documentation has been updated as part of this PR
N/A

coderabbitai · 2026-04-17T05:57:24Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e8a395d6-730c-4c57-aa15-76d01e6be85b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

…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.

Copilot

Pull request overview

Refactors rawdb trie-node access APIs (especially path-scheme) and consolidates test-only hashing helpers into a shared internal package.

Changes:

Simplify path-scheme trie node accessors to return only node blobs and recompute hashes where needed.
Centralize the test hasher implementation under internal/blocktest and reuse it across multiple test packages.
Remove the unused NodeSet.Hashes() helper.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`trie/trienode/node.go`	Removes unused `NodeSet.Hashes()` helper.
`core/rawdb/accessors_trie.go`	Refactors trie node read/has APIs and hashing helper usage for path-scheme.
`internal/blocktest/test_hash.go`	Adds shared test hasher implementation (used to avoid import cycles).
`internal/ethapi/api_test.go`	Switches to shared `blocktest.NewHasher` helper.
`core/types/block_test.go`	Switches benchmark block construction to shared `blocktest.NewHasher`.
`core/rawdb/accessors_indexes_test.go`	Switches test block construction to shared `blocktest.NewHasher`.
`core/rawdb/accessors_chain_test.go`	Uses shared `newTestHasher()` in body/block storage tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T06:00:55Z

+		var blob []byte
 		if owner == (common.Hash{}) {
-			return HasAccountTrieNode(db, path, hash)
+			blob = ReadAccountTrieNode(db, path)
+		} else {
+			blob = ReadStorageTrieNode(db, owner, path)
 		}
-		return HasStorageTrieNode(db, owner, path, hash)
+		if len(blob) == 0 {
+			return false
+		}
+		h := newHasher()
+		defer h.release()
+		return h.hash(blob) == hash // exists but not match


In the PathScheme branch, existence is inferred via len(blob)==0 after calling Read*TrieNode (which currently ignores db.Get errors). This makes "missing" depend on the stored value length (and treats a present-but-empty value as absent) and also prevents distinguishing genuine DB read errors from NotFound. Prefer checking the db.Get error (or a boolean ok) to decide existence, then hash the returned bytes (even if empty).

Copilot · 2026-04-17T06:00:55Z

+		}
+		h := newHasher()
+		defer h.release()
+		return h.hash(blob) == hash // exists but not match


This inline comment is misleading: the function returns whether the computed hash matches the provided hash; it doesn't specifically indicate the "exists but not match" case. Please reword the comment (or remove it) so it describes the boolean result accurately.

Suggested change

return h.hash(blob) == hash // exists but not match

return h.hash(blob) == hash // whether the stored node matches the provided hash

Copilot · 2026-04-17T06:00:56Z

+// Package utesting provides a standalone replacement for package testing.
+//
+// This package exists because package testing cannot easily be embedded into a
+// standalone go program. It provides an API that mirrors the standard library
+// testing API.


The file-level package comment appears to be copy/pasted and refers to "Package utesting" and a replacement for the standard testing package, but this file declares package blocktest and implements a test hasher. Please update or remove this comment block so that it accurately describes the blocktest package (or move the correct package comment to a dedicated doc.go).

Suggested change

// Package utesting provides a standalone replacement for package testing.

//

// This package exists because package testing cannot easily be embedded into a

// standalone go program. It provides an API that mirrors the standard library

// testing API.

// Package blocktest provides helpers for block-related tests.

//

// In particular, it contains a lightweight hasher used for transaction and

// receipt list hashing in tests without importing the trie hasher and creating

// an import cycle.

Copilot · 2026-04-17T06:00:56Z

+// hasher is used to compute the sha256 hash of the provided data.
+type hasher struct{ sha crypto.KeccakState }

 var hasherPool = sync.Pool{
-	New: func() interface{} { return &nodeHasher{sha: keccak.NewLegacyKeccak256().(crypto.KeccakState)} },
+	New: func() interface{} { return &hasher{sha: crypto.NewKeccakState()} },


The comment says this helper computes a sha256 hash, but the implementation uses Keccak (via crypto.NewKeccakState/crypto.HashData). Please update the comment (and any related naming) to avoid misleading future maintainers about the hashing algorithm used here.

Copilot · 2026-04-17T06:00:56Z

+
+// ReadAccountTrieNode retrieves the account trie node with the specified node path.
+func ReadAccountTrieNode(db ethdb.KeyValueReader, path []byte) []byte {
+	data, _ := db.Get(accountTrieNodeKey(path))


ReadAccountTrieNode discards the db.Get error and returns the raw data unconditionally. Downstream code uses len(data)==0 to infer absence, which changes semantics if the key exists with an empty value and also hides non-NotFound read errors. Consider returning (data, error)/(data, ok) or ensuring callers check the Get error rather than using len(data) as the existence check.

Suggested change

data, _ := db.Get(accountTrieNodeKey(path))

key := accountTrieNodeKey(path)

has, err := db.Has(key)

if err != nil {

log.Crit("Failed to check account trie node", "err", err)

}

if !has {

return nil

}

data, err := db.Get(key)

if err != nil {

log.Crit("Failed to read account trie node", "err", err)

}

Copilot · 2026-04-17T06:00:56Z

+// ReadStorageTrieNode retrieves the storage trie node with the specified node path.
+func ReadStorageTrieNode(db ethdb.KeyValueReader, accountHash common.Hash, path []byte) []byte {
+	data, _ := db.Get(storageTrieNodeKey(accountHash, path))
+	return data


ReadStorageTrieNode discards the db.Get error and returns the raw data unconditionally. Callers later treat len(data)==0 as "missing", which is not equivalent to "key not found" and can also mask non-NotFound DB read failures. Consider returning (data, error)/(data, ok) or ensure callers check the Get error.

…um#29362

Copilot AI review requested due to automatic review settings April 17, 2026 05:57

gzliudan requested review from AnilChinchawale, anunay-xin, benjamin202410, liam-lai and wanwiset25 April 17, 2026 05:57

Copilot started reviewing on behalf of gzliudan April 17, 2026 05:57 View session

refactor(internal/blocktest): add package for shared test code ethere…

e96e944

…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.

Copilot AI reviewed Apr 17, 2026

View reviewed changes

refactor(core/rawdb): improve db APIs for accessing trie nodes ethere…

01d9676

…um#29362

gzliudan force-pushed the change-trienode-db-api branch from 5ef7710 to 01d9676 Compare April 17, 2026 06:20

wanwiset25 approved these changes Apr 20, 2026

View reviewed changes

anunay-xin approved these changes Apr 20, 2026

View reviewed changes

benjamin202410 approved these changes Apr 25, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core/rawdb): improve db APIs for accessing trie nodes #29362#2313

refactor(core/rawdb): improve db APIs for accessing trie nodes #29362#2313
gzliudan wants to merge 2 commits intoXinFinOrg:dev-upgradefrom
gzliudan:change-trienode-db-api

gzliudan commented Apr 17, 2026

Uh oh!

coderabbitai Bot commented Apr 17, 2026 •

edited

Loading

Review skipped

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	return h.hash(blob) == hash // exists but not match
	return h.hash(blob) == hash // whether the stored node matches the provided hash

-	data, _ := db.Get(accountTrieNodeKey(path))
+	key := accountTrieNodeKey(path)
+	has, err := db.Has(key)
+	if err != nil {
+		log.Crit("Failed to check account trie node", "err", err)
+	}
+	if !has {
+		return nil
+	}
+	data, err := db.Get(key)
+	if err != nil {
+		log.Crit("Failed to read account trie node", "err", err)
+	}

Conversation

gzliudan commented Apr 17, 2026

Proposed changes

Types of changes

Impacted Components

Checklist

Uh oh!

coderabbitai Bot commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coderabbitai Bot commented Apr 17, 2026 •

edited

Loading