Skip to content

Comments

Subnet search#79

Merged
jking-aus merged 9 commits intosigp:unstablefrom
diegomrsantos:subnet-search
Jan 16, 2025
Merged

Subnet search#79
jking-aus merged 9 commits intosigp:unstablefrom
diegomrsantos:subnet-search

Conversation

@diegomrsantos
Copy link
Member

@diegomrsantos diegomrsantos commented Dec 23, 2024 •
edited
Loading

Issue Addressed

Related to #35

Proposed Changes

  • Dependencies: Updated dependencies in Cargo.toml including ethereum_serde_utils, ethereum_ssz, and ssz_types.
  • Discovery Module: Added support for subnet queries, including new enums and functions for handling subnets.
  • ENR Modifications: Enhanced build_enr to include a "subnets" field and added related bitfield functions.
  • Network Initialization: Included a call to start_subnet_query during network behavior setup.
  • Protocol Identity: Introduced ProtocolId struct, implemented ProtocolIdentity for it, updated Discovery to use Discv5, replaced ssv_node_predicate with tcp_predicate, and refined debug statements. The discv5's ProtocolID field is an identifier that nodes use to diverge away from other discv5 nodes (such as Ethereum). In the ssv network, it's set to 'ssvdv5' so that we find only SSV nodes in discovery right now. The ssv field in the ENR isn't necessary anymore.

Additional Info

It advertises and searches for subnet number 9. This is an arbitrary choice for test purposes and does not have any specific significance.

@diegomrsantos diegomrsantos force-pushed the subnet-search branch 2 times, most recently from fbf8961 to 13d8afb Compare January 7, 2025 12:16
@diegomrsantos diegomrsantos marked this pull request as ready for review January 7, 2025 14:08
@diegomrsantos diegomrsantos added the ready-for-review This PR is ready to be reviewed label Jan 8, 2025
dknopik
dknopik reviewed Jan 8, 2025
View reviewed changes
AgeManning
AgeManning reviewed Jan 13, 2025
View reviewed changes
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

I notice the subnet queries are still a TODO, so just noting that.

I don't actually know about the SSV subnet specification. We might be able to simplify some things by having all the subnets long-lived (i.e we dont have to try and rotate or find peers for various subnets given a fixed operator set).

In Lighthouse we've recently changed to deterministic subnets, where the first few bits of a node-id is used to determine who should listen on which subnet. This way we can search for a node-id with a given prefix (so the XOR metric in kad) is efficient at finding them, rather than a random walk. We probably should suggest doing the same for SSV.

The other thing we need to know is how the backbone structure is formed. i.e which nodes are supposed to be permantently subscribed to which subnet.

All of these things are just spec related, maybe we already know. This PR looks good to me as base structure to move on from. Nice :)

@diegomrsantos
Copy link
Member Author

diegomrsantos commented Jan 13, 2025

I notice the subnet queries are still a TODO, so just noting that.

What part exactly? In this PR we can search for peers in a subnet, some comments may be misleading. I left them as there are more things to do, this is the bare minimum.

AgeManning
AgeManning reviewed Jan 14, 2025
View reviewed changes
anchor/network/src/discovery.rs
subnets_searched_for = ?subnets_searched_for,
"Grouped subnet discovery query yielded no results.",
);
// TODO queries.iter().for_each(|query| {
Copy link
Member

@AgeManning AgeManning Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to these todos

Copy link
Member Author

@diegomrsantos diegomrsantos Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because it's based on LH code and there're more things that I don't understand yet and I'm not sure it's necessary now. What do you think?

anchor/network/src/discovery.rs
warn!(error = %e, "Subnet query failed");
}
}
// TODO handle subnet queries
Copy link
Member

@AgeManning AgeManning Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this one

jking-aus
jking-aus approved these changes Jan 16, 2025
View reviewed changes
Copy link
Member

@jking-aus jking-aus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jking-aus jking-aus merged commit f7627b3 into sigp:unstable Jan 16, 2025
9 checks passed
@diegomrsantos diegomrsantos added network and removed ready-for-review This PR is ready to be reviewed labels Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AgeManning AgeManning AgeManning left review comments

@dknopik dknopik dknopik left review comments

@jking-aus jking-aus jking-aus approved these changes

@jxs jxs Awaiting requested review from jxs

Assignees

No one assigned

Labels

network

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@diegomrsantos @AgeManning @jking-aus @dknopik