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

[rklaehn]

Description

Change the way a disabled token store or token log is represented internally from Arc<dyn Token...> with a Noop impl to Option<Arc<dyn Token...>> that is None.

This allows the code that uses the store/log to recognise if the mechanism is disabled and to exit early. It also avoids a dyn call to a noop fn, but that is a tiny benefit.

Breaking Changes

None

Notes & open questions

Note: the way this is implemented is via deprecating, not removing, the NoopToken... impls, and adding a builder fn to disable the store/log. A more idiomatic impl would remove the Noop impls and change the builder setter to take an Option.

Note: Given that neither the token store nor the token log provide much benefit for iroh connections, I would like to disable both in iroh. But since they do provide a lot of benefit for traditional QUIC connections, they would have to be enabled by default in noq, as is the case now. So we need a way to clear them.

But the latter would require a breaking change.

Change checklist

• Self-review.
• Documentation updates following the style guide, if relevant.
• Tests if relevant.
• All breaking changes documented.

[github-actions]

Performance Comparison Report

c76925f826bf87520814f7b159ee1314afbcc16a - artifacts

No results available

---

f4bf3adf77f557c596f96e6970d54659ab8ce939 - artifacts

Raw Benchmarks (localhost)

Scenario	noq	upstream	Delta	CPU (avg/max)
large-single	5291.3 Mbps	8021.6 Mbps	-34.0%	95.2% / 100.0%
medium-concurrent	5443.9 Mbps	7315.0 Mbps	-25.6%	95.7% / 151.0%
medium-single	4045.1 Mbps	4749.2 Mbps	-14.8%	98.5% / 152.0%
small-concurrent	3809.2 Mbps	5204.7 Mbps	-26.8%	93.5% / 103.0%
small-single	3536.3 Mbps	4763.7 Mbps	-25.8%	94.3% / 103.0%

Netsim Benchmarks (network simulation)

Condition	noq	upstream	Delta
ideal	3086.7 Mbps	3995.0 Mbps	-22.7%
lan	782.4 Mbps	810.3 Mbps	-3.4%
lossy	69.8 Mbps	69.8 Mbps	~0%
wan	83.8 Mbps	83.8 Mbps	~0%

Summary

noq is 25.3% slower on average

---

fd5bc915b5190f63be77089fa1f3665e64fa2c6e - artifacts

Raw Benchmarks (localhost)

Scenario	noq	upstream	Delta	CPU (avg/max)
large-single	5415.0 Mbps	7916.1 Mbps	-31.6%	95.1% / 101.0%
medium-concurrent	5488.0 Mbps	7956.3 Mbps	-31.0%	94.6% / 100.0%
medium-single	4265.8 Mbps	4703.8 Mbps	-9.3%	95.5% / 148.0%
small-concurrent	3759.3 Mbps	5352.3 Mbps	-29.8%	91.0% / 99.1%
small-single	3423.4 Mbps	4693.8 Mbps	-27.1%	92.5% / 104.0%

Netsim Benchmarks (network simulation)

Condition	noq	upstream	Delta
ideal	3100.7 Mbps	4148.7 Mbps	-25.3%
lan	796.3 Mbps	810.3 Mbps	-1.7%
lossy	69.8 Mbps	69.8 Mbps	~0%
wan	83.8 Mbps	83.8 Mbps	~0%

Summary

noq is 26.1% slower on average

[github-actions]

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/697/docs/noq/

Last updated: 2026-06-10T18:31:12Z

Giving it a second look after more understanding

[divagant-martian]

I started a discussion off-band about the api change. I'd rather not have the new version since it's not consistent. Honestly I'd make the breaking change or leave it like this.

I do see the value in this change. It's rather silly to have to import a do-nothing type when a None is more natural and means the same

[divagant-martian]

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]

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]

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]

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.

[divagant-martian]

This would now be official docs that point to deprecated stuff

[rklaehn]

Right. If we deprecate NoneTokenLog we could drop the link here and just say that it is disabled by default. We don't have to elaborate on how exactly it is disabled.