Skip to content

electrsd: set default features for bitreq#593

Merged
tcharding merged 1 commit into
rust-bitcoin:masterfrom
tankyleo:2026-05-set-bitreq-std-if-download
May 21, 2026
Merged

electrsd: set default features for bitreq#593
tcharding merged 1 commit into
rust-bitcoin:masterfrom
tankyleo:2026-05-set-bitreq-std-if-download

Conversation

@tankyleo
Copy link
Copy Markdown
Contributor

@tankyleo tankyleo commented May 19, 2026

If and only if the download feature is set, bitreq is required. In that case the bitreq std feature is needed, so we enable default-features for bitreq to set the std feature. No other bitreq feature is enabled.

@tankyleo
Copy link
Copy Markdown
Contributor Author

cc @tnull

Copy link
Copy Markdown
Collaborator

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

The title and change don't match. I think the title is correct and the bitreq/std feature should only be enabled when download is by removing std from bitreq and adding instead to the download fearture:
download = ["bitcoin_hashes", "zip", "bitreq/std"]

@tankyleo
Copy link
Copy Markdown
Contributor Author

The title and change don't match. I think the title is correct and the bitreq/std feature should only be enabled when download is by removing std from bitreq and adding instead to the download fearture: download = ["bitcoin_hashes", "zip", "bitreq/std"]

If we do this, then this applies to the https feature too right? So we would do: download = ["bitcoin_hashes", "zip", "bitreq/std", "bitreq/https"]

I would find this uglier, especially since only download uses bitreq, but let me know.

@jamillambert
Copy link
Copy Markdown
Collaborator

The title and change don't match. I think the title is correct and the bitreq/std feature should only be enabled when download is by removing std from bitreq and adding instead to the download fearture: download = ["bitcoin_hashes", "zip", "bitreq/std"]

If we do this, then this applies to the https feature too right? So we would do: download = ["bitcoin_hashes", "zip", "bitreq/std", "bitreq/https"]

I would find this uglier, especially since only download uses bitreq, but let me know.

Hmm, yeah, you could change the tile so it is correct instead. Removing default-features = false from bitreq also fixes the problem since the only default feature is std.

Comment thread electrsd/Cargo.toml Outdated
Comment on lines +32 to +34
bitreq = { version = "0.3.5", path = "../bitreq", default-features = false, optional = true, features = [
"https",
"std",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While correct I also rekon this would be better:

bitreq = { version = "0.3.5", path = "../bitreq", optional = true, features = ["https"] }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, done below

@tcharding
Copy link
Copy Markdown
Member

Thanks Leo, I debugged this myself today and got lost wondering how the heck this got into master.

If and only if the download feature is set, bitreq is required. In that
case the bitreq std feature is needed, so we enable default-features for
bitreq to set the std feature. No other bitreq feature is enabled.
@tankyleo tankyleo force-pushed the 2026-05-set-bitreq-std-if-download branch from d5fb0fa to adde75f Compare May 20, 2026 06:21
@tankyleo tankyleo changed the title electrsd: set bitreq/std if download is set electrsd: set default features for bitreq May 20, 2026
Copy link
Copy Markdown
Collaborator

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

ACK adde75f

@tankyleo tankyleo requested a review from tcharding May 20, 2026 19:18
Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK adde75f

@tcharding tcharding merged commit 9104baf into rust-bitcoin:master May 21, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants