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

PartialMmr::untrack panics #379

Closed
PhilippGackstatter opened this issue Feb 14, 2025 · 1 comment · Fixed by #382
Closed

PartialMmr::untrack panics #379

PhilippGackstatter opened this issue Feb 14, 2025 · 1 comment · Fixed by #382
Assignees
Labels
bug Something isn't working

Comments

@PhilippGackstatter
Copy link
Contributor

#[test]
fn test_partial_mmr_untrack() {
    // build the MMR
    let mmr: Mmr = LEAVES.into();

    // get path and node for position 1
    let node1 = mmr.get(1).unwrap();
    let proof1 = mmr.open(1).unwrap();

    // create partial MMR and add authentication path to node at position 1
    let mut partial_mmr: PartialMmr = mmr.peaks().into();
    partial_mmr.track(1, node1, &proof1.merkle_path).unwrap();

    partial_mmr.untrack(1);
}

This test panics when calling untrack with:

thread 'merkle::mmr::partial::tests::test_partial_mmr_untrack' panicked at src/merkle/mmr/inorder.rs:95:19:
attempt to shift left with overflow

I would expect this to not panic when untracking a tracked leaf.

@PhilippGackstatter PhilippGackstatter added the bug Something isn't working label Feb 14, 2025
@bobbinth
Copy link
Contributor

Great catch! Seems like this method wasn't really tested. I haven't really looked into it in-depth, but I think maybe re-writing untrack() like so could fix the issue:

pub fn untrack(&mut self, leaf_pos: usize) {
    let mut idx = InOrderIndex::from_leaf_pos(leaf_pos);
    while self.nodes.remove(&idx.sibling()).is_some() && !self.nodes.contains_key(&idx) {
        idx = idx.parent();
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants