multi: Refactor and optimize inv discovery. #1239
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davecgh
force-pushed
the
blockchain_locateblocks
branch
2 times, most recently
from
b3dfcd4
to
a73b902
Compare
jrick
reviewed
May 30, 2018
blockchain/chain.go
Outdated
| // the best chain, so there is nothing more to do. | ||
| if startNode != nil { | ||
| b.heightLock.RLock() | ||
| if startNode.height+1 < int64(len(b.mainNodesByHeight)) { |
There was a problem hiding this comment.
This makes me wonder whether mainNodesByHeight should be a slice rather than a map, where the index in the slice is the node's block height.
There was a problem hiding this comment.
Good observation. The eventual chain view will in fact use a slice, but for now this is a map because it will simplify the transition. That said, there really is no need for this length check here. It should just be setting startNode to the result of the lookup, which will either be the next node or nil as desired. The length check was actually because I forgot it was a map for a moment and thought it was a slice!
Changed.
davecgh
force-pushed
the
blockchain_locateblocks
branch
from
a73b902
to
a9dd488
Compare
jrick
approved these changes
May 30, 2018
davecgh
force-pushed
the
blockchain_locateblocks
branch
from
a9dd488
to
23abca8
Compare
alexlyp
approved these changes
Jun 1, 2018
This refactors the code that locates blocks (inventory discovery) out of server and into blockchain where it can make use of the fact that all block nodes are now in memory and more easily be tested. As an aside, it really belongs in blockchain anyways since it's purely dealing with the block index and best chain. In order to do this reasonably efficiently, a new memory-only height to block node mapping for the main chain is introduced to allow efficient forward traversal of the main chain. This is ultimately intended to be replaced by a chain view. Since the network will be moving to header-based semantics, this also provides an additional optimization to allow headers to be located directly versus needing to first discover the hashes and then fetch the headers. The new functions are named LocateBlocks and LocateHeaders. The former returns a slice of located hashes and the latter returns a slice of located headers. Finally, it also updates the RPC server getheaders call and related plumbing to use the new LocateHeaders function. A comprehensive suite of tests is provided to ensure both functions behave correctly for both correct and incorrect block locators.
davecgh
force-pushed
the
blockchain_locateblocks
branch
from
23abca8
to
4f6ed9a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This requires PRs #1229, #1230, and #1237.
This refactors the code that locates blocks (inventory discovery) out of
serverand intoblockchainwhere it can make use of the fact that all block nodes are now in memory and more easily be tested. As an aside, it really belongs in blockchain anyways since it's purely dealing with the block index and best chain.In order to do this reasonably efficiently, a new memory-only height to block node mapping for the main chain is introduced to allow efficient forward traversal of the main chain. This is ultimately intended to be replaced by a chain view.
Since the network will be moving to header-based semantics, this also provides an additional optimization to allow headers to be located directly versus needing to first discover the hashes and then fetch the headers.
The new functions are named
LocateBlocksandLocateHeaders. The former returns a slice of located hashes and the latter returns a slice of located headers.Finally, it also updates the RPC server
getheaderscall and related plumbing to use the newLocateHeadersfunction.A comprehensive suite of tests is provided to ensure both functions behave correctly for both correct and incorrect block locators.
This is work towards #1145 and fixes #427.