Add path to Branch-Nodes (don't use Extension-Nodes) #344

richardpringle · 2023-11-07T22:36:06Z

Add path to branch-node
Split leaf, branch, and extension node files
NodeType should box the branch-node
Use leaf-node size
Use an enum instead of numbers
Refactor node-serialization
WIP

firewood/src/merkle.rs

I'm going to open a new PR

firewood/src/merkle/node.rs

xinifinity

Do you mind updating the description accordingly with the latest changes? Thanks!

xinifinity

Partial review as a first pass.

xinifinity · 2024-01-18T06:35:57Z

firewood/src/merkle.rs

+            let fetched_val = merkle.get(key, root).unwrap();
+
+            assert_eq!(fetched_val.as_deref(), val.as_slice().into());
+        }
+
+        for (key, val) in key_val {
+            let fetched_val = merkle.get(key, root).unwrap();
+
+            assert_eq!(fetched_val.as_deref(), val.as_slice().into());


Why do we need a test to verify retrieve multiple times? What is the motivation?

We insert a value, then make sure that it's still there. Then we check to see if all the values are there at the end. In an improper implementation, it's possible that inserting new nodes will improperly move nodes (through splitting etc) and values will be inadvertently moved.

xinifinity · 2024-01-18T06:47:46Z

firewood/src/merkle.rs

+    #[test_case(branch(&[2], b"", None); "branch with path")]
+    #[test_case(branch(b"", b"", vec![1, 2, 3].into()); "branch with children")]
+    #[test_case(branch(&[2], b"value", None); "branch with path and value")]
+    #[test_case(branch(b"", b"value", vec![1, 2, 3].into()); "branch with value and children")]


Isn't this the same test as L2067?

Please use permalinks... the code changed and now I don't know what line you were talking about.
Maybe this one?

Looks the same. This will get fixed with #512

xinifinity · 2024-01-18T06:50:12Z

firewood/src/merkle.rs

        let children = Default::default();
-        let value = value.map(Data);
+        // TODO: Properly test empty data as a value


Why do we need to test empty data in the no data case?

There can be subtle differences in serializing and deserializing Some(vec![]) vs None. It's valid to store an empty byte-vector at any key.

xinifinity · 2024-01-18T06:52:02Z

firewood/src/merkle.rs

@@ -1528,17 +2018,44 @@ mod tests {
        create_generic_test_merkle::<Bincode>()
    }

-    fn branch(value: Option<Vec<u8>>, encoded_child: Option<Vec<u8>>) -> Node {
+    fn branch(path: &[u8], value: &[u8], encoded_child: Option<Vec<u8>>) -> Node {


Why do we need to split this to two functions? It seems there are repeated code after the split.

I don't think this comment is relevant anymore due to changes that landed in main. Please re-open if that's not the case.

xinifinity · 2024-01-18T06:53:59Z

firewood/src/merkle.rs

-    #[test_case(PlainCodec::new(), branch(Some(Vec::new()), vec![1, 2, 3].into()); "branch with empty value with PlainCodec")]
+    #[test_case(PlainCodec::new(), branch(b"", b"value", vec![1, 2, 3].into()) ; "branch with chd with PlainCodec")]
+    #[test_case(PlainCodec::new(), branch(b"", b"value", Some(Vec::new())); "branch with empty chd with PlainCodec")]
+    #[test_case(PlainCodec::new(), branch(b"", b"", vec![1, 2, 3].into()); "branch with empty value with PlainCodec")]


It doesn't seem we have a test case for branch with non empty path?

Rather than rolling each permutation by hand, the test_case crate also has test_matrix which can test all the combinations for you. Not only is this more readable, it names the arguments so you can see which permutations are actually being tested.

#[test_matrix( [b"", b"key"], [b"", b"value"], [None, Some(vec![1,2,3])] )] fn encodes_then_decode_branches(key: &'static [u8], value: &'static [u8], vec: Option<Vec<u8>>) { encode(branch(key, value, vec)); } #[test_matrix( [vec![], vec![1, 2. 3]], [vec![], vec![1, 2, 3]] )] fn encode_then_decode_leaves(key: Vec<u8>, value: Vec<u8>) { encode(leaf(key, value)) }

See issue #512 for the test-matrix comment.

@xinifinity

It doesn't seem we have a test case for branch with non empty path?

Correct, the new encoding/decoding implementation isn't currently used anywhere. Can we push the implementation out after this PR lands? It's impossible that this doesn't get caught because all the proofs will start failing once we switch to the new encoding/decoding. It should also be trivial to implement

firewood/src/merkle.rs

rkuris · 2024-02-05T17:03:10Z

We decided to land this despite completing the review as it is for some reason blocking adding a configuration flag to enable extension nodes

xinifinity mentioned this pull request Nov 9, 2023

DiskAddress is already a more efficient Option<> #341

Closed

rkuris previously requested changes Nov 11, 2023

View reviewed changes

richardpringle self-assigned this Nov 13, 2023

richardpringle force-pushed the remove-extension-node branch from f25a9f1 to 7c01d0d Compare November 20, 2023 19:31

richardpringle mentioned this pull request Nov 20, 2023

split node files #360

Merged

richardpringle force-pushed the remove-extension-node branch 15 times, most recently from a2e3c30 to fcdcfce Compare November 27, 2023 16:32

richardpringle force-pushed the remove-extension-node branch 10 times, most recently from ca59f9a to 6365bef Compare November 29, 2023 05:47

richardpringle force-pushed the remove-extension-node branch from 44591c3 to cd22881 Compare January 16, 2024 00:43

richardpringle force-pushed the remove-extension-node branch from cd22881 to 3c5ea5b Compare January 16, 2024 01:32

richardpringle marked this pull request as ready for review January 16, 2024 22:46

richardpringle requested review from xinifinity and danlaine as code owners January 16, 2024 22:46

richardpringle requested a review from rkuris January 17, 2024 18:15

rkuris reviewed Jan 18, 2024

View reviewed changes

firewood/src/merkle/node.rs Outdated Show resolved Hide resolved

xinifinity reviewed Jan 18, 2024

View reviewed changes

rkuris reviewed Jan 19, 2024

View reviewed changes

firewood/src/merkle.rs Show resolved Hide resolved

richardpringle force-pushed the remove-extension-node branch from 2d90bb3 to 1e51e15 Compare January 29, 2024 17:04

richardpringle added 9 commits February 2, 2024 08:49

Add path to branch-node

567004a

Fix stream

a38d093

Rename concat_proof

76d6e3c

Make proof code handle branches with paths

ef947cd

Handle unsetting of in-mem merkle for range-proofs

05d157d

Fix clippy warnings

3f3f6b5

Self review

60c7257

Simplify branch conversion for writing to branches

ff3fdb4

Remame Overlap and add doc-comments

74e264f

richardpringle force-pushed the remove-extension-node branch from a0854fa to 74e264f Compare February 2, 2024 13:56

richardpringle requested review from rkuris and xinifinity February 2, 2024 17:25

xinifinity approved these changes Feb 2, 2024

View reviewed changes

richardpringle merged commit 7713f2d into main Feb 2, 2024
5 checks passed

richardpringle deleted the remove-extension-node branch February 2, 2024 19:23

richardpringle mentioned this pull request Feb 2, 2024

Make Trie Size MerkleDB compatible #451

Closed

3 tasks

xinifinity mentioned this pull request Feb 16, 2024

ObjWriteError with large number of batches during insert #532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add path to Branch-Nodes (don't use Extension-Nodes) #344

Add path to Branch-Nodes (don't use Extension-Nodes) #344

richardpringle commented Nov 7, 2023

xinifinity left a comment

xinifinity left a comment

xinifinity Jan 18, 2024

richardpringle Feb 2, 2024

xinifinity Jan 18, 2024

richardpringle Feb 2, 2024

xinifinity Jan 18, 2024

richardpringle Feb 2, 2024

xinifinity Jan 18, 2024

richardpringle Feb 2, 2024

xinifinity Jan 18, 2024

rkuris Jan 18, 2024 •

edited

Loading

richardpringle Feb 2, 2024

rkuris commented Feb 5, 2024

Add path to Branch-Nodes (don't use Extension-Nodes) #344

Add path to Branch-Nodes (don't use Extension-Nodes) #344

Conversation

richardpringle commented Nov 7, 2023

xinifinity left a comment

Choose a reason for hiding this comment

xinifinity 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

Choose a reason for hiding this comment

rkuris Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkuris commented Feb 5, 2024

rkuris Jan 18, 2024 •

edited

Loading