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

mssmt: Add CompressedProof serialization #29

Merged
merged 4 commits into from
May 11, 2022
Merged

Conversation

wpaulino
Copy link
Contributor

@wpaulino wpaulino commented May 6, 2022

We don't add one for the decompressed variant as there's no reason to serialize those when the compressed variant can yield a more efficient serialization.

The serialization scheme for CompressedProof is as follows:

  • Nodes:
    • Big endian encoding of the number of nodes present as uint16
    • Each node:
      • 32 byte key
      • Big endian encoding of the node sum as uint64
  • Bits:
    • 32 bytes of the packed bits (256 bits total)

@wpaulino wpaulino force-pushed the compressed-proof-encoding branch 2 times, most recently from 02e2e4d to 9899774 Compare May 6, 2022 23:35
This is currently unused, but will serve useful when introducing proof
serialization. Proof nodes have their NodeKey and NodeSum serialized
without their pre-image, so we need a way to reconstruct a Node without
them.
We don't add one for the decompressed variant as there's no reason to
serialize those when the compressed variant can yield a more efficient
serialization.

The serialization scheme for CompressedProof is as follows:

* Nodes:
  * Big endian encoding of the number of nodes present as uint16
  * Each node:
    * 32 byte key
    * Big endian encoding of the node sum as uint64
* Bits:
  * 32 bytes of the compressed bits (256 bits total)
The SplitCommitmentRoot struct only existed as we wouldn't be able to
recover it without knowledge of the node's pre-image. Now that
mssmt.ComputeNode exists, this is no longer an issue.
@wpaulino wpaulino requested a review from Roasbeef May 10, 2022 19:14
@Roasbeef Roasbeef added this to Review in progress in On Chain May 10, 2022
@@ -2,10 +2,6 @@ package mssmt

// Proof represents a merkle proof for a MS-SMT.
type Proof struct {
// Leaf is the leaf node the proof is valid for. If the leaf is empty,
Copy link
Member

Choose a reason for hiding this comment

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

What's the motivation here? Making it more similar to Bitcoin's control block structure? (where the leaf is actually obtained from elsewhere in the stack)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah and also noticed that in most cases we already have the raw leaf as a serialized Asset so it wasn't really being used anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah makes sense, you typically already know the leaf that you want to check inclusion of, or that can be transmitted prepended to the proof itself.

// ComputedNode is a node within a MS-SMT that has already had its NodeKey and
// NodeSum computed, i.e., its preimage is not available.
type ComputedNode struct {
key NodeKey
Copy link
Member

Choose a reason for hiding this comment

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

Ultimately I think the node key terminology here is pretty confusing. It's actually the node digest/hash, but it reads as though it's somehow the key used to traverse the tree to arrive at the given node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I would be ok with s/NodeKey/NodeDigest/g before we merge any other in-flight PRs.

Copy link
Member

Choose a reason for hiding this comment

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

Don't think this is blocking though, will likely do another sweep afterwards to clean up naming.

@@ -181,7 +181,7 @@ func (t Tree) MerkleProof(key [hashSize]byte) *Proof {
// VerifyMerkleProof determines whether a merkle proof for the leaf found at the
// given key is valid.
func VerifyMerkleProof(key [hashSize]byte, leaf *LeafNode, proof *Proof,
root *BranchNode) bool {
root Node) bool {
Copy link
Member

Choose a reason for hiding this comment

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

What do we gain by making this the interface? If it's a leaf, then this operation can never succeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would allow us to use asset.SplitCommitmentRoot (which is now of type mssmt.ComputedNode directly without doing a type conversion.

@Roasbeef
Copy link
Member

Agreed, I would be ok with s/NodeKey/NodeDigest/g before we merge any other in-flight PRs.
Makes sense for another smol PR.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 💵

On Chain automation moved this from Review in progress to Reviewer approved May 11, 2022
@Roasbeef Roasbeef merged commit 9f3160c into main May 11, 2022
On Chain automation moved this from Reviewer approved to Done May 11, 2022
@wpaulino wpaulino deleted the compressed-proof-encoding branch May 11, 2022 16:38
dstadulis pushed a commit that referenced this pull request May 16, 2023
add multi address send RPC and itest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants