Skip to content
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.

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

Support for MSRV shipped with Debian stable for default features #331

Closed
notmandatory opened this issue Apr 17, 2021 · 24 comments · Fixed by #1183
Closed

Support for MSRV shipped with Debian stable for default features #331

notmandatory opened this issue Apr 17, 2021 · 24 comments · Fixed by #1183
Assignees
Labels
discussion There's still a discussion ongoing
Milestone

Comments

@notmandatory
Copy link
Member

notmandatory commented Apr 17, 2021

For improved build tool security we should support as MSRV the rustc version shipped with Debian stable, currently 1.41.1. This may not be ppossible for all features, but should be possible for our wallet with default features, key-value-db and electrum.

Discord discussion here:
https://discord.com/channels/753336465005608961/753336465005608964/832682901950431323

If other features can also be made to work with a lower MSRV we should create separate issues for those. Another approach for libraries with higher MSRV may be to create a guide for verifying dependencies and/or creating reproducible builds.

@notmandatory notmandatory added the discussion There's still a discussion ongoing label Apr 17, 2021
@notmandatory
Copy link
Member Author

notmandatory commented Apr 17, 2021

The current testing debian release is 11 (bullseye) and it will ship with rustc 1.48.0. I think it's worth considering that bullseye may be the stable debian by the time any mainnet ready projects adopt BDK.

https://tracker.debian.org/pkg/rustc

@notmandatory
Copy link
Member Author

notmandatory commented Apr 17, 2021

Something else to note is there appear to be CVEs that affect rustc versions prior to 1.52.0. It looks like the debian rustc maintainer is trying to get the fixes back-ported to 1.48. 🤞

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=986803****

CVE-2021-28875
CVE-2021-28876
CVE-2021-28877
CVE-2021-28878
CVE-2021-28879
CVE-2020-36317
CVE-2020-36318

@LLFourn
Copy link
Contributor

LLFourn commented Apr 20, 2021

As I mentioned on the call I think setting a conservative MSRV is a good long term goal but I wouldn't fix it now. It takes time and effort to maintain things pinned to old versions of rustc. I think so far the version bumps in bdk were made to save developer time and keep things simple. I think this is the right thing to do since the library is more likely to survive and thrive based on functionality and getting the APIs right rather than compiler toolchain reproducibility concerns or default Debian package versions. Once the functionality and the users are there then the balance tips the other way of course but I don't think this library is at that stage yet.

I am biased btw since I use bdk to explore ideas mostly rather than to "put things in production".

@afilini
Copy link
Member

afilini commented Apr 21, 2021

Yeah I think lowering the MSRV for the entire project is kind of unrealistic right now. I also don't think it would really make any difference in practice because it's very likely that larger projects using BDK would have other dependencies that require a higher MSRV as well.

I think it makes sense to try doing that for the core wallet stuff though, because that part by itself only requires very few deps and it's also the most critical in terms of security. So if someone is building a wallet and really cares about security he would still be able to use the core of BDK and then maybe implement custom Blockchain or Database however he likes to replace our implementation which requires a lot of dependencies and higher MSRV.

@notmandatory
Copy link
Member Author

Based on discussion above, how does this sound as a policy:

  1. The bdk lib with default features ("key-value-db", "electrum") will have MSRV equal to version of rustc shipped with Debian stable (currently 1.48.0). Stable Debian releases come out approximately every 2 years.

  2. Optional features will be have a MSRV that is a minimum of 4 minor rustc release versions old (currently 1.52.0). Minor releases are every 6 weeks, so users of optional features will have at least 6 months before they may have to upgrade.

To make this policy work we'll have to do a few things:

  • Update README.md + website docs to inform users of policy
  • Update CI to test default features and optional features with different MSRVs

Anything else?

@LLFourn
Copy link
Contributor

LLFourn commented Oct 28, 2021

Note that rust development team recommend at least 1.52.1 due to incremental compilation bugs in previous versions.

[edit] whoops I see this was mentioned earlier: #331 (comment)

@notmandatory
Copy link
Member Author

notmandatory commented Oct 28, 2021

Good point, how does this sound for a third rule:

  1. The MSRV for default or non-default features will be incremented the minimum over the above policies if needed to fix a high priority Rust CVE, as determined by rough consensus of the bdk team.

@LLFourn
Copy link
Contributor

LLFourn commented Oct 29, 2021

My 2c on your plan:

  1. I think it's preferable to say that YMMV with MSRV on non-default features and bump dependencies/testing versions whenever needed. Having a rigid policy will just lead to more of our time being wasted with fixing different versions of things again and again.
  2. If you want to have a MSRV policy per feature it makes more sense to extract those features into their own crates e.g. bdk-esplora and manage them separately. I don't think we're ready for that though because I expect APIs to be changing a bit still.

If we wanted to have a strict MSRV policy that was low hassle we could fix the MSRV to 1.56 and refuse to change it. This will allow us to recognise the rust-version attribute in dependencies. We can then make sure all our dependencies set this in their cargo.toml and then complain loudly if they violate this promise or change their rust-version in a patch release. Our main problem atm is that new minor versions and patch releases are changing the effective MSRV of their crate without even the authors of the crate being aware. This annoying problem may go away after 1.56 as rust-version gives an explicit way of signalling changes to MSRV and hopefully raises awareness of this problem.

My preference now is to not make any MSRV promises about non-default features for now and if we have a particular feature that is stable and we want to make sure it has a fixed MSRV different than the main one then we extract it into its own crate.

@notmandatory
Copy link
Member Author

notmandatory commented Oct 29, 2021

I agree it doesn't make sense to extract features right now. And makes sense to not pin down the MSRV on non-default features yet. I'd like to try keeping the default features supported back to rust 1.48.0 but if it gets too complicated to try to support default and non-default features at different versions then we can try extracting features or moving MSRV up to 1.56.

@notmandatory
Copy link
Member Author

At this point I think the best option is set MSRV to 1.56.X, it's not the current stable for Debian but is being tested and should be shipped with next stable Debian "bookworm" release. https://tracker.debian.org/pkg/rustc

@notmandatory
Copy link
Member Author

notmandatory commented Feb 17, 2022

I think as a policy we can state, the bdk MSRV:

  1. Will only be changed with a new MINOR release, never a PATCH release.
  2. Will target Debian "stable" or "testing" release rust versions as long as possible given required dependencies.

@notmandatory
Copy link
Member Author

A good discussion in favor of supporting older MSRVs here: tokio-rs/tokio#3412

@LLFourn
Copy link
Contributor

LLFourn commented Feb 18, 2022

Since we started discussing this we've spent a lot of time having our CI break. It feels like it happens every other week. From my perspective the main security concern for BDK isn't having to use a recent version of the compiler it's losing coins because nobody has had time to fix things because the CI is often broken and we are backlogged with PRs. I acknowledge that not all the CI problems are MSRV related but most of them are.

I do think that a library like BDK should eventually have a conservative MSRV. If everyone thinks that the time is now then I think the right way to move forward is to extract components with dependencies into their own crate. That way we can keep the main BDK one lower and stable. We are doing a lot of work towards this but one of the main things slowing us down is the CI breaking. It's a getting a little bit demoralizing TBH. So I suggest the following:

  1. Fix MSRV to 1.56 for now which is the rust 2021 edition release and available on debian unstable.
  2. Remove all testing against any other version (stop testing against nightly which can be buggy).
  3. Have CI working for a while
  4. Do the work to extract things that have dependencies like Blockchains and DB backends to their own crate. This includes testing tools which are one of the main culprits. i.e. move the electrum/bitcoin RPC TestClient to its own crate.
  5. Do a v1.0.0 release where we fix the MSRV to debian stable and try to keep the API stable.

Of course I am only putting this forward as a suggestion. It's not me that suffers from this the most. @notmandatory is the one who has to fix this every time this happens. When I see that BDK is going through a rough period of CI stuff I just work on other things until I see that it's over.

@notmandatory
Copy link
Member Author

I agree we need to get to a point where the BDK CI isn't breaking and when it does it's easier to fix. I like the plan above, two small corrections, if we go to MSRV 1.56.0 that will put us on Debian "testing" so it's not as bad as "unstable". And also we're not currently doing any testing with nightly but we do use it to build docs in order to use some unreleased feature but hopefully those will be released at some point.

Anyone else interested in this topic please comment here, and we'll talk about it at the team call on Tuesday.

@afilini
Copy link
Member

afilini commented Feb 18, 2022

I've always been very careful bumping the MSRV, but I agree that now it's starting to get very annoying and maybe it's time to relax our policy a little bit.

