WIP: 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
|
|
| // - A map, where the key of the map is the custody group, the value is the peer that custodies the group. | ||
| // - A slice of descriptions for non admissible peers. | ||
| // - An error if any. | ||
|
|
| // | ||
| // It returns: | ||
| // - A map, where the key of the map is the peer, the value is the custody groups of the peer. | ||
| // - A map, where the key of the map is the custody group, the value is the peer that custodies the group. |
There was a problem hiding this comment.
| // - A map, where the key of the map is the custody group, the value is the peer that custodies the group. | |
| // - A map, where the key of the map is the custody group, the value is a list of peers that custody the group. |
| return dataColumnsFromSelectedPeers, nil | ||
| } | ||
|
|
||
| // custodyGroupsFromPeer compute all the custody groups indexed by peer. |
There was a problem hiding this comment.
| // custodyGroupsFromPeer compute all the custody groups indexed by peer. | |
| // custodyGroupsFromPeer computes all the custody groups indexed by peer. |
| custodyGroupCount := s.CustodyGroupCountFromPeer(peer) | ||
|
|
||
| // Get the custody groups of the peer. | ||
| custodyGroups, err := peerdas.CustodyGroups(nodeID, custodyGroupCount) |
There was a problem hiding this comment.
Could you use
peerdas.Info(...)instead?
Rationale: The Info function uses cached results.
beacon-chain/p2p/custody.go
Outdated
| descriptions := make([]string, 0, peerCount) | ||
|
|
||
| // Compute custody groups for each peer. | ||
| dataColumnsByPeer, err := s.custodyGroupsFromPeer(peers) |
There was a problem hiding this comment.
dataColumnsByPeer is misnamed, since it corresponds to custody groups, and not data columns.
There was a problem hiding this comment.
Then, we should get data columns from custody groups with peerdas.CustodyColumns.
| sidecars, err := SendDataColumnSidecarsByRootRequest(ctx, s.cfg.clock, s.cfg.p2p, peerID, s.ctxMap, &request) | ||
| // Assemble the peers who can provide the needed data columns. | ||
| peers := s.getBestPeers() | ||
| dataColumnsByAdmissiblePeer, _, _, err := s.cfg.p2p.AdmissiblePeersForDataColumns(peers, nil) | ||
| if err != nil { | ||
| return err |
| if err != nil { | ||
| return err | ||
| } | ||
| peersToFetchFrom, err := p2p.SelectPeersToFetchDataColumnsFrom(dataColumns, dataColumnsByAdmissiblePeer) | ||
| if err != nil { | ||
| return err |
| return err | ||
| } | ||
| if len(peersToFetchFrom) == 0 { | ||
| return errors.Wrapf(errNoPeersForPending, "block root=%#x", blkRoot) |
There was a problem hiding this comment.
This error does not really describe the issue.
| return errors.Wrap(err, "send and save data column sidecars") | ||
| } | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| // FIXME: Does this need to be in an else branch? It doesn't seem to apply if PeerDAS is active. |
There was a problem hiding this comment.
It's ok, if peerDAS is active we'll never get here, thanks to the continue at on line 99.
| @@ -9,18 +12,14 @@ import ( | |||
| "github.com/prysmaticlabs/prysm/v5/config/params" | |||
| ) | |||
|
|
|||
| var _ DataColumnsHandler = (*Service)(nil) | |||
|
|
|||
| // AdmissibleCustodyGroupsPeers returns a list of peers that custody a super set of the local node's custody groups. | |||
| func (s *Service) AdmissibleCustodyGroupsPeers(peers []peer.ID) ([]peer.ID, error) { | |||
There was a problem hiding this comment.
This function should not be used at all.
Why is the issue when trying to get rid of it?
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