search

bitcoindevkit / rust-esplora-client

Bitcoin Esplora API client library. Supports plaintext, TLS and Onion servers. Blocking or async.
MIT License
28 stars 44 forks source link

Upgrade bitcoin to v0.30.0 #51

Closed tcharding closed 1 year ago

tcharding commented 1 year ago

Upgrade rust-bitcoin to v0.30.3, including:

Also includes addition of a dependency on bitcoin-internals for the DisplayHex trait. This is a temporary fix while we wait for release of the new github.com/rust-bitcoin/hex-conservative crate.

tcharding commented 1 year ago

Code LGTM.

Thanks for the review.

I was wondering why we suddenly need a std feature for rust-bitcoin. Will having this feature stop using the client in a no-std environment?

I didn't see #[no_std] and also in lib.rs there are a bunch of std imports so I assumed this crate was not trying to be no-std compatible.

I think you should pin the log dependency to pass CI MSRV test. See: bitcoindevkit/bdk@fa54a2e

Cool, thanks - I'll have a go at that.

tcharding commented 1 year ago

I can re-order the commits if this passes CI, I'm not totally sure of the syntax used in github actions to achieve pinning.

notmandatory commented 1 year ago

Looks like build is breaking because rustls changed their MSRV from 1.57.0 (in 0.21.1) to 1.60.0 (in 0.21.2). Yet again I wish crates wouldn't changes their MSRV in patch releases. Can we pin this the same way you did log?

https://github.com/rustls/rustls/blob/v/0.21.1/rustls/Cargo.toml

tcharding commented 1 year ago

I re-ordered the commits to put the pinning first. (And I removed attack on myself since others won't necessarily know it was me who both wrote the comment and caused the problem :)

LLFourn commented 1 year ago

Looks like build is breaking because rustls changed their MSRV from 1.57.0 (in 0.21.1) to 1.60.0 (in 0.21.2). Yet again I wish crates wouldn't changes their MSRV in patch releases. Can we pin this the same way you did log?

https://github.com/rustls/rustls/blob/v/0.21.1/rustls/Cargo.toml

The cargo documentation states that changing the rust version is only a "possibly breaking change": https://doc.rust-lang.org/cargo/reference/semver.html#env-new-rust

...where the possibility is 100% if you use an older version of rust.

FWIW my opinion is to look at the difference between the two versions and if there's nothing we need just pin it for now. It's not nice pinning things but making possibly (definitely) breaking changes in a patch version is an extreme action for such a core dependency like rustls to take which requires us to take extreme measures. As long as we can use the latest version of the HTTP client library it's fine I'd say.

tcharding commented 1 year ago

Why not just pin in CI for MSRV build and add a comment in the README to tell folks wishing to build with MSRV?

Just because rustls did the wrong thing we don't want to punish users that are not using MSRV by forcing them to use the old version, right?

tcharding commented 1 year ago

Added notes on pinning to readme.

LLFourn commented 1 year ago

Since this PR is a breaking change anyway why not just increase the MSRV for this crate?

Why not just pin in CI for MSRV build and add a comment in the README to tell folks wishing to build with MSRV?

Writing in the README is code that has to be maintained and can't be tested. It's better not to.

vladimirfomene commented 1 year ago

@tcharding, the CI is still failing. If you can fix it, I will merge asap

tcharding commented 1 year ago

Writing in the README is code that has to be maintained and can't be tested. It's better not to.

Fair point. Will remove.

tcharding commented 1 year ago

Since this PR is a breaking change anyway why not just increase the MSRV for this crate?

I don't know how the MSRV was chosen so I don't want to bump it. More generally I don't want to do it just because rustls did it.

tcharding commented 1 year ago

CI is still failing. If you can fix it, I will merge asap

The problem was that we have two versions of rustls in the dependency graph. One, that needs pinning, comes from hyper stack. The other comes from electrum-client. I have a feeling the pinning is going to be fragile now because it uses -p rustls@0.21.2 - we should probably update the dependency in electrum-client.

vladimirfomene commented 1 year ago

@tcharding do you mind creating a PR on the electrum-client repo for this update?

tcharding commented 1 year ago

I added a patch to remind us to fix the pinning introduced by this PR after https://github.com/bitcoindevkit/rust-electrum-client/pull/110 merges. I'm not across your release cycle so please instruct me if you'd like it removed or anything else different.

Thanks.

tcharding commented 1 year ago

Thanks for catching me @ vladimirfomene :)