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

Improved (more compact) Merkle Proofs #1015

Merged

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Apr 27, 2018

Reworking Merkle proofs so we only store a path and the underlying mmr_size.
The path includes the full sibling path and the necessary hashes derived from the peaks.
We need the mmr_size as we cannot rely on this being available when we come to verify the proof.
The mmr_size is used to determine the pos of the various entries in the path.

For siblings in the path we hash the node and the sibling together using the index of the parent.
For peak derived hashes we hash with the mmr_size (as a substitute for the non-existent pos).

Everything should hash up to the root itself.

TODO -

  • get verify() working for ImproveMerkleProof
  • think through how we can do this in a backward-compatible way (if its even possible or worth doing)
  • rename ImprovedMerkleProof -> MerkleProof
  • how to check validity of txs (when adding to tx pool etc.) as we need MMR pos etc.
  • get chain tests passing
  • fixup wallet code (Merkle proof PartialOrd?)
  • cleanup and fn docs
  • cleanup println! all over Merkle Proofs...
  • test locally (sending txs etc.)
  • move merkle_proofs into their own file

@antiochp antiochp changed the base branch from master to consensus_breaking May 4, 2018 19:21
@antiochp antiochp added enhancement consensus breaking Use for issues or PRs that will break consensus and force a hard fork labels May 4, 2018
@antiochp antiochp force-pushed the shorter_merkle_proofs branch 2 times, most recently from 0de5421 to 6b676fe Compare May 8, 2018 19:10
antiochp added 15 commits May 9, 2018 14:15
* block sums and reworked block validation
read and write block_sums
refactor validate on both block and txhashset
write block_sum on fast sync
we store the kernel_sum (need to account for the offset)

* block_sums

* rustfmt

* cleanup
* txhashset now implements committed for consistency

* rustfmt and cleanup
* make kernel offsets commitments, they used to be blinding factors

* cleanup various zero_commit inits

* cleanup api and wallet to reflect offsets as commitments
verify maturity via merkle proofs
TODO - how to check validity of txs?
@antiochp
Copy link
Member Author

antiochp commented May 9, 2018

This I think is ready for review.

  • Simplified MerkleProof impl - just a path and the mmr_size
  • We now verify Merkle proofs in one place and we require the root and the node_pos to do so.

/cc @ignopeverell @tromp

@antiochp antiochp changed the title [WIP] improved merkle proofs Improved (more compact) Merkle Proofs May 9, 2018
Copy link
Contributor

@ignopeverell ignopeverell left a comment

Choose a reason for hiding this comment

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

Looks good overall, also seems slightly cleaner.

}

/// A Merkle proof that proves a particular element exists in the MMR.
#[derive(Debug, Eq, PartialEq, Clone, PartialOrd, Ord)]
pub struct MerkleProof {

This comment was marked as spam.

This comment was marked as spam.

@@ -309,6 +297,52 @@ where
.collect()
}

fn peak_path(&self, peak_pos: u64) -> Vec<Hash> {
let rhs = self.bag_the_rhs(peak_pos);
println!("rhs bagged - {:?}", rhs);

This comment was marked as spam.

@antiochp
Copy link
Member Author

@tromp - want to take a look over this before I merge onto consensus_breaking branch?

@antiochp antiochp requested a review from tromp May 11, 2018 00:05
@tromp
Copy link
Contributor

tromp commented May 11, 2018

will review

return Err(MerkleProofError::RootMismatch);
}
} else if self.mmr_size == 1 {
if self.path.len() == 1 && root == node_hash && vec![root] == self.path {

This comment was marked as spam.

This comment was marked as spam.

let sibling = self.path.remove(0);
let (parent_pos, sibling_pos) = pmmr::family(node_pos);

let peaks_pos = pmmr::peaks(self.mmr_size);

This comment was marked as spam.

if mmr_size == 1 {
return Ok(MerkleProof {
mmr_size,
path: vec![node],

This comment was marked as spam.

.iter()
.map(|x| (self.get_from_file(x.1).unwrap_or(Hash::default()), x.1))
.filter_map(|x| self.get_from_file(x.1))

This comment was marked as spam.

This comment was marked as spam.

@@ -940,6 +811,9 @@ pub fn bintree_postorder_height(num: u64) -> u64 {
/// We know the positions of all leaves based on the postorder height of an MMR of any size
/// (somewhat unintuitively but this is how the PMMR is "append only").
pub fn is_leaf(pos: u64) -> bool {
if pos == 0 {

This comment was marked as spam.

assert_eq!(pmmr.get_hash(1).unwrap(), pos_0);

let proof = pmmr.merkle_proof(1).unwrap();
assert_eq!(proof.path, [pos_0]);

This comment was marked as spam.

Copy link
Contributor

@tromp tromp left a comment

Choose a reason for hiding this comment

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

Looks ok except for non empty merkle proof paths when there are no siblings

@antiochp
Copy link
Member Author

Addressed feedback, will merge once all tests pass.

@antiochp antiochp merged commit de353cf into mimblewimble:consensus_breaking May 12, 2018
@antiochp antiochp deleted the shorter_merkle_proofs branch May 12, 2018 20:09
@yeastplume yeastplume mentioned this pull request Feb 23, 2022
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus breaking Use for issues or PRs that will break consensus and force a hard fork enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants