fix(trie): make TrieImpl.insert() idempotent for duplicate key#126
Open
halibobo1205 wants to merge 2 commits intodevelopfrom
Open
fix(trie): make TrieImpl.insert() idempotent for duplicate key#126halibobo1205 wants to merge 2 commits intodevelopfrom
halibobo1205 wants to merge 2 commits intodevelopfrom
Conversation
…puts
Reorder condition checks in insert() so that commonPrefix.equals(k) is
evaluated before commonPrefix.isEmpty(). When both k and currentNodeKey
are empty (which happens on a duplicate put of a fully-split key), the
old order incorrectly fired the "no common prefix" branch and replaced
KVNode("",v) with BranchNode{terminal:v}, corrupting the root hash.
Also short-circuit kvNodeSetValueOrNode() when the new value equals the
existing one (by reference or by byte content) to avoid unnecessary
dirty marking and downstream hash recomputation.
Re-enable testOrder() with both deterministic regression sequences and
1000 random shuffles. Fix test() to expect rootHash equality after a
same-value re-put.
Refs tronprotocol#6608
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
the
size:L
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+876
to
+880
| if (children[1] == valueOrNode) { | ||
| return this; | ||
| } | ||
| if (valueOrNode instanceof byte[] && children[1] instanceof byte[] | ||
| && java.util.Arrays.equals((byte[]) children[1], (byte[]) valueOrNode)) { |
There was a problem hiding this comment.
Suggestion: The reference-equality short-circuit in kvNodeSetValueOrNode() is unsafe for child Node updates: delete/insert can mutate a child node in place and return the same object reference, and this early return skips marking the parent dirty. That leaves ancestor hashes stale because encode short-circuits on dirty == false and won't traverse the modified child. Restrict the no-op shortcut to byte-array values only. [logic error]
Severity Level: Critical 🚨
- ❌ TrieImpl root hash can ignore subtree updates.
- ❌ Merkle proofs built from trie may be inconsistent.
- ⚠️ Any external consumer relying on TrieImpl affected.
- ⚠️ Bug arises on normal put/delete usage under KV nodes.
Suggested change
| if (children[1] == valueOrNode) { | |
| return this; | |
| } | |
| if (valueOrNode instanceof byte[] && children[1] instanceof byte[] | |
| && java.util.Arrays.equals((byte[]) children[1], (byte[]) valueOrNode)) { | |
| if (valueOrNode instanceof byte[] && children[1] instanceof byte[] | |
| && (children[1] == valueOrNode | |
| || java.util.Arrays.equals((byte[]) children[1], (byte[]) valueOrNode))) { |
Steps of Reproduction ✅
1. In any consumer (or a new test), construct a trie using the public API `TrieImpl.put()`
at `framework/src/main/java/org/tron/core/trie/TrieImpl.java:144-155`, with keys such that
`insert()` takes the "partial common prefix" branch and creates a KV node whose value is a
child `Node`. This happens in `insert()` at `TrieImpl.java:194-200`, where `newKvNode =
new Node(commonPrefix, newBranchNode);` creates a KVNodeNode whose `children[1]` is a
`Node`.
2. Call `trie.getRootHash()` (entry point `TrieImpl.getRootHash()` at
`TrieImpl.java:285-289`). This invokes `encode()` at `TrieImpl.java:84-87`, which walks
the tree, sets hashes, and sets `dirty=false` on all nodes via `Node.encode(...):710` so
that parent KV nodes (including the KVNodeNode created in step 1) now have `dirty ==
false` and a cached `hash`/`rlp`.
3. Delete a key that lies strictly under that KVNodeNode's subtree by calling
`TrieImpl.delete(byte[] key)` at `TrieImpl.java:205-211`. The recursive `delete(Node n,
TrieKey k)` at `TrieImpl.java:213-283` will:
- Recurse into the child `Node` via `n.kvNodeGetChildNode()` when the parent is a
KVNodeNode (`delete(...)` else-branch at `TrieImpl.java:260-262`),
- Mutate that child subtree in place (e.g., updating a `BranchNode` via
`branchNodeSetChild()` / `branchNodeSetValue()` at `TrieImpl.java:779-809`, which set
`child.dirty = true` but often return the same `Node` instance from `delete()` at
`TrieImpl.java:226-237`),
- Then call `n.kvNodeSetValueOrNode(newChild)` at `TrieImpl.java:265` with `newChild`
being the same `Node` reference already stored in `children[1]`.
4. In the current PR code, `kvNodeSetValueOrNode()` at `TrieImpl.java:873-885` first
checks `if (children[1] == valueOrNode) { return this; }` (lines 876-878) and returns
early without setting `dirty = true`. As a result, the parent KVNodeNode remains `dirty ==
false`. A subsequent `trie.getRootHash()` again calls `encode()` (lines 640-719), but
because the parent KV node is not dirty, `encode()` short-circuits at
`TrieImpl.java:645-647` and returns the cached hash/rlp for that node without recursing
into the now-dirty child. The trie's root hash therefore does not reflect the deletion,
even though the subtree contents changed. This behavior directly follows from the existing
code paths and is not tested away: `TrieImpl` is only referenced from
`framework/src/test/java/org/tron/core/tire/TrieTest.java` (verified via Grep), so any
external consumer or new test using `put()`, `delete()`, and `getRootHash()` in this
pattern will hit this bug.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/main/java/org/tron/core/trie/TrieImpl.java
**Line:** 876:880
**Comment:**
*Logic Error: The reference-equality short-circuit in `kvNodeSetValueOrNode()` is unsafe for child `Node` updates: delete/insert can mutate a child node in place and return the same object reference, and this early return skips marking the parent dirty. That leaves ancestor hashes stale because encode short-circuits on `dirty == false` and won't traverse the modified child. Restrict the no-op shortcut to byte-array values only.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
Owner
Author
|
@codex: review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
halibobo1205
changed the title
Remove the bare reference-equality check (`children[1] == valueOrNode`) that was unsafe for child Node updates. delete() can mutate a child node in place and return the same object reference; the early return skipped marking the parent dirty, leaving ancestor hashes stale. Merge the reference check into the byte[] branch so only leaf values (byte[]) are short-circuited. Also replace fully-qualified java.util.Arrays with the existing import. Add testDeleteDirtyPropagation to verify delete properly propagates dirty flags through KVNodeNode parents. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Owner
Author
|
@codex: review |
codeant-ai
bot
added
size:L
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
CodeAnt AI Incremental review completed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Reorder condition checks in insert() so that commonPrefix.equals(k) is evaluated before commonPrefix.isEmpty(). When both k and currentNodeKey are empty (which happens on a duplicate put of a fully-split key), the old order incorrectly fired the "no common prefix" branch and replaced KVNode("",v) with BranchNode{terminal:v}, corrupting the root hash.
Also short-circuit kvNodeSetValueOrNode() when the new value equals the existing one (by reference or by byte content) to avoid unnecessary dirty marking and downstream hash recomputation.
Re-enable testOrder() with both deterministic regression sequences and 1000 random shuffles. Fix test() to expect rootHash equality after a same-value re-put.
CodeAnt-AI Description
Keep trie root hashes stable for duplicate writes and deletions
What Changed
Impact
✅ Stable root hashes after duplicate writes✅ Fewer stale trie hashes after delete✅ Stronger protection against insert-order regressions💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.