Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parallel IBD p2p messages #68

Merged
merged 12 commits into from
Dec 21, 2020
Merged

Conversation

jaspervdm
Copy link
Contributor

Unfinished, mostly technical details while the reference implementation of these objects happens in parallel.

Rendered text

@jaspervdm jaspervdm marked this pull request as draft September 20, 2020 21:31
@lehnberg lehnberg added the node dev Related to node dev team label Sep 21, 2020
@lehnberg
Copy link
Contributor

@j01tz assigned as shepherd

@jaspervdm jaspervdm changed the title [WIP] Parallel IBD p2p messages Parallel IBD p2p messages Nov 10, 2020
@jaspervdm jaspervdm marked this pull request as ready for review November 10, 2020 15:09
Clients are free to choose a segment size by setting the height within a certain range in their requests.
This allowed range is set differently depending on the MMR contents.
The following table has concrete values for the different MMR types and the allowed height range.
The upper bound has been chosen such that a single segment roughly corresponds to the maximum size of a block (1,348,032 B).
Copy link
Member

@antiochp antiochp Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Segment sizes and prunable MMRs potentially complicate things.
For the output and rangeproof MMRs the early segments are likely to be relatively sparsely populated and for a given height will be smaller than these estimated sizes, potentially significantly smaller.

We could end up requesting many small (in bytes) output and rangeproof segments.
I'm not sure how much we care about this right now though.

Given the reconstructed bitmap it would be possible to better estimate potentially larger (in height) segments that are still a reasonable (in bytes) size.
But maybe this gets overly complex on the serving side as we can no longer verify max height without also reading the MMR data, which is more expensive.
So maybe lots of smaller segments is simpler and more efficient overall?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the current height limits are based on completely filled segments. We could instead place a limit on the number of leaves in a segment. This means either iterating through the MMR twice (once to count, the second to actually create the segment) or alternatively we use to bitmap to count the number of unpruned leaves in the segment range. One complication here is that we cannot simply take the cardinality in the range but have to account for pruned leaves where the direct sibling is unpruned.

@antiochp
Copy link
Member

I'm also wondering if we want to consider p2p messages for a possible "header segment" also.

We have a header MMR and the header sync is slow as we use the locator + sequential header chunks.
It might be valuable to consider future improvements to the header sync where we can request multiple segments of headers, effectively identical to the other segment types.

@lehnberg lehnberg added the in FCP Currently in Final Comment Period label Dec 1, 2020
@j01tz
Copy link
Member

j01tz commented Dec 1, 2020

In line with our governance process, this RFC can be considered being in Final Comment Period, with a disposition to merge in two weeks time, on December 15.

Please ensure any comments are made before then!

Copy link
Member

@j01tz j01tz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this important change 👍 It conceptually makes sense at a high level.

My only note is that since the implementation and RFC evolved in parallel, with the implementation informing the RFC to a degree, I haven't manually reviewed that the RFC describes the implementation accurately or that the implementation follows the RFC exactly. Splitting up the implementation PR's do help make this much easier anyway, so nicely done there.

@lehnberg lehnberg merged commit fe5d61c into mimblewimble:master Dec 21, 2020
@lehnberg
Copy link
Contributor

In line with our process, this RFC has now been merged. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in FCP Currently in Final Comment Period node dev Related to node dev team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants