Clean up dependencies, feature flags, kick the tires on the code to bring it up to date #37

nyonson · 2025-11-14T21:53:34Z

First two patches just get the code compiling again, some fallout from the BufRead -> Read change in rust-bitcoin. The rest of the patches use the 1.60+ rust features for a cleaner feature flag interface with less footguns for users.

Supersedes #36
Closes #32

tcharding · 2025-11-17T00:42:58Z

Thanks! All good except a1e6753

Doesn't really do anything
bitcoin 0.32 still has this actual_serde kludge but the MSRV here is 1.63 and there is 1.56 (I went digging for the PR that removed it but couldn't find it. I don't know which MSRV allowed getting rid of it.)

I'd say just drop that patch and we remove the whole kludge once we upgrade to bitcoin 0.33

nyonson · 2025-11-17T21:38:28Z

Thanks! All good except a1e6753

Doesn't really do anything

My intention was to hide the actual-serde flag from the end user, which I think is what cargo does when we introduce the dep: usage, it turns off the implicit flag for the dependency.

bitcoin 0.32 still has this actual_serde kludge but the MSRV here is 1.63 and there is 1.56 (I went digging for the PR that removed it but couldn't find it. I don't know which MSRV allowed getting rid of it.)

I started experimenting with this and it got a little hairy, which is why I thought it would be worth to at least clean up the interface for now.

tcharding · 2025-11-18T00:26:46Z

Ah no worries, TIL.

tcharding

ACK fa93071

nyonson · 2025-11-18T00:37:23Z

I'm dealing with some dependency issues atm, still teasing it apart. I think easiest to just bump the bitcoin dep to 0.32.7, but learning the whys.

The underlying consensus encoding trait was "loosened" from requiring a BufRead to just a Read.

nyonson · 2025-11-21T22:19:04Z

@tcharding I went kinda ham on this and made the leap to rbmt, but I have higher confidence it will pass CI now. Is there a reason the workflow doesn't auto-run here?

tcharding · 2025-11-21T22:25:39Z

Is there a reason the workflow doesn't auto-run here?

I assume its the 'don't run CI for first time contributors setting' but I didn't bother checking.

tcharding · 2025-11-21T22:26:03Z

I went kinda ham on this and made the leap to rbmt,

Mad, happy to use this repo as a testbed.

tcharding · 2025-11-21T22:27:40Z

I invited you as a collaborator to the repo, can make you a maintainer if you like?

nyonson · 2025-11-21T23:38:18Z

@tcharding I think collaborator is fine. Do you want to ACK these PRs before merge or should I just go wild?

Separately, should we bump the MSRV here to 1.74 as well?

tcharding · 2025-11-23T21:21:52Z

@tcharding I think collaborator is fine. Do you want to ACK these PRs before merge or should I just go wild?

We want to have at least one review before merge. Lets use the ack with commit hash same as in rust-bitcoin and then merge with Core's merge script. Do you know about that?

nyonson · 2025-11-23T21:29:17Z

We want to have at least one review before merge. Lets use the ack with commit hash same as in rust-bitcoin

Sounds good!

merge with Core's merge script. Do you know about that?

Nope, what's that?

(just noticed The min versions are still broken, i'll iron that out)

tcharding · 2025-11-23T21:42:50Z

It comes from Core's maintainer tools repo. I have a local copy, I don't track its developement.

https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/github-merge.py

Its what I use to merge into any repos I control. Andrew used to use it but now he has a custom tool for merging into rust-bitcoin.

tcharding · 2025-11-24T23:18:15Z

src/v0/bitcoin/mod.rs

+                    let signature = {
+                        let mut rng = thread_rng();
+                        secp.sign_schnorr_with_rng(&msg, &key_pair, &mut rng)
+                    };


Why this change please?

Ah, i think I was getting tripped up on not having rand-std enabled or something. Let me see where I got confused.

tcharding · 2025-11-24T23:20:47Z

In 984eacd there are changes BufRead / Read but the patch is about secp.

tcharding · 2025-11-24T23:22:12Z

Cargo.toml

+# master branch uses hex-conservative 0.3.0+ which has the fix.
+#
+# This override can be removed when upgrading to bitcoin 0.33.0 or later.
+hex-conservative = { version = "0.2.1", default-features = false }


Can we keep the optional deps separated by a line of whitespace below the non-optional ones please?

tcharding · 2025-11-24T23:26:48Z

Cargo.toml

-actual-serde = { package = "serde", version = "1.0.103", default-features = false, features = [ "derive", "alloc" ], optional = true }
+actual-serde = { package = "serde", version = "1.0.103", default-features = false, features = ["derive", "alloc"], optional = true }


Excuse me in advance, I'm in a state today.

If you put random whitespace changes like this in a patch it makes the reviewer have to spend some clock cycles trying to workout why the diff contains these lines, and what exactly changed. As a general principle just throw whitespace changes in a separate patch up front. Thanks

Yea, my bad man, I ran with this one too long tryin to get it a happy spot. I'll try to clean up the commits.

tcharding · 2025-11-24T23:30:21Z

contrib/install-rbmt.sh

+
+cargo install --quiet --git https://github.com/rust-bitcoin/rust-bitcoin-maintainer-tools \
+    --rev "$RBMT_VERSION" \
+    rust-bitcoin-maintainer-tools


This script could live in rust-bitcoin-maintainer-tools repo also, right? Is it here for a reason? And we could install rmbt duing the prepare job, would that be better? Open questions, I'm in no way implying how you should proceed.

Yea, I am not sure the best way to integrate the tooling yet. I originally thought it would be nice to have the hash set in Cargo.toml in a dev-dependency, but after some trial and error, realized it is kinda difficult to run a binary from there (please let me know if I am mistaken there).

If the script lived over in rust-bitcoin-maintainer-tools we would need to like, curl it over here at somepoint or something right?

Oh I meant the install-rbmt.sh script could live there. Its going to be the same everywhere, right?

tcharding · 2025-11-24T23:33:35Z

contrib/rbmt.toml

+# Tests each feature alone, all pairs, and all together.
+# Note: rand is a little weird until we can upgrade secp256k1 to v0.30.0
+# Note: miniscript is not included here as it has its own no-std handling
+features_without_std = ["serde", "base64"]


Whip time again, sorry bro. In patch 99c1db8 you add empty arrays for exact_features and features_with_no_std then later in the PR they are removed. In general please don't make changes to a series of patches like this because it makes reviewers who review with there terminal (read: proper geeks) jump over to github to add a review suggestion only to find the change no longer exists. Each patch should be discreet both in what it does and in how it is reviewed. Just slap me for the sermons, I warned you I was in a state.

tcharding

ACK b5fd7f2

tcharding · 2025-11-24T23:38:31Z

I whinged alot there, I guess I'm not exactly sure what level of review you want. I'm also totally happy for you to just patch this repo however you like. We don't have to be super anal if you want to just go wild.

nyonson · 2025-11-24T23:40:16Z

I whinged alot there, I guess I'm not exactly sure what level of review you want. I'm also totally happy for you to just patch this repo however you like. We don't have to be super anal if you want to just go wild.

Eh, I got sloppy here. Let me clean it up. Now that I know its possible to get the builds to pass I can take my time gettin the code and history right.

tcharding · 2025-11-25T00:13:53Z

Thanks man

nyonson force-pushed the feature-flags branch from 3f82c1c to fa93071 Compare November 17, 2025 21:50

tcharding approved these changes Nov 18, 2025

View reviewed changes

nyonson added 2 commits November 19, 2025 20:16

Update secp256k1 interface

984eacd

Match rust-bitcoin's looser encoding requirements

af8a05e

The underlying consensus encoding trait was "loosened" from requiring a BufRead to just a Read.

nyonson force-pushed the feature-flags branch 3 times, most recently from ccf55ed to 348c69a Compare November 21, 2025 22:15

nyonson force-pushed the feature-flags branch from 6193316 to f91e4ad Compare November 24, 2025 18:16

nyonson changed the title ~~Clean up dependencies and feature flags~~ Clean up dependencies, feature flags, kick the tires on the code to bring it up to date Nov 24, 2025

tcharding reviewed Nov 24, 2025

View reviewed changes

nyonson added 2 commits November 24, 2025 15:34

Combine miniscript feature flags into one

db3b6ca

Hide actual-serde feature flag from end users

3b6e884

tcharding approved these changes Nov 24, 2025

View reviewed changes

nyonson force-pushed the feature-flags branch 4 times, most recently from e0a9510 to 4ffcab3 Compare November 25, 2025 00:00

nyonson added 6 commits November 24, 2025 16:36

Remove unnecssary rand-std flag

9fffb6b

ci: switch to rmbt tooling

1c8a774

Get ci lint to pass by updating deprecated function

e43c1df

Bump nightly version, fix new lints and format

35ab547

Fix up minimal versions

5cdcee8

Fix version numbers for synced code

3db7939

nyonson force-pushed the feature-flags branch from 4ffcab3 to 3db7939 Compare November 25, 2025 00:37

nyonson marked this pull request as draft November 25, 2025 01:17

		actual-serde = { package = "serde", version = "1.0.103", default-features = false, features = [ "derive", "alloc" ], optional = true }
		actual-serde = { package = "serde", version = "1.0.103", default-features = false, features = ["derive", "alloc"], optional = true }

Clean up dependencies, feature flags, kick the tires on the code to bring it up to date #37

Are you sure you want to change the base?

Clean up dependencies, feature flags, kick the tires on the code to bring it up to date #37

Uh oh!

Conversation

nyonson commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Nov 17, 2025

Uh oh!

nyonson commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Nov 18, 2025

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

nyonson commented Nov 18, 2025

Uh oh!

nyonson commented Nov 21, 2025

Uh oh!

tcharding commented Nov 21, 2025

Uh oh!

tcharding commented Nov 21, 2025

Uh oh!

tcharding commented Nov 21, 2025

Uh oh!

nyonson commented Nov 21, 2025

Uh oh!

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

Uh oh!

Uh oh!

nyonson commented Nov 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

tcharding Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Nov 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Nov 24, 2025

Uh oh!

nyonson commented Nov 24, 2025

Uh oh!

tcharding commented Nov 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nyonson commented Nov 14, 2025 •

edited

Loading

nyonson commented Nov 17, 2025 •

edited

Loading

tcharding commented Nov 23, 2025 •

edited

Loading

tcharding commented Nov 23, 2025 •

edited

Loading

tcharding Nov 24, 2025 •

edited

Loading