v1.16: unpin tokio for solana client crate

[afalaleev]

Problem

Tokio version specifier "~1.14.1" only allows patch updates and v1.14.1 is more than a year old.

Summary of Changes

Change to tokio = "1" in the client/ crate.

Fixes #32878

[afalaleev]

@mvines please review this PR.

[mvines]

thanks, this seems good to me. let's see how CI likes it

[mvines]

Looks like we have an audit issue on v1.16 currently. It should be fixed by #32948, so once that PR lands please rebase this PR to pick up the audit fix.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (v1.16@87373f0). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 5a0d766 differs from pull request most recent head af64fb7. Consider uploading reports for the commit af64fb7 to get more accurate results

@@           Coverage Diff            @@
##             v1.16   #32943   +/-   ##
========================================
  Coverage         ?    81.9%           
========================================
  Files            ?      762           
  Lines            ?   208055           
  Branches         ?        0           
========================================
  Hits             ?   170576           
  Misses           ?    37479           
  Partials         ?        0           

[mvines]

@t-nelson - can you please take a look at this as well? I think it's safe enough. The PR title/description is not helpful unfortunately. tldr is that they need the token-dependency of the v1.16 version of the solana-client crate to be just "1", like it was for v1.14. The explicit mention of tokio v1.14.1 (in the workspace Cargo.toml) causes v1.14.1 to be thrust upon users of the v1.16 solana-client crate as published to crates.io

[t-nelson]

where was the motivation expressed? all i see is "a year old" which... sounds like a preference.

i'm pretty dubious of the major-only specifier in general. i don't think we want to allow downgrades to below 1.14.1, but i don't recall why we added the ~ here in the first place. i'll have to hunt that up later. dropping the tilde seems less invasive.

vendoring is always an option

[afalaleev]

@t-nelson sorry for the lack of motivation in PR.

The reason why v1.16 is locked on the tokio v1.14.1 is described in this issue #24644.
However, in actuality only solana-rpc needs to be pinned to avoid the RPC stalls, and the pin is causing problems for downstream users of the solana client crate.

This PR resolves the issues for downstream users importing the solana client crate.
For example, in this PR #26957 the tokio was unpinned.

Please, check that the PR is safe enough.

[t-nelson]

