feat(descriptor): backport `GetKey` impl from #851 #860

oleonardolima · 2025-09-29T05:37:43Z

backports #851 to release/12.x

Description

It's an initial attempt to backport the implementation of GetKey for KeyMap introduced in #851.

Notes to the reviewers

I noticed that the type KeyMapWrapper is already being used internally for parse_descriptor, though, as it's not part of the public API, there's no conflict, but I'm fine with changing it to another name if needed.

Changelog Notice

# Added
- add `KeyMapWrapper` with `GetKey` trait for PSBT signing
- implement `GetKey` for `DescriptorSecretKey`
- add `From<KeyMap>` conversion
- export new `KeyMapWrapper` for downstream crate usage

- it's required in order to have all the added variants for `KeyRequest` type.

- add `KeyMapWrapper` with `GetKey` trait for PSBT signing - implement `GetKey` for `DescriptorSecretKey` - add `From<KeyMap>` conversion - export new `KeyMapWrapper` for downstream crate usage

apoelstra · 2025-09-29T17:27:53Z

In c7d4171:

Wait, why can't we directly implement GetKey on KeyMap without the wrapper? it's our type and our trati.

oleonardolima · 2025-09-29T23:57:43Z

In c7d4171:

Wait, why can't we directly implement GetKey on KeyMap without the wrapper? it's our type and our trati.

As it's a type alias for BTreeMap and GetKey is defined in rust-bitcoin::psbt we can't implement it directly.

apoelstra · 2025-09-30T13:41:34Z

Ohh, derp. Yes, I forgot that KeyMap is a type alias, not one of our types. (And that GetKey is from rust-bitcoin, which always throws me off and I wonder if we can revisit it..)

apoelstra · 2025-09-30T14:05:34Z

src/descriptor/key_map.rs

