Skip to content

Conversation

@sashass1315
Copy link

In BranchNode::remove() return None when the branch has zero children and no value after removal, instead of returning Mutated. Node::remove() relies on new_root.is_none() to mark the subtree as empty, and Trie::remove() uses that signal to reset the root to the empty trie (keccak(RLP_NULL)). LeafNode::remove() and ExtensionNode::remove() already adhere to this contract, so this change aligns BranchNode with the established semantics and prevents persisting a structurally empty branch at the root which would otherwise break the empty trie invariant.

@sashass1315 sashass1315 requested a review from a team as a code owner November 17, 2025 11:06
@mpaulucci
Copy link
Collaborator

is this a bug? A perf optimization? How did you get into this? Why l2?

@mpaulucci mpaulucci requested a review from Copilot November 18, 2025 14:51
Copilot finished reviewing on behalf of mpaulucci November 18, 2025 14:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug in BranchNode::remove() where an empty branch (no children and no value) would return Mutated instead of None. The fix ensures that when a branch becomes completely empty after removal, it properly signals this by returning None, which allows Node::remove() to propagate the empty state and Trie::remove() to reset the root to the empty trie hash.

Key changes:

  • Added an early return in BranchNode::remove() to check if the branch is empty after removal
  • Returns (None, value) when both children and value are empty, aligning with LeafNode and ExtensionNode behavior

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants