Merkle tree construction #504

tbekas · 2023-08-02T16:55:15Z

No description provided.

codex/merkletree/merkletree.nim

tests/codex/merkletree/testmerkletree.nim

codex/merkletree/merkletree.nim

benbierens

We can rap-battle some more about the 1D array index stuff, if you'd like. (Or we can delay it till we get to serialization where we need to do it anyway.)

codex/merkletree/merkletree.nim

elcritch · 2023-08-08T22:14:43Z

We can rap-battle some more about the 1D array index stuff, if you'd like. (Or we can delay it till we get to serialization where we need to do it anyway.)

But @benbierens it's like log n less allocations!! ;) You can tell my embedded background shows -- I'm allergic to allocations.

benbierens · 2023-08-09T09:56:16Z

Oh yeah, no doubt the 1D thing is much better in this way. It's just, we were just starting and needed a way to think about what we were trying to do. So we started with lots of allocs.

gmega

I have what are mostly petty comments. 🙂 Overall it looks good, particularly given that it passes the tests.

codex/merkletree/merkletree.nim

gmega · 2023-08-09T13:07:22Z

codex/merkletree/merkletree.nim

+          digestSize =? leaves.?[0].?size:
+    return failure("At least one leaf is required")
+
+  if not leaves.allIt(it.mcodec == mcodec):


I get why you are doing this, but to me this is a symptom that mcodec should be a property of the tree and not a property of the node. Not sure there's much of a way around it though.

Yeah, initially I started implementing it this way, but the new code has to be compatible with already existing code and it's just easier to do it this way than converting to/from MultiHash in each public function. For the serialization though we will likely use a tree property instead of a node property.

I don't think it's relevant to check the type of hash of the leafs, in fact we can just treat them as abstract blobs. We should just have this as a property of the entire tree.

In fact, we have some potential use cases, where the leafs can be pretty much anything, for example KZG commitments...

I've address this by hardcoding a hashing algorithm for merkle trees tho SHA-256 in #516

codex/merkletree/merkletree.nim

gmega

Looks good to me!

elcritch · 2023-08-08T22:03:09Z

tests/codex/merkletree/testmerkletree.nim

+
+  test "tree with three leaves has expected root":
+    let
+      expectedRoot = combine(combine(leaves[0], leaves[1]), combine(leaves[2], leaves[2]))


Looks like you're duplicating the last element when the tree is odd? It would probably be better to use a specific "null node".

The reason comes from reviewing the Bitcoin merkle trees there were some issues with duplicating issues at the end. With a properly constructed dataset in Bitcoin you could do a denial-of-service. See CVE-2012-2459.

There was a proposal for a better merkle design for Bitcoin at BIP-98. It describes the issue better. I didn't dive much into the proof side of things (maybe it's better).

It may all not apply to us, but I think prudence would say to use the fixes others did in case.

@gmega your question about balanced merkle tree reminded me of this

Fixed in the #516

elcritch · 2023-08-08T22:04:59Z

codex/merkletree/merkletree.nim

+proc init*(
+  T: type MerkleTree,
+  leaves: seq[MerkleHash]
+): ?!MerkleTree =


Could you add a short description noting the ordering of the nodes? When this comes up later it's nice to know "leaves start at 0..k, with proofs being k+1..n" or something similar.

P.S. you could also add docs to the module. Just use ## like ## about this module as the comment block type and Nim doc gen will turn it into a module doc.

Fixed in the #516

elcritch · 2023-08-08T22:05:52Z

codex/merkletree/merkletree.nim

+  T: type MerkleProof,
+  index: int,
+  path: seq[MerkleHash]
+): MerkleProof =


Same as above! Just enough details to what the proof is of.

Fixed in the #516

elcritch · 2023-08-09T19:19:11Z

Ok, created an issue to follow up on the pre-image attack thing to get this unblocked: #511

dryajov · 2023-08-10T00:30:57Z

codex/merkletree/merkletree.nim

+    index: int
+    path: seq[MerkleHash]
+
+# Tree constructed from leaves H0..H2 is


Thanks for documenting this!

Thanks! Note if you use ## here then it'd show up if we ever run docgen. :)

Fixed in #516

dryajov

OK, I think this is looking good for a first pass.

We're missing proof validation (or am I blind?) and and tree validation when constructed with a merkle root. We can cover that in a separate PR tho.

We should also address the security concernes, but I'd leave it for a separate PR as well.

dryajov · 2023-08-10T14:58:13Z

codex/merkletree/merkletree.nim

@@ -0,0 +1,189 @@
+## Nim-Codex
+## Copyright (c) 2022 Status Research & Development GmbH


Suggested change

## Copyright (c) 2022 Status Research & Development GmbH

## Copyright (c) 2023 Status Research & Development GmbH

Fixed in #516

tbekas · 2023-08-15T11:22:49Z

I'm going to address all of the comments in the upcoming PR.

tbekas and others added 4 commits August 1, 2023 15:53

Building a merkle tree

a404cf1

setting up some tests

49699d2

Adds test for merkle proofs

8fe1eef

