-
Notifications
You must be signed in to change notification settings - Fork 122
fix: avoid collision in PartialAccountTree
#2598
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
Open
igamigo
wants to merge
2
commits into
release/v0.14.0-beta
Choose a base branch
from
igamigo-fix-partial-smt
base: release/v0.14.0-beta
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+53
−9
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,15 +111,20 @@ impl PartialAccountTree { | |
| let id_prefix = witness.id().prefix(); | ||
| let id_key = AccountIdKey::from(witness.id()).as_word(); | ||
|
|
||
| // If a leaf with the same prefix is already tracked by this partial tree, consider it an | ||
| // If there exists a tracked leaf with a non-empty entry whose key differs from the one | ||
| // we're about to track, then two different account IDs share the same prefix, which is an | ||
| // error. | ||
| // | ||
| // We return an error even for empty leaves, because tracking the same ID prefix twice | ||
| // indicates that different IDs are attempted to be tracked. It would technically not | ||
| // violate the invariant of the tree that it only tracks zero or one entries per leaf, but | ||
| // since tracking the same ID twice should practically never happen, we return an error, out | ||
| // of an abundance of caution. | ||
| if self.smt.get_leaf(&id_key).is_ok() { | ||
| // Note that if the leaf is empty, that's fine: `PartialSmt::get_leaf` returns | ||
| // `Ok(SmtLeaf::Empty)` for any leaf position reachable through provably-empty subtrees, | ||
| // even if no proof was explicitly added for that position. In a sparse tree this covers | ||
| // most of the leaf space, so treating empty leaves as duplicates would reject nearly every | ||
| // second witness. | ||
| // | ||
| // Also note that the multiple variant cannot occur by construction of the account tree. | ||
| if let Ok(SmtLeaf::Single((existing_key, _))) = self.smt.get_leaf(&id_key) | ||
| && id_key != existing_key | ||
| { | ||
| return Err(AccountTreeError::DuplicateIdPrefix { duplicate_prefix: id_prefix }); | ||
| } | ||
|
|
||
|
|
@@ -275,6 +280,41 @@ mod tests { | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Verifies that tracking multiple witnesses succeeds in a sparse tree, where most leaf | ||
| /// positions are reachable through provably-empty subtrees, including `SmtLeaf::Empty` | ||
| /// leaves that are provably empty but not actually occupied. | ||
| #[test] | ||
| fn track_succeeds_for_multiple_witnesses_in_sparse_tree() -> anyhow::Result<()> { | ||
| let id0 = AccountIdBuilder::default().build_with_seed([10; 32]); | ||
| let id1 = AccountIdBuilder::default().build_with_seed([11; 32]); | ||
| let id2 = AccountIdBuilder::default().build_with_seed([12; 32]); | ||
|
|
||
| let commitment0 = Word::from([1, 2, 3, 4u32]); | ||
| let commitment1 = Word::from([5, 6, 7, 8u32]); | ||
|
|
||
| // Create a tree with only one account (very sparse). | ||
| let account_tree = AccountTree::with_entries([(id0, commitment0)])?; | ||
|
|
||
| // Get witnesses for one existing and two new (empty) accounts. | ||
| let witness0 = account_tree.open(id0); | ||
| let witness1 = account_tree.open(id1); | ||
| let witness2 = account_tree.open(id2); | ||
|
|
||
| // Building a partial tree from all three witnesses should succeed: | ||
| // id1 and id2 have empty leaves that are provably empty via the sparse tree structure, | ||
| // but they are NOT duplicates of id0. | ||
| let mut partial_tree = PartialAccountTree::with_witnesses([witness0, witness1, witness2])?; | ||
|
|
||
|
Comment on lines
+306
to
+307
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add a |
||
| // Verify the existing account has its commitment. | ||
| assert_eq!(partial_tree.get(id0)?, commitment0); | ||
|
|
||
| // We should be able to insert new state commitments for the new accounts. | ||
| partial_tree.upsert_state_commitments([(id1, commitment1)])?; | ||
| assert_eq!(partial_tree.get(id1)?, commitment1); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn track_fails_on_duplicate_prefix() { | ||
| // Use a raw Smt since an account tree would not allow us to get the witnesses for two | ||
|
|
||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking out loud: This allows adding the same account witness twice, but that should be fine. Since
PartialSmt::add_proofwill error if a root mismatch occurs, the only allowed case is if the previously added account witness is the same as the current one, so we could technically return early ifid_key == existing_key. Though, I'd leave things as is to avoid introducing special logic that needs to be tested, etc.