-
Notifications
You must be signed in to change notification settings - Fork 111
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
Inbound FindBlocks
and FindHeaders
#1347
Conversation
e84cb5f
to
680d270
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some good work here, but we need to be careful about protocol conformance.
I don't think the original ticket listed all the Zcash network protocol requirements, so there are some gaps in this PR.
Here are the requirements that aren't implemented in this PR:
- return hashes from the best chain, including non-finalized blocks
- start from the first matching hash, ignoring all other hashes
- the list includes the stop hash
- the minimum size of a protocol response is 1 hash
We should describe these protocol requirements in the function documentation.
There aren't any tests yet.
I feel like this code would have been easier to write and review if we agreed on a design first. I tried to do a quick design sketch in #1306, but this PR seems to do some things differently.
We should also try harder to match the existing Zebra network protocol data structures:
https://github.com/ZcashFoundation/zebra/blob/main/zebra-network/src/protocol/internal/request.rs#L81
Co-authored-by: teor <teor@riseup.net>
Co-authored-by: Jane Lusby <jlusby42@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still making changes to the specification, and we're still struggling to follow the specification exactly. I feel like we're in a bit of a cycle where we're using PR suggestions to converge on a design, rather than just doing that design.
So I think we need to do a quick design for the find_chain_hashes
function. As part of that design, we should add some new functions to split up its tasks.
So let's create separate functions:
find_chain_hashes
: take the arguments from theFindBlocks
andFindHeaders
requests, and call other functionsfind_chain_intersection
: find the intersection height between the local best chain and the peer'sknown_blocks
collect_chain_hashes
: create the list of hashes- starting the list
- at the block after the intersection height
- if there is no intersection, at the genesis block
- stopping the list:
- after we reach the
stop
hash - after we reach the
max_len
supplied to the function (FindBlocks
: 500,FindHeaders
: 160), or - after we reach the best tip
- after we reach the
- starting the list
We should specify function names, arguments, return types, and documentation comments. The documentation comment should describe what the function does, how each argument is used, and what the return value means.
For an example, see the median time functions design:
https://github.com/ZcashFoundation/zebra/pull/1246/files#diff-3e636ad0596bc279407be7ac69096f4bea1da79cd5aa0ac76b7c4ef3f8b64ad9R442
@oxarbitrage it's the end of my day now, so I can't really do much more on the design or PR review right now. But I can make this my top priority tomorrow - which starts in about 16-18 hours from this message :-) |
Co-authored-by: teor <teor@riseup.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great, just one style tweek that I find easier to read
Also add a `best_chain_contains` function.
Add a `max_len` argument to support `FindHeaders` requests. Rewrite the hash collection code to use heights, so we can handle the `stop` hash and "no intersection" cases correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes I've made are complete, but I'd like @yaahc and @oxarbitrage to double-check before we merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks great, thank you for putting all the pieces together and getting it sorted out.
No worries! Sorry we didn't have more time to work through the design and implementation changes together. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, have a couple comments but I think this is more or less ready to merge
Co-authored-by: Jane Lusby <jlusby42@gmail.com>
Motivation
Respond to peer
FindBlocks
andFindHeaders
messages.Solution
known_blocks
stop
, the best tip, or the maximum response lengthFindBlocks
andFindHeaders
using various requests that call those functionsThe code in this pull request has:
collect_chain_hashes
#1399Review
@oxarbitrage and @teor2345 have written most of this code.
@yaahc, @oxarbitrage and @teor2345 are going to review it.
Related Issues
Closes #1078 - FindBlocks
Closes #1079 - FindHeaders
Closes #1306 - state requests
Follow Up Work
Write unit tests and property tests for
collect_chain_hashes
- #1399The other functions are trivially simple.