polygon/p2p: implement download headers#9399
Conversation
battlmonstr
left a comment
battlmonstr
left a comment
There was a problem hiding this comment.
Good progress 👍 I've commented some things. Going to merge it straight away. Feel free to do the changes in the follow-up PRs.
| "github.com/ledgerwatch/erigon/rlp" | ||
| ) | ||
|
|
||
| const reqRespTimeout = 5 * time.Second |
There was a problem hiding this comment.
| const reqRespTimeout = 5 * time.Second | |
| const responseTimeout = 5 * time.Second |
|
|
||
| const reqRespTimeout = 5 * time.Second | ||
|
|
||
| var invalidDownloadHeadersRangeErr = errors.New("invalid download headers range") |
There was a problem hiding this comment.
| var invalidDownloadHeadersRangeErr = errors.New("invalid download headers range") | |
| var invalidDownloadHeadersRangeError = errors.New("invalid download headers range") |
There was a problem hiding this comment.
Err is a standard go convention naming, I'd prefer if we stick to it
example: ctx.Err()
|
|
||
| type RequestIdGenerator func() uint64 | ||
|
|
||
| type Downloader interface { |
There was a problem hiding this comment.
I'd like to have a different name for this, because p2p.Downloader and sync.HeaderDownloader are too similar names. Some examples: HeadersChunkDownloader, HeadersSliceDownloader, HeadersRangeDownloader. Consider also "fetcher" instead of "downloader".
There was a problem hiding this comment.
I dont want to include the word Header in this because I plan to use this same component for getting block bodies too in the future. Having a separate fetcher/downloader per data model feels too fine grained to me. I can change the naming to just Fetcher
|
|
||
| g.Go(func() error { | ||
| select { | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
This seems unlikely, do we really need to call GetBlockHeaders66 from a goroutine? 🤔 although this code will probably change when you implement the TODO above...
There was a problem hiding this comment.
yes you are right ill change this
| g, ctx := errgroup.WithContext(ctx) | ||
|
|
||
| // | ||
| // TODO chunk request into smaller ranges if needed to fit in the 10 MiB response size |
There was a problem hiding this comment.
The limits are: softResponseLimit (2Mb) and MaxHeadersServe (1024) - see AnswerGetBlockHeadersQuery.
There was a problem hiding this comment.
thanks - this is useful
| "github.com/ledgerwatch/erigon/rlp" | ||
| ) | ||
|
|
||
| type MessageBroadcaster interface { |
There was a problem hiding this comment.
| type MessageBroadcaster interface { | |
| type MessageSender interface { |
I recommend to avoid "broadcast", because this word is already used to mean P2P broadcast (sending to multiple peers at once).
| RegisterBlockHeaders66Observer(observer MessageObserver[*sentry.InboundMessage]) | ||
| UnregisterBlockHeaders66Observer(observer MessageObserver[*sentry.InboundMessage]) |
There was a problem hiding this comment.
| RegisterBlockHeaders66Observer(observer MessageObserver[*sentry.InboundMessage]) | |
| UnregisterBlockHeaders66Observer(observer MessageObserver[*sentry.InboundMessage]) | |
| RegisterBlockHeadersObserver(observer MessageObserver[*sentry.InboundMessage]) | |
| UnregisterBlockHeadersObserver(observer MessageObserver[*sentry.InboundMessage]) |
Suggest to remove 66 (eth version) from method names here, because it is a lower-level concern.
|
|
||
| func (ml *messageListener) statusDataFactory() sentrymulticlient.StatusDataFactory { | ||
| return func() *sentry.StatusData { | ||
| return &sentry.StatusData{} |
There was a problem hiding this comment.
I actually dont think updating the status should happen in the message listener - I have plans to add a new component for updating our status but that is something we can do further down the line
|
|
||
| func (ml *messageListener) inboundMessageFactory() sentrymulticlient.MessageFactory[*sentry.InboundMessage] { | ||
| return func() *sentry.InboundMessage { | ||
| return new(sentry.InboundMessage) |
There was a problem hiding this comment.
This looks very generic. What's the use case where we need to customize this?
There was a problem hiding this comment.
if the question is why is the MessageFactory generic - you can have a factory for PeerEvents messages too in addition to a factory for InboundMessage
| allPeers := hd.sentry.PeersWithBlockNumInfo() | ||
| allPeers := hd.p2pService.PeersSyncProgress() | ||
| if len(allPeers) == 0 { | ||
| hd.logger.Warn(fmt.Sprintf("[%s] zero peers, will try again", headerDownloaderLogPrefix)) |
There was a problem hiding this comment.
Please remind me to discuss the log levels, and revise the Warn's here.
addresses follow up comments left on #9399
2 participants
No description provided.