Skip to content

fix(autonat::v2::client): DialBackError visibility #6168

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

fix(autonat::v2::client): DialBackError visibility #6168

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4870f2f
fix(autonat::v2::client): visibility
skaunov Sep 26, 2025
bb96556
docstrings
skaunov Sep 26, 2025
c017f34
changelog entry
skaunov Sep 26, 2025
7e9d65e
"bump the crate versions if needed"
skaunov Sep 26, 2025
d4eb99d
propagate the version bump
skaunov Sep 26, 2025
67a63e6
Update protocols/autonat/CHANGELOG.md
skaunov Sep 26, 2025
a50084c
Merge branch 'master' into DialBackError_visibility
jxs Oct 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ edition = "2021"
[workspace.dependencies]
libp2p = { version = "0.56.0", path = "libp2p" }
libp2p-allow-block-list = { version = "0.6.0", path = "misc/allow-block-list" }
libp2p-autonat = { version = "0.15.0", path = "protocols/autonat" }
libp2p-autonat = { version = "0.16.0", path = "protocols/autonat" }
libp2p-connection-limits = { version = "0.6.0", path = "misc/connection-limits" }
libp2p-core = { version = "0.43.1", path = "core" }
libp2p-dcutr = { version = "0.14.0", path = "protocols/dcutr" }
Expand Down
3 changes: 3 additions & 0 deletions protocols/autonat/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
## 0.16.0
- Fix `DialBackError` visibility so downstream could differentiate between `NoConnection` and `StreamFailed`.

## 0.15.0

- Fix infinity loop on wrong `nonce` when performing `dial_back`.
Expand Down
2 changes: 1 addition & 1 deletion protocols/autonat/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ name = "libp2p-autonat"
edition.workspace = true
rust-version = { workspace = true }
description = "NAT and firewall detection for libp2p"
version = "0.15.0"
version = "0.16.0"
authors = [
"David Craven <david@craven.ch>",
"Elena Frank <elena.frank@protonmail.com>",
Expand Down
1 change: 1 addition & 0 deletions protocols/autonat/src/v2/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ mod behaviour;
mod handler;

pub use behaviour::{Behaviour, Config, Event};
pub use handler::dial_request::DialBackError;
22 changes: 3 additions & 19 deletions protocols/autonat/src/v2/client/behaviour.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{
collections::{HashMap, VecDeque},
fmt::{Debug, Display, Formatter},
fmt::Debug,
task::{Context, Poll},
time::Duration,
};
Expand Down Expand Up @@ -239,7 +239,7 @@ where
tested_addr,
bytes_sent,
server: peer_id,
result: result.map_err(|e| Error { inner: e }),
result,
}));
}

Expand Down Expand Up @@ -374,22 +374,6 @@ impl Default for Behaviour<OsRng> {
}
}

pub struct Error {
pub(crate) inner: dial_request::DialBackError,
}

impl Display for Error {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
Display::fmt(&self.inner, f)
}
}

impl Debug for Error {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
Debug::fmt(&self.inner, f)
}
}
Comment on lines -377 to -391
Copy link
Member

Choose a reason for hiding this comment

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

Wouldnt it be better to leave this in place but implement Error (or maybe use thiserror::Error and use #[error(transparent)]) so you can downcast to DialBackError?

Copy link
Contributor Author

@skaunov skaunov Sep 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I honestly gave that a small thought initially. I definitely like the second approach more. But I fail to identify a benefit from that layer of indirection, so came with this approach; do you have some on your mind? Like IIRC the idea of thiserror is that someone downstream who uses it can wrap this easily for their needs, and we don't imply it on those who don't want to use it. 😆

Copy link
Member

Choose a reason for hiding this comment

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

yup, I agree with Darius I don't think this needs to be a breaking change, we can probably implement downcast(&self) -> DialBackError wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jxs
It's hard for me to think about this thoroughly as I'm lacking so much of the wider context. I only have the thought that if it lives in pub mod v2 then this doesn't seem to be a big deal to straighten it in a breaking manner. Like it will anyway either be replaced with v3 or become autonat::client sooner or later.

Copy link
Contributor Author

@skaunov skaunov Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As DialBackError already has thiserror::Error on it I'd argue the change is not even breaking, as all downstream could do is match Result::Err, Display or format it -- a private field prevented anything else hence everything would continue to work after this.

Tbh, I'd rather see here Result<Result> for more obvious distinguishing networking failure and autonat success. But that would be breaking.


#[derive(Debug)]
pub struct Event {
/// The address that was selected for testing.
Expand All @@ -402,7 +386,7 @@ pub struct Event {
pub server: PeerId,
/// The result of the test. If the test was successful, this is `Ok(())`.
/// Otherwise it's an error.
pub result: Result<(), Error>,
pub result: Result<(), dial_request::DialBackError>,
}

struct ConnectionInfo {
Expand Down
2 changes: 2 additions & 0 deletions protocols/autonat/src/v2/client/handler/dial_request.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,10 @@ impl From<io::Error> for Error {

#[derive(thiserror::Error, Debug)]
pub enum DialBackError {
/// Tells that the client is actually behind NAT for this connection.
#[error("server failed to establish a connection")]
NoConnection,
/// Can't tell new information as dialing back couldn't be finished for a lower level reason.
#[error("dial back stream failed")]
StreamFailed,
}
Expand Down