p2p: Handle IsContinuationOfLowWorkHeadersSync return value correctly when new headers sync is started #26355
Conversation
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
sipa
reviewed
Oct 20, 2022
…ue correctly when new headers sync is started
91b0fdd to
7ad15d1
Compare
|
ACK 7ad15d1 |
sipa
approved these changes
Oct 21, 2022
glozow
reviewed
Oct 24, 2022
|
Backport in #26382 |
mzumsande
reviewed
Oct 24, 2022
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 25, 2022
…eturn value correctly when new headers sync is started 7ad15d1 [net processing] Handle IsContinuationOfLowWorkHeadersSync return value correctly when new headers sync is started (dergoegge) Pull request description: This PR fixes a bug in the headers sync logic that enables submitting headers to a nodes block index that don't lead to a chain that surpasses our DoS limit. The issue is that we ignore the return value on [the first `IsContinuationOfLowWorkHeadersSync` call after a new headers sync is started](https://github.com/bitcoin/bitcoin/blob/fabc0310480b49e159a15d494525c5aa15072cba/src/net_processing.cpp#L2553-L2568), which leads to us passing headers to [`ProcessNewBlockHeaders`](https://github.com/bitcoin/bitcoin/blob/fabc0310480b49e159a15d494525c5aa15072cba/src/net_processing.cpp#L2856) when that initial `IsContinuationOfLowWorkHeadersSync` call returns `false`. One easy way (maybe the only?) to trigger this is by sending 2000 headers where the last header has a different `nBits` value than the prior headers (which fails the pre-sync logic [here](https://github.com/bitcoin/bitcoin/blob/fabc0310480b49e159a15d494525c5aa15072cba/src/headerssync.cpp#L189)). Those 2000 headers will be passed to `ProcessNewBlockHeaders`. I haven't included a test here so far because we can't test this without changing the default value for `CRegTestParams::consensus.fPowAllowMinDifficultyBlocks` or doing some more involved refactoring. ACKs for top commit: sipa: ACK 7ad15d1 glozow: ACK 7ad15d1 Tree-SHA512: 9aabb8bf3700401e79863d0accda0befd2a83c4d469a53f97d827e51139e2f826aee08cdfbc8866b311b153f61fdac9b7aa515fcfa2a21c5e2812c2bf3c03664
sdaftuar
reviewed
Oct 25, 2022
There was a problem hiding this comment.
Great catch, thanks again for fixing this. I think it should be possible to write a functional test to exercise the code path that was broken before, so I'll give that a try.
I think the code merged is correct but I think it could be cleaned up a little bit, left one comment.
glozow
added a commit
that referenced
this pull request
Oct 27, 2022
…eturn value correctly when new headers sync is started e23def8 [net processing] Handle IsContinuationOfLowWorkHeadersSync return value correctly when new headers sync is started (dergoegge) Pull request description: Backport of #26355. ACKs for top commit: dergoegge: ACK e23def8 stickies-v: ACK e23def8 Tree-SHA512: 051ecb08f1f96557b5b6d01cc9d29a5dfabbb48afffd52cba662251c23277938fcbb6f207fc7575774ef627a9484ceb056cc75476861b920723c35c2f5da36c8
fanquake
added a commit
to bitcoin-core/gui
that referenced
this pull request
Oct 31, 2022
784b023 [net processing] Simplify use of IsContinuationOfLowWorkHeadersSync in TryLowWorkHeaderSync (dergoegge) e891aab [net processing] Fixup TryLowWorkHeadersSync comment (dergoegge) Pull request description: See bitcoin/bitcoin#26355 (comment) and bitcoin/bitcoin#26355 (comment) ACKs for top commit: hernanmarino: ACK 784b023 brunoerg: crACK 784b023 mzumsande: ACK 784b023 Tree-SHA512: b47ac0d78a09ca3a1806e38c5d2e2fcf1e5f0668f202450b5079c5cb168e168ac6828c0948d23f3610696375134986d75ef3c6098858173023bcb743aec8004c
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 31, 2022
784b023 [net processing] Simplify use of IsContinuationOfLowWorkHeadersSync in TryLowWorkHeaderSync (dergoegge) e891aab [net processing] Fixup TryLowWorkHeadersSync comment (dergoegge) Pull request description: See bitcoin#26355 (comment) and bitcoin#26355 (comment) ACKs for top commit: hernanmarino: ACK 784b023 brunoerg: crACK 784b023 mzumsande: ACK 784b023 Tree-SHA512: b47ac0d78a09ca3a1806e38c5d2e2fcf1e5f0668f202450b5079c5cb168e168ac6828c0948d23f3610696375134986d75ef3c6098858173023bcb743aec8004c
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 PR fixes a bug in the headers sync logic that enables submitting headers to a nodes block index that don't lead to a chain that surpasses our DoS limit.
The issue is that we ignore the return value on the first
IsContinuationOfLowWorkHeadersSynccall after a new headers sync is started, which leads to us passing headers toProcessNewBlockHeaderswhen that initialIsContinuationOfLowWorkHeadersSynccall returnsfalse. One easy way (maybe the only?) to trigger this is by sending 2000 headers where the last header has a differentnBitsvalue than the prior headers (which fails the pre-sync logic here). Those 2000 headers will be passed toProcessNewBlockHeaders.I haven't included a test here so far because we can't test this without changing the default value for
CRegTestParams::consensus.fPowAllowMinDifficultyBlocksor doing some more involved refactoring.