fix: fix two issues in trie iterator (ethereum/go-ethereum#24539)

[panos-xyz]

1. Purpose or design rationale of this PR

Sync from upstream (ethereum/go-ethereum#24539)

1. Fix memory leak in trie iterator
   In the trie iterator, live nodes are tracked in a stack while iterating.
   Popped node states should be explicitly set to nil in order to get
   garbage-collected.

2. Fix empty trie iterator
   The empty trie shouldn't be iterated out any values. Changed the check
   from incorrect 'emptyState' to correct 'emptyRoot', and return an
   iterator with errIteratorEnd to indicate iteration is complete.

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:

• build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
• ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
• docs: Documentation-only changes
• feat: A new feature
• fix: A bug fix
• perf: A code change that improves performance
• refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
• style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
• test: Adding missing tests or correcting existing tests

3. Deployment tag versioning

Has the version in params/version.go been updated?

• This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
• Yes

4. Breaking change label

Does this PR have the breaking-change label?

• This PR is not a breaking change
• Yes

Summary by CodeRabbit

• Bug Fixes

  • Fixed iterator behavior and internal state cleanup during traversal of empty structures.

• Tests

  • Added comprehensive test for iterator functionality on empty data structures.

• Refactor

  • Optimized internal state management and streamlined dependency structure.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The changes modify the trie iterator to properly initialize an empty iterator state when the trie root is empty, refactor the pop operation to explicitly clear stack entries before resizing, remove unused crypto imports and variables, and add a test to verify empty trie iteration behavior.

Changes

Cohort / File(s)	Change Summary
Iterator Logic Updates trie/iterator.go	Modified early return for empty trie root to initialize nodeIterator with trie and err set to end-of-iteration; refactored pop to reference the last stack element explicitly, adjust path length, nil the entry, and then shrink the stack.
Iterator Testing trie/iterator_test.go	Added TestEmptyIterator test to verify that iterating over an empty trie yields no nodes.
Package Cleanup trie/trie.go	Removed unused crypto import and emptyState variable; relocated emptyRoot variable declaration with updated comment placement.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

• trie/iterator.go: Core iterator initialization and stack management changes warrant close inspection of the logic, particularly the pop operation's stack cleanup sequence and correctness of the empty trie handling.
• Stack manipulation in pop: The explicit reference, path adjustment, nil assignment, and slice resizing pattern should be verified for correctness and consistency.

Poem

🐰 Through empty paths the iterator now roams,
With careful stack pops and well-ordered homes,
No stray references left behind,
Cleaner imports and variables aligned,
A trie refined, so tidy and true! 🌿

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: fixing two issues in the trie iterator. It is concise, specific, and references the upstream issue.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/trie-iterator

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49fa27b and c4f8785.

📒 Files selected for processing (3)

• trie/iterator.go (2 hunks)
• trie/iterator_test.go (1 hunks)
• trie/trie.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

trie/iterator_test.go (1)

trie/iterator.go (1)

• NodeIterator (69-119)

trie/trie.go (1)

common/types.go (1)

• HexToHash (66-66)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Analyze (go)

🔇 Additional comments (4)

trie/iterator_test.go (1)

32-43: LGTM! Good test coverage for the empty trie iterator fix.

The test correctly validates that iterating over an empty trie yields no nodes. The implementation follows established test patterns in this file.

trie/trie.go (1)

32-33: LGTM! Clean refactoring of the emptyRoot constant.

The emptyRoot hash value is the well-known Keccak256 hash of the RLP-encoded empty string, which is the canonical empty trie root in Ethereum. Removing the crypto import dependency for this constant simplifies the code.

trie/iterator.go (2)

154-159: Correctly fixes empty trie iterator handling.

The fix properly returns an iterator in the end-of-iteration state for empty tries. With err: errIteratorEnd:

• Next() returns false immediately (line 245-247)
• Error() returns nil (line 231-232), correctly indicating no error occurred

Setting trie: trie ensures the iterator remains valid for any method calls that might access the trie reference.

---

485-490: Good fix for the memory leak in pop().

The changes correctly address the memory leak:

1. Line 486-487: Captures the popped element first, then uses its pathlen to restore it.path to the correct length before this node was pushed.

2. Line 488: Setting the slot to nil before shrinking is essential—without this, the underlying slice array retains a reference to the *nodeIteratorState, preventing garbage collection of the state and its node field.

This pattern is the idiomatic Go approach for preventing memory leaks when shrinking slices that hold pointers.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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