+            .find_map(|(_desc_pk, desc_sk)| -> Option<PrivateKey> {
+                match desc_sk.get_key(key_request.clone(), secp) {
+                    Ok(Some(pk)) => Some(pk),
+                    Ok(None) | Err(_) => None,


In c7d4171:

Here (and below) you swallow the error from DescriptorSecretKey::get_key and turn it into a "key not found" by returning None.

This is an issue with the code on master. I will open a fix PR and then I'd like you to also backport that.

Actually, now I'm unsure. We did not discuss this at all in #851 but I think your original code was correct. These errors occur if you try to get an x-only key from an xpriv (even this I'm unsure of, now..). But when doing a lookup in a map where we may have a combination of xprivs and bare secret keys, then individual errors aren't actually errors. They're just "key not found".

Lemme PR some unit tests and I'll just ask you to backport those :P.

Thanks, I've cherry-picked the commits and updated the required types for KeyMapWrapper.

Claude 4 wrote the original tests; I then replaced its fix (which incorrectly also propagated errors in GetKey for KeyMap), updated the tests, and removed some extra tests which seemed uninformative and just noisy. To backport this to 12.x, just do s/KeyMap/KeyMapWrapper/ on the new tests. Oh, and add some `let keymap = KeyMapWrapper::from(keymap)` lines. Co-authored-by: aider (anthropic/claude-sonnet-4-20250514) <aider@aider.chat>

oleonardolima · 2025-10-01T04:56:24Z

Also, I've discussed with Tobin, and should I go ahead and add a commit already deprecating the type, or should it be done when the v13.0.0 release is closer?

tcharding · 2025-10-02T00:11:06Z

Yeah I suggested deprecating

In c7d4171:
Wait, why can't we directly implement GetKey on KeyMap without the wrapper? it's our type and our trati.

As it's a type alias for BTreeMap and GetKey is defined in rust-bitcoin::psbt we can't implement it directly.

I'm late to the party but on master KeyMap is a struct? What am I missing? The docs are wrong too

EDIT: derp, its an alias on this branch - woops.

oleonardolima · 2025-10-02T00:14:25Z

Wait, why can't we directly implement GetKey on KeyMap without the wrapper? it's our type and our trati.

As it's a type alias for BTreeMap and GetKey is defined in rust-bitcoin::psbt we can't implement it directly.

I'm late to the party but on master KeyMap is a struct? What am I missing? The docs are wrong too
/// Alias type for a map of public key to secret key.
///
/// This map is returned whenever a descriptor that contains secrets is parsed using
/// [`Descriptor::parse_descriptor`], since the descriptor will always only contain
/// public keys. This map allows looking up the corresponding secret key given a
/// public key from the descriptor.
#[derive(Debug, Clone, Eq, PartialEq)]
pub struct KeyMap {
    map: BTreeMap<DescriptorPublicKey, DescriptorSecretKey>,
}

Yes, on master it's already the correct type because of #851. But in release/12.x it's still the old type alias. Indeed, the docs are wrong, will create a commit to fix it.

tcharding · 2025-10-02T00:15:30Z

Is the next release 12.3.6 or 12.4.0? I ask because we can't bump bitcoin point version since its pre-1.0 in a point release here, right? (Since point version pre-1.0 is a minor release.)

tcharding · 2025-10-02T00:17:34Z

Also, I've discussed with Tobin, and should I go ahead and add a commit already deprecating the type, or should it be done when the v13.0.0 release is closer?

Yeah I suggested deprecating the KeyMapWrapper type with a note "will be removed in the next major release" because its not needed once master is released (assuming next release of master is a major release).

tcharding · 2025-10-02T00:18:31Z

FWIW everything else looks fine to me, I did not review closely that the backport patch mirrors the original.

oleonardolima · 2025-10-02T00:58:51Z

[...]

Yes, on master it's already the correct type because of #851. But in release/12.x it's still the old type alias. Indeed, the docs are wrong, will create a commit to fix it.

I opened #862

oleonardolima · 2025-10-02T00:59:52Z

Is the next release 12.3.6 or 12.4.0? I ask because we can't bump bitcoin point version since its pre-1.0 in a point release here, right? (Since point version pre-1.0 is a minor release.)

I think we would need 12.3.6 then, but I'm not sure what @apoelstra had in mind, so I'll leave it to him to answer.

apoelstra · 2025-10-02T17:11:59Z

Interesting question. Yeah, let's do 12.4.0. I think that's the right thing to do anyway since we are adding functionality.

apoelstra

ACK 08fa87a; successfully ran local tests

oleonardolima force-pushed the backport-851-getkey-to-12.x branch 2 times, most recently from 61512c1 to 9d752e5 Compare September 29, 2025 05:40

oleonardolima marked this pull request as draft September 29, 2025 05:42

chore(deps): bump bitcoin to 0.32.6

58d1df9

- it's required in order to have all the added variants for `KeyRequest` type.

oleonardolima force-pushed the backport-851-getkey-to-12.x branch from 9d752e5 to bfeae8e Compare September 29, 2025 05:56

feat(descriptor): backport GetKey impl from rust-bitcoin#851

c7d4171

- add `KeyMapWrapper` with `GetKey` trait for PSBT signing - implement `GetKey` for `DescriptorSecretKey` - add `From<KeyMap>` conversion - export new `KeyMapWrapper` for downstream crate usage

oleonardolima force-pushed the backport-851-getkey-to-12.x branch from bfeae8e to c7d4171 Compare September 29, 2025 06:04

apoelstra reviewed Sep 30, 2025

View reviewed changes

apoelstra mentioned this pull request Sep 30, 2025

key_map.rs: propagate "invalid key" errors from multipath keys #861

Merged

apoelstra and others added 2 commits October 1, 2025 14:51

key_map: handle lookup errors on multipath xprivs correctly

5f089aa

oleonardolima marked this pull request as ready for review October 1, 2025 04:58

apoelstra approved these changes Oct 2, 2025

View reviewed changes

feat(descriptor): backport GetKey impl from #851 #860

Are you sure you want to change the base?

feat(descriptor): backport GetKey impl from #851 #860

Uh oh!

Conversation

oleonardolima commented Sep 29, 2025

Description

Notes to the reviewers

Changelog Notice

Uh oh!

apoelstra commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oleonardolima commented Sep 29, 2025

Uh oh!

apoelstra commented Sep 30, 2025

Uh oh!

apoelstra Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

oleonardolima Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

oleonardolima commented Oct 1, 2025

Uh oh!

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

Uh oh!

Uh oh!

oleonardolima commented Oct 2, 2025

Uh oh!

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

Uh oh!

Uh oh!

tcharding commented Oct 2, 2025

Uh oh!

tcharding commented Oct 2, 2025

Uh oh!

oleonardolima commented Oct 2, 2025

Uh oh!

oleonardolima commented Oct 2, 2025

Uh oh!

apoelstra commented Oct 2, 2025

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feat(descriptor): backport `GetKey` impl from #851 #860

feat(descriptor): backport `GetKey` impl from #851 #860

apoelstra commented Sep 29, 2025 •

edited

Loading

tcharding commented Oct 2, 2025 •

edited

Loading

tcharding commented Oct 2, 2025 •

edited

Loading