feat: refactor Merkle proof for ergonomics

[mrain]

closes: #642, #658

This PR:

• Remove leaf information (index, element) from the MerkleProof struct.
• Change the verifications APIs to take a Merkle commitment instead of a simple root digest.

This PR does not:

Key places to review:

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (main)
• Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
• Wrote unit tests
• Updated relevant documentation in the code
• Added relevant changelog entries to the CHANGELOG.md of touched crates.
• Re-reviewed Files changed in the GitHub PR explorer

[ggutoski]

Change the verifications APIs to take a Merkle commitment instead of a simple root digest.

Why is this needed?

[mrain]

Why is this needed?

I think it looks nicer.

And make sure that users won't miss this check: https://github.com/EspressoSystems/zkrollup-integration/blob/89e86d0ea3f6ea25b92e310d131d177512b0ff80/sp1/program/src/main.rs#L82

[ggutoski]

Thanks @mrain .

• I did not spot any issues but it's hard to follow the code from the diff.
• Anything specific to review with greater care? Seems like most changes are boilerplate: renaming variables, accommodating the removal of (index, element) from merkle proof, etc.
• @alxiong should review the gadget stuff. I'm unfamiliar with our gadget code.

[ggutoski]

And make sure that users won't miss this check: https://github.com/EspressoSystems/zkrollup-integration/blob/89e86d0ea3f6ea25b92e310d131d177512b0ff80/sp1/program/src/main.rs#L82

Sorry but I don't understand this comment.

[mrain]

Sorry but I don't understand this comment.

This is an additional check that the height information is correct. With previous API users may forget. to do so.
But I'm not quite sure whether it is necessary now. cc @alxiong

Anything specific to review with greater care? Seems like most changes are boilerplate: renaming variables, accommodating the removal of (index, element) from merkle proof, etc.

True. Most of them are boilerplate. Changes in forget_internal() and remember_internal() may require additional care. But they are all covered by unit tests. And this is also kind of internal audit.

[alxiong]

Sorry but I don't understand this comment.

This is an additional check that the height information is correct. With previous API users may forget. to do so.
But I'm not quite sure whether it is necessary now. cc @alxiong

This is to ensure correct tree height is being used to prevent extension attack (i.e. find a root collision via much larger tree)

meanwhile, I agree it's unclear from API design that why we need this check outside, instead of being part of BlockMerkleTree::verify() 🤔

[mrain]

meanwhile, I agree it's unclear from API design that why we need this check outside, instead of being part of BlockMerkleTree::verify() 🤔

Therefore it's checked inside verify() now.
My point is, as we are supplying the leaf information to verify(), is the height check still necessary to prevent the extension attack?

[alxiong]

I agree with the trait changes.

• Why are we completely removing namespaced_merkle_tree?

[mrain]

• Why are we completely removing namespaced_merkle_tree?

Because it's no longer in use and takes too much efforts to refactor.

[alxiong]

Why are we completely removing namespaced_merkle_tree?

Because it's no longer in use and takes too much efforts to refactor.

wait, so we are not going to use them? are we sticking to using a normal MT with a Namespace table to delineate the boundary of different namespaces?
I thought might be useful to keep a NMT, if the effort is high, we can do it in a separate PR?

[mrain]

wait, so we are not going to use them? are we sticking to using a normal MT with a Namespace table to delineate the boundary of different namespaces? I thought might be useful to keep a NMT, if the effort is high, we can do it in a separate PR?

No we are not. The namespace table now is just a plain vector with offsets @ggutoski

I think it's better to bring back NMT on request.

[ggutoski]

wait, so we are not going to use them? are we sticking to using a normal MT with a Namespace table to delineate the boundary of different namespaces? I thought might be useful to keep a NMT, if the effort is high, we can do it in a separate PR?

No we are not. The namespace table now is just a plain vector with offsets @ggutoski

I think it's better to bring back NMT on request.

I was not even aware we have a NMT implementation. 😳 We don't need one IMO. We can revive it later if we find that we do need NMT.

[mrain]

Also do you think we should merge #685 together?
cc @alxiong @ggutoski

[alxiong]

Also do you think we should merge #685 together?

we should keep them as two PRs, but yeah, we can rebase that PR after this is merge and then got that one in as well.

[alxiong]

In principle approval, LGTM. I'll let Gus have the final call.