-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
polygon/p2p: add request chunking to FetchHeaders #9536
Conversation
…c-header-downloader-interface
…ledgerwatch/erigon into astrid-sync-header-downloader-interface
…-fetch-headers-validations
…gerwatch/erigon into astrid-p2p-chunk-fetch-headers
polygon/p2p/fetcher.go
Outdated
const responseTimeout = 5 * time.Second | ||
const ( | ||
responseTimeout = 5 * time.Second | ||
maxFetchHeadersRange = 16384 |
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.
what's the reason for this limit? I feel that it should be up to the client to decide
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.
this was so that we have an upper bound on number of requests sent to a peer at the same time - it is no longer applicable since we now changed to do 1 request at a time when chunking so ive removed it
polygon/p2p/fetcher.go
Outdated
requestId := f.requestIdGenerator() | ||
requestIds[requestId] = i | ||
|
||
if err := f.messageSender.SendGetBlockHeaders(ctx, peerId, eth.GetBlockHeadersPacket66{ |
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.
we shouldn't spam a peer with requests. let's do it sequentially: send one, and wait for a response, then send a 2nd one. if the response doesn't come in time, we should retry the request a couple of times.
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.
ok, this actually simplifies the code a lot - fixed
re: retrying - im doing this in a subsequent PR #9537 (it needs to be updated with the changes ive just done here)
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.
LG ™️
This PR adds chunking logic to FetchHeaders and corresponding unit tests so that we stay within the soft limits: 1. 2 MB response size 2. 1024 headers
This PR adds chunking logic to FetchHeaders and corresponding unit tests so that we stay within the soft limits: