feat: peerdas - ensure there are at least n peers per sampling column subnet #7274
Conversation
twoeths
changed the title
|
review note: look at packages/beacon-node/src/sync/range/utils/peerBalancer.ts and talk to @twoeths about sync strategy and how this PR affects that (or should it if not) |
| type CachedENR = { | ||
| peerId: PeerId; | ||
| multiaddrTCP: Multiaddr; | ||
| subnets: Record<SubnetType, boolean[]>; | ||
| addedUnixMs: number; | ||
| custodySubnetCount: number; | ||
| peerCustodySubnets: number[]; |
There was a problem hiding this comment.
this ain't the spec right? if not are you proposing a spec change in line with this PR?
There was a problem hiding this comment.
this is implementation specific and not part of the spec
here we store peerCustodySubnets once to reuse later
| if (oldMetadata === null || oldMetadata.csc !== peerData.metadata.csc) { | ||
| void this.requestStatus(peer, this.statusCache.get()); | ||
| } | ||
| // TODO: why request status again? |
There was a problem hiding this comment.
should request metadata if metadata is old or non existent
There was a problem hiding this comment.
need to request status again in order to update network's datacolumns of this peer, will add that to the comment
twoeths
changed the title
twoeths
changed the title
twoeths
changed the title
|
as requested by @g11tech I added back |
| // TODO: could be optimized by directly using the previously calculated subnet | ||
| const dataColumns = getDataColumns(nodeId, peerCustodySubnetCount); |
There was a problem hiding this comment.
just remove in favor of peerCustodySubnets
| /** | ||
| * A map of column subnet id to maxPeersToDiscover | ||
| */ | ||
| type ColumnSubnetQueries = Map<ColumnSubnetId, number>; |
There was a problem hiding this comment.
this is defined several places, maybe pick one place?
| @@ -332,15 +332,21 @@ export class PeerManager { | |||
| }); | |||
| if (peerData) { | |||
| const oldMetadata = peerData.metadata; | |||
| const csc = (metadata as Partial<peerdas.Metadata>).csc ?? this.config.CUSTODY_REQUIREMENT; | |||
| const nodeId = peerData?.nodeId ?? computeNodeId(peer); | |||
| const custodySubnets = (csc !== oldMetadata?.csc || oldMetadata?.custodySubnets == null)? getDataColumnSubnets(nodeId, csc) : oldMetadata?.custodySubnets; | |||
There was a problem hiding this comment.
would oldMetadata?.custodySubnets ever be null?
Maybe more clear to do
const custodySubnets = oldMetadata == null || csc !== oldMetadata.csc
? getDataColumnSubnets(nodeId, csc)
: oldMetadata.custodySubnets;
Motivation
8data columns from sampling subnetsnnumber of peers per subnet for nownnumber of peers per column sampling subnetDescription
custodySubnetsinpeersDataso that we don't have to compute it all the timestargetColumnSubnetPeers = 6which make it easy to achieve after a few heartbeats but this could be changed with a newly added cli flag with the same nameonlyConnect*flags