Computing intermediate nodes in merkle tree

9764d36

tbekas requested review from elcritch, dryajov and gmega August 2, 2023 16:55

Propagate errors from hash calculation

0e6b53f

elcritch reviewed Aug 3, 2023

View reviewed changes

codex/merkletree/merkletree.nim Outdated Show resolved Hide resolved

elcritch reviewed Aug 3, 2023

View reviewed changes

codex/merkletree/merkletree.nim Outdated Show resolved Hide resolved

elcritch reviewed Aug 3, 2023

View reviewed changes

codex/merkletree/merkletree.nim Outdated Show resolved Hide resolved

elcritch reviewed Aug 3, 2023

View reviewed changes

tests/codex/merkletree/testmerkletree.nim Show resolved Hide resolved

elcritch reviewed Aug 3, 2023

View reviewed changes

codex/merkletree/merkletree.nim Outdated Show resolved Hide resolved

tbekas added 4 commits August 4, 2023 15:00

Obtaining merkle proof from a tree

9a15aa3

Remove MerkleTreeBuilder

da3fcae

Adding merkletree test to other codex tests

dba8bf4

Merge branch 'master' into merkle-tree-construction

4d77319

tbekas marked this pull request as ready for review August 4, 2023 14:17

benbierens previously approved these changes Aug 7, 2023

View reviewed changes

codex/merkletree/merkletree.nim Outdated Show resolved Hide resolved

tbekas dismissed benbierens’s stale review via e162f43 August 8, 2023 09:35

Replace 2D seq with 1D seq

6238664

tbekas force-pushed the merkle-tree-construction branch from e162f43 to 6238664 Compare August 8, 2023 09:36

tbekas requested review from elcritch and benbierens August 8, 2023 09:41

benbierens previously approved these changes Aug 8, 2023

View reviewed changes

codex/merkletree/merkletree.nim Outdated Show resolved Hide resolved

Rename a function parameter from i to level

ad6b07c

tbekas dismissed benbierens’s stale review via ad6b07c August 8, 2023 11:44

dryajov reviewed Aug 8, 2023

View reviewed changes

codex/merkletree/merkletree.nim Outdated Show resolved Hide resolved

Rename i->level j->i and use openArray for params

d560d5e

tbekas force-pushed the merkle-tree-construction branch from b60c442 to d560d5e Compare August 9, 2023 10:41

gmega reviewed Aug 9, 2023

View reviewed changes

Rename functions that compute a value

e9fb455

gmega approved these changes Aug 9, 2023

View reviewed changes

elcritch approved these changes Aug 9, 2023

View reviewed changes

dryajov reviewed Aug 10, 2023

View reviewed changes

dryajov approved these changes Aug 10, 2023

View reviewed changes

benbierens self-requested a review August 10, 2023 06:29

benbierens approved these changes Aug 10, 2023

View reviewed changes

dryajov reviewed Aug 10, 2023

View reviewed changes

tbekas merged commit e860127 into master Aug 15, 2023
8 checks passed

tbekas deleted the merkle-tree-construction branch August 15, 2023 11:23

tbekas mentioned this pull request Aug 15, 2023

Refine merkle tree construction #516

Closed

jessiebroke added the client label Oct 18, 2023

jessiebroke assigned tbekas and jessiebroke and unassigned jessiebroke Oct 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merkle tree construction #504

Merkle tree construction #504

tbekas commented Aug 2, 2023 •

edited

benbierens left a comment

elcritch commented Aug 8, 2023

benbierens commented Aug 9, 2023

gmega left a comment

gmega Aug 9, 2023

tbekas Aug 9, 2023

gmega Aug 9, 2023

dryajov Aug 9, 2023

tbekas Aug 15, 2023

gmega left a comment

elcritch Aug 8, 2023

elcritch Aug 8, 2023

tbekas Aug 15, 2023

elcritch Aug 8, 2023

elcritch Aug 8, 2023

tbekas Aug 15, 2023

elcritch Aug 8, 2023

tbekas Aug 15, 2023

elcritch commented Aug 9, 2023

dryajov Aug 10, 2023

elcritch Aug 10, 2023

tbekas Aug 15, 2023

dryajov left a comment

dryajov Aug 10, 2023 •

edited

tbekas Aug 15, 2023

tbekas commented Aug 15, 2023

		@@ -0,0 +1,189 @@
		## Nim-Codex
		## Copyright (c) 2022 Status Research & Development GmbH

	## Copyright (c) 2022 Status Research & Development GmbH
	## Copyright (c) 2023 Status Research & Development GmbH

Merkle tree construction #504

Merkle tree construction #504

Conversation

tbekas commented Aug 2, 2023 • edited

benbierens left a comment

Choose a reason for hiding this comment

elcritch commented Aug 8, 2023

benbierens commented Aug 9, 2023

gmega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elcritch commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov left a comment

Choose a reason for hiding this comment

dryajov Aug 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbekas commented Aug 15, 2023

tbekas commented Aug 2, 2023 •

edited

dryajov Aug 10, 2023 •

edited