Add support for tracking legacy frontiers and witnesses in `ShardTree` instances. #69

nuttycom · 2023-05-16T23:07:42Z

No description provided.

dconnolly · 2023-05-18T19:40:50Z

A linter blurp

incrementalmerkletree/src/lib.rs

nathan-at-least

Partial utACK from commits 3d5810f through 3d5810f.

I'm posting now because I'm anxious about losing state or getting pulled off on other stuff on Monday, but I will attempt to review the remaining commits on Monday.

No blockers. Lots of clarifying questions, some api doc & code style suggestions.

shardtree/src/lib.rs

nathan-at-least · 2023-06-02T21:04:04Z

incrementalmerkletree/src/frontier.rs

-    /// allowed depth or if the list of ommers provided is not consistent
-    /// with the position of the leaf.
+    /// Returns `None` if the new frontier would exceed the maximum allowed depth or if the list of
+    /// ommers provided is not consistent with the position of the leaf.


[Non-blocking future nice-to-have:] I wish we had automated tooling for comment whitespace formatting and could then compartmentalize all such changes in a single tool-generated commit. Without that I had to spend a couple seconds squinting to see if there was a substantial change here. Not a big deal, but a tiny context switching cost.

shardtree/src/lib.rs

nathan-at-least · 2023-06-02T22:32:37Z

shardtree/src/lib.rs

+    fn from(err: InsertionError) -> Self {
+        ShardTreeError::Insert(err)
+    }
+}


In my personal code, I make heavy use of "boiler-plate removal" dependencies. In this case derive_more crate enables #[derive(derive_more::From)] on enums that remove this boilerplate. Do we have a policy for or against such crates?

I just checked that derive_more::From can be instructed to ignore the ::Storage variant (which would otherwise attempt an incoherent blanket impl).

shardtree/src/lib.rs

daira

utACK 4732ea6 (modulo conflicts).

nathan-at-least

utACK - Reviewed the remaining commits that I didn't cover last week.

shardtree/src/lib.rs

nathan-at-least · 2023-06-05T20:03:39Z

ACK w/ local cargo test to complement my source review.

shardtree/src/lib.rs

