Domain Type Discovery #1039
Domain Type Discovery #1039
Conversation
|
Something is wrong with the GitHub actions, says "unused" but it's used just below 😅 |
|
TODO: check the domain type in handshake as well as discovery (in case the discovery record was out of date) |
are you planning to do it as part of this PR or should we create a ticket for it? |
network/discovery/dv5_service.go
Outdated
| genesis.DecorateWithDomainType(dvs.domainType), | ||
| genesis.DecorateWithSubnets(opts.Subnets), |
There was a problem hiding this comment.
should genesis be hardcoded? shouldn't it be derived from the domain type? probably relates to what @olegshmuelov is working on right now.
There was a problem hiding this comment.
now that forks are removed, these became package methods
yes however, you'd still get messages from peers with different domain type until the handshake fails and the connection is closed, but i guess that's a different PR, no? |
network/discovery/dv5_service.go
Outdated
| nodeDomainType, err := records.GetDomainTypeEntry(e.Node.Record()) | ||
| if err != nil { | ||
| logger.Debug("could not read domain type", zap.Error(err)) | ||
| return | ||
| } | ||
| if nodeDomainType != dvs.domainType { | ||
| logger.Debug("skipping node with different domain type") | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
@moshe-blox
Due to it being not backward compatible and requiring a fork, let's not enforce the check.
and add it once we ready with fork support
There was a problem hiding this comment.
|
I just noticed that discovery rejects peers without shared subnets only when we're at the connection limit, however the handshake rejects them in either case (iirc). So we should either match this condition in the handshake as well or remove it altogether. |
network/discovery/dv5_service.go
Outdated
| } | ||
|
|
||
| dvs.subnetsIdx.UpdatePeerSubnets(e.AddrInfo.ID, nodeSubnets) | ||
| if !dvs.limitNodeFilter(e.Node) { |
There was a problem hiding this comment.
can these two ifs be combined?
network/discovery/node_record.go
Outdated
| func DecorateNode(node *enode.LocalNode, decorations ...NodeRecordDecoration) error { | ||
| for _, decoration := range decorations { | ||
| if err := decoration(node); err != nil { | ||
| return errors.Wrap(err, "failed to decorate node record") |
There was a problem hiding this comment.
let's use fmt.Errorf for new code because github.com/pkg/errors is discontinued
|
No changes, just forgot to revert |
This PR introduces domain type-based peer discovery to our discovery service.
The code now checks for domain type compatibility when discovering peers. A mismatched domain type results in the node being skipped, ensuring nodes connect only with peers in the same domain. The domain type of each node is stored in the Ethereum Node Record (ENR).
We also adjust how we decorate node records, moving away from a
map[string]interface{}-based approach to using type-safe decorators for the different parameters.Incidentally, the field
operatorIDhas been removed from the node record because it exposes the operator's IP address (plus, it was unused).This upgrade ensures better network segmentation and can help to prevent potential cross-talk between nodes operating in different network domains, which we've recently witnessed on JatoV1 & JatoV2.