Giter Club home page Giter Club logo

discv5's Introduction

discv5

Build Status Doc Status Crates Status

Documentation at docs.rs

Overview

This is a rust implementation of the Discovery v5 peer discovery protocol.

Discovery v5 is a protocol designed for encrypted peer discovery. Each peer/node on the network is identified via it's ENR (Ethereum Node Record), which is essentially a signed key-value store containing the node's public key and optionally IP address and port.

Discv5 employs a kademlia-like routing table to store and manage discovered peers and topics. The protocol allows for external IP discovery in NAT environments through regular PING/PONG's with discovered nodes. Nodes return the external IP address that they have received and a simple majority is chosen as our external IP address. If an external IP address is updated, this is produced as an event to notify the swarm (if one is used for this behaviour).

For a simple CLI discovery service see discv5-cli

Usage

A simple example of creating this service is as follows:

   use discv5::{enr, enr::{CombinedKey, NodeId}, TokioExecutor, Discv5, ConfigBuilder};
   use discv5::socket::ListenConfig;
   use std::net::SocketAddr;

   // construct a local ENR
   let enr_key = CombinedKey::generate_secp256k1();
   let enr = enr::Enr::empty(&enr_key).unwrap();

   // build the tokio executor
   let mut runtime = tokio::runtime::Builder::new_multi_thread()
       .thread_name("Discv5-example")
       .enable_all()
       .build()
       .unwrap();

   // configuration for the sockets to listen on
   let listen_config = ListenConfig::Ipv4 {
       ip: Ipv4Addr::UNSPECIFIED,
       port: 9000,
   };

   // default configuration
   let config = ConfigBuilder::new(listen_config).build();

   // construct the discv5 server
   let mut discv5: Discv5 = Discv5::new(enr, enr_key, config).unwrap();

   // In order to bootstrap the routing table an external ENR should be added
   // This can be done via add_enr. I.e.:
   // discv5.add_enr(<ENR>)

   // start the discv5 server
   runtime.block_on(discv5.start());

   // run a find_node query
   runtime.block_on(async {
      let found_nodes = discv5.find_node(NodeId::random()).await.unwrap();
      println!("Found nodes: {:?}", found_nodes);
   });

Addresses in ENRs

This protocol will drop messages (i.e not respond to requests) from peers that advertise non-contactable address in their ENR (e.g 127.0.0.1 when connecting to non-local nodes). This section explains the rationale behind this design decision.

An ENR is a signed record which is primarily used in this protocol for identifying and connecting to peers. ENRs have OPTIONAL ip and port fields.

If a node does not know its contactable address (i.e if it is behind a NAT), it should leave these fields empty. This is done for the following reasons:

  1. When we receive an ENR we must decide whether to add it to our local routing table and advertise it to other peers. If a node has put some non-contactable address in the ENR (e.g 127.0.0.1 when connecting to non-local nodes) we cannot use this ENR to contact the node and we therefore do not wish to advertise it to other nodes. Putting a non-contactable address is therefore functionally equivalent to leaving the fields empty.
  2. For every new inbound connection, we do not wish to check that the address given to us in an ENR is contactable. We do not want the scenario, where any peer can give us any address and force us to attempt a connection to arbitrary addresses (to check their validity) as it consumes unnecessary bandwidth and we want to avoid DOS attacks where malicious users spam many nodes attempting them all to send messages to a victim IP.

How this protocol handles advertised IPs in ENRs

To handle the above two cases this protocol filters out and only advertises contactable ENRs. It doesn't make sense for a discovery protocol to advertise non-contactable peers.

This is done in the following way:

  1. If a connecting node provides an ENR without specifying an address (this should be the default case for most nodes behind a NAT, or ones that have just started) we consider this valid. Typically this will occur when a node has yet to determine its external IP address via PONG responses and has not updated its ENR to a contactable address. In this case, we respond to all requests this peer asks for but we do not store or add its ENR to our routing table.
  2. If a peer connects to us with an ENR that specifies an IP address that matches the src address we received the packet from, we consider this peer valid and attempt to add it to our local routing table and therefore may advertise its ENR to others.
  3. If a peer connects to us with an ENR that specifies an IP address that does not match the src socket it connects to us on (e.g 127.0.0.1, or potentially some internal subnet IP that is unreachable from our current network) we consider this peer malicious/faulty and drop all packets. This way we can efficiently drop peers that may try to get us to send messages to arbitrary remote IPs, and we can be sure that all ENRs in our routing table are contactable (at least by our local node at some point in time).

discv5's People

Contributors

ackintosh avatar agemanning avatar armaganyildirak avatar burtonqin avatar carver avatar divagant-martian avatar emhane avatar jacobkaufmann avatar jmcph4 avatar jrhea avatar jtraglia avatar jxs avatar kolbyml avatar mattsse avatar michaelsproul avatar morph-dev avatar njgheorghita avatar ogenev avatar onbjerg avatar ordian avatar paulhauner avatar pawanjay176 avatar realbigsean avatar samwilsn avatar vorot93 avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

