Monero: expose merkle root function #629
Conversation
There was a problem hiding this comment.
ACK to the concept. NACK to the implementation which I believe is faulty.
Also, the monero folder has maintained a code freeze pending audit. How should we handle this? Merge this on Serai's next? Merge this into a new branch without the code freeze? Cuprate vendors a version with select patches we roll in post-audit?
| match leafs.len() { | ||
| 0 => root, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll change it to return an option, I don't really like having the first leaf as a separate arg.
On Cuprate's end we already vendor |
Boog900
changed the title
This function is needed in the RPC: https://github.com/monero-project/monero/blob/893916ad091a92e765ce3241b94e706ad012b62a/src/rpc/core_rpc_server.cpp#L2172
I also made a small change to the function signature, taking all leafs in one arg,