…PrunableTree`

The cap is a truncated tree that can be used to cache precomputed subtree roots at tree heights greater than `SHARD_HEIGHT`. Only roots whose hashes cannot be altered by rewinds or by the addition of data to the tree may be persisted to the cap.

This introduces a `ShardTreeError` type that is used to wrap errors that arise from the undelying `ShardStore` when used in `ShardTree` methods. The previous approach that relied on `From<QueryError> + From<InsertionError>` bounds resulted in a situation where the shard store error was forced to encompass the bounds of these types, even though the methods on the `ShardStore` itself could never generate them. This resulted in a number of awkward problems for error conversion.

nuttycom · 2023-06-19T17:58:29Z

@daira @nathan-at-least here are the changes since your previous approvals; these are substantial enough to require a re-review if you don't mind.

daira · 2023-06-20T03:18:00Z

incrementalmerkletree/Cargo.toml

 legacy-api = []
+# The test-depenencies feature guards types and functions that are 


Suggested change

# The test-depenencies feature guards types and functions that are

# The test-dependencies feature guards types and functions that are

daira

utACK, but I did not review new logic with any thoroughness (so this should not be merged based on my review alone).

shardtree/src/lib.rs

daira

utACK accb8d7 (single commit; caveats about thoroughness of my previous review still apply).

AloeareV

I've reviewed all of the changes not inside shardtree/src/lib.rs. I'm partway through reading that file, and it's proving extensive enough I'm gonna break it off into its own review.

A note on terminology: The standard lingo in use here is that the root of the tree is the most-ancestral node. Each non-root node has one parent, and each non-leaf node has two children. Intuitively (from the genetic meaning of the word ancestor), the leaves would be ancestors, and two nodes would be hashed together to create a child node. Hashing two nodes together to create the ancestor of those nodes is the opposite what I expect the term 'ancestor' to mean.

I don't think this is something that could (or should) be changed, as flipping the meaning of already established terms would just cause more confusion. Maybe it could be better clarified somewhere?

Edit: The terms ancestor/child is going to get more and more baked in...but as the codebase stands, there's only a few instances of the terms. I don't think the terms have crystallized yet, and I think it would be more intuitive to flip them before they're too baked into the vocabulary.

incrementalmerkletree/src/lib.rs

incrementalmerkletree/src/witness.rs

incrementalmerkletree/src/lib.rs

incrementalmerkletree/src/witness.rs

AloeareV

utACK, this looks great. I'm not sure to draw the line between a long review process where my end result is three sentences, but it's that or add another dozen comments saying "This bit took me a long time to grok but seems correct"

shardtree/src/lib.rs

zancas · 2023-06-27T18:05:35Z

I've reviewed all of the changes not inside shardtree/src/lib.rs. I'm partway through reading that file, and it's proving extensive enough I'm gonna break it off into its own review.

A note on terminology: The standard lingo in use here is that the root of the tree is the most-ancestral node. Each non-root node has one parent, and each non-leaf node has two children. Intuitively (from the genetic meaning of the word ancestor), the leaves would be ancestors, and two nodes would be hashed together to create a child node. Hashing two nodes together to create the ancestor of those nodes is the opposite what I expect the term 'ancestor' to mean.

I don't think this is something that could (or should) be changed, as flipping the meaning of already established terms would just cause more confusion. Maybe it could be better clarified somewhere?

Edit: The terms ancestor/child is going to get more and more baked in...but as the codebase stands, there's only a few instances of the terms. I don't think the terms have crystallized yet, and I think it would be more intuitive to flip them before they're too baked into the vocabulary.

Hi! It also seems to me that this use of word ancestor is inverted relative to the meaning in genetics/genealogy.

I proposed some commits that would refine terminology and replace ancestor with descendant.

nuttycom#2

Since that proposal changes the public API I grepped the librustzcash repo to check for the pattern -i -e"ancestor" and didn't see it. Given that, my guess is that the impact on consumers would be minimal.

nuttycom · 2023-06-27T21:59:12Z

Hi! It also seems to me that this use of word ancestor is inverted relative to the meaning in genetics/genealogy.

The reason that ancestor nodes are named as they are is that, in generic tree terminology, the nodes closer to the root are named "parents" and nodes closer to the leaves are named "children". Inverting this relationship would run afoul of a ton of computer science precedent; the fact that in our use case parent nodes are derived via a computation over their children is incidental. So I'm strongly opposed to changing this relationship.

ebfull · 2023-06-28T21:24:43Z

incrementalmerkletree/src/frontier.rs

-    /// Returns `None` if the new frontier would exceed the maximum
-    /// allowed depth or if the list of ommers provided is not consistent
-    /// with the position of the leaf.
+    /// Returns `None` if the new frontier would exceed the maximum allowed depth or if the list of


This returns Err, not None, in the failure case.

fixed in 701d3e6

ebfull · 2023-06-28T21:34:20Z

incrementalmerkletree/src/lib.rs

+    /// Returns whether this address is the right-hand child of its parent
+    pub fn is_left_child(&self) -> bool {


This should say left-hand child.

Fixed in 701d3e6

ebfull · 2023-06-28T21:42:32Z

incrementalmerkletree/src/witness.rs

+                }
+            })
+            .take(self.filled.len())
+            .fold(0u64, |acc, addr| acc + 2u64.pow(addr.level().into()));


nonblocking nit: acc + (1u64 << addr.level().into()) might be more consistent with the other arithmetic in these crates.

Fixed in 701d3e6

ebfull · 2023-06-28T21:52:17Z

shardtree/src/lib.rs

+                        Tree::leaf((right.clone(), RetentionFlags::EPHEMERAL)),
+                    );
+                } else {
+                    break;


Tests don't appear to be exercising this scenario.

Fixed in 082109d

ebfull · 2023-06-28T21:53:28Z

shardtree/src/lib.rs

+                subtree =
+                    Tree::parent(None, Tree::leaf((left, RetentionFlags::EPHEMERAL)), subtree);
+            } else {
+                break;


Tests don't appear to be exercising this scenario.

Fixed in 082109d

AloeareV · 2023-06-29T16:05:02Z

Hi! It also seems to me that this use of word ancestor is inverted relative to the meaning in genetics/genealogy.

The reason that ancestor nodes are named as they are is that, in generic tree terminology, the nodes closer to the root are named "parents" and nodes closer to the leaves are named "children". Inverting this relationship would run afoul of a ton of computer science precedent; the fact that in our use case parent nodes are derived via a computation over their children is incidental. So I'm strongly opposed to changing this relationship.

Yeah, I suspected as much. I find it extremely confusing for merkle trees, but I'll accept that it's a lesser confusion than using terms opposite to everyone else.

…rd-sized values.

nuttycom force-pushed the cap_cache branch 2 times, most recently from 390fa2b to 9510fc9 Compare May 17, 2023 21:58

nuttycom force-pushed the cap_cache branch 10 times, most recently from d9e51bd to 2dbdd34 Compare May 25, 2023 14:09

daira reviewed Jun 2, 2023

View reviewed changes

incrementalmerkletree/src/lib.rs Show resolved Hide resolved

nathan-at-least approved these changes Jun 2, 2023

View reviewed changes

daira reviewed Jun 2, 2023

View reviewed changes

shardtree/src/lib.rs Outdated Show resolved Hide resolved

daira reviewed Jun 2, 2023

View reviewed changes

shardtree/src/lib.rs Outdated Show resolved Hide resolved

daira approved these changes Jun 2, 2023

View reviewed changes

nathan-at-least approved these changes Jun 5, 2023

View reviewed changes

shardtree/src/lib.rs Outdated Show resolved Hide resolved

nuttycom force-pushed the cap_cache branch 2 times, most recently from 1ae6fc3 to e57c252 Compare June 5, 2023 19:52

daira reviewed Jun 6, 2023

View reviewed changes

shardtree/src/lib.rs Outdated Show resolved Hide resolved

nuttycom added 7 commits June 6, 2023 16:50

Add a mechanism to insert the nodes from a Frontier into a `Located…

aab7c96

…PrunableTree`

Add insertion of frontier nodes into a ShardTree

19c514b

Add construction of LocatedPrunableTree from IncrementalWitness

f5b06c8

Add insertion of legacy incremental witness data to ShardTree

f8c13d1

Add caching of the "cap" to root & witness computation.

290b66d

nuttycom added 2 commits June 16, 2023 12:59

Generalize check_remove_mark and check_checkpoint_rewind

33ad808

Generalize remaining tests to arbitrary checkpoint id types.

cb8ef79

nuttycom force-pushed the cap_cache branch from adfc6bc to cb8ef79 Compare June 16, 2023 19:53

Address comments from code review.

f5889df

nuttycom force-pushed the cap_cache branch from 4b95520 to f5889df Compare June 16, 2023 23:13

nuttycom requested review from daira and nathan-at-least June 19, 2023 17:58

daira reviewed Jun 20, 2023

View reviewed changes

daira approved these changes Jun 20, 2023

View reviewed changes

Refactor to make shardtree tests reusable for checking ShardStore impls.

accb8d7

daira reviewed Jun 22, 2023

View reviewed changes

shardtree/src/lib.rs Show resolved Hide resolved

daira approved these changes Jun 22, 2023

View reviewed changes

nuttycom added 2 commits June 22, 2023 14:46

Replace put_root with single-node insertion.

00eb47f

Add shardtree::LocatedTree::take_root

3c4a660

AloeareV reviewed Jun 23, 2023

View reviewed changes

AloeareV reviewed Jun 26, 2023

View reviewed changes

shardtree/src/lib.rs Show resolved Hide resolved

shardtree/src/lib.rs Show resolved Hide resolved

nuttycom mentioned this pull request Jun 27, 2023

Cap cache nuttycom/incrementalmerkletree#2

Closed

str4d mentioned this pull request Jun 28, 2023

Migrations & data storage for pre-DAG-sync zcash/librustzcash#831

Merged

ebfull suggested changes Jun 28, 2023

View reviewed changes

nuttycom added 3 commits June 29, 2023 14:52

Add tests that verify ShardTree state post-truncation.

30c592a

Apply suggestions from code review.

701d3e6

Add tests to verify frontier & witness insertion behavior for sub-sha…

082109d

…rd-sized values.

nuttycom requested a review from ebfull June 29, 2023 21:32

ebfull approved these changes Jul 3, 2023

View reviewed changes

nuttycom merged commit 313c072 into zcash:master Jul 3, 2023

nuttycom deleted the cap_cache branch July 3, 2023 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for tracking legacy frontiers and witnesses in `ShardTree` instances. #69

Add support for tracking legacy frontiers and witnesses in `ShardTree` instances. #69

nuttycom commented May 16, 2023 •

edited

Loading

dconnolly commented May 18, 2023

nathan-at-least left a comment

nathan-at-least Jun 2, 2023

nathan-at-least Jun 2, 2023

nathan-at-least Jun 2, 2023

daira left a comment •

edited

Loading

nathan-at-least left a comment

nathan-at-least commented Jun 5, 2023

nuttycom commented Jun 19, 2023 •

edited

Loading

daira Jun 20, 2023

daira left a comment •

edited

Loading

daira left a comment

AloeareV left a comment •

edited

Loading

AloeareV left a comment

zancas commented Jun 27, 2023

nuttycom commented Jun 27, 2023

ebfull Jun 28, 2023

nuttycom Jun 29, 2023

ebfull Jun 28, 2023

nuttycom Jun 29, 2023

ebfull Jun 28, 2023

nuttycom Jun 29, 2023

ebfull Jun 28, 2023

nuttycom Jun 29, 2023

ebfull Jun 28, 2023

nuttycom Jun 29, 2023

AloeareV commented Jun 29, 2023

		legacy-api = []
		# The test-depenencies feature guards types and functions that are

	# The test-depenencies feature guards types and functions that are
	# The test-dependencies feature guards types and functions that are

		/// Returns whether this address is the right-hand child of its parent
		pub fn is_left_child(&self) -> bool {

Add support for tracking legacy frontiers and witnesses in ShardTree instances. #69

Add support for tracking legacy frontiers and witnesses in ShardTree instances. #69

Conversation

nuttycom commented May 16, 2023 • edited Loading

dconnolly commented May 18, 2023

nathan-at-least 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

daira left a comment • edited Loading

Choose a reason for hiding this comment

nathan-at-least left a comment

Choose a reason for hiding this comment

nathan-at-least commented Jun 5, 2023

nuttycom commented Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

daira left a comment • edited Loading

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

AloeareV left a comment • edited Loading

Choose a reason for hiding this comment

AloeareV left a comment

Choose a reason for hiding this comment

zancas commented Jun 27, 2023

nuttycom commented Jun 27, 2023

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

AloeareV commented Jun 29, 2023

Add support for tracking legacy frontiers and witnesses in `ShardTree` instances. #69

Add support for tracking legacy frontiers and witnesses in `ShardTree` instances. #69

nuttycom commented May 16, 2023 •

edited

Loading

daira left a comment •

edited

Loading

nuttycom commented Jun 19, 2023 •

edited

Loading

daira left a comment •

edited

Loading

AloeareV left a comment •

edited

Loading