Relax tokio version requirements by nyonson · Pull Request #103 · rust-bitcoin/bip324

nyonson · 2025-03-05T18:54:42Z

Allow any compatible Tokio 1.x version to be used with the tokio feature flag, rather than restricting to specific version ranges. This gives users more flexibility to select their preferred Tokio version. But when using the tokio feature with Tokio 1.39.0+, the effective MSRV increases to 1.70.0.

The library only depends on the stable tokio traits AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt, so should be compatible with a lot of versions. I'd rather give the caller control in this case, especially since its behind a feature flag, instead of enforcing the strict MSRV requirements of the library.

rustaceanrob · 2025-03-06T01:57:18Z

Ah okay. So if a crate compiles targets 1.37 in the dependency list then this change will set the bip324 tokio version to 1.37 as well?

nyonson · 2025-03-06T02:10:31Z

Ah okay. So if a crate compiles targets 1.37 in the dependency list then this change will set the bip324 tokio version to 1.37 as well?

Yea, if no other 3rd party dependency constraints on tokio. Cargo looks at all constraints on a dep and tries to auto choose a version that fits all. If it can't it throws up its hands and requires manual intervention with something like a patch setting.

The current constraints are very specific so I think will cause problems at some point.

Kixunil

ACK 79b05a9

If you can be bothered feel free to update the doc. The other fishy things are for other PRs.

Kixunil · 2025-03-06T08:50:55Z

protocol/Cargo.toml

 [dependencies]
 futures = { version = "0.3.30", default-features = false, optional = true }
-# Must be under 1.39.0 due to MSRV 1.63.0 requirement.
-tokio = { version = ">=1.37.0, <1.39.0", default-features = false, optional = true }


Oh, yes, this was previously straight up wrong.

Wrong as in, the consumer could always just override a the tokio version with a patch or something and break the MSRV?

Wrong that pinning belongs to Cargo.lock not Cargo.toml. Putting these into Cargo.toml causes huge problems with updates that would be otherwise trivial one-command things.

Yea, using this lib downstream was what made me realize this is painful and not super productive. The intention wasn't to pin, but I forget the justification for the >=1.37.0 floor, at least the <1.39.0 ceiling is attempting to convey useful information to the consumer.

