Skip to content

eth/downloader: ignore headers not requested#22214

Closed
holiman wants to merge 2 commits intoethereum:masterfrom
holiman:dl_fix
Closed

eth/downloader: ignore headers not requested#22214
holiman wants to merge 2 commits intoethereum:masterfrom
holiman:dl_fix

Conversation

@holiman
Copy link
Copy Markdown
Contributor

@holiman holiman commented Jan 22, 2021

This is a bit of work in progress.
Explanation here: #22209 (comment)

@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Jan 22, 2021

What this PR does, is ignore 'spurious' headers that arrive while we're waiting for some specific headers (specifically: during ancestor search).
So whereas previously we would reject the peer, we now don't. One consequence, is that we'll wait out the entire timeout. I'm not 100% sure what that timeout is -- in one of the downloader tests, e.g. TestShiftedHeaderAttack65Full, it's a full minute.

So does this mean that now, instead of rejecting quickly from a malicious peer, we now 'hang' for a full minute? Well, one can then switch it, and say: the attacker can already perform that attack, simply by not answering our header-request.

So regardless of what we do, the starvation/timeout attack is present. But if we roll with this PR, we might be able to continue the sync without stop/rollback/resync in case it's just a matter of a misdelivered header.

@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Feb 2, 2021

Hm, this PR unintentionally surfaced another bug in the downloader, which now causes certiain tests to hang.

https://github.com/ethereum/go-ethereum/blob/master/eth/downloader/downloader.go#L973:L978

If we request header N, and the remote peer delivers M, (e.g. N-1), and both of those are outside of the range of blocks that we have locally, then we''ll happily accept that and continue the algorithm.

The reason the tests hang (e.g. TestFailedSyncProgress65Full) is that it expects the ancestor search to find genesis, but this PR makes the findAncestor algo error out, since the remote part doesn't deliver the correct numbers.

@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Feb 23, 2021

Context

@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Feb 23, 2021

After triage: this is the right approach, need to fix up the tests and also take a look at why/of the block filter in the fetcher does not filter out any fetcher-initiated headers

@fjl fjl added this to the 1.10.0 milestone Feb 23, 2021
@fjl fjl removed the status:triage label Feb 23, 2021
@fjl fjl removed this from the 1.10.0 milestone Feb 23, 2021
@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Sep 22, 2021

I'll revisit this once the req-id downloader changes are in

@s1na
Copy link
Copy Markdown
Contributor

s1na commented Jan 13, 2022

I was going through old PRs and noticed this PR and #22249 were put on hold until req ids were in. Since req ids are now merged I'll go ahead and remove the labels. Just a friendly reminder that they're up for grabs.

@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Apr 27, 2022

Can be closed now that we have request ids

@holiman holiman closed this Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants