Clean up dependencies, feature flags, format, lint, and kick the tires on rbmt by nyonson · Pull Request #37 · rust-bitcoin/rust-psbt

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

Bit of mess, but the first patch gets the code compiling again, some fallout from the BufRead -> Read change in rust-bitcoin. The next three patches use the 1.60+ rust features for a cleaner feature flag interface with less footguns for users. And finally the rest of the patches introduce the rbmt tooling and fix up format, lint, and minimal version issues.

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.

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?

Yea, but if it lived there how would this repo integrate with it? Just clone rust-bitcoin-maintainer-tools kinda like we do now in rust-bitcoin for the script version?

Another thought I had is that this script is so simple, a single cargo install at this point, I could just drop the script.

Oh of coures, you are right. Now we install rmbt (or cargo rmbt) we don't need the repo. My bad.

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

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

tcharding

ACK 233ff10

tcharding · 2025-11-26T03:45:51Z

Nice, clean.

nyonson · 2025-11-26T16:55:33Z

@tcharding I gave the merge script a whirl, I think all went well.

tcharding · 2025-11-26T22:27:46Z

Boss!

tcharding · 2025-11-26T22:28:15Z

I had a shower thought that I should have merged this yesterday, glad you did it. The oversight was not intentional.

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

tcharding approved these changes Nov 24, 2025

View reviewed changes

nyonson force-pushed the feature-flags branch 2 times, most recently from b75766f to f31949a Compare November 24, 2025 23:49

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

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

Match rust-bitcoin's looser encoding requirements

0970655

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

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

nyonson force-pushed the feature-flags branch from 3db7939 to 8f949f2 Compare November 25, 2025 12:57

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

nyonson added 7 commits November 25, 2025 05:08

Combine miniscript feature flags into one

5c42a53

Hide actual-serde feature flag from end users

c2d88c6

Remove unnecssary rand-std flag

e8ac0ab

Switch to rmbt tooling

260081f

Bump nightly version, fix new lints and format

3a8a97b

Fix up minimal versions

085b9b9

Fix version numbers for synced code

233ff10

nyonson force-pushed the feature-flags branch from 8f949f2 to 233ff10 Compare November 25, 2025 13:11

tcharding approved these changes Nov 26, 2025

View reviewed changes

nyonson marked this pull request as ready for review November 26, 2025 16:08

nyonson merged commit ef510e4 into rust-bitcoin:master Nov 26, 2025
11 checks passed

nyonson mentioned this pull request Nov 26, 2025

Update consensus_decode to use Read as trait bound #36

Closed

		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 }

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!

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!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Nov 26, 2025

Uh oh!

Uh oh!

nyonson commented Nov 26, 2025

Uh oh!

tcharding commented Nov 26, 2025

Uh oh!

tcharding commented Nov 26, 2025

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