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.

Sign up for GitHub

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

Monero: expose merkle root function #629

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

Monero: expose merkle root function #629

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
980c5f0
Monero: expose merkel root function
Boog900 Nov 15, 2024
05cda97
review fixes
Boog900 Nov 15, 2024
e6fdef6
change `merkle_root` return to `Option`
Boog900 Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions networks/monero/src/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,13 @@ impl Block {
/// use the [`Block::hash`] function.
pub fn serialize_pow_hash(&self) -> Vec<u8> {
let mut blob = self.header.serialize();
blob.extend_from_slice(&merkle_root(self.miner_transaction.hash(), &self.transactions));
write_varint(&(1 + u64::try_from(self.transactions.len()).unwrap()), &mut blob).unwrap();

let mut transactions = Vec::with_capacity(self.transactions.len() + 1);
transactions.push(self.miner_transaction.hash());
transactions.extend_from_slice(&self.transactions);

blob.extend_from_slice(&merkle_root(&transactions));
write_varint(&(1 + self.transactions.len()), &mut blob).unwrap();
blob
}

Expand All @@ -132,7 +137,7 @@ impl Block {
// Monero pre-appends a VarInt of the block-to-hash'ss length before getting the block hash,
// but doesn't do this when getting the proof of work hash :)
let mut hashing_blob = Vec::with_capacity(9 + hashable.len());
write_varint(&u64::try_from(hashable.len()).unwrap(), &mut hashing_blob).unwrap();
write_varint(&hashable.len(), &mut hashing_blob).unwrap();
hashing_blob.append(&mut hashable);

let hash = keccak256(hashing_blob);
Expand Down
3 changes: 2 additions & 1 deletion networks/monero/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ pub use monero_io as io;
pub use monero_generators as generators;
pub use monero_primitives as primitives;

mod merkle;
/// Merkel tree functionality.
Boog900 marked this conversation as resolved.
Show resolved Hide resolved
pub mod merkle;

/// Ring Signature structs and functionality.
pub mod ring_signatures;
Expand Down
11 changes: 5 additions & 6 deletions networks/monero/src/merkle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@ use std_shims::vec::Vec;

use crate::primitives::keccak256;

pub(crate) fn merkle_root(root: [u8; 32], leafs: &[[u8; 32]]) -> [u8; 32] {
/// Calculates the merkel root of the given tree. Equivalent to `tree_hash` in monero-core.
Boog900 marked this conversation as resolved.
Show resolved Hide resolved
pub fn merkle_root(leafs: &[[u8; 32]]) -> [u8; 32] {
match leafs.len() {
0 => root,
Copy link
Member

Choose a reason for hiding this comment

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

We used to handle no leaves being passed. Now no hashes falls through to the n-leaves case. Is that correct?

I doubt it as our n-leaf code pads to the nearest power of 2 which is assumed to be 4 as we assumed exceptional handling for 0/1/2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah it will panic, I guess the best thing to do would be to explicitly panic, tree_hash doesn't allow a 0 length input: https://github.com/monero-project/monero/blob/893916ad091a92e765ce3241b94e706ad012b62a/src/crypto/tree-hash.c#L62

Copy link
Member

@kayabaNerve kayabaNerve Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That would've been so easy for me to check if there was a link to tree_hash present :p

*There wasn't such a link prior, I don't legitimately want to hold this against you.

I'd vote we keep the existing explicit argument (potentially renamed from root to first_leaf?) making this unreachable, or return an Option. We can document the panic but I'd consider that a downgrade. Even the Option would be but it isn't such a downgrade I wouldn't agree to it if you believe avoiding these allocations so worthwhile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it to return an option, I don't really like having the first leaf as a separate arg.

1 => keccak256([root, leafs[0]].concat()),
1 => leafs[0],
2 => keccak256([leafs[0], leafs[1]].concat()),
_ => {
let mut hashes = Vec::with_capacity(1 + leafs.len());
hashes.push(root);
hashes.extend(leafs);
let mut hashes = leafs.to_vec();
Boog900 marked this conversation as resolved.
Show resolved Hide resolved

// Monero preprocess this so the length is a power of 2
let mut high_pow_2 = 4; // 4 is the lowest value this can be
Expand Down
Loading