b72f56c4 made no_std MSRV 1.53

[Kixunil]

no_std MSRV is supposed to be 1.47 but b72f56c calls finish_non_exhaustive() with MSRV 1.53

So we have two bugs: the MSRV bug and bad tests failing to catch it. I caught it by accident when inspecting #905

[ass3rt]

It looks like finish_non_exhaustive() was used, simply to avoid printing the private_key member. An MSRV compatible alternative, might be to make the debug implementation exhaustive, but just print a dummy/redacted value for private_key instead of the actual secret data.

I.e:

impl fmt::Debug for ExtendedPrivKey {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.debug_struct("ExtendedPrivKey")
            ... other fields unchanged ...
            .field("private_key", "[redacted]")
            .finish()
    }
}

Although, I prefer the use of finish_non_exhaustive() if bumping the MSRV for no-std is an option.

[ass3rt]

Although, I prefer the use of finish_non_exhaustive() if bumping the MSRV for no-std is an option.

I see no-std has a different MSRV for this project. How is the decision made to bump MSRV?

[tcharding]

Like everything else in open source, someone makes a suggestion and everyone else tears shreds off them ... just kidding. More seriously, put forward an argument why it should be a certain version and others will chime in with counter arguments. AFAIK the no-std stuff is not well tested ATM, any improvement on it is most welcome. The MSRV for std was debated in a few places, here is one, basically, as far as I understand, we are trying to stay two years behind current stable toolchain.

[Kixunil]

My argument is that most no_std stuff requires fairly new compiler anyway, so there's little point keeping it low. But disregard my opinion in favor of people who actually use no_std for bitcoin stuff.

[mcroad]

I'm trying to get bdk working on no_std. Most embedded development happens on stable or even on nightly so keeping the MSRV at 1.47 seems unnecessary. I'd suggest bumping it all the way to current stable at 1.60

[TheBlueMatt]

Most embedded development happens on stable or even on nightly so keeping the MSRV at 1.47 seems unnecessary.

I'd very, very, very strongly disagree. Just because "most" Rust development uses rustup to download completely untrusted and unverified binaries from an HTTP server and trusts it completely doesn't mean we shouldn't support people taking their build security seriously.

[mcroad]

Given this bug, we know for certain that no one is using the library under version 1.53. Setting the MSRV to 1.53 would not break anyone's build.

All tests pass on 1.53.

[TheBlueMatt]

I'd like to regularly build this library for no-std with versions prior to the latest rustc. We however haven't updates the dependency on rust-bitcoin in a while as there hasn't been a release in a while, plus the no-std feature is quite new so we don't really expect many users of it yet. When "real" users of it start appearing, I'd expect them to not use rustup and maybe use fairly old versions of rustc as bootstrapping is somewhat painful.

[mcroad]

So, even though it's not currently compatible with 1.47, you'd want it to be so you can maintain ldk's MSRV at 1.47?

[TheBlueMatt]

We currently support 1.47 with no-std just fine - the fact that it doesn't currently build with 1.47 is a regression, not an intentional thing, and a bug in the CI pipeline.

[Kixunil]

I personally would be open to detecting the compiler version and supporting advanced features in newer versions if people need them. (e.g. the one causing current regression looks useful) There are multiple crates implementing this or we could perhaps NIH something simple. However I'd personally not go out of my way to implement these myself.

[tcharding]

This can be closed now. We do test MSRV in CI for both std and no-std builds. Also the offending call finish_non_exhaustive no longer exists.