seems like bumping to "1.29.1" (as per #32430) would be safer that "1" given that there were issues somewhere between "1.14.1" and "1.29.1" that we don't want devs to stumble in to again. which would be possible with "1"

[mvines]

seems like bumping to "1.29.1" (as per #32430) would be safer that "1" given that there were issues somewhere between "1.14.1" and "1.29.1" that we don't want devs to stumble in to again. which would be possible with "1"

Backporting #32430 is way to risky at the moment no? "1" is exactly what the v1.14 version of the solana-client crate requires, so this is actually reverting back to the previous setup that the devs were happy about

[t-nelson]

seems like bumping to "1.29.1" (as per #32430) would be safer that "1" given that there were issues somewhere between "1.14.1" and "1.29.1" that we don't want devs to stumble in to again. which would be possible with "1"

Backporting #32430 is way to risky at the moment no? "1" is exactly what the v1.14 version of the solana-client crate requires, so this is actually reverting back to the previous setup that the devs were happy about

"1" allows any version with major-version=1, including the range we know has issues.

it's still not clear to me why 1.14.1 is insufficient. without knowing that, i can't really characterize risk. downstream vendoring is sounding more and more like the right solution

[mvines]

"1" allows any version with major-version=1, including the range we know has issues.

Cargo.lock prevents this from happening for our tree. For the crate users, "1" provides the same experience they already have when using the v1.14 version of solana-client.

it's still not clear to me why 1.14.1 is insufficient. without knowing that, i can't really characterize risk

1.14.1 is fine for the v1.16 validator client, we're not changing that here at all. Again the Cargo.lock file saves us. So there's no risk for us, the only risk is for crates.io consumers but not really because they already deal with this same behaviour ("1") in v1.14

[andreisilviudragnea]

@t-nelson I agree with dropping the tilde only for solana-client, since "1.14.4" allows any update of the form "1.x.x".

[t-nelson]

Cargo.lock prevents this from happening for our tree.

"1" allows any version with major-version=1, including the range we know has issues.

Cargo.lock prevents this from happening for our tree. For the crate users, "1" provides the same experience they already have when using the v1.14 version of solana-client.

Cargo.lock only applies to published bin crates and local builds. not published lib crates. cargo will take the most recent version that matches the dependency's semver pattern

it's still not clear to me why 1.14.1 is insufficient. without knowing that, i can't really characterize risk

1.14.1 is fine for the v1.16 validator client, we're not changing that here at all. Again the Cargo.lock file saves us. So there's no risk for us, the only risk is for crates.io consumers but not really because they already deal with this same behaviour ("1") in v1.14

the risk to us is breaking solana-client consumers should tokio carelessly bump minors during 1.16. the dev community is already annoyed about the 1.16 migration pain from abi/msrv nonsense they've endured, client-side deploy regression, etc. so i'd rather not just yolo a last minute dep bump

---

seems like bumping to "1.29.1" (as per #32430) would be safer that "1" given that there were issues somewhere between "1.14.1" and "1.29.1" that we don't want devs to stumble in to again. which would be possible with "1"

i realize now that i may not have been clear that i was proposing bumping the solana-client tokio dependency to 1.29.1, not taking the workspace to that version. i only referenced #32430 as evidence that 1.29.1 doesn't not work

---

that's the risk i'm considering. i still have no idea what the reward is. if i proposed backporting a dependency version bump to beta a week before it's slated to be promoted to stable with the justification that "there is a newer version available," i certainly hope the response from reviewers would be, "lol, no"

so, ignoring the "age" of 1.14.1, what is the motivation for this dependency bump?

[andreisilviudragnea]

@t-nelson Tokio is used in an Axum web app that also uses Solana client v1.14 inside it.

We use the latest possible versions for our dependencies. Upgrading Solana to v1.16 forces us to downgrade tokio to v1.14.4 and with it also Axum to v0.5.17 (from v0.6.20). Axum v0.6.0 got released in November 2022 and it has breaking changes compared to v0.5, we never actually used Axum v0.5.17 in our project, so we cannot use Solana v1.16 in our project.

For me as a developer using a dependency, it is unexpected to be forced to downgrade some of my dependencies when upgrading another dependency.

We will probably use our Solana fork for unpinning Tokio version on v1.16.

[t-nelson]

@t-nelson Tokio is used in an Axum web app that also uses Solana client v1.14 inside it.

got it. so taking solana-client to 1.29.1 instead of 1 would be sufficient? looks like axum is ^1.25.0

For me as a developer using a dependency, it is unexpected to be forced to downgrade some of my dependencies when upgrading another dependency.

understood, but we can't force preferences like this on the entire developer ecosystem without evidence of broad advantage

We will probably use our Solana fork for unpinning Tokio version on v1.16.

vendoring is always an option!

---

once we clear 1.16, these long latent dependencies and msrv should be in our past. we're well aware of the pain that they cause. we feel it too 🙂

[fabioberger]

@andreisilviudragnea is your fork available for others to use? We have this exact same issue.

[andreisilviudragnea]

Hi @fabioberger,

Please show your support for #32878 and comment on the issue with the details of your problem. I still believe this is a problem that should be fixed by Solana team and not by the users of the library.

We use a backport of #32430 on branch v1.16 https://github.com/neonlabsorg/solana/tree/backport-32430, which will be rebased constantly over the latest changes from v1.16 branch.

[CriesofCarrots]

In the interest of thoroughness, noting that we lost the more permissive tokio definition in solana-client and solana-storage-bigtable when we switched to workspace dependencies. Looks like this was inadvertent.

[afalaleev]

The better decision is suggested in #33058