-
Notifications
You must be signed in to change notification settings - Fork 295
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
netsync: Track best known blocks per peer. #3443
netsync: Track best known blocks per peer. #3443
Conversation
This reworks the net sync manager block announcement handling to keep track of the best known block announced by each peer as determined by having the most cumulative proof of work. The primary motivation is to provide a relatively efficient mechanism to discover which blocks are available to download from each peer to eventually support downloading multiple blocks in parallel. It also doubles to increase robustness of best height reporting of each peer once the initial headers sync process is complete since the values are only updated when the header has more cumulative proof of work versus simply having a larger height since, although exceedingly rare in practice, it is possible for a chain with fewer blocks to have more cumulative work.
39265e0
to
01dfc85
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.
I'm completely new to this code but from a purely logical perspective it seems like it would be sensible to call maybeResolveOrphanBlock
on all peers in onInitialChainSyncDone
. Once the sync is known/suspected to be complete, it should be possible to resolve all orphan blocks announced by peers (if they are part of the best chain).
EDIT: Now that I am looking further, perhaps it doesn't matter because the announcedOrphanBlock
field is only used before sync is completed, so updating it upon completion is unnecessary?
for peer := range m.peers { | ||
m.maybeResolveOrphanBlock(peer) |
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'm completely new to this code but from a purely logical perspective it seems like it would be sensible to call
maybeResolveOrphanBlock
on all peers inonInitialChainSyncDone
. Once the sync is known/suspected to be complete, it should be possible to resolve all orphan blocks announced by peers (if they are part of the best chain).EDIT: Now that I am looking further, perhaps it doesn't matter because the
announcedOrphanBlock
field is only used before sync is completed, so updating it upon completion is unnecessary?
It's probably not entirely clear from this PR alone since nothing actually uses it yet, but ultimately it applies both before and after the initial chain sync is completed. However, it isn't needed in onInitialchainSyncDone
because of how the overall sync works.
For a quick recap, there are 2 phases. The first phase is the initial headers sync and the second phase is the chain (block) sync. onInitialChainSyncDone
happens at the end of the second phase.
The first phase is where you're realistically going to see all orphans barring some kind of chain split, because you don't know all the headers yet, so any block announcement headers will necessarily not be able to connect and thus be orphans.
However, in the second phase, all headers are known (or at least believed to be known) and thus any announcements for new blocks that show up while the chain sync is taking place will realistically be known. In the event they aren't known, attempting to resolve them once all the blocks are downloaded and linked will not gain you anything because there are no new headers involved.
Also, once phase one is done, new headers continue to be added and resolved in parallel with the chain (block) sync.
Notice that the code I'm commenting on resolves the orphans once the initial headers sync is done which is likely what you were mostly thinking about with your keen observation.
This reworks the net sync manager block announcement handling to keep track of the best known block announced by each peer as determined by having the most cumulative proof of work.
The primary motivation is to provide a relatively efficient mechanism to discover which blocks are available to download from each peer to eventually support downloading multiple blocks in parallel.
It also doubles to increase robustness of best height reporting of each peer once the initial headers sync process is complete since the values are only updated when the header has more cumulative proof of work versus simply having a larger height since, although exceedingly rare in practice, it is possible for a chain with fewer blocks to have more cumulative work.
This is work towards #1145.