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

Expose API for PublicAddresses #212

Merged
merged 33 commits into from
Sep 4, 2024
Merged

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Aug 20, 2024

This PR exposes an object for handling public addresses.

  • add_address - adds an address to the given set, inserting the local peer ID or returning an error on different peer Ids
  • remove_address - removes an exact address from the set
  • get_addresses - list of actual addresses

Users of litep2p can obtain the given list via litep2p::public_addresses method.
The lists are exposed internally by a similar API on the transport handler (the handler that communicates with the transport manager).

The identify protocol provides a concatenated list of:

  • user provided addresses via the config
  • listen addresses
  • external addresses

This ensures that Substrate can use a custom heuristic to update the list of external addresses.

Testing Done

  • Added an integration test with 2 litep2p nodes

Closes: #191

cc @paritytech/networking

@lexnv lexnv added the enhancement New feature or request label Aug 20, 2024
@lexnv lexnv self-assigned this Aug 20, 2024
Comment on lines 153 to 154
IdentifyPublicAddresses,
Box<dyn Stream<Item = IdentifyEvent> + Send + Unpin>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This could return IdentifyHandle which would hold IdentifyPublicAddresses and would implement Stream<Item = IdentifyEvent>, following the design of other protocols.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like that idea, will certainly implement it! Thanks for the review!

Have put this on pending shortly, I have a hunch we'll need to expose an ExternalAddrConfirmed (like libp2p) to suffice other usecases (#211). Will wait for Dmitry to confirm that these are needed for the bootnodes on DHT🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry to have not responded here. We discussed #211 with @lexnv, and my idea was to introduce a globally accessible list of public addresses in litep2p. If some protocol needs external addresses, it can fetch them. This includes Identify and Kademlia protocols now.

As a first step, we can manually notify litep2p about confirmed external addresses from the client code. As a second step, we can move the heuristic of confirming external addresses seen by three or more peers from polkadot-sdk to litep2p. I have updated #211 to be more clear about this.

I don't think ExternalAddrConfirmed event will help with providing up-to-date peer addresses for local content providers in Kademlia from within litep2p, but the option to get addresses from the global list will.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your plan sounds good. The easiest place to store the external addresses is probably TransportManager, partly since there already exists code for storing known addresses via TransportManagerHandle and since every protocol has a handle to TransportManager, it should require little architectural changes.

lexnv added 18 commits August 28, 2024 14:27
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Copy link
Collaborator

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

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

The PublicAddresses struct is quite nice and has a convenient API, but I think we need to keep clear separation between listen addresses and public addresses (see inline comments).

src/public_addresses.rs Outdated Show resolved Hide resolved
src/public_addresses.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/protocol/libp2p/identify.rs Outdated Show resolved Hide resolved
src/protocol/libp2p/identify.rs Outdated Show resolved Hide resolved
src/transport/manager/mod.rs Show resolved Hide resolved
src/transport/manager/mod.rs Outdated Show resolved Hide resolved
src/transport/manager/mod.rs Outdated Show resolved Hide resolved
@dmitry-markin
Copy link
Collaborator

The PR description and title don't match the PR now :)

Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv lexnv changed the title identify: Expose API to update public addresses to the identify protocol Expose API for PublicAddresses and ListenAddresses Sep 3, 2024
src/addresses.rs Outdated Show resolved Hide resolved
public_addresses: PublicAddresses,

/// User provided list of addresses.
user_addresses: Vec<Vec<u8>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need one more place to specify public addresses. Getting public addresses via a handle from TransportManager should be enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oki doki, have removed these thanks! 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

I probably was not quite clear on this, but I meant we don't need to let user pass addresses directly to Identify config if we already have an API to notify litep2p about public addresses.

Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv lexnv changed the title Expose API for PublicAddresses and ListenAddresses Expose API for PublicAddresses Sep 3, 2024
Copy link
Collaborator

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

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

Looks good! As now we can notify litep2p about public addresses, IMO we can safely remove public addresses from the Identify config.

@lexnv lexnv merged commit 25ddf91 into master Sep 4, 2024
8 checks passed
@lexnv lexnv deleted the lexnv/indentify-confirmed-addresses branch September 4, 2024 15:12
lexnv added a commit that referenced this pull request Sep 5, 2024
## [0.7.0] - 2024-09-05

This release introduces several new features, improvements, and fixes to
the litep2p library. Key updates include enhanced error handling,
configurable connection limits, and a new API for managing public
addresses.

### [Exposing Public Addresses
API](#212)

A new `PublicAddresses` API has been added, enabling users to manage the
node's public addresses. This API allows developers to add, remove, and
retrieve public addresses, which are shared with peers through the
Identify protocol.

```rust
    // Public addresses are accessible from the main litep2p instance.
    let public_addresses = litep2p.public_addresses();

    // Add a new public address to the node.
    if let Err(err) = public_addresses.add_address(multiaddr) {
        eprintln!("Failed to add public address: {:?}", err);
    }

    // Remove a public address from the node.
    public_addresses.remove_address(&multiaddr);

    // Retrieve all public addresses of the node.
    for address in public_addresses.get_addresses() {
        println!("Public address: {}", address);
    }
```

**Breaking Change**: The Identify protocol no longer includes public
addresses in its configuration. Instead, use the new `PublicAddresses`
API.

Migration Guide:

```rust
    // Before:
    let (identify_config, identify_event_stream) = IdentifyConfig::new(
        "/substrate/1.0".to_string(),
        Some(user_agent),
        config.public_addresses,
    );

    // After:
    let (identify_config, identify_event_stream) =
        IdentifyConfig::new("/substrate/1.0".to_string(), Some(user_agent));
    // Public addresses must now be added using the `PublicAddresses` API:
    for address in config.public_addresses {
        if let Err(err) = public_addresses.add_address(address) {
            eprintln!("Failed to add public address: {:?}", err);
        }
    }
```

### [Dial Error and List Dial Failures
Event](#206)

The `DialFailure` event has been enhanced with a new `DialError` enum
for more precise error reporting when a dial attempt fails.
Additionally, a `ListDialFailures` event has been introduced, listing
all dialed addresses and their corresponding errors when multiple
addresses are involved.

Other litep2p errors, such as `ParseError`, `AddressError`, and
`NegotiationError`, have been refactored for improved error propagation.

### [Immediate Dial Error and Request-Response Rejection
Reasons](#227)

This new feature paves the way for better error handling in the
`litep2p` library and moves away from the overarching
`litep2p::error::Error` enum.

The newly added `ImmediateDialError` enum captures errors occurring
before a dial request is sent (e.g., missing peer IDs). It also enhances
the `RejectReason` enum for request-response protocols, offering more
detailed rejection reasons.


```rust
match error {
    RequestResponseError::Rejected(reason) => {
        match reason {
            RejectReason::ConnectionClosed => "connection-closed",
            RejectReason::DialFailed(Some(ImmediateDialError::AlreadyConnected)) => "already-connected",
            _ => "other",
        }
    }
    _ => "other",
}
```

### [Connection Limits](#185)

Developers can now set limits on the number of inbound and outbound
connections to manage resources and optimize performance.

```rust
    // Configure connection limits for inbound and outbound established connections.
    let litep2p_config = Config::default()
        .with_connection_limits(ConnectionLimitsConfig::default()
            .max_incoming_connections(Some(3))
            .max_outgoing_connections(Some(2))
        );
```

### [Feature Flags for Optional
Transports](#192)

The library now supports feature flags to selectively enable or disable
transport protocols. By default, only the `TCP` transport is enabled.
Optional transports include:

- `quic` - Enables QUIC transport.
- `websocket` - Enables WebSocket transport.
- `webrtc` - Enables WebRTC transport.

### [Configurable Keep-Alive
Timeout](#155)

The keep-alive timeout for connections is now configurable, providing
more control over connection lifecycles.

```rust
    // Set keep alive timeout for connections.
    let litep2p_config = Config::default()
        .with_keep_alive_timeout(Duration::from_secs(30));
```

Thanks for contributing to this @[Ma233](https://github.com/Ma233)!

### Added

- errors: Introduce immediate dial error and request-response rejection
reasons ([#227](#227))
- Expose API for `PublicAddresses`
([#212](#212))
- transport: Implement `TransportService::local_peer_id()`
([#224](#224))
- find_node: Optimize parallelism factor for slow to respond peers
([#220](#220))
- kad: Handle `ADD_PROVIDER` & `GET_PROVIDERS` network requests
([#213](#213))
- errors: Add `DialError` error and `ListDialFailures` event for better
error reporting ([#206](#206))
- kad: Add support for provider records to `MemoryStore`
([#200](#200))
- transport: Add accept_pending/reject_pending for inbound connections
and introduce inbound limits
([#194](#194))
- transport/manager: Add connection limits for inbound and outbound
established connections
([#185](#185))
- kad: Add query id to log messages
([#174](#174))

### Changed

- transport: Replace trust_dns_resolver with hickory_resolver
([#223](#223))
- crypto/noise: Generate keypair only for Curve25519
([#214](#214))
- transport: Allow manual setting of keep-alive timeout
([#155](#155))
- kad: Update connection status of an existing bucket entry
([#181](#181))
- Make transports optional
([#192](#192))

### Fixed

- kad: Fix substream opening and dialing race
([#222](#222))
- query-executor: Save the task waker on empty futures
([#219](#219))
- substream: Use write_all instead of manually writing bytes
([#217](#217))
- minor: fix tests without `websocket` feature
([#215](#215))
- Fix TCP, WebSocket, QUIC leaking connection IDs in `reject()`
([#198](#198))
- transport: Fix double lock and state overwrite on disconnected peers
([#179](#179))
- kad: Do not update memory store on incoming `GetRecordSuccess`
([#190](#190))
- transport: Reject secondary connections with different connection IDs
([#176](#176))

### Testing Done

- pulled the latest changes into Substrate
- performed warp sync in kusama

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

identify: Update local node listen addresses
3 participants