Skip to content

ExtensionNode::insert reads its child at the wrong database key #7174

Description

@MegaRedHand

ExtensionNode::insert reads its child using the extension node's own path as the database key, but NodeRef::commit stores that child under own_path ++ prefix. When the child is a NodeRef::Hash, the read misses and the insert fails with InconsistentTreeError::ExtensionNodeChildNotFound.

This is the same defect that #7173 fixes in Trie::get_node, in a different function. Found while reviewing that PR; left alone there because it needs its own test and the PR was already large.

Where

crates/common/trie/node/extension.rs, the match_index == 0 arm of ExtensionNode::insert:

} else if match_index == 0 {
    let mut new_node = if self.prefix.len() == 1 {
        self.child.clone()                          // <-- child's key is `consumed ++ prefix`
    } else {
        Node::from(ExtensionNode::new(self.prefix.offset(1), self.child.clone())).into()
    };
    let mut choices = BranchNode::EMPTY_CHOICES;
    let mut branch_node = if self.prefix.at(0) == 16 {
        match new_node.get_node_mut(db, path.consumed())? {   // <-- reads at `consumed`

NodeRef::commit (crates/common/trie/node.rs) keys an extension's child as:

Node::Extension(node) => {
    node.child.commit(path.concat(&node.prefix), acc, crypto);
}

So the correct read key is consumed ++ prefix, not consumed.

Reachability

Narrow. Both conditions must hold:

  • self.prefix.at(0) == 16, and since a leaf flag cannot appear mid-prefix this means self.prefix == [16], hence self.prefix.len() == 1 and new_node = self.child.clone(). In the len() > 1 branch new_node is a freshly built in-memory ExtensionNode, so get_node_mut takes the NodeRef::Node arm and the key is never used.
  • The child is a NodeRef::Hash. An embedded NodeRef::Node ignores the key.

An extension node whose entire prefix is the leaf flag is a ProofTrie shape rather than something the state trie produces, which is presumably why this has not been hit. Two reviewers found it independently while reviewing #7173; both rated it plausible-but-unexercised and neither constructed the trie.

Suggested fix

Read at consumed ++ prefix. Note this cannot be expressed as a slice of the cursor's path, so it needs a small local Vec, the way BranchNode::remove builds its surviving-child key after #7173.

Why it needs a test first

The reason the sibling bug in Trie::get_node shipped is that nothing exercised the code: before #7173 every trie proptest used Trie::new_temp(), which leaves all nodes as in-memory NodeRef::Node, so TrieDB::get was never called with a path key at all. #7173 adds proptest_reopened_trie_reads_and_writes_path_keys, which commits, reopens from the root hash and mutates, so the read-key surface is now covered for ordinary shapes. Whoever fixes this should first construct the prefix == [16] shape with a hashed child and watch it fail.

Found while reviewing #7173 (issue #5825).

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    L1Ethereum clientmerkleizationSection of performance which requires updating the trie to get the new state root

    Type

    No type

    Projects

    • Status
      No status

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions