Skip to content

Merkle tree API improvements#162

Merged
jbearer merged 4 commits intomainfrom
fix/merkle-tree-api
Dec 15, 2022
Merged

Merkle tree API improvements#162
jbearer merged 4 commits intomainfrom
fix/merkle-tree-api

Conversation

@jbearer
Copy link
Copy Markdown
Member

@jbearer jbearer commented Dec 14, 2022

Description

Was playing around with Jellyfish for a hobby project and ran into a couple of ergonomics issues in the Merkle tree API.

commit 699dea9
Author: Jeb Bearer jeb@espressosys.com
Date: Wed Dec 14 11:43:07 2022 -0800

Make serde usable for MerkleTree types

The Merkle trees are ususally instantiated with cryptographic types,
e.g. field elements, which do not implement serde traits. This commit
changes the bound on the derived serde impls for Merkle tree types to
require ark_serialize impls on the element types and uses #[serde(with)]
to serialize these elements using ark_serialize.

We also add an instantiation of the Rescue hash scheme for field elements
as the _index_ type, since the BigUint instantiation just coverts to field
elements anyways, but BigUint does not implement serde _or_ ark_serialize,
while field elements do.

commit bdbd5b1
Author: Jeb Bearer jeb@espressosys.com
Date: Wed Dec 14 10:18:37 2022 -0800

Expose index and leaf element of Merkle proof.

Since MerkleNode is a private type, users cannot pattern match to
extract a MerkleNode::Leaf, so this is the only way to get the leaf
elemenet of a Merkle proof. This is also, therefore, the only way
to verify that a Merkle proof proves inclusion of the desired element,
since `verify` doesn't check this. (Technically you can also use
`remember`, but that includes side-effects that may be undesirable.)

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 a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

Since MerkleNode is a private type, users cannot pattern match to
extract a MerkleNode::Leaf, so this is the only way to get the leaf
elemenet of a Merkle proof. This is also, therefore, the only way
to verify that a Merkle proof proves inclusion of the desired element,
since `verify` doesn't check this. (Technically you can also use
`remember`, but that includes side-effects that may be undesirable.)
The Merkle trees are ususally instantiated with cryptographic types,
e.g. field elements, which do not implement serde traits. This commit
changes the bound on the derived serde impls for Merkle tree types to
require ark_serialize impls on the element types and uses #[serde(with)]
to serialize these elements using ark_serialize.

We also add an instantiation of the Rescue hash scheme for field elements
as the _index_ type, since the BigUint instantiation just coverts to field
elements anyways, but BigUint does not implement serde _or_ ark_serialize,
while field elements do.
@jbearer jbearer requested review from alxiong and mrain December 14, 2022 19:47
@jbearer jbearer self-assigned this Dec 14, 2022
Copy link
Copy Markdown
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

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

I like the idea of requiring serde(bound=) to enforce the usage of CanonicalSerde during serialization.

but meanwhile, I'm not sure about #[serde(with = "field_elem")], what if the type is not a primitive types, and not a field elements from arkwork?
the benefit of using BigUint compared to F is its generality.

@jbearer what do you think of renaming mod field_elem to something like canoncial_serdeable (idk maybe there's better names? 🤷‍♂️ ) and the serializer takes in
elem: impl Into<T> where T: CanonicalSerize instead?

@jbearer
Copy link
Copy Markdown
Member Author

jbearer commented Dec 15, 2022

Yeah that's a good idea. mod field_elem actually already deals with T: CanonicalSerialize, so field_elem was already kind of a misleading name. I'll change it to mod canonical (#[serde(with = "canonical")] is concise an accurate).

Also totally agree with the benefit of BigUint. Only drawback is serialization. That's why I kept the BigUint Rescue scheme and added a new one with F as the index just in case user's need serialization. I also think indexing by F might be slightly more intuitive when you're interacting with circuits, since in that case you're mostly dealing with field elements everywhere, and BigUint just gets converted to a field element anyway.

@jbearer jbearer requested a review from alxiong December 15, 2022 15:58
Copy link
Copy Markdown
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

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

Thanks Jeb! LGTM

@jbearer jbearer merged commit 4e017ca into main Dec 15, 2022
@jbearer jbearer deleted the fix/merkle-tree-api branch December 15, 2022 18:05
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.

2 participants