I also think we should eventually have many different crates (which would also help with the no-std work), but if you recall testutils used to be a separate crate and it was a pain in the ass to maintain, because we kept changing the API which meant having to constantly publish new releases of all the separate crates. I would wait at least for the big changes to Blockchain (splitting it up) and Database (removing &mut) before starting the "splitting up" work.

Btw I don't know why we keep talking so much about Debian, it's not the only distro out there and I believe Ubuntu is much more popular on the market (not sure which rustc version is shipped with Ubuntu though). Maybe a better target is based on how old a release is? i.e. "we will never use a compiler less than 6 months old" or something like that. We could go to 1.50.0 for a one year old release, or something more recent if there are some specific benefits from doing so (e.g. from using edition 2021).

Once we split up BDK in multiple crates we will consider going back to older versions (2y+) on the core lib.

@notmandatory
Copy link
Member Author

notmandatory commented Feb 18, 2022

I agree we should look at multiple distros, I was only focused on Debian because it seems to be one of the more conservative. But really what matters is which platforms our users want to distribute their apps for, and I don't think we have any data there yet.

Ubuntu 18.04 LTS and 20.04 LTS currently include rustc 1.57.0.

This comment in a tokio issue about supporting only back to the rust version the oldest supported version of Firefox needs, because most distributions will want to support Firefox, also makes sense.

@LLFourn
Copy link
Contributor

LLFourn commented Feb 20, 2022

I've always been very careful bumping the MSRV, but I agree that now it's starting to get very annoying and maybe it's time to relax our policy a little bit.

I also think we should eventually have many different crates (which would also help with the no-std work), but if you recall testutils used to be a separate crate and it was a pain in the ass to maintain, because we kept changing the API which meant having to constantly publish new releases of all the separate crates. I would wait at least for the big changes to Blockchain (splitting it up) and Database (removing &mut) before starting the "splitting up" work.

Agree with this. The reason I moved testutils into the main crate was because of the test blockchains macros. I don't 100% recall the order of things but at some point @RCasatta developed this improved "TestClient" thing that helped us do testing without docker. I was suggesting moving TestClient out not the blockchain tests macro which still should be kept in the main repo until the blockchains have been extracted into their own crates. TestClient doesn't change too much during development and is useful for crates outside of BDK (I use it in gun for example).

Btw I don't know why we keep talking so much about Debian, it's not the only distro out there and I believe Ubuntu is much more popular on the market (not sure which rustc version is shipped with Ubuntu though). Maybe a better target is based on how old a release is? i.e. "we will never use a compiler less than 6 months old" or something like that. We could go to 1.50.0 for a one year old release, or something more recent if there are some specific benefits from doing so (e.g. from using edition 2021).

# on ubuntu
$ apt show rustc
Package: rustc
Version: 1.57.0+dfsg1+llvm-0ubuntu1~21.10.1

FWIW I am also not too concerned about what debian is doing. I use FreeBSD and Ubunutu which both keep up relatively well. I do use rustup for development of course.

I think I lean towards doing things based on rust editions because big changes tend to cluster around those. So set BDK MSRV to 1.56 now and work to keep it there until some time after next edition and only change it if we need it. If some dependency needs a newer one we should try to spin it off into its own crate. I'm hoping that setting MSRV to 1.56 will buy us enough time to make this work.

@notmandatory notmandatory self-assigned this Mar 9, 2022
@afilini afilini mentioned this issue May 3, 2022
4 tasks
notmandatory added a commit that referenced this issue May 4, 2022
cca6948 Bump MSRV to 1.56 (Alekos Filini)

Pull request description:

  ### Description

  Following the discussion in #331, bump the MSRV to `1.56`. We already have other PRs bumping it to at least `1.51` (#593), but I'm felling like we are always lagging behind and our CI breaks regularly. As @LLFourn suggested, this PR makes a relatively large bump, hoping this buys us enough time to finish splitting up BDK, which will allow us to have a lower MSRV for the "core" crate.

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  * [x] I've updated `CHANGELOG.md`

ACKs for top commit:
  notmandatory:
    ACK cca6948

Tree-SHA512: bc6572dc94e1c4cbb6d21f6e06a9730af5763fb4811311a61a6a6ec850b5a65664a21e4a7070a3ebcd702529fbba97b2e9a43c1277b9b9f092e194f16a39bc1a
@notmandatory
Copy link
Member Author

