Upgrade bitcoin to v0.30.0 by tcharding · Pull Request #51 · bitcoindevkit/rust-esplora-client

tcharding · 2023-06-10T14:33:58Z

Upgrade rust-bitcoin to v0.30.3, including:

bitcoin
electrsd
electrum-client

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.

vladimirfomene

Code LGTM. 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 think you should pin the log dependency to pass CI MSRV test. See: bitcoindevkit/bdk@fa54a2e

tcharding · 2023-06-21T22:49:12Z

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 · 2023-06-21T23:13:19Z

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 · 2023-06-22T01:17:37Z

 serde = { version = "1.0", features = ["derive"] }
-bitcoin = { version = "0.29.1", features = ["serde"], default-features = false }
+bitcoin = { version = "0.30.0", features = ["serde", "std"], default-features = false }
+# Temporary dependency on internals until the rust-bitcoin devs get their shit together and release the hex-conservative crate.


Go easy on the poor devs! ;-)

Suggested change

# Temporary dependency on internals until the rust-bitcoin devs get their shit together and release the hex-conservative crate.

# Temporary dependency on internals until the rust-bitcoin devs release the hex-conservative crate.

Ha, I am pretty disappointed with our effort so thought the personal dig was justified :)

notmandatory · 2023-06-22T01:30:43Z

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 · 2023-06-22T02:06:28Z

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 :)

notmandatory · 2023-06-22T02:16:23Z

      run: rustup update
+    - name: pin dependencies
+      if: ${{ matrix.rust.version }} == 1.57.0
+      run: cargo update -p log --precise 0.4.18 && cargo update --p rustls --precise 0.21.1


Little typo here:

Suggested change

run: cargo update -p log --precise 0.4.18 && cargo update --p rustls --precise 0.21.1

run: cargo update -p log --precise 0.4.18 && cargo update -p rustls --precise 0.21.1

LLFourn · 2023-06-22T02:37:13Z

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 · 2023-06-22T04:39:49Z

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 · 2023-06-22T04:45:49Z

Added notes on pinning to readme.

LLFourn · 2023-06-22T06:50:37Z

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 · 2023-06-22T17:59:44Z

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

tcharding · 2023-06-23T01:28:15Z

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 · 2023-06-23T01:30:11Z

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.

Recently the MSRV build broke. We can pin the dependencies in the MSRV CI job. Pin `log` and `rustls` when running the MSRV CI job.

Upgrade `rust-bitcoin` to v0.30.3, including: - bitcoin - electrsd - electrum-client 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 · 2023-06-23T02:37:23Z

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 · 2023-06-26T07:17:16Z

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

A pending to `rust-electrum-client` [0] fixes the pinning here in our CI, add a comment to remind us to fix the pinning when the PR is merged and released. [0] bitcoindevkit/rust-electrum-client#110

tcharding · 2023-06-27T22:07:47Z

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

Thanks.

f5a438c Bump version to 0.16.0 (Tobin C. Harding) 9a7cc14 Update rustls dependency to version 0.21 (Tobin C. Harding) Pull request description: Update the `rustls` dependency to version 0.21 and bump the version of this crate to 0.16 so the change can be released. I don't know what stage in your release cycle you guys are up to so I put the version bump as a separate patch - can drop it if not needed. Done to help with bitcoindevkit/rust-esplora-client#51 ACKs for top commit: danielabrozzoni: utACK f5a438c Tree-SHA512: 106cbe57651a6e1365f87836598cbec9b1e837f0376ddc1c56cb75e5615543659f5c05cea7d5eb8da8b6cf278fcbe6ef866a019b9829db40cc8055798eb0f541

tcharding · 2023-06-29T06:52:56Z

Thanks for catching me @ vladimirfomene :)

vladimirfomene

@tcharding thanks for these changes! Re-ACK d48ac22

cd497a75a21bad18b514f4ff3da050c223295aaa Redundant import (Vladimir Fomene) 8d1f90745103aaf5e90a6502c770f9572de5311d Upgrade electrum-client to 0.16.0 (Vladimir Fomene) 73768c5e2c6a37c4bebc50068774b5f6be013279 Add pinning note to electrum-client dependency (Tobin C. Harding) ea1b6d1b447cdcf36240080dafea2004d0ce419b Upgrade bitcoin to v0.30.0 (Tobin C. Harding) d48ac225d3be20ba980391920eb17441a8d91697 Pin dependencies in CI for MSRV build (Tobin C. Harding) Pull request description: Upgrade `rust-bitcoin` to v0.30.3, including: - bitcoin - electrsd - electrum-client 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`](https://github.com/rust-bitcoin/hex-conservative) crate. Top commit has no ACKs. Tree-SHA512: 6611f3de827bc5ee81d7e3d4b0facef45b9833ced86bc3467942d7817f7fa806b137ef285a7120dc3e591acf9dbecd2a189e3119a2c5c1ff8b04764386e83721

f5a438cd0a5b68b3f74d3c144e90e2b099c21261 Bump version to 0.16.0 (Tobin C. Harding) 9a7cc14c94f2587a8a794900dea2ba5665c4aa04 Update rustls dependency to version 0.21 (Tobin C. Harding) Pull request description: Update the `rustls` dependency to version 0.21 and bump the version of this crate to 0.16 so the change can be released. I don't know what stage in your release cycle you guys are up to so I put the version bump as a separate patch - can drop it if not needed. Done to help with bitcoindevkit/rust-esplora-client#51 ACKs for top commit: danielabrozzoni: utACK f5a438cd0a5b68b3f74d3c144e90e2b099c21261 Tree-SHA512: 106cbe57651a6e1365f87836598cbec9b1e837f0376ddc1c56cb75e5615543659f5c05cea7d5eb8da8b6cf278fcbe6ef866a019b9829db40cc8055798eb0f541

This was referenced Jun 19, 2023

chore: bump nixpkgs fedimint/fedimint#2643

Closed

wip: update to bitcoin 0.30.0 fedimint/fedimint#2667

Closed

vladimirfomene reviewed Jun 21, 2023

View reviewed changes

tcharding mentioned this pull request Jun 21, 2023

log dependency forces version for non-MSRV builds bitcoindevkit/bdk#1013

Closed

notmandatory reviewed Jun 22, 2023

View reviewed changes

tcharding force-pushed the 06-10-upgrade-bitcoin branch 2 times, most recently from 8c99930 to 491efb9 Compare June 22, 2023 02:05

notmandatory reviewed Jun 22, 2023

View reviewed changes

tcharding force-pushed the 06-10-upgrade-bitcoin branch 2 times, most recently from ceaa9fe to d62ddea Compare June 22, 2023 04:45

tcharding force-pushed the 06-10-upgrade-bitcoin branch from d62ddea to 9e4389a Compare June 23, 2023 02:31

tcharding added 2 commits June 23, 2023 12:33

Pin dependencies in CI for MSRV build

d48ac22

Recently the MSRV build broke. We can pin the dependencies in the MSRV CI job. Pin `log` and `rustls` when running the MSRV CI job.

tcharding force-pushed the 06-10-upgrade-bitcoin branch from 9e4389a to ea1b6d1 Compare June 23, 2023 02:35

tcharding mentioned this pull request Jun 27, 2023

Update rustls dependency bitcoindevkit/rust-electrum-client#110

Merged

Add pinning note to electrum-client dependency

73768c5

A pending to `rust-electrum-client` [0] fixes the pinning here in our CI, add a comment to remind us to fix the pinning when the PR is merged and released. [0] bitcoindevkit/rust-electrum-client#110

vladimirfomene force-pushed the 06-10-upgrade-bitcoin branch 10 times, most recently from 4d7487c to 81e2be6 Compare June 28, 2023 20:48

Upgrade electrum-client to 0.16.0

8d1f907

vladimirfomene force-pushed the 06-10-upgrade-bitcoin branch from 81e2be6 to 8d1f907 Compare June 28, 2023 20:57

Redundant import

cd497a7

vladimirfomene reviewed Jun 29, 2023

View reviewed changes

vladimirfomene approved these changes Jun 29, 2023

View reviewed changes

vladimirfomene merged commit a08ebe3 into bitcoindevkit:master Jun 29, 2023

notmandatory added this to the Release 0.6.0 milestone Jul 3, 2023

tcharding deleted the 06-10-upgrade-bitcoin branch July 12, 2023 06:14

	# Temporary dependency on internals until the rust-bitcoin devs get their shit together and release the hex-conservative crate.
	# Temporary dependency on internals until the rust-bitcoin devs release the hex-conservative crate.

	run: cargo update -p log --precise 0.4.18 && cargo update --p rustls --precise 0.21.1
	run: cargo update -p log --precise 0.4.18 && cargo update -p rustls --precise 0.21.1

Conversation

tcharding commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladimirfomene left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Jun 21, 2023

Uh oh!

tcharding commented Jun 21, 2023

Uh oh!

notmandatory Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Jun 22, 2023

Uh oh!

notmandatory Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

LLFourn commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Jun 22, 2023

Uh oh!

LLFourn commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladimirfomene commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Jun 23, 2023

Uh oh!

tcharding commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladimirfomene commented Jun 26, 2023

Uh oh!

tcharding commented Jun 27, 2023

Uh oh!

tcharding commented Jun 29, 2023

Uh oh!

vladimirfomene left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tcharding commented Jun 10, 2023 •

edited

Loading

notmandatory commented Jun 22, 2023 •

edited

Loading

LLFourn commented Jun 22, 2023 •

edited

Loading

tcharding commented Jun 22, 2023 •

edited

Loading

LLFourn commented Jun 22, 2023 •

edited

Loading

vladimirfomene commented Jun 22, 2023 •

edited

Loading

tcharding commented Jun 23, 2023 •

edited

Loading

tcharding commented Jun 23, 2023 •

edited

Loading