PMMR Backend Support for append_pruned_root (Continued) #3659
Conversation
core/src/core/pmmr/pmmr.rs
Outdated
| pub fn bintree_leaf_pos_iter(pos: u64) -> impl Iterator<Item = u64> { | ||
| let leaf_start = max(1, bintree_leftmost(pos as u64)); | ||
| let leaf_end = bintree_rightmost(pos as u64); | ||
| (leaf_start..=leaf_end).filter(|x| is_leaf(*x)) |
There was a problem hiding this comment.
It would be more efficient to iterate over leaf_indices, and map those into node_indices with the very cheap
map( \leaf_index -> 2 * leaf_index - count_ones(leaf_index) )
probably should add that as a method leaf_to_node_index()...
There was a problem hiding this comment.
Is there a way to iterate over the leaf indices of a subtree without reading them from an already-stored bitmap, (which is implementation-specific and not available in these helpers which I assume are supposed to be 'pure' MMR operations)?
There was a problem hiding this comment.
i don't see what a bitmap has to do with this routine which is only about indices.
i'm suggesting to convert both leaf_start and leaf_end into leaf indices (which is as expensive as an is_leaf test), using a node_to_leaf_index method, then construct a range from those, and map each element back to a node index.
There was a problem hiding this comment.
thanks, that makes it clearer, will implement
There was a problem hiding this comment.
the max(1, part is also redundant here.
PS: I just made a PR rewriting that MMR doc to stick with 0-based positions.
core/src/core/pmmr/pmmr.rs
Outdated
|
|
||
| /// Iterator over all pos beneath the provided subtree root (including the root itself). | ||
| pub fn bintree_pos_iter(pos: u64) -> impl Iterator<Item = u64> { | ||
| let leaf_start = max(1, bintree_leftmost(pos as u64)); |
There was a problem hiding this comment.
the max(1, part is redundant.
bintree_leftmost, like bintree_postorder_height uses 1-based positions
(which BTW I don't like. one of these days i will try to make everything 0-based)
and can never return 0.
bintree_leftmost also has a wrong description, copy-pasted from bintree_rightmost.
There was a problem hiding this comment.
it's redundant in theory however the tests are constructed with the assumption that bintree_pos_iter(0) returns an empty iterator which is produced by this, so I don't want to break this assumption ATM.
| shift_cache: vec![], | ||
| leaf_shift_cache: vec![], | ||
| }; | ||
|
|
||
| for pos in bitmap.iter().filter(|x| *x > 0) { |
There was a problem hiding this comment.
the non-zero test is either wrong (for 0-based pos) or redundant (for 1-based pos).
There was a problem hiding this comment.
agreed in theory, but removing this test breaks existing assumptions about the underlying data
| @@ -179,6 +180,18 @@ impl PruneList { | |||
| } | |||
| } | |||
|
|
|||
| // Calculate the next shift based on provided pos and the previous shift. | |||
| fn calculate_next_shift(&self, pos: u64) -> u64 { | |||
| let prev_shift = self.get_shift(pos.saturating_sub(1)); | |||
There was a problem hiding this comment.
pos.saturating_sub(1) should be simply pos-1 since pos is 1-based and cannot be 0.
There was a problem hiding this comment.
I suggest we change all occurrences of pos to either pos0 or pos1 depending on whether they're 0-based or 1-based.
There was a problem hiding this comment.
In this case yes, we can just change the saturating sub since it's only called in one place at the moment that will fail with assertion beforehand is pos is 0
There was a problem hiding this comment.
I'm going to do a bit of a 'clean as you go' on changing pos to pos0 or pos1 at the moment, but if we do a complete pass on all instances it should be within a separate PR.
There was a problem hiding this comment.
Let me know when you're done making pmmr changes so I can do my own cleanup...
| let prev_shift = self.get_shift(pos.saturating_sub(1)); | ||
| let shift = if self.is_pruned_root(pos) { | ||
| let height = bintree_postorder_height(pos); | ||
| 2 * ((1 << height) - 1) |
There was a problem hiding this comment.
Why is shift 2 less than a 2-power (1 less than a subtree size)?
…_iter to use it. Note there's still an unwrap that needs to be dealt with sanely
…rsion between 1 and 0 based in bintree_leaf_pos_iter
This is a continuation/completion of #3476
All of the comments and discussion from the previous PR still apply. I've merged this with with the most recent code, and reviewed/tested, particularly with respect to the simplification of the prune list cache in #3573.
As of now, this all appears to be working, and at least doesn't appear to break the basic existing case of expanding the pmmr by adding individual leaf nodes. Fuller testing of appending pruned segments to the pmmr will be performed in later tests/PRs.