Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(trie): sparse trie tree masks #13760

Merged
merged 9 commits into from
Jan 10, 2025
Merged

Conversation

shekhirin
Copy link
Collaborator

@shekhirin shekhirin commented Jan 9, 2025

Two bugs:

  1. store_in_db_trie for branch nodes wasn't set when revealing a node
    store_in_db_trie: Some(
    hash_mask.is_some_and(|mask| !mask.is_empty()) ||
    tree_mask.is_some_and(|mask| !mask.is_empty()),
    ),
  2. Blinded children of a branch node should inherit the tree mask bit as-is
    // Determine whether we need to set trie mask bit.
    let should_set_tree_mask_bit =
    // A blinded node has the tree mask bit set
    (
    child_node_type.is_hash() &&
    self.branch_node_tree_masks
    .get(&path)
    .is_some_and(|mask| mask.is_bit_set(last_child_nibble))
    ) ||
    // A branch or an extension node explicitly set the
    // `store_in_db_trie` flag
    child_node_type.store_in_db_trie();

@shekhirin shekhirin added C-bug An unexpected or incorrect behavior A-trie Related to Merkle Patricia Trie implementation labels Jan 9, 2025
@shekhirin shekhirin marked this pull request as ready for review January 9, 2025 21:13
Copy link

codspeed-hq bot commented Jan 9, 2025

CodSpeed Performance Report

Merging #13760 will not alter performance

Comparing alexey/sparse-trie-tree-masks (f698d2c) with main (986c754)

Summary

✅ 77 untouched benchmarks

@shekhirin
Copy link
Collaborator Author

@codspeed-hq thanks bro but I don't think it affects the perf


Ok(MultiProof { account_subtree, branch_node_hash_masks, storages })
let (branch_node_hash_masks, branch_node_tree_masks) =
if self.collect_branch_node_hash_masks {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

need to rename the field so it means collecting both masks

@shekhirin shekhirin enabled auto-merge January 10, 2025 11:06
@shekhirin shekhirin added this pull request to the merge queue Jan 10, 2025
Merged via the queue into main with commit 69f9e16 Jan 10, 2025
43 checks passed
@shekhirin shekhirin deleted the alexey/sparse-trie-tree-masks branch January 10, 2025 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trie Related to Merkle Patricia Trie implementation C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants