Skip to content

refactor(noq): change the way a disabled token store or token log is represented internally #697

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
rklaehn wants to merge 4 commits into
base: main
Choose a base branch
from
Open

refactor(noq): change the way a disabled token store or token log is represented internally #697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7c0bf48
Switch from Arc<dyn ...> to Option<Arc<dyn ...>> to make it visible
rklaehn Jun 9, 2026
c76925f
Add ways to disable both the client side token store and the server s…
rklaehn Jun 9, 2026
f4bf3ad
Merge branch 'main' into rklaehn/refactor-token-log-2
rklaehn Jun 9, 2026
fd5bc91
Merge branch 'main' into rklaehn/refactor-token-log-2
rklaehn Jun 10, 2026
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
28 changes: 19 additions & 9 deletions noq-proto/src/config/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ use thiserror::Error;

#[cfg(feature = "bloom")]
use crate::BloomTokenLog;
#[cfg(not(feature = "bloom"))]
use crate::NoneTokenLog;
#[cfg(all(feature = "rustls", any(feature = "aws-lc-rs", feature = "ring")))]
use crate::crypto::rustls::{QuicServerConfig, configured_provider};
use crate::{
Expand Down Expand Up @@ -475,7 +473,7 @@ impl fmt::Debug for ServerConfig {
#[derive(Clone)]
pub struct ValidationTokenConfig {
pub(crate) lifetime: Duration,
pub(crate) log: Arc<dyn TokenLog>,
pub(crate) log: Option<Arc<dyn TokenLog>>,
pub(crate) sent: u32,
}

Expand All @@ -500,7 +498,13 @@ impl ValidationTokenConfig {
/// which makes the server ignore all address validation tokens (that is, tokens originating
/// from NEW_TOKEN frames--retry tokens are not affected).
pub fn log(&mut self, log: Arc<dyn TokenLog>) -> &mut Self {
self.log = log;
self.log = Some(log);
self
}

/// Disable the [`TokenLog`], making the server ignore all address validation tokens
pub fn disable_token_log(&mut self) -> &mut Self {

@divagant-martian divagant-martian Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

After checking more, this would introduce an API that doesn't follow the style of the rest. As you pointed out, this would rather by an Option, making the whole API slightly more inconsistent. And we would be forced to maintain this api during 1.0.

I hope I'm not wrong, but can't we make send=0 disable this? It seems to me both achieve the same, and having a positive value in sent, means sending NEW_TOKEN frames that will be ultimately be ignored/considered unvalidated.

If that is a correct route - which I'm not sure, I'm learning on the go to review this PR - it would save us one inconsistent api to support and maintain

@rklaehn rklaehn Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hm, send=0 means eliminating sending tokens. But you would still have the store, which will rarely get a token to store (we don't accept tokens from previous instances of the remote endpoint, even though this would be allowed by the QUIC spec, so it would have to be the same instance of the remote but a new instance of the server side with send=0).

So all else being equal I would prefer the option route. The reason being that I don't even want an instance of the thing created if the mechanism provides very little benefit for normal iroh connections. But it isn't a big deal one way or another.

Maybe a better route would be to add in the future a fn token_log(&mut self, log: Option<Arc<dyn TokenLog>>) -> &mut Self and deprecate the somewhat weirdly named pub fn log(&mut self, log: Arc<dyn TokenLog>) -> &mut Self {.

Same with the client side: add pub fn token_store_opt(&mut self, store: Option<Arc<dyn TokenStore>>) -> &mut Self { and deprecate pub fn token_store(&mut self, store: Arc<dyn TokenStore>) -> &mut Self {.

Now if we are still allowed to do breaking changes I would do:

  • remove fn log - the name is very generic. And add fn token_log(&mut self, log: Option<Arc>) -> &mut Self`

  • change signature of pub fn token_store(&mut self, store: Arc<dyn TokenStore>) -> &mut Self { to take an Option.

@flub flub Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree that adding this method does break the style quite a bit. I don't have a lot of appetite for doing that at this point.

Also, ValidationTokenConfig::log vs ValidationTokenConfig::token_log. I honestly don't know which is weirder...

And I don't think this meets the bar for breaking changes, but you're probably not surprised by that :)

So all else being equal I would prefer the option route. The reason being that I don't even want an instance of the thing created if the mechanism provides very little benefit for normal iroh connections. But it isn't a big deal one way or another.

IIUC the benefit is that you get so save a dyn call if you do not want a store? But this is only for new connections that do provide a token so very, very few. This is a rather marginal benefit compared to some API consistency questions. I'd be inclined to let the status-quo win and do nothing...

@rklaehn rklaehn Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The dyn call is too small to measure probably. The more important thing is being able to say, yes, this thing is definitely off, and return early.

The biggest win from doing this now and not later would be able to remove the two NoopTokenXXX rhings from the public API instead of having them laying around deprecated for a year.

self.log = None;
self
}

Expand All @@ -519,9 +523,9 @@ impl ValidationTokenConfig {
impl Default for ValidationTokenConfig {
fn default() -> Self {
#[cfg(feature = "bloom")]
let log = Arc::new(BloomTokenLog::default());
let log = Some(Arc::new(BloomTokenLog::default()) as Arc<dyn TokenLog>);
#[cfg(not(feature = "bloom"))]
let log = Arc::new(NoneTokenLog);
let log = None;
Self {
lifetime: Duration::from_secs(2 * 7 * 24 * 60 * 60),
log,
Expand Down Expand Up @@ -553,7 +557,7 @@ pub struct ClientConfig {
pub(crate) crypto: Arc<dyn crypto::ClientConfig>,

/// Validation token store to use
pub(crate) token_store: Arc<dyn TokenStore>,
pub(crate) token_store: Option<Arc<dyn TokenStore>>,

/// Provider that populates the destination connection ID of Initial Packets
pub(crate) initial_dst_cid_provider: Arc<dyn Fn() -> ConnectionId + Send + Sync>,
Expand All @@ -568,7 +572,7 @@ impl ClientConfig {
Self {
transport: Default::default(),
crypto,
token_store: Arc::new(TokenMemoryCache::default()),
token_store: Some(Arc::new(TokenMemoryCache::default())),
initial_dst_cid_provider: Arc::new(|| {
RandomConnectionIdGenerator::new(MAX_CID_SIZE).generate_cid()
}),
Expand Down Expand Up @@ -602,7 +606,13 @@ impl ClientConfig {
///
/// Defaults to [`TokenMemoryCache`], which is suitable for most internet applications.
pub fn token_store(&mut self, store: Arc<dyn TokenStore>) -> &mut Self {
self.token_store = store;
self.token_store = Some(store);
self
}

/// Disable the [`TokenStore`], so that no validation tokens are stored
pub fn disable_token_store(&mut self) -> &mut Self {
self.token_store = None;
self
}

Expand Down
13 changes: 9 additions & 4 deletions noq-proto/src/connection/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5275,7 +5275,9 @@ impl Connection {
return Err(TransportError::FRAME_ENCODING_ERROR("empty token"));
}
trace!("got new token");
token_store.insert(server_name, token);
if let Some(token_store) = token_store {
token_store.insert(server_name, token);
}
}
Frame::Datagram(datagram) => {
if self
Expand Down Expand Up @@ -7361,7 +7363,7 @@ enum ConnectionSide {
Client {
/// Sent in every outgoing Initial packet. Always empty after Initial keys are discarded
token: Bytes,
token_store: Arc<dyn TokenStore>,
token_store: Option<Arc<dyn TokenStore>>,
server_name: String,
},
Server {
Expand Down Expand Up @@ -7393,7 +7395,10 @@ impl From<SideArgs> for ConnectionSide {
token_store,
server_name,
} => Self::Client {
token: token_store.take(&server_name).unwrap_or_default(),
token: token_store
.as_ref()
.and_then(|token_store| token_store.take(&server_name))
.unwrap_or_default(),
token_store,
server_name,
},
Expand All @@ -7409,7 +7414,7 @@ impl From<SideArgs> for ConnectionSide {
/// Parameters to `Connection::new` specific to it being client-side or server-side
pub(crate) enum SideArgs {
Client {
token_store: Arc<dyn TokenStore>,
token_store: Option<Arc<dyn TokenStore>>,
server_name: String,
},
Server {
Expand Down
4 changes: 3 additions & 1 deletion noq-proto/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,9 @@ pub use crate::cid_generator::{

mod token;
use token::ResetToken;
pub use token::{NoneTokenLog, NoneTokenStore, TokenLog, TokenReuseError, TokenStore};
#[allow(deprecated)]
pub use token::{NoneTokenLog, NoneTokenStore};
pub use token::{TokenLog, TokenReuseError, TokenStore};

mod address_discovery;

Expand Down
11 changes: 8 additions & 3 deletions noq-proto/src/token.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,10 @@ pub trait TokenLog: Send + Sync {
pub struct TokenReuseError;

/// Null implementation of [`TokenLog`], which never accepts tokens
#[deprecated(note = "use `ValidationTokenConfig::disable_token_log` instead")]
pub struct NoneTokenLog;

#[allow(deprecated)]
impl TokenLog for NoneTokenLog {
fn check_and_insert(&self, _: u128, _: SystemTime, _: Duration) -> Result<(), TokenReuseError> {
Err(TokenReuseError)
Expand All @@ -92,8 +94,10 @@ pub trait TokenStore: Send + Sync {
}

/// Null implementation of [`TokenStore`], which does not store any tokens
#[deprecated(note = "use `ClientConfig::disable_token_store` instead")]
pub struct NoneTokenStore;

#[allow(deprecated)]
impl TokenStore for NoneTokenStore {
fn insert(&self, _: &str, _: Bytes) {}
fn take(&self, _: &str) -> Option<Bytes> {
Expand Down Expand Up @@ -170,9 +174,10 @@ impl IncomingToken {
{
return Ok(unvalidated);
}
if server_config
.validation_token
.log
let Some(log) = &server_config.validation_token.log else {
return Ok(unvalidated);
};
if log
.check_and_insert(retry.nonce, issued, server_config.validation_token.lifetime)
.is_err()
{
Expand Down
11 changes: 6 additions & 5 deletions noq/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,13 @@ pub use proto::{
ClosedStream, ConfigError, ConnectError, ConnectionClose, ConnectionError, ConnectionId,
ConnectionIdGenerator, ConnectionStats, DecryptedInitial, Dir, EcnCodepoint, EndpointConfig,
FourTuple, FrameStats, FrameType, IdleTimeout, InvalidCid, MtuDiscoveryConfig,
NetworkChangeHint, NoneTokenLog, NoneTokenStore, PathError, PathEvent, PathId, PathStats,
PathStatus, ServerConfig, SetPathStatusError, Side, StdSystemTime, StreamId, TimeSource,
TokenLog, TokenMemoryCache, TokenReuseError, TokenStore, Transmit, TransportConfig,
TransportErrorCode, UdpStats, ValidationTokenConfig, VarInt, VarIntBoundsExceeded, congestion,
crypto,
NetworkChangeHint, PathError, PathEvent, PathId, PathStats, PathStatus, ServerConfig,
SetPathStatusError, Side, StdSystemTime, StreamId, TimeSource, TokenLog, TokenMemoryCache,
TokenReuseError, TokenStore, Transmit, TransportConfig, TransportErrorCode, UdpStats,
ValidationTokenConfig, VarInt, VarIntBoundsExceeded, congestion, crypto,
};
#[allow(deprecated)]
pub use proto::{NoneTokenLog, NoneTokenStore};
#[cfg(feature = "qlog")]
pub use proto::{QlogConfig, QlogFactory, QlogFileFactory};
#[cfg(feature = "rustls")]
Expand Down
Loading