Fetch data columns from multiple peers instead of just supernodes #14977
Conversation
…it can be used in both by range and by root requests
…rs instead of supernodes
niran
requested review from
terencechain,
rkapka and
dB2510
and removed request for
a team
|
|
…equests to multiple peers
Co-authored-by: Manu NALEPA <[email protected]>
niran
changed the title
beacon-chain/sync/data_columns.go
Outdated
|
|
||
| for len(peers) > len(badPeers) && len(remainingColumns) > 0 { | ||
| // Filter out bad peers from the admissible peers | ||
| filteredDataColumnsByAdmissiblePeer := make(map[peer.ID]map[uint64]bool) |
There was a problem hiding this comment.
Instead of having dataColumnsByAdmissiblePeer, badPeers and filteredDataColumnsByAdmissiblePeer, what about having only dataColumnsByAdmissiblePeer and goodPeers?
At the end of the loop, we remove a peer from the goodPeers list if some columns from the peer are missing. It then avoid this:
for p, cols := range dataColumnsByAdmissiblePeer {
if !badPeers[p] {
filteredDataColumnsByAdmissiblePeer[p] = cols
}
}
beacon-chain/sync/data_columns.go
Outdated
| } | ||
|
|
||
| if len(remainingColumns) == 0 { | ||
| break |
There was a problem hiding this comment.
Instead of breaking here (which is the happy path), I think it's better to put all the code after
// Validate the received sidecarsSo, exiting the for loop is the unhappy path, and does not need to test any more
if len(remainingColumns) > 0There was a problem hiding this comment.
That check was leftover from a previous iteration and isn't needed at all anymore. But I don't understand what you were suggesting about moving code around. I just removed it for now but let me know if there was something else you were looking for!
| // RequestDataColumnSidecars sends a data column sidecars by root request to one | ||
| // or more peers that can provide the needed data columns. | ||
| func RequestDataColumnSidecars( | ||
| ctx context.Context, |
There was a problem hiding this comment.
This function is not tested at all.
I agree it's the kind of function hard to test.
You can look at the test that have been done for data columns by range to see how to create fake peers etc...
There was a problem hiding this comment.
Yep, started working on this already based on the blocks fetcher tests. Will be done today!
What type of PR is this?
Feature
What does this PR do? Why is it needed?
The initial implementation of data column sampling required all columns to be retrieved from the same peer (typically a supernode). This PR extracts the peer selection logic from the initial sync's block fetcher to be used when pending blocks are received as well. Requests for subsets of data columns are sent to each selected peer.
Which issues(s) does this PR fix?
Implements a portion of #14129
Other notes for review
Acknowledgements