I had to bump the MSRV to 1.57 again in #842. Even if we put things like the blockchain modules in different crates (as in bdk_core wip) to make an online wallet the users will still have to use a higher MSRV. Below are my notes on which dependencies we would need to find lower MSRV replacements for, or get PRs in to establish lower MSRVs:

  • rand (1.56)
  • electrum-client (1.57 due to rustls)
  • use-esplora-async (1.56 due to reqwest)
  • use-esplora-blocking (1.57 due to rustls)
  • tokio (1.49 but only needed for async)

If we still need sqlite in bdk_core also:

  • rusqlite (no MSRV)
  • ahash (no MSRV)

BDK Features

minimal

crate version MSRV (with no_std)
bitcoin 0.29.2 1.41.1 (1.47.0)
log 0.4.17 1.31.0
miniscript 9.0.0 1.41.1 (1.47.0)
rand 0.8.5 1.56.0
serde 1.0.152 1.19.0
serde_json 1.0.92 1.36.0
tokio 1.25.0 1.49.0

with bitcoinconsensus

crate version MSRV
bitcoinconsensus 0.19.0 1.41.1

with electrum

crate version MSRV
electrum-client 0.12.0 1.57.0

with use-esplora-async

crate version MSRV
esplora-client 0.3.0 1.56.1
futures 0.3.26 1.36.0

with use-esplora-blocking

crate version MSRV
esplora-client 0.3.0 1.57.0

with key-value-db

crate version MSRV
sled 0.34.7 1.39.0

with sqlite

crate version MSRV
ahash 0.7.6 none
rusqlite 0.27.0 none

@LLFourn
Copy link
Contributor

LLFourn commented Mar 16, 2023

So this could be done now for bdk and bdk_chain with a few tiny fixups here and there. The problem is that it's extremely hard to get CI to build only bdk and bdk_chain now that they're in a workspace. There must be a way to hack around this. At the very least you could copy those two crates out of the workspace and just build them independently on 1.48.0. There's no need to run tests on that rust version just check that they build. Maybe there can be a "custom" workspace Cargo.toml that only includes things that are 1.48.0 compatible.

I think file_store will also probably build on 1.48.0 now.

@evanlinjin
Copy link
Member

evanlinjin commented Mar 17, 2023

@tnull mentioned on Discord that it will be nice for bdk_chain to have a MSRV of 1.48:

LDK is in the middle of raising it's MSRV to 1.48 for all its crates, which is a general target in the rust-bitcoin space I believe. For consistency's sake it would be awesome if BDK 1.0 could target that, too. (my two cents)

Related tickets:

@notmandatory
Copy link
Member Author

Tasks from our call last night:

  • Update CI
  • make sure complete subsets of bdk crates needed for a wallet compile with rust v1.48
  • Create different .toml files for different MSRVs, does this work with cargo workspaces?

@LLFourn
Copy link
Contributor

LLFourn commented Mar 23, 2023

See this PR which makes progress on how to build for 1.48.0 separately: #924 (it's not pretty).

@notmandatory
Copy link
Member Author

We're still targeting rust 1.48, but in case that ever changes here's link from the Rust team on glibc and Linux kernel requirements: https://blog.rust-lang.org/2022/08/01/Increasing-glibc-kernel-requirements.html

Also for reference, glibc version for RHEL 6 is 2.11 and RHEL 7 is at 2.17, Debian 11 (bullseye) is at 2.31 and Debian 12 (bookworm) is 2.36.

@notmandatory notmandatory added this to the 1.0.0-alpha.3 milestone Oct 25, 2023
@notmandatory
Copy link
Member Author

It looks like LDK team is working on updating their MSRV to 1.63.0 which is the version that ships with the current debian stable (bookworm). If there are no objections from key users then I want to do the same for BDK 1.0.0-alpha.3 release.

lightningdevkit/rust-lightning/pull/268

@notmandatory notmandatory mentioned this issue Oct 25, 2023
3 tasks
@notmandatory notmandatory modified the milestones: 1.0.0-alpha.3, 1.0.0-alpha.4 Nov 13, 2023
@notmandatory notmandatory modified the milestones: 1.0.0-alpha.4, 1.0.0-alpha.3 Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion There's still a discussion ongoing
Projects
Archived in project
4 participants