consensus-specs icon indicating copy to clipboard operation
consensus-specs copied to clipboard

Add MetadataV3 with custody_subnet_count

Open dapplion opened this issue 1 year ago • 4 comments

When a node receives an inbound connection from another node it does not have a way to learn its custody_subnet_count. Currently, this value is only propagated via ENRs, so it's only guaranteed to be available in the outbound connection code-path.

It's important to learn the custody_subnet_count of all peers to discover and utilize supernodes.

dapplion avatar Jun 28 '24 13:06 dapplion

Makes sense to extend the Metadata with this.

I do wonder however why there is no GetENR among the req/resp messages. Wouldn't that be useful to avoid the need for these updates to the Metadata?

cskiraly avatar Jul 02 '24 13:07 cskiraly

I do wonder however why there is no GetENR among the req/resp messages. Wouldn't that be useful to avoid the need for these updates to the Metadata?

Because the ENR changes and adds keys we need strict versioning and a way to serialize the ENR as SSZ. Otherwise, we should stream an SSZ list of bytes with the ENR encoded as RLP.

dapplion avatar Jul 03 '24 08:07 dapplion

Is there any reason you can't use gossipsub for this ? Not against this PR, but the gossipsub router tracks all connected peers and their subscribed topics. So once you are connected to a peer, you simply need to check the router for which topics a particular peer is subscribed to without requiring its custody count. This would save you a network request which might becomes a bottleneck if you have to determine this for many peers in a short amount of time.

nisdas avatar Jul 03 '24 10:07 nisdas

So once you are connected to a peer, you simply need to check the router for which topics a particular peer is subscribed to without requiring its custody count

There's some asynchrony there, as the peer may send the subscriptions later. You are right that both pieces of information should match. I recall Nimbus having some logic to penalize peers whose Gossipsub subscriptions do not match their ENR.

dapplion avatar Jul 03 '24 12:07 dapplion

Is there any reason you can't use gossipsub for this ? Not against this PR, but the gossipsub router tracks all connected peers and their subscribed topics. So once you are connected to a peer, you simply need to check the router for which topics a particular peer is subscribed to without requiring its custody count. This would save you a network request which might becomes a bottleneck if you have to determine this for many peers in a short amount of time.

I think the downside with looking only at subscribed topics is that we assume that peers are all honest and subscribe to their custody subnets. With custody count, we'll be able to identity what the nodes are supposed to custody, and still send sampling requests to them (even if they're not subscribed for some reasons) and penalise them accordingly if they can't serve the requests.

jimmygchen avatar Jul 17 '24 01:07 jimmygchen

I do wonder however why there is no GetENR among the req/resp messages.

we should stream an SSZ list of bytes with the ENR encoded as RLP.

Is this a crazy idea? For peers who have dialed us, we won't have discovery port information, so can't easily query the peer's discv5 for its ENR. This potentially could be useful there too.

wemeetagain avatar Jul 18 '24 18:07 wemeetagain

I do wonder however why there is no GetENR among the req/resp messages. Wouldn't that be useful to avoid the need for these updates to the Metadata?

@cskiraly You can already do the GetENR thing using devp2p (not libp2p). See https://github.com/ethereum/devp2p/blob/master/discv5/discv5-wire.md#findnode-request-0x03

You can do FINDNODE with distance 0

ppopth avatar Jul 26 '24 14:07 ppopth

For information we implemented it in Prysm: https://github.com/prysmaticlabs/prysm/pull/14274

nalepae avatar Aug 06 '24 10:08 nalepae