discv5's Issues

Make `Enr` generic with `Enr<CombinedKey>` as default

To not limit the flexibility of which key type to use offered by the Enr crate, Enr should be made generic in discv5. The default generic should still be CombinedKey, the key type being used now. This change also requires changing the impl bodies to be extend to work for all types of keys, not just be implemented for the default CombinedKey. This shouldn't be hard because the EnrKey trait is at disposal from the enr crate.

impl<P: ProtocolIdentity, K: EnrKey> Discv5<P, K> { ...

Packet Filtering by Node

The more sophisticated packet filtering including handling the maximum requests by node still needs to be implemented

Use zero ports in tests

Similarly to sigp/lighthouse#4704, some of our tests race on port number allocation. The main offenders are:

These both hardcode UDP ports (like 10001 and 10002). Locally, all it takes me is 3-4 runs of the test suite to induce failures due to AddrInUse. The specific failure case is always a panic when the port can't be allocated:

thread 'service::test::test_connection_direction_on_inject_session_established' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 98, kind: AddrInUse, message: "Address already in use" }', src/service/test.rs:58:14

errors after update, related to alloy_rlp

Not sure if this is known already, but here is the output (v0.4.1)

error[E0432]: unresolved import `enr::EnrError`
  --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/discv5-0.4.1/src/discv5.rs:26:24
   |
26 | use enr::{CombinedKey, EnrError, EnrKey, NodeId};
   |                        ^^^^^^^^ no `EnrError` in the root
error[E0277]: the trait bound `T: alloy_rlp::encode::Encodable` is not satisfied
   --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/discv5-0.4.1/src/discv5.rs:443:26
    |
443 |             .insert(key, value, &self.enr_key.read())
    |              ------      ^^^^^ the trait `alloy_rlp::encode::Encodable` is not implemented for `T`
    |              |
    |              required by a bound introduced by this call
    |
note: required by a bound in `enr::Enr::<K>::insert`
   --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/enr-0.10.1/src/lib.rs:486:22
    |
486 |     pub fn insert<T: Encodable>(
    |                      ^^^^^^^^^ required by this bound in `Enr::<K>::insert`
help: consider further restricting this bound
    |
436 |     pub fn enr_insert<T: rlp::Encodable + alloy_rlp::encode::Encodable>(
    |                                         ++++++++++++++++++++++++++++++

   Compiling jsonrpsee v0.20.3
error[E0277]: the trait bound `enr::Enr<CombinedKey>: Encodable` is not satisfied
   --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/discv5-0.4.1/src/packet/mod.rs:176:35
    |
176 |                 let node_record = enr_record.as_ref().map(rlp::encode);
    |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Encodable` is not implemented for `enr::Enr<CombinedKey>`
    |
    = help: the following other types implement trait `Encodable`:
              bool
              usize
              u8
              u16
              u32
              u64
              u128
              Box<T>
            and 7 others
note: required by a bound in `rlp::encode`
   --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rlp-0.5.2/src/lib.rs:98:5
    |
96  | pub fn encode<E>(object: &E) -> BytesMut
    |        ------ required by a bound in this function
97  | where
98  |     E: Encodable,
    |        ^^^^^^^^^ required by this bound in `encode`

error[E0277]: the trait bound `enr::Enr<CombinedKey>: Encodable` is not satisfied
   --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/discv5-0.4.1/src/packet/mod.rs:176:59
    |
176 |                 let node_record = enr_record.as_ref().map(rlp::encode);
    |                                                           ^^^^^^^^^^^ the trait `Encodable` is not implemented for `enr::Enr<CombinedKey>`
    |
    = help: the following other types implement trait `Encodable`:
              bool
              usize
              u8
              u16
              u32
              u64
              u128
              Box<T>
            and 7 others
note: required by a bound in `rlp::encode`
   --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rlp-0.5.2/src/lib.rs:98:5
    |
96  | pub fn encode<E>(object: &E) -> BytesMut
    |        ------ required by a bound in this function
97  | where
98  |     E: Encodable,
    |        ^^^^^^^^^ required by this bound in `encode`

error[E0277]: the trait bound `enr::Enr<CombinedKey>: Decodable` is not satisfied
   --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/discv5-0.4.1/src/packet/mod.rs:262:39
    |
262 |                         rlp::decode::<Enr>(&remaining_data[total_size..])
    |                                       ^^^ the trait `Decodable` is not implemented for `enr::Enr<CombinedKey>`
    |
    = help: the following other types implement trait `Decodable`:
              bool
              usize
              u8
              u16
              u32
              u64
              u128
              Box<T>
            and 5 others
note: required by a bound in `rlp::decode`
   --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rlp-0.5.2/src/lib.rs:75:5
    |
73  | pub fn decode<T>(bytes: &[u8]) -> Result<T, DecoderError>
    |        ------ required by a bound in this function
74  | where
75  |     T: Decodable,
    |        ^^^^^^^^^ required by this bound in `decode`

error[E0277]: the trait bound `enr::Enr<CombinedKey>: Encodable` is not satisfied
   --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/discv5-0.4.1/src/rpc.rs:208:34
    |
208 |                         s.append(&node);
    |                           ------ ^^^^^ the trait `Encodable` is not implemented for `enr::Enr<CombinedKey>`
    |                           |
    |                           required by a bound introduced by this call
    |
    = help: the following other types implement trait `Encodable`:
              bool
              usize
              u8
              u16
              u32
              u64
              u128
              Box<T>
            and 7 others
note: required by a bound in `RlpStream::append`
   --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rlp-0.5.2/src/stream.rs:113:6
    |
111 |     pub fn append<E>(&mut self, value: &E) -> &mut Self
    |            ------ required by a bound in this associated function
112 |     where
113 |         E: Encodable,
    |            ^^^^^^^^^ required by this bound in `RlpStream::append`

error[E0277]: the trait bound `enr::Enr<CombinedKey>: Decodable` is not satisfied
   --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/discv5-0.4.1/src/rpc.rs:442:48
    |
442 |                         enr_list_rlp.as_list::<Enr<CombinedKey>>()?
    |                                      -------   ^^^^^^^^^^^^^^^^ the trait `Decodable` is not implemented for `enr::Enr<CombinedKey>`
    |                                      |
    |                                      required by a bound introduced by this call
    |
    = help: the following other types implement trait `Decodable`:
              bool
              usize
              u8
              u16
              u32
              u64
              u128
              Box<T>
            and 5 others
note: required by a bound in `Rlp::<'a>::as_list`
   --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rlp-0.5.2/src/rlpin.rs:281:6
    |
279 |     pub fn as_list<T>(&self) -> Result<Vec<T>, DecoderError>
    |            ------- required by a bound in this associated function
280 |     where
281 |         T: Decodable,
    |            ^^^^^^^^^ required by this bound in `Rlp::<'a>::as_list`

error[E0277]: the trait bound `enr::Enr<CombinedKey>: Encodable` is not satisfied
    --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/discv5-0.4.1/src/service.rs:1058:46
     |
1058 |                 let entry_size = rlp::encode(&enr).len();
     |                                  ----------- ^^^^ the trait `Encodable` is not implemented for `enr::Enr<CombinedKey>`
     |                                  |
     |                                  required by a bound introduced by this call
     |
     = help: the following other types implement trait `Encodable`:
               bool
               usize
               u8
               u16
               u32
               u64
               u128
               Box<T>
             and 7 others
note: required by a bound in `rlp::encode`
    --> /home/bufulin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rlp-0.5.2/src/lib.rs:98:5
     |
96   | pub fn encode<E>(object: &E) -> BytesMut
     |        ------ required by a bound in this function
97   | where
98   |     E: Encodable,
     |        ^^^^^^^^^ required by this bound in `encode`

   Compiling ssz_types v0.5.4
Some errors have detailed explanations: E0277, E0432.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `discv5` (lib) due to 8 previous errors
warning: build failed, waiting for other jobs to finish...

"PROTOCOL_ID has already been initialized" error when creating more than one `Discv5` instance

I'm getting "PROTOCOL_ID has already been initialized" error when trying to create more than one Discv5 instance.

discv5 version: 0.2.0

Here is an example code to replicate the issue:

use discv5::{Discv5, Discv5ConfigBuilder};
use discv5::enr::{CombinedKey, EnrBuilder};

fn main() {
    let config_1 = Discv5ConfigBuilder::new().protocol_id("fooooo").build();
    let enr_key_1 = CombinedKey::generate_secp256k1();
    let enr_1 = EnrBuilder::new("v4").build(&enr_key_1).unwrap();
    let _ = Discv5::new(enr_1, enr_key_1, config_1).unwrap();

    let config_2 = Discv5ConfigBuilder::new().build();
    let enr_key_2 = CombinedKey::generate_secp256k1();
    let enr_2 = EnrBuilder::new("v4").build(&enr_key_2).unwrap();
    let _ = Discv5::new(enr_2, enr_key_2, config_2).unwrap();
}

The last line panics with:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: "PROTOCOL_ID has already been initialized"'

[Tracking] Implement Topics

This is a tracking issue to implement the new topic (aka service discovery) mechanism that is soon to be finalized.

Once specifications and further details are released, this issue will be updated with those details.

hash(local_node_id) can be cached

Each time a RandomPacket is received, the src_node_id must be calculated from the tag field. This requires the hash(local_node_id) to be calculated:

let hash = Sha256::digest(&self.node_id.raw());

This can be avoided if the hash(local_node_id) is cached.

Encoded RPC Message bytes are not same

When RPC Message was decoded and encoded these bytes [6, 193, 0, 75, 252], Encoding message returns different result [6, 194, 0, 75]

When the message is decoded, The message is Response(Response { id: RequestId([0]), body: Talk { response: [75] } })

Multiple Nodes Responses and Packet Filtering

The Handler manages single requests per nodes.

It currently checks a response id against an awaiting request id. If they match, the awaiting request is removed and any queued requests then get processed.
For the case of a Nodes response, there can be multiple responses to a single request. Although this is handled in the Service (currently the Handler returns expired and unmatched Responses to the Service) It would be nicer to also handle this logic in the Handler. This would help for a number of reasons:

  1. The Handler would not return any expired or unmatched Responses
  2. We would maintain a single request per node at a time, rather than sending new requests whilst still potentially awaiting Nodes responses
  3. The packet filter requires knowledge about responses that we are awaiting. The Handler keeps track of which responses we are awaiting. It could then manage these for Nodes responses accurately.

The limit of packet size

From the wire protocol, there's a limited range of packet size which implementations should consider the packet is valid:

  • the maximum size of any packet is 1280 bytes
  • the minimum size of any packet is 63 bytes

Currently we support the limit partially, not sufficient. I'll file a PR to improve that. ๐Ÿ˜ƒ

Handle IP changes gracefully

A connected node, when its external ip changes will have an ENR with a set ip which does not match its outbound socket.

A discv5 node will not create a session with these because the ENR is currently non-contactable. We should allow for a mechanism (i.e PING) whereby these nodes can get PONG responses to find out their IP has changed an update their ENR's accordingly. This is being worked on in #184

move discv5 to structured logging

transform logging statements from using formating in strings to structured logging, for example:

- debug!("Node set to disconnected: {}", node_id)
+ debug!(%node_id, "Node set to disconnected")

This would make it easier to make sense of emitted logs

Re-implement lru-time-cache

The current lru_time_cache dependency is in efficient under most operations.

We are using this cache for a variety of fields in the handler. The sessions in particular can be quite large and we don't want to be doing O(n) operations on every get for every message.

See eg, maidsafe/lru_time_cache#143

Unexpected "Requested a node find itself" error

I got the error when a response of FINDNODE contains ENR of the target node (the node passed to Discv5::find_node()).

Error message

2022-04-14T01:11:36.349333Z ERROR discv5::service: Send RPC failed: Requested a node find itself

Details of the case which occurs the issue

20220414_requested_a_node_find_itself

How to fix

I think we should skip to insert the peer into closest_peers if the peer received via NODES response and the target are same.

let peer = QueryPeer::new(key, QueryPeerState::NotContacted);
self.closest_peers.entry(distance).or_insert(peer);

I will file a PR to fix this issue. ๐Ÿ˜ƒ

Speed up ENR update

We do pings every 5 minutes after connections for security reasons.

I think its probably safe for us to ping outgoing connections that end up in our routing table on initial connection.

Add Error Metric

It would be useful to add a warning and error metric to the global metrics.

This way if an error or warning occurs we increment these counts. Often in Lighthouse, we dont emit logs for discovery and a variety of errors and warnings can go unnoticed.

If we had these in the metrics we could just monitor metrics for discovery to observe if anything is going wrong.
I think it would be good to add the following metrics:

  • Error count
  • Warning count
  • Error Type - Give some string context as to which type of error this is. As the debug logs are not saved and we see a large error/warning count it would be good to provide some context into what these errors are.

Request ENR for NodeAddress

Is it possible to request the Enr for a given NodeAddress? The trin portal network client builds multiple overlay networks on top of the base Discovery v5 network. All communications on these overlay networks are via TalkRequest messages. The overlay networks rely on discv5 for session management and message transmission.

When we receive a ping from a previously unknown node, we are not able to request the Enr for that node, so we cannot add it to our routing table. The request_enr method of Discv5 requires a TryInto<Multiaddr>. This method then attempts to convert the MultiAddr into a NodeContact via TryFrom<MultiAddr> for NodeContact. That function requires that the MultiAddr contain a multi-hash that corresponds to a valid public key for the p2p protocol. However, the associated TalkRequest only contains a NodeAddress, which does not include public key information.

Is there a way to either (1) add a function to request the Enr for a NodeAddress, or (2) add additional information to the TalkRequest object that would allow us to send such a request?

From my investigations, it seems that there is no trivial way to achieve (1) or (2) without some additional structure in the Handler or the Service, so I'm opening this issue to get thoughts from the developers.

Support configuring a different protocol id

Background

I am working on rust-waku and, more precisely, on the 33/WAKU2-DISCV5 RFC implementation.

The reference implementation, the Nim implementation (nwaku), of the Waku discv5 node discovery mechanism uses a different protocol ID for convenience. This is d5waku.

Description

Right now, in v0.1.0, the PROTOCOL_ID is hardcoded:

https://github.com/sigp/discv5/blob/v0.1.0/src/packet/mod.rs#L32-L33

To make rust-waku compatible with the Waku reference implementation, I would like to add support to configure the protocol ID (similar to what rust-libp2p does with the Gossipsub protocol).

Acceptance criteria

  • Add the protocol_id field to Discv5Config. The default value must be "discv5"
  • Add a matching protocol_id function to the Discv5ConfigBuilder.
  • Add a protocol_id field to packet::{Packet, PacketHeader}.
  • Encapsulate the packet::Packet codec logic to support checking the message validity against non-constant (non-static) values.

Tests for Service

The tests for the new Service task need to be upgraded and improved upon

low latency light overlays: k-peer-disjoint updates, scaling the p2p with bounded peer-updates

duplicate of: ethereum/trin#750, if anybody wants to work on this that's great. possibly before digging into this it's beneficial to take a second look at rust-libp2p's kbuckets as they are also using the same parity code, and see if there are any parts before left out that are now relevant to add.

Last year I started working on this enr-bank-discv5, as I was researching on Felix's topics spec. The topics spec as well as Trin uses KBucket overlays, basically subsets of the discv5 network stored with respect to a different order, or in other words different partial views of the same network.
When the overlays overlap, ENR or connectivity updates that happen on one layer need to be visible on all overlays so that the overlays can scale efficiently. For example we can optimise memory usage and latency in the case of running more than one portal, like state and history, in the same instance as different top-level tasks, but also just running one portal on top of discv5 which very probably means a big overlap of peers between the discv5 DHT and the portal DHT already.
Currently, when a peer goes offline or updates their ENR, this needs to be found out anew by trial and error on each overlay - this latency, of 2 RTT for a PING-PONG ENR update and of 2 seconds for the default discv5 query peer time out upon unresponsiveness, can be minimised to once per peer - a constant bound, k = 1, on k-per-peer updates! Rn the per-peer updates are bound by the input number of overlays. That takes a toll when we want to increase query parallelism and increase the number of virtualisation layers - portals of different services (not random) - on top of the discv5 DHT secured by random unstructured structure. So I suggest in terms of DHT entries we flatten all the layers of DHTs out, to one big pancake of k-bucket entries, with links from each entry to the DHT layers that includes the entry, as opposed to copies of the entry.

I have 2 different approaches in mind:

  • If we don't have a memory constraint, we can add some Discv5Events to know when a peer is disconnected
SessionDisconnected(NodeId),

and when an ENR of a peer is updated

EnrUpdated { enr: Enr, replaced: Option<Enr> },

We emit these events from service.rs here and here respectively, and also here but first making the logic separation between connection update, value update and both updated (emit 2 events).

All Discv5Events rn clone the ENR to thread-safely pass up to the app (move it) running discv5, which then stores the ENR in the app's k-buckets - if discv5 had space for the ENR in its k-buckets, it has also stored the ENR before passing it up.

  • If we have a memory constraint: we use thread-safe references and locks. ENRs take up to 300 bytes in memory. Following code snippets originate from kbucket/buckets.rs. How we lock lock the Node depends on if we want to update connection status and the ENR independently. Locking the whole Node
pub struct KBucket<TNodeId, TVal: Eq> {
    /// The nodes contained in the bucket.
    nodes: ArrayVec<Arc<RwLock<Node<TNodeId, TVal>>>, MAX_NODES_PER_BUCKET>,

or, locking connection status and ENR independently

pub struct Node<TNodeId, TVal: Eq> {
    /// The key of the node, identifying the peer.
    pub key: Key<TNodeId>,
    /// The associated value.
    pub value: Arc<RwLock<TVal>>,
    /// The status of the node.
    pub status: Arc<RwLock<NodeStatus>>,
}

or if we can't make our mind up, with our current understanding of how we will use overlapping DHT overlays in the future, we make the whole node generic (definitely the best idea!), with traits so little of the existing code needs to be changed (i.e. so calls to methods of NodeStatus and Node types already in the code will still work).

pub trait AsNodeStatus {...}

pub trait AsNode<TNodeId, TVal: Eq, NodeStatus: AsNodeStatus> {...}

pub struct KBucket<Node: AsNode> {
    /// The nodes contained in the bucket.
    nodes: ArrayVec<Node, MAX_NODES_PER_BUCKET>,

This way each entry only exists once in memory, but can be mutated by any overlay. Any overlay can mutate any entry with just a read lock on the shared collection of all entries (the pancake) and that change will happen on all overlays, including the discv5 DHT: check out my playground example. Which is great when using a RwLock that allows for multiple concurrent reads. Without this lock on each entry, to disconnect a peer in discv5 based on some portal specific criteria requires waiting on a write lock here.

Why would we have a memory constraint? The more efficiently portals or any other discv5 DHT overlay handles memory, the larger we can make k-buckets which in turn means me have more gains from increasing query parallelisation to tackle latency. A memory-efficient way to increase k-buckets size is to handle this interval in at least two parts when allocating memory for nodes , where for example all buckets until log2distance 200 initialise with the current MAX_NODES_PER_BUCKET= 16 ArrayVec<Node<TNodeId, TVal>, MAX_NODES_PER_BUCKET> and all nodes log2distance 200 to 256 initialise allocating for 3 times as many nodes ArrayVec<Node<TNodeId, TVal>, MAX_NODES_PER_BUCKET * 3>. Tbh, the odds that even one single peer will be in the k-buckets aprox. log2distance 1-100 is so low, that imo we should change the data structure from ArrayVec to another data structure that doesn't allocate memory initially for that interval and dynamically allows that interval to allocate memory up to a MAX_NODES_PER_BUCKET limit. If no such data structure exists giving the option to set either a fixed or dynamic memory allocation, we could use an enum

enum Nodes {
    Fixed(ArrayVec),
    Dynamic(Vec),
}

and create a trait AsArrayVec with all the methods of ArrayVec called in the kbucket/bucket.rs, and then implement that trait for the enum Nodes to allow for easy compatibility with parity's existing k-buckets code (i.e. without changing the existing method calls).

(A related PR would be to remove this event, which is not used anywhere in the code as this method and it's return type are sync.)

allow per-query predicates

Right now the predicate we have for queries receives an enr and the query finishes when a number of peers is found. I think an improvement would be to allow a per query predicate that can accumulate the found peers and decide if it found the peers it needs.

The difference is that, as we have this right now, the grouped predicate is if n peers satisfy a predicate. The new version would allow to identify if a group satisfies a predicate.

In lh this would be useful since we have grouped queries for subnets and it might be more beneficial to ensure that at least one peers belongs to each subnet in the query than checking if every peer belongs to at least one, where this would finish when potentially all peers belong to the same one. Changing this logic would mitigate queries where the "individual" predicates have a distribution such that some are harder to satisfy than others

Update `lru` to 0.12

discv5's version of lru is quite outdated at 0.7

After this PR (sigp/lighthouse#4994) discv5 will be the only dep forcing duplication of the lru crate in Lighthouse. Updating it to 0.12.1 would remove the duplication.

reduce libp2p dependencies

the libp2p feature pulls in libp2p entirely:

libp2p = { version = "0.53", features = ["ed25519", "secp256k1"], optional = true }

but only identity and MultiAddr are used

discv5/src/node_info.rs

Lines 7 to 11 in 44051ef

use libp2p::{
identity::{KeyType, PublicKey},
multiaddr::Protocol,
Multiaddr,
};

which are available as standalone crates:

multiaddr and libp2p-identity

Combine active requests mapping into a struct

There are now two active requests mapping and currently they are manually kept in sync with each other.

Grouping these data structures will make upgrades more reliable and force the two mappings to always be in sync and prevent coding errors.

Documentation Update

The documentation for the re-write needs to be updated throughout the code base

Examples on MacOS

Hi! I tried to run find_nodes example on MacOS to setup two "nodes" on localhost and it was a little bit unclear how to setup this. On Linux everything worked fine.

The problem is that by default SocketKind is dual stack (ip4 + ip6), so I supposed it should work when I bind to "ip6 socket" , which is by default, and try to send packets to the "ip4 socket". But I failed it to work and was getting an IO error: Error: Os { code: 22, kind: InvalidInput, message: "Invalid argument" }. The solution is to bind to the ip4 socket, but the error itself is very unclear and maybe it should be addressed to tokio itself, but could we make it more clear in the example documentation for MacOS? Or find some other ways like adding conditional compilation and require to specify SocketKind when #[cfg(target_os = "macos")]?

`entered unreachable code: Stored ENR is not contactable`

Goes through unreachable path.

debug_unreachable!("Stored ENR is not contactable. {}", enr);

Have a hunch this has to do with adding multiaddresses, since this is how I'm adding enodes in my pr, entry point here

https://github.com/paradigmxyz/reth/blob/993036014eb6c3cb51e308b885339e483771dbb4/crates/net/network/src/config.rs#L553

thread 'tokio-runtime-worker' panicked at /home/ubuntu/.cargo/git/checkouts/discv5-7af694118de5b208/04ac004/src/service.rs:598:29:                                                                                                                   
internal error: entered unreachable code: Stored ENR is not contactable. enr:-Le4QAAHJBTIJ1hP4eLJgvFFJ2E29tIVz9LEgQeACShg3sMxGgt-zi7bVlaHdEICENFuir0QAxMuW_Cj_39hhTjPUZSCB3-HYXR0bmV0c4hgAAAAAAAAAIRldGgykGmuDpkFAXAA__________-CaWSCdjSEcXVpY4IjM4lz
ZWNwMjU2azGhA7gc3n5LM3gKW91FEtG_4J_a-lgwJxot6tLQtHnmomEOiHN5bmNuZXRzDIN0Y3CCIzw                                                                                                                                                                      
stack backtrace:                                                                                                                                                                                                                                     
   0: rust_begin_unwind                                                                                                                                                                                                                              
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:645:5                                                                                                                                                   
   1: core::panicking::panic_fmt                                                                                                                                                                                                                     
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panicking.rs:72:14                                                                                                                                                  
   2: discv5::service::Service::handle_rpc_request                                                                                                                                                                                                   
             at /home/ubuntu/.cargo/git/checkouts/discv5-7af694118de5b208/04ac004/src/service.rs:598:29                                                                                                                                              
   3: discv5::service::Service::start::{{closure}}                                                                                                                                                                                                   
             at /home/ubuntu/.cargo/git/checkouts/discv5-7af694118de5b208/04ac004/src/service.rs:385:33                                                                                                                                              
   4: discv5::service::Service::spawn::{{closure}}::{{closure}}                                                                                                                                                                                      
             at /home/ubuntu/.cargo/git/checkouts/discv5-7af694118de5b208/04ac004/src/service.rs:328:33                                                                                                                                              
   5: <core::pin::Pin<P> as core::future::future::Future>::poll                                                                                                                                                                                      
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/future/future.rs:124:9                                                                                                                                              
   6: tokio::runtime::task::core::Core<T,S>::poll::{{closure}}                                                                                                                                                                                       
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/core.rs:328:17                                                                                                                       
   7: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut                                                                                                                                                                                         
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/loom/std/unsafe_cell.rs:16:9                                                                                                                      
   8: tokio::runtime::task::core::Core<T,S>::poll                                                                                                                                                                                                    
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/core.rs:317:13                                                                                                                       
   9: tokio::runtime::task::harness::poll_future::{{closure}}                                                                                                                                                                                        
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/harness.rs:485:19                                                                                                                    
  10: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once                                                                                                                                                  
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panic/unwind_safe.rs:272:9                                                                                                                                          
  11: std::panicking::try::do_call                                                                                                                                                                                                                   
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:552:40                                                                                                                                                  
  12: __rust_try                                                                                                                                                                                                                                     
  13: std::panicking::try                                                                                                                                                                                                                            
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:516:19                                                                                                                                                  
  14: std::panic::catch_unwind                                                                                                                                                                                                                       
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panic.rs:142:14                                                                                                                                                      
  15: tokio::runtime::task::harness::poll_future                                                                                                                                                                                                     
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/harness.rs:473:18                                                                                                                    
  16: tokio::runtime::task::harness::Harness<T,S>::poll_inner                                                                                                                                                                                        
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/harness.rs:208:27                                                                                                                    
  17: tokio::runtime::task::harness::Harness<T,S>::poll                                                                                                                                                                                              
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/harness.rs:153:15                                                                                                                    
  18: tokio::runtime::task::raw::poll                                                                                                                                                                                                                
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/raw.rs:271:5                                                                                                                         
  19: tokio::runtime::task::raw::RawTask::poll                                                                                                                                                                                                       
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/raw.rs:201:18                                                                                                                        
  20: tokio::runtime::task::LocalNotified<S>::run                                                                                                                                                                                                    
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/mod.rs:416:9                                                                                                                         
  21: tokio::runtime::scheduler::multi_thread::worker::Context::run_task::{{closure}}                                                                                                                                                                
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/scheduler/multi_thread/worker.rs:639:17                                                                                                   
  22: tokio::runtime::coop::with_budget                                                                                                                                                                                                              
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/coop.rs:107:5                                                                                                                             
  23: tokio::runtime::coop::budget                                                                                                                                                                                                                   
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/coop.rs:73:5                                                                                                                              
  24: tokio::runtime::scheduler::multi_thread::worker::Context::run_task                                                                                                                                                                             
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/scheduler/multi_thread/worker.rs:575:9
  25: tokio::runtime::scheduler::multi_thread::worker::Context::run
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/scheduler/multi_thread/worker.rs:526:24
  26: tokio::runtime::scheduler::multi_thread::worker::run::{{closure}}::{{closure}}
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/scheduler/multi_thread/worker.rs:491:21
  27: tokio::runtime::context::scoped::Scoped<T>::set
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/context/scoped.rs:40:9
  28: tokio::runtime::context::set_scheduler::{{closure}}
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/context.rs:176:26
  32: tokio::runtime::scheduler::multi_thread::worker::run::{{closure}}
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/scheduler/multi_thread/worker.rs:486:9
  33: tokio::runtime::context::runtime::enter_runtime
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/context/runtime.rs:65:16
  34: tokio::runtime::scheduler::multi_thread::worker::run
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/scheduler/multi_thread/worker.rs:478:5
  35: tokio::runtime::scheduler::multi_thread::worker::Launch::launch::{{closure}}
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/scheduler/multi_thread/worker.rs:447:45
  36: <tokio::runtime::blocking::task::BlockingTask<T> as core::future::future::Future>::poll
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/blocking/task.rs:42:21
  37: tokio::runtime::task::core::Core<T,S>::poll::{{closure}}
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/core.rs:328:17
  38: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/loom/std/unsafe_cell.rs:16:9
  39: tokio::runtime::task::core::Core<T,S>::poll
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/core.rs:317:13
  40: tokio::runtime::task::harness::poll_future::{{closure}}
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/harness.rs:485:19
  41: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panic/unwind_safe.rs:272:9
  42: std::panicking::try::do_call
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:552:40
  43: __rust_try
  44: std::panicking::try
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:516:19
  45: std::panic::catch_unwind
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panic.rs:142:14
  46: tokio::runtime::task::harness::poll_future
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/harness.rs:473:18
  47: tokio::runtime::task::harness::Harness<T,S>::poll_inner 
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/harness.rs:208:27
  48: tokio::runtime::task::harness::Harness<T,S>::poll
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/harness.rs:153:15
  49: tokio::runtime::task::raw::poll
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/raw.rs:271:5
  50: tokio::runtime::task::raw::RawTask::poll
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/raw.rs:201:18
  51: tokio::runtime::task::UnownedTask<S>::run
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/task/mod.rs:453:9
  52: tokio::runtime::blocking::pool::Task::run
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/blocking/pool.rs:159:9
  53: tokio::runtime::blocking::pool::Inner::run
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/blocking/pool.rs:513:17
  54: tokio::runtime::blocking::pool::Spawner::spawn_thread::{{closure}}
             at /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.36.0/src/runtime/blocking/pool.rs:471:13
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Expose PING and FINDNODE rpc to public API

We at Trin would benefit greatly if we have public API access to raw PING and FINDNODE RPC via Discv5 pub struct. For example, something like send_ping(enr: Enr) and send_findnode(enr: Enr, distances: Vec<u64>) would be great.

Are you guys open to such a change? If you agree, we can discuss the implementation approach and I can work on a PR to commit this upstream?

Handling multiple in-flight requests

When sending multiple requests, the current design queues them, waiting for a response to the first request before sending the next one:

discv5/src/handler/mod.rs

Lines 468 to 481 in a9ded04

if self.active_requests.get(&node_address).is_some()
|| self.active_challenges.get(&node_address).is_some()
{
trace!("Request queued for node: {}", node_address);
self.pending_requests
.entry(node_address)
.or_insert_with(Vec::new)
.push(PendingRequest {
contact,
request_id,
request,
});
return Ok(());
}

If a client has a 100ms roundtrip time with a peer, that puts a hard upper limit of 10 messages per second with that peer. That limits the "bandwidth" to less than 12.8kB/s (102 kb/s).

There's nothing apparent in the protocol spec that requires 1 message at a time. Including the request ID suggests that the spec intends to support multiple in-flight requests.

What are maintainers' thoughts on reviewing a PR that would support multiple in-flight requests?

Permit Invalid ENRS to perform FINDNODE?

There are a number of nodes out there that often misconfigure their ENRs or potentially undergo an IP shift.

Currently we simple drop the session and do not respond to the request leaving the node unaware of its mis-configuration.

We could potentially respond to a FINDNODE and not record the ENR in our table (as it is not contactable).

This would allow discovery to function perfectly fine for misconfigured nodes. Potentially this may allow more misconfigurations to exist and go unnoticed however.

Simple install results in failing compilation

Following the install instructions, simply adding a single dependency discv5 = "0.1.0-beta.3" or discv5 = { version = "0.1.0-beta.3", features = ["libp2p"] } in Cargo.toml results in failing compilation.

error[E0425]: cannot find function `spawn` in module `tokio::task`
  --> /Users/~/.cargo/registry/src/github.com-1ecc6299db9ec823/discv5-0.1.0-beta.3/src/executor.rs:34:22
   |
34 |         tokio::task::spawn(future);
   |                      ^^^^^ not found in `tokio::task`
   |
help: consider importing one of these items
   |
2  | use crate::kbucket::arrayvec::io::sys::ext::net::sys_common::util::thread::spawn;
   |
2  | use std::thread::spawn;
   |

Adding an explicit dependency on tokio = { version = "1", features = ["full"] } fixes the failing compilation. I'm fairly new to the rust world, so it's hard for me to say if this is expected or not, but if so it might be worth adding to the docs.

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    ๐Ÿ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. ๐Ÿ“Š๐Ÿ“ˆ๐ŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google โค๏ธ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.