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.

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

chore(deps): bump hashbrown to v0.14.5 #1721

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

tvolk131
Copy link
Contributor

@tvolk131 tvolk131 commented Nov 15, 2024

Bump hashbrown to v0.15

I ran cargo build-all-features and cargo test-all-features locally for the core crate with no issues. Here's the output for the former:

cargo build-all-features

    Building crate=bdk_core features=[]
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s
    Building crate=bdk_core features=[hashbrown]
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s
    Building crate=bdk_core features=[serde]
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.05s
    Building crate=bdk_core features=[std]
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s
    Building crate=bdk_core features=[hashbrown,serde]
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s
    Building crate=bdk_core features=[hashbrown,std]
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s
    Building crate=bdk_core features=[serde,std]
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s
    Building crate=bdk_core features=[hashbrown,serde,std]
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s

The relevant changelog can be found between here and here

The only notable breaking change I found was that the MSRV was bumped to v1.63. However, this is the same MSRV as we already use, so it's not a breaking change for us.

@notmandatory notmandatory added the dependencies Pull requests that update a dependency file label Nov 18, 2024
crates/core/Cargo.toml Outdated Show resolved Hide resolved
crates/core/Cargo.toml Outdated Show resolved Hide resolved
@notmandatory notmandatory added this to the 1.0.0-beta milestone Nov 19, 2024
@notmandatory
Copy link
Member

notmandatory commented Nov 19, 2024

Sorry about the force pushes, I committed my suggestion instead of closing it. But at least this give the branch a chance to run with the fixed hashbrown dependency.

Looks like hashbrown team doesn't want to fix this and Tokio pinned it in their CI and readme. We can do the same for this PR. Let me know if you want me to push a commit with those changes.

@tvolk131
Copy link
Contributor Author

Let me know if you want me to push a commit with those changes.

Pushed! Looks like the only feature needed is default-hasher. I re-ran cargo build-all-features and everything's passing locally.

Copy link
Contributor

@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 eba1dd0

It's failing on CI at the MSRV step, you'll need to pin it to 0.15.0 here and update the README.md as well.

@notmandatory
Copy link
Member

notmandatory commented Nov 19, 2024

For some reason getting errors with hashbrown pinned to 0.15.0 for MSRV 1.63.0, also reproducible locally:

---- test_into_tx_graph stdout ----
thread 'test_into_tx_graph' panicked at 'a failure ordering can't be stronger than a success ordering', /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/sync/atomic.rs:2700:18
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tx_can_become_unconfirmed_after_reorg stdout ----
thread 'tx_can_become_unconfirmed_after_reorg' panicked at 'a failure ordering can't be stronger than a success ordering', /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/sync/atomic.rs:2700:18

@notmandatory
Copy link
Member

notmandatory commented Nov 19, 2024

I force pushed a commit to only bump hashbrown to 0.14.5 and disable default features, building and testing for MSRV looks like it's working now. EDIT: with this version I have to leave default features on.

@notmandatory notmandatory changed the title chore(deps): bump hashbrown to v0.15 chore(deps): bump hashbrown to ~~v0.15~~ v0.14.5 Nov 19, 2024
@notmandatory notmandatory changed the title chore(deps): bump hashbrown to ~~v0.15~~ v0.14.5 chore(deps): bump hashbrown to v0.14.5 Nov 19, 2024
@tnull
Copy link
Contributor

tnull commented Nov 19, 2024

EDIT: with this version I have to leave default features on.

No, I believe you simply need to enable ahash as a default hasher, although inline-more probably also makes sense to leave enabled.

So

hashbrown = { version = "0.14.5", optional = true, default-features = false, features = ["ahash", "inline-more"] }

works for me.

@notmandatory
Copy link
Member

notmandatory commented Nov 20, 2024

EDIT: with this version I have to leave default features on.

No, I believe you simply need to enable ahash as a default hasher, although inline-more probably also makes sense to leave enabled.

@tnull thanks for testing it out, I pushed your suggestion and CI passing so all good. Just FYI the only default feature not included is:

allocator-api2: Enables support for allocators that support allocator-api2.

@tvolk131
Copy link
Contributor Author

Thanks for updating and testing this guys! Is there anything else needed here, or is this good to merge?

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.

ACK 52fa540

@notmandatory
Copy link
Member

@tvolk131 I was just giving anyone else interested a chance to comment, but looks good to me so I'll merge it. Thanks for the suggestion to do this version bump.

@notmandatory notmandatory merged commit 62e84d7 into bitcoindevkit:master Nov 21, 2024
21 checks passed
@tvolk131 tvolk131 deleted the bump_hashbrown branch November 26, 2024 04:05
@notmandatory notmandatory mentioned this pull request Dec 12, 2024
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants