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.

Sign up for GitHub

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

Jump to bottom

Upgrade bitcoin to v0.31 #79

Merged
merged 1 commit into from
Feb 29, 2024
Merged

Upgrade bitcoin to v0.31 #79

merged 1 commit into from
Feb 29, 2024

Conversation

tcharding
Copy link
Contributor

@tcharding tcharding commented Feb 7, 2024 •
edited
Loading

Upgrade dependencies to use the latest rust-bitcoin v0.31.

While we are at it, bump the crate version ready for release.

(Includes pinning dependencies for MSRV build in CI.)

@tcharding tcharding changed the title Upgrade Upgrade bitcoin Feb 7, 2024
@tcharding
Copy link
Contributor Author

tcharding commented Feb 7, 2024 •
edited
Loading

Excuse the noise, this is another go at #78 because I saw #76.

This is #60 with pinning in CI. I previously handed off to @getong because he did #74 but that seems to be stalling. Lets see if I can get this past CI.

I copied the pinning done by @notmandatory's PR and tweaked it a bit after upgrading the electrsd dependency.

@tcharding tcharding mentioned this pull request Feb 7, 2024
Closed
tcharding added a commit to tcharding/bdk that referenced this pull request Feb 7, 2024
@tcharding
Upgrade miniscript/bitcoin dependency
6d4ea1c 
Upgrade:

- bitcoin to v0.31.0
- miniscript to v11.0.0

Requires currently unreleased:

- rust-hwi: master branch
- rust-esplora-clinet: bitcoindevkit/rust-esplora-client#79
@tcharding tcharding mentioned this pull request Feb 7, 2024
Merged
tcharding added a commit to tcharding/bdk that referenced this pull request Feb 7, 2024
@tcharding
Upgrade miniscript/bitcoin dependency
6233210 
Upgrade:

- bitcoin to v0.31.0
- miniscript to v11.0.0

Requires currently unreleased:

- rust-hwi: master branch
- rust-esplora-clinet: bitcoindevkit/rust-esplora-client#79
tnull
tnull reviewed Feb 7, 2024
View reviewed changes
.github/workflows/cont_integration.yml Outdated
@@ -51,10 +51,12 @@ jobs:
- name: pin dependencies
if: matrix.rust.version == '1.63.0'
run: |
cargo update -p zstd-sys --precise "2.0.8+zstd.1.5.5"
cargo update -p zstd-sys --precise "2.0.6+zstd.1.5.2"
cargo update -p zstd --precise 0.11.2+zstd.1.5.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we now require to pin zstd individually? I think the +zstd.1.5.5 already should pin it to a particular version? Moreover, any reason why we reduce the pinned zstd-sys version from 2.0.8 to 2.0.6?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think too hard about it to be honest, I find pinning to be a total waist of brain space so I just wildly try versions until something works.

Copy link
Contributor Author

@tcharding tcharding Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I remember. I brought zstd-sys back to be in line with the new zstd, but like you say we may not need both. (Referring to the +1.5.2)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we don't need to pin both and if pinning zstd-sys to 2.0.8+zstd.1.5.5 is sufficient to meet the MSRV, I'd always prefer to pin to the higher version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @tnull, can we change this back to zstd-sys and zstd 1.5.5? You can verify on crates.io that 1.5.5 is the latest version with MSRV under 1.63: https://crates.io/crates/zstd-sys/versions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as requested, lets let CI prove if it works for us

.github/workflows/cont_integration.yml Outdated
cargo update -p time --precise "0.3.20"
cargo update -p jobserver --precise "0.1.26"
cargo update -p home --precise 0.5.5
cargo update -p jobserver --precise 0.1.26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pinned above already, no need to duplicate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, I thought I remove that locally. Will re-spin, thanks.

@tcharding
Copy link
Contributor Author

I left, zstd and zstd-sys in there, likely unnecessary but the additional explicitness doesn't hurt. Will remove if anyone really wants it gone.

@tcharding
Copy link
Contributor Author

Thanks for the review @tnull!

@notmandatory
Copy link
Member

I've merged #76 so this should be rebased on master so we just see e41b360.

@tcharding
Copy link
Contributor Author

Done, thanks.

@coveralls
Copy link

coveralls commented Feb 8, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 7896060347

Details

  • -3 of 3 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 78.934%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/api.rs 0 3 0.0%
Totals Coverage Status
Change from base Build 7822386543: 0.0%
Covered Lines: 918
Relevant Lines: 1163
💛 - Coveralls

oleonardolima
oleonardolima reviewed Feb 8, 2024
View reviewed changes
Copy link
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 581cfb0

@notmandatory
Copy link
Member

Overall looks good to me, just need to resolve above comments about zstd.

@tnull is it OK with you to merge this one before #75? This one is a smaller change so should be easier to rebase on top of.

@notmandatory notmandatory added this to the Release 0.7.0 milestone Feb 14, 2024
@tcharding
Upgrade bitcoin
c046047 
Upgrade dependencies to use the latest `rust-bitcoin v0.31`.

While we are at it, bump the crate version ready for release.

(Includes pinning dependencies for MSRV build in CI.)
@tnull
Copy link
Contributor

tnull commented Feb 14, 2024

Overall looks good to me, just need to resolve above comments about zstd.

@tnull is it OK with you to merge this one before #75? This one is a smaller change so should be easier to rebase on top of.

Sure, happy to rebase once this landed!

@tcharding
Copy link
Contributor Author

You guys were right about the pins, thanks for pushing me.

notmandatory
notmandatory approved these changes Feb 29, 2024
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK c046047

oleonardolima
oleonardolima approved these changes Feb 29, 2024
View reviewed changes
Copy link
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@notmandatory notmandatory merged commit e5fb3a3 into bitcoindevkit:master Feb 29, 2024
15 checks passed
@notmandatory notmandatory mentioned this pull request Feb 29, 2024
Merged
@notmandatory notmandatory changed the title Upgrade bitcoin Upgrade bitcoin to v0.31 Mar 18, 2024
@notmandatory notmandatory mentioned this pull request Mar 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnull tnull tnull left review comments

@oleonardolima oleonardolima oleonardolima approved these changes

@notmandatory notmandatory notmandatory approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 0.7.0
Development

Successfully merging this pull request may close these issues.
5 participants
@tcharding @notmandatory @coveralls @tnull @oleonardolima