-
Notifications
You must be signed in to change notification settings - Fork 312
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
Comments
The current |
Something else to note is there appear to be CVEs that affect https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=986803**** CVE-2021-28875 |
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 I am biased btw since I use bdk to explore ideas mostly rather than to "put things in production". |
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 |
Based on discussion above, how does this sound as a policy:
To make this policy work we'll have to do a few things:
Anything else? |
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) |
Good point, how does this sound for a third rule:
|
My 2c on your plan:
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 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. |
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. |
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 |
I think as a policy we can state, the
|
A good discussion in favor of supporting older MSRVs here: tokio-rs/tokio#3412 |
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:
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. |
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 Anyone else interested in this topic please comment here, and we'll talk about it at the team call on Tuesday. |
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 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 Once we split up BDK in multiple crates we will consider going back to older versions (2y+) on the core lib. |
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 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. |
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).
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. |
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
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:
If we still need sqlite in bdk_core also:
BDK Featuresminimal
with bitcoinconsensus
with electrum
with use-esplora-async
with use-esplora-blocking
with key-value-db
with sqlite
|
So this could be done now for I think |
@tnull mentioned on Discord that it will be nice for
Related tickets: |
Tasks from our call last night:
|
See this PR which makes progress on how to build for 1.48.0 separately: #924 (it's not pretty). |
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. |
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. |
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.
The text was updated successfully, but these errors were encountered: