Fix elements multiple-header-download issue.#1069
Merged
stevenroose merged 1 commit intoElementsProject:masterfrom Nov 26, 2021
Merged
Fix elements multiple-header-download issue.#1069stevenroose merged 1 commit intoElementsProject:masterfrom
stevenroose merged 1 commit intoElementsProject:masterfrom
Conversation
This fixes an issue which causes Elements to download the blockchain headers multiple times during initial block download. In particular: each time we receive an INV P2P message with a new block (about once a minute), we start downloading the headers, again, in parallel with any existing download(s) in progress. With this change, after we receive each batch of headers, we check whether any of the headers in it were new to us. If not (they were all duplicates), we stop there, and do not ask the peer for another batch. This reduces the maximum amount of duplication to about 2x, which is not ideal, but a HUGE improvement.
Contributor
Author
|
A note of context: I have not been able to figure out how our behavior on this differs from upstream Bitcoin Core. It is not out of the question that Core might also have this issue, but because Core's headers are much smaller than ours, and blocks much less frequent, it may not actually trigger for them, or else it may result in a totally insignificant amount of extra work and have never been noticed. |
stevenroose
approved these changes
Nov 26, 2021
Contributor
stevenroose
left a comment
stevenroose
left a comment
There was a problem hiding this comment.
looks good, utACK cdfb4c9
| uint256 hash = block.GetHash(); | ||
| BlockMap::iterator miSelf = m_block_index.find(hash); | ||
| CBlockIndex *pindex = nullptr; | ||
| if (duplicate) { |
Contributor
There was a problem hiding this comment.
It took me a minute to realize that if (duplicate) was a null-check on the pointer, not using the bool 😅
Contributor
|
Could you backport this to 0.18. |
gwillen
added a commit
to gwillen/elements
that referenced
this pull request
Apr 21, 2022
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 fixes an issue which causes Elements to download the blockchain headers
multiple times during initial block download.
In particular: each time we receive an INV P2P message with a new block
(about once a minute), we start downloading the headers, again, in parallel
with any existing download(s) in progress.
With this change, after we receive each batch of headers, we check whether
any of the headers in it were new to us. If not (they were all duplicates),
we stop there, and do not ask the peer for another batch. This reduces the
maximum amount of duplication to about 2x, which is not ideal, but a HUGE
improvement.
With luck, this may help with #1013. But it's primarily directed at fixing slow initial block download, which I can't immediately find an open issue for on this repo, but has been a long-standing problem.