Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deterministic subnet peer discovery #4

Conversation

ackintosh
Copy link

@ackintosh ackintosh commented Dec 11, 2023

Issue Addressed

sigp#3648

This PR is based on the deterministic subnets upgrade.

Proposed Changes

  • Added a CLI param --prefix-search-for-subnet to enable prefix search.
    • Running Lighthouse without the parameter behaves as before, defaulting to searching on a random id.
  • Added PrefixMapping that stores mappings of SubnetId to NodeIds.
    • Discovery calls PrefixMapping::get_target_nodes to get target NodeIds when run prefix searching.

Additional Info

I've measured the improvement in (attestation subnet) peer discovery brought about by this PR, based on the sent/received bytes and the number of nodes found during the discovery process.

# Random search
$ cargo run --bin lighthouse -- -l bn --enr-address {public ip address}
# Prefix search
$ cargo run --bin lighthouse -- -l bn --enr-address {public ip address} --prefix-search-for-subnet

I've run lighthouse five times each for Random search and Prefix search, observing the sent/received bytes for discovery and the number of nodes found. Based on the results below, the discovery process with Prefix search has improved by approximately 3x compared to Random search.

image

Note for self: Here are the changes used for the measurement.

@ackintosh
Copy link
Author

@divagant-martian @AgeManning This is still a work in progress, but please check if it is progressing in the right direction.

@AgeManning
Copy link
Collaborator

Hey @ackintosh - Nice.

I had a quick skim, and noticed you threaded the slot clock all the way through lighthouse_network.

The original design was to try and keep many of the consensus-level types/traits away from the lighthouse_network as best we could, so that it could behave more like a network-messaging objects without needing the concept of a blockchain, or a genesis time, or any kind of big blockchain attached to it. This way, we can create network objects without having a blockchain attached.

The way around this has been to add all kinds of consensus objects into the network crate. The network crate then passes messages into lighhtouse_network which get executed.
You'll notice the network crate probably has a bunch of consensus objects that we need. The idea was that for discovery searches, the network crate figures out when and who we need to discover and sends the message to the lighthouse_network crate to execute.

So I think it might be best to add the discovery logic somewhere in the network crate and avoid the extra trait bounds on slot-clock in the lighthouse_network crate.

@ackintosh
Copy link
Author

📝
The failure on release-tests-ubuntu will be fixed by sigp#5021.

https://github.com/divagant-martian/lighthouse/actions/runs/7327518748/job/19954506433?pr=4

CRIT Unable to listen on libp2p address listen_multiaddr: /ip4/0.0.0.0/udp/1/quic-v1, error: Other(Custom { kind: Other, error: Other(Right(Custom { kind: Other, error: Transport(Custom { kind: Other, error: Right(Custom { kind: Other, error: Io(Os { code: 13, kind: PermissionDenied, message: "Permission denied"

@ackintosh ackintosh changed the title [WIP] Deterministic subnet peer discovery Deterministic subnet peer discovery Jan 5, 2024
@ackintosh
Copy link
Author

@AgeManning @divagant-martian

Could you have another look at this? I think I have implemented what we need for deterministic subnet discovery.

I confirmed that prefix search (--prefix-search-for-subnet) improves the discovery process by approx 3x compared to random search. Please see Additional Info for the details.

@ackintosh ackintosh marked this pull request as ready for review January 5, 2024 07:37
@ackintosh
Copy link
Author

I've created another PR for the sigp repo from this branch, so I'm closing this one.

sigp#5090

@ackintosh ackintosh closed this Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants