Skip to content

Conversation

@mducroux
Copy link
Collaborator

No description provided.

liuchengxu and others added 28 commits February 26, 2025 10:06
…gle()` incompatibility with Bitcoin Core

18c2cad Add test for sighash_single_bug incompatility fix (Liu-Cheng Xu)
068c3f2 Fix `is_invalid_use_of_sighash_single()` incompatibility with Bitcoin Core (Liu-Cheng Xu)

Pull request description:

  Backport rust-bitcoin#4113

  Cherry-picked the 2 commits and manually fixed merge conflicts.

ACKs for top commit:
  apoelstra:
    ACK 18c2cad; successfully ran local tests

Tree-SHA512: 9ddc3950e6f35c1fb6d44156a26a8111fe73e6189b1e6c57464173671ce1fab95558485844f72abb33f632b5015089733f3a0e4acdb11005c06c58d466a63706
The `taproot_control_block` did not properly detect whether it deals
with script spend or key spend. As a result, if key spend with annex was
used it'd return the first element (the signature) as if it was a
control block.

Further, the conditions identifying which kind of spend it was were
repeated multiple times but behaved subtly differently making only
`taproot_control_block` buggy but the other places confusing.

To resolve these issues this change adds a `P2TrSpend` enum that
represents a parsed witness and has a single method doing all the
parsing. The other methods can then be trivially implemented by matching
on that type. This way only one place needs to be verified and the
parsing code is more readable since it uses one big `match` to handle
all possibilities.

The downside of this is a potential perf impact if the parsing code
doesn't get inlined since the common parsing code has to shuffle around
data that the caller is not intersted in. I don't think this will be a
problem but if it will I suppose it will be solvable (e.g. by using
`#[inline(always)]`).

The enum also looks somewhat nice and perhaps downstream consumers could
make use of it. This change does not expose it yet but is written such
that after exposing it the API would be (mostly) idiomatic.

Closes rust-bitcoin#4097
The previous commit fixed a bug when `taproot_control_block` returned
`Some` on key-spends. This adds a test case for it which succeeds when
applied after the previous commit and fails if applied before it.
We already have `tapscript` method on `Witness` which is broken because
it doesn't check that the leaf script is a tapscript, however that
behavior might have been intended by some consumers who want to inspect
the script independent of the version. To resolve the confusion, we're
going to add a new method that returns both the leaf script and, to
avoid forgetting version check, also the leaf version.

This doesn't touch the `tapscript` method yet to make backporting of
this commit easier. It's also worth noting that leaf script is often
used together with version. To make passing them around easier it'd be
helpful to use a separate type. Thus this also adds a public POD type
containing the script and the version. In anticipation of if being
usable in different APIs it's also generic over the script type.
Similarly to the `tapscript` method, this also only adds the type and
doesn't change other functions to use it yet. Only the newly added
`taproot_leaf_script` method uses it now.

This is a part of rust-bitcoin#4073
Now that an alternative exists we can deprecate the method with an
expalantion of what's going on.
9e87bc5 Deprecate the `Witness::tapscript` method (Martin Habovstiak)
730baeb Add `taproot_leaf_script` methood to `Witness` (Martin Habovstiak)
74138d5 Add a test case checking `taproot_control_block` (Martin Habovstiak)
39e280a Fix key/script spend detection in `Witness` (Martin Habovstiak)

Pull request description:

  This backports fixes from rust-bitcoin#4100

ACKs for top commit:
  tcharding:
    ACK 9e87bc5
  apoelstra:
    ACK 9e87bc5; successfully ran local tests

Tree-SHA512: 3d2928dc09c39dbe1d8a6f34e41e86ed47e9b257994eb308a70841db437e19f0947353d325b511304567b59cc6b17beefe0b257bfad3b50847d0cb304b284914
The corresponding PR on master is rust-bitcoin#4387, but this doesn't really
resemble that PR. Rather than changing all the error enums, this just
adds a new variant to the #[non_exhaustive] bip32::Error enum and
returns that when adding 1 to 255 when deriving child keys.

This is therefore not an API break and can be released in a minor
version. Although it does change the error return on private derivation
from a "succeeds except with negligible probability" to "there is an
error path you may need to check for".

Fixes rust-bitcoin#4308
…ttempting to derive past maximum depth

315750d bip32: return error when attempting to derive past maximum depth (Andrew Poelstra)

Pull request description:

  The corresponding PR on master is rust-bitcoin#4387, but this doesn't really resemble that PR. Rather than changing all the error enums, this just adds a new variant to the #[non_exhaustive] bip32::Error enum and returns that when adding 1 to 255 when deriving child keys.

  This is therefore not an API break and can be released in a minor version. Although it does change the error return on private derivation from a "succeeds except with negligible probability" to "there is an error path you may need to check for".

  Fixes rust-bitcoin#4308

ACKs for top commit:
  tcharding:
    ACK 315750d

Tree-SHA512: 8224a3462169ab1e31211b4b00b423d11b8778452096617eaf384ef8ec3dd5f8325b7a0e29519db1660ea4bc5620828b996fca329272900145b96486e2a7e653
… signing

This commit enhances PSBT signing functionality by:

1. Added new KeyRequest::XOnlyPubkey variant to support direct retrieval using XOnly public keys
2. Implemented GetKey for HashMap<XOnlyPublicKey, PrivateKey> for more efficient Taproot key management
3. Modified HashMap<PublicKey, PrivateKey> implementation to handle XOnlyPublicKey requests by checking both even and odd parity variants

These changes allow for more flexible key management in Taproot transactions.
Specifically, wallet implementations can now store keys indexed by either
PublicKey or XOnlyPublicKey and successfully sign PSBTs with Taproot inputs.

Added tests for both implementations to verify correct behavior.

Added test for odd parity key retrieval.

Closes rust-bitcoin#4150
Backport rust-bitcoin#4373, authored by Shing Him Ng.

Original gitlog:

For TweakedKeypair, `to_inner` is also renamed to `to_keypair` to maintain
consistency. Similarly, `to_inner` is renamed to `to_x_only_pubkey` for
TweakedPublicKey

Co-authored-by: Shing Him Ng <[email protected]>
c67adcd backport: Add methods to retrieve inner types (Shing Him Ng)

Pull request description:

  Backport rust-bitcoin#4373, authored by Shing Him Ng.

  Original gitlog:

  For TweakedKeypair, `to_inner` is also renamed to `to_keypair` to maintain consistency. Similarly, `to_inner` is renamed to `to_x_only_pubkey` for TweakedPublicKey

ACKs for top commit:
  apoelstra:
    ACK c67adcd; successfully ran local tests
  shinghim:
    ACK c67adcd
  storopoli:
    ACK c67adcd

Tree-SHA512: 310f0f99c3ffd0937b83739d30998afec471b0e41d9af2f4863e8405ffc4f4ee6650d1e73d170bf93cefee59a89331eff4d93984fc3ab3dfcbe3830e559ce659
…T key retrieval and improve Taproot signing

95eb255 Add XOnlyPublicKey support for PSBT key retrieval and improve Taproot signing (Erick Cestari)
2858b6c Support GetKey where the Xpriv is a direct child of the looked up KeySource (Nadav Ivgi)
d005ddd Refactor GetKey for sets to internally use Xpriv::get_key() (Nadav Ivgi)
b75b2e3 Fix GetKey for sets to properly compare the fingerprint (Nadav Ivgi)

Pull request description:

  Backport two PRs:

  - rust-bitcoin#3356
  - rust-bitcoin#4238

  The first includes a bug fix in a `GetKey` impl and the second is a feature we want to release. And the three refactor patches touch code that rust-bitcoin#4238 builds on so I figured we should backport them all.

  All 5 patches required a small amount of massaging to get in. The first 4 was just adding a `?` to the calls to `derive_priv`. The last patch needed a few calls in the unit test changing.

ACKs for top commit:
  storopoli:
    ACK 95eb255
  apoelstra:
    ACK 95eb255; successfully ran local tests

Tree-SHA512: 5b73c4cd3ddfeef5d4e64a6e236c905c39c560dc704dba8d925f636d3c9b12c0706f27c3e4b29d92ab9e2d15ddbee8bbe5d0955836529d72461d6ee505d899c5
In preparation for release bump the version, add a changelog entry, and
update the lock files.
916982a bitcoin: Bump version to 0.32.6 (Tobin C. Harding)

Pull request description:

  In preparation for release bump the version, add a changelog entry, and update the lock files.

ACKs for top commit:
  apoelstra:
    ACK 916982a; successfully ran local tests
  storopoli:
    ACK 916982a

Tree-SHA512: 922dc8af49479d7777ee73ddf142f62372c6423af17ce367bf66b9d2c933cff648153856a35f1ba3a10ec61844c897d4c5a25e4c216d17c1f899f39d9cbc47c0
Manually backport rust-bitcoin#4538.

If we use a `u32` then the constructor no longer panics. 32 bits is
plenty for an sane usage.
…stead of _unchecked

c7b20f4 backport: Use _u32 in FeeRate constructor instead of _unchecked (Tobin C. Harding)

Pull request description:

  Manually backport rust-bitcoin#4538.

  If we use a `u32` then the constructor no longer panics. 32 bits is plenty for an sane usage.

ACKs for top commit:
  apoelstra:
    ACK c7b20f4; successfully ran local tests

Tree-SHA512: c247ad0b95ed1b193626fd5d6cb6dba287b779be4bfc90602972921f1dd4a064b15bf4233f0496d238af4fcff2639bd9b3e52adbff0a370a0d67ea7eeb51b2ff
Manually backport PR rust-bitcoin#4111. Of note, here we put `new_p2a` on
`ScriptBuf` instead of on the `script::Builder` because that seems to
be where all the other `new_foo` methods are in this release. 

Note the `WitnessProgram::p2a` is conditionally const on Rust `v1.61`
because MSRV is only `v1.56.1`.
    
From the original patch:

    Add support for the newly created Pay2Anchor output-type.
    
    See bitcoin/bitcoin#30352
c2481e4 backport: Add support for pay to anchor outputs (Tobin C. Harding)

Pull request description:

  Manually backport PR rust-bitcoin#4111. Of note, here we put `new_p2a` on `ScriptBuf` instead of on the `script::Builder` because that seems to be where all the other `new_foo` methods are in this release. 
      
  From the original patch:
  
      Add support for the newly created Pay2Anchor output-type.
      
      See bitcoin/bitcoin#30352


ACKs for top commit:
  apoelstra:
    ACK c2481e4; successfully ran local tests


Tree-SHA512: 016919914750adf6f8226acb4e6b36c0dcd8ce230df8cca13f19bcc97709caf07a076be367c76f9519a421fc93fdf73caa078ee65a85a750af7a9d9e6c757e75
    
Issue rust-bitcoin#2225 is long and has many valid opposing opinions. The main
argument for having non_exhaustive is that it helps future proof the
ecosystem at the cost of pain now. The main argument against having
non_exhaustive is why have pain now when adding a network is so rare
that having pain then is ok.
    
At the end of the thread Andrew posts:
    
> I continue to think we should have an exhaustive enum, with a bunch of
> documentation about how to use it properly. I am warming up to the
> "don't have an enum, just have rules for defining your own" but I think
> this would be needless work for people who just want to grab an
> off-the-shelf set of networks or people who want to make their own enum
> but want to see an example of how to do it first.
    
In order to make some forward progress lets remove the `non_exhaustive`
now and backport this change to 0.32, 0.31, an 0.30.
    
Later we can add, and release in 0.33, whatever forward protection /
libapocalyse protection we want to add.
    
This removes the pain now and gives us a path to prevent future pain -
that should keep all parties happy.
… from `Network`

3cf4a91 Remove non_exhausive from Network (Tobin C. Harding)

Pull request description:

  This is rust-bitcoin#4640 backported manually. Remove `non_exhaustive` from `Network` enum.


ACKs for top commit:
  apoelstra:
    ACK 3cf4a91; successfully ran local tests


Tree-SHA512: 690cdcf82a9e87f2100aa8e996583bd54f969d8d55eba062ace66bdf2ae9ae1c482d6220bb26690ebfaf3a35ca3f5ce0ecaac3a79b4061c75e9fb7adbe032f48
In preparation for release bump the version, add a changelog entry,
and update the lock files.
571cd7f bitcoin: Bump version to 0.32.7 (Tobin C. Harding)

Pull request description:

  In preparation for release bump the version, add a changelog entry, and update the lock files.


ACKs for top commit:
  apoelstra:
    ACK 571cd7f; successfully ran local tests


Tree-SHA512: c1a9ddaebe60c3d048220855543de50757a0465ebd2f5d45e0794c9b0a617a82a521c04ab420719838e15dff66566cbbee0903e11c740faad4fcc8654a0fe947
@mducroux mducroux requested a review from ninegua December 23, 2025 13:38
Copy link
Member

@ninegua ninegua left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@mducroux mducroux merged commit 3ca9601 into doge-master Dec 23, 2025
24 checks passed
@mducroux mducroux deleted the mducroux/dogecoin-v0.32.7 branch December 23, 2025 15:07
@coveralls
Copy link

coveralls commented Dec 23, 2025

Pull Request Test Coverage Report for Build 20462048804

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 208 of 246 (84.55%) changed or added relevant lines in 10 files are covered.
  • 3 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.5%) to 83.843%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/address/mod.rs 13 14 92.86%
bitcoin/src/bip32.rs 2 3 66.67%
bitcoin/src/blockdata/script/witness_program.rs 5 7 71.43%
bitcoin/src/blockdata/witness.rs 64 66 96.97%
bitcoin/src/crypto/key.rs 10 12 83.33%
bitcoin/src/psbt/mod.rs 88 118 74.58%
Files with Coverage Reduction New Missed Lines %
bitcoin/src/blockdata/witness.rs 1 87.95%
bitcoin/src/crypto/key.rs 1 67.54%
bitcoin/src/psbt/mod.rs 1 84.66%
Totals Coverage Status
Change from base Build 19462406518: 0.5%
Covered Lines: 18687
Relevant Lines: 22288

💛 - Coveralls

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.

10 participants