The lower bound tends to be legitimate (but it's also often too strict) the upper one is the problem.

Kixunil · 2025-03-06T08:51:52Z

protocol/Cargo.toml

-tokio = { version = ">=1.37.0, <1.39.0", default-features = false, optional = true }
+# The tokio feature may increase the MSRV beyond 1.63.0
+# depending on which version of tokio is selected by the caller.
+tokio = { version = "1", default-features = false, optional = true }


FTR I've seen people saying that writing 1.0.0 is better since it conveys the intention. But I don't really care much.

So same semantics, but 1.0.0 conveys something like "Starting exactly at this point version"? Just making sure I understand what I am signing up for here.

Same semantics in the eyes of cargo but 1.0.0 conveys to some people "I didn't write '1' just because I was lazy, I really intended 1.0.0". (I personally don't agree with those people that they should care, but they do exist and perhaps making them happy is worth 4 extra characters. :))

Kixunil · 2025-03-06T08:54:12Z

protocol/README.md

+
+This crate has a baseline MSRV of **1.63.0**.
+
+However, when using the `tokio` feature flag, the effective MSRV may be higher depending on which version of tokio is selected by the caller.


I'd even say that any external dependency may possibly do this. tokio is currently the only one that actually does but it may happen if they backport MSRV bump to rand for instance.

Ah, yea good point. I guess my thinking is that the current feature flag interface really invites the consumer to unintentionally, and quietly, break the MSRV with their tokio version.

It's not that bad if they set rust-version and let a new cargo generate the lock file (they can run it in a VM or whatever, no risk there).

Yea, I think the v3 resolver will help out here.

IIRC it's unrelated to resolver version, just cargo being new enough but I might be wrong.

I have flipped things to resolver = 3 temporarily to grab good lock files. I think it will be the default soon: https://doc.rust-lang.org/edition-guide/rust-2024/cargo-resolver.html

Interesting!

Kixunil · 2025-03-06T08:55:30Z

protocol/Cargo.toml

 [features]
 default = ["std"]
+# High-level wrappers using futures traits.
 async = ["std", "futures/std"]


Sounds like this feature should've been called futures and use the dep: syntax.

Is futurues a more conventional name for the async interface of a library? That is what this feature flag exposes, which I think is a little more than just futures. But it is using the async traits from the futures dep.

No, the convention is that if you clearly use a public dependency then the feature should be named after the dependency. E.g. we have serde feature in other crates that activates the serde dependency and whatever else is required to implement it. This is even in the API guidelines. If the dependency is private and you're not sure if it'll even stay then using a different name is fine. (Here, it's public but sadly, futures is unstable.)

I found these API guidelines on feature names, but they don't mention exactly what to base the name on. Although, lookin around the ecosystem, I do see the convention that they generally are tied to a dependency's name. I don't feel too terrible about this name though since the intention was to capture "async capabilities without specifying a particular runtime", and to me "futures" would be kinda confusing since the intention isn't to directly interact with futures...but will definitely keep this in mind for naming things next time.

I remember being in some discussion around this and the desire to name the features the same even motivated some changes to cargo. So it's definitely a thing even if we can't find a specific doc saying so.

I haven't looked into the code, but if you don't use futures publicly (their types in the API) then async should be fine and maybe even really a better name. But if it's public I'd recommend renaming it.

The types are used as trait bounds on the input parameters for public functions, so I think that counts? I can rename it in that case in another patch.

Yeah, rename would be appropriate in that case.

Kixunil · 2025-03-06T08:56:10Z

protocol/Cargo.toml

+# High-level wrappers using futures traits.
 async = ["std", "futures/std"]
+# High-level wrappers using tokio traits - may affect MSRV requirements.
 tokio = ["std", "tokio/io-util"]


I''m confused how is this not rejected for not using dep:.

dep: is new to me, and we were originally targeting MSRV 1.56 here so technically wasn't available (it was introduced in 1.60). Digging into it though, it sounds like dep is a way to make things a little more explicit. I believe things are just working right now because this tokio feature flag is overriding the implicit one created by the optional tokio dependency? In any case, would rather be more explicit on intentions.

I pushed 5443dac to switch to the dep syntax.

Well, that's the thing. IIRC Cargo complained before when hitting same-named things that they are conflicting. That's what I'm confused about - why it wasn't rejected by it here. But yes, dep: is more explicit and thus nicer.

I can't for the life of me find any docs on how explicit features interact with the implicit ones, but it hasn't been exploding so I think it must overwrite or merge them somehow.

EDIT: maybe a post 1.60+ thing?

I don't know it by heart either but I remember I had to do a bit more digging when I already knew it's possible but didn't know the details. Someone slacked on "how do we teach this" section in the RFC :D

Allow any compatible Tokio 1.x version to be used with the tokio feature flag, rather than restricting to specific version ranges. This gives users more flexibility to select their preferred Tokio version. When using the tokio feature with Tokio 1.39.0+, the effective MSRV increases to 1.70.0.

Replace implicit feature enabling of optional dependencies with the more explicit dep: syntax. Dep: prevents implicit feature flag creation ("tokio" and "futures") of the optional dependencies. Things just worked before because the tokio feature flag was overwritten, but this makes it clear.

nyonson · 2025-03-06T18:39:26Z

Folded in doc improvements and then switched to better dep: syntax in 05f725b

Kixunil reviewed Mar 6, 2025

View reviewed changes

nyonson added 2 commits March 6, 2025 10:36

nyonson force-pushed the loosen-tokio-version branch from 5443dac to 05f725b Compare March 6, 2025 18:36

rustaceanrob merged commit 221fd6d into rust-bitcoin:main Mar 7, 2025
7 checks passed

nyonson mentioned this pull request Apr 2, 2025

build: run basic feature matrix in CI 2140-dev/kyoto#313

Closed


		This crate has a baseline MSRV of 1.63.0.

		However, when using the `tokio` feature flag, the effective MSRV may be higher depending on which version of tokio is selected by the caller.

Conversation

nyonson commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustaceanrob commented Mar 6, 2025

Uh oh!

nyonson commented Mar 6, 2025

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nyonson Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nyonson Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nyonson Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nyonson Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nyonson commented Mar 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

nyonson commented Mar 5, 2025 •

edited

Loading

nyonson Mar 6, 2025 •

edited

Loading

nyonson Mar 6, 2025 •

edited

Loading

nyonson Mar 6, 2025 •

edited

Loading

nyonson Mar 6, 2025 •

edited

Loading