Skip to content

fix(proof): verify inclusion proofs for leaves a deletion promoted above row 0 - #152

Open
eastagiletracker wants to merge 1 commit into
mit-dci:mainfrom
eastagiletracker:agile-board/proof-verify-promoted-targets
Open

fix(proof): verify inclusion proofs for leaves a deletion promoted above row 0#152
eastagiletracker wants to merge 1 commit into
mit-dci:mainfrom
eastagiletracker:agile-board/proof-verify-promoted-targets

Conversation

@eastagiletracker

Copy link
Copy Markdown

This PR proposes a one-line fix in Proof::calculate_hashes so that an inclusion proof for a leaf that a deletion promoted above row 0 verifies instead of being rejected as invalid, which is the MemForest half of #150. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/385. You can sign in with your GitHub ID to claim ownership of the project.

What goes wrong

calculate_hashes computes translated — the targets mapped from MAX_FOREST_ROWS space into the forest's own rows — and uses it for get_proof_positions, but then builds the node list that those proof hashes get paired with out of the untranslated self.targets:

let translated: Vec<_> = self
    .targets
    .iter()
    .copied()
    .map(|pos| translate(pos, MAX_FOREST_ROWS, total_rows))
    .collect();
let proof_positions = get_proof_positions(&translated, num_leaves, total_rows);

// As we calculate nodes upwards, it accumulates here
let mut nodes: Vec<_> = self
    .targets            // <- MAX_FOREST_ROWS space, unlike proof_positions
    .iter()
    .copied()
    .zip(del_hashes.to_owned())
    .collect();

translate is the identity at row 0, so the two spaces agree for as long as every target is a leaf sitting at the bottom of the forest and the mismatch stays invisible. It stops being invisible the moment a deletion promotes a leaf: with eight leaves, deleting leaf 0 moves leaf 1 up to row 1, MemForest::prove correctly reports it as 2^63 instead of 8, and that value then sorts past every entry in proof_positions, so get_next hands calculate_hashes the wrong sibling. Stump::verify returns Err(InvalidProof(MissingSibling(9))) for a proof that is perfectly good. calculate_hashes_delete, the twin used by Stump::modify, already zips translated, which is why deleting a promoted leaf keeps working while proving one does not.

The fix is to zip translated in calculate_hashes too, so both halves of the function agree on which space they are in. Everything that reaches this code today passes row-0 targets, where translate returns the position unchanged, so nothing that works now changes behaviour.

Reproducing and verifying

Both added tests fail on main at b7af3b7 with the src/proof/mod.rs change reverted and pass with it, so cargo test --lib proof::tests::test_verify_promoted is enough to see it in either direction:

---- proof::tests::test_verify_promoted_target stdout ----
assertion `left == right` failed
  left: Err(InvalidProof(MissingSibling(9)))
 right: Ok(true)

---- proof::tests::test_verify_promoted_target_after_growth stdout ----
assertion `left == right` failed
  left: Err(InvalidProof(MissingSibling(17)))
 right: Ok(true)

test_verify_promoted_target walks the reported case end to end through MemForest and Stump — one promotion, a two-deletion promotion two rows up, a promoted target proved alongside one still at row 0, and a control assertion that an untouched leaf was never affected. test_verify_promoted_target_after_growth covers the reason non-row-0 positions travel in MAX_FOREST_ROWS space at all: it deletes, then grows the forest past a power of two, then proves the promoted leaf. Every step asserts that the forest's roots and the stump's still agree, so a test can't pass by both sides drifting together.

cargo test on main at b7af3b7 is 58 unit tests and 29 doc tests green; with this branch it is 60 and 29, the two new ones being the whole difference. Also green on --no-default-features and --all-features, cargo clippy --all-targets --all-features is clean, and cargo fmt --check passes.

What this does not cover

The Pollard variant reported in the same issue is a separate root cause and is left alone here: Pollard::batch_proof fails inside Pollard::get_pos with CouldNotUpgradeNode after any deletion, before verification is ever reached, because leaf_map still holds a Weak to a node the deletion dropped. It wants its own change rather than riding along with this one. Worth noting too that #140 rewrites the imports throughout src/proof/mod.rs; it doesn't touch the lines here, but whichever lands second may want a look.

How this was managed

We imported your issues and pull requests into a board — 144 stories and 7 labels — and worked this change on it as MemForest::prove returns invalid proofs for the sibling of a deleted leaf. The whole board is at https://eastagiletracker.com/projects/385.

board

If you'd rather not receive contributions like this, reply no-more-prs on this pull request and we won't open any further ones on your repositories.


Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com

`calculate_hashes` builds its node list out of `self.targets`, which are
positions in `MAX_FOREST_ROWS` space, and then pairs it with proof positions
that are in the forest's own rows. Both spaces agree for targets at row 0, so
this stays invisible until a deletion promotes a leaf: from then on that target
sorts nowhere near its sibling and verification fails with `MissingSibling`,
even though the proof is valid.

Use the already-computed `translated` positions instead, which is what the
`calculate_hashes_delete` twin does.
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.

1 participant