Skip to content

feat: implement GetKey for KeyMap and DescriptorSecretKey #851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Sep 9, 2025
Merged

feat: implement GetKey for KeyMap and DescriptorSecretKey #851

merged 3 commits into from
Sep 9, 2025

Conversation

oleonardolima
Copy link
Contributor

fixes #709

Description

I've built upon Tobin's work in #765 (see #765 (comment)) to implement the GetKey trait for KeyMap, a feature that will enable us to entirely remove the signers API from bdk_wallet, see: bitcoindevkit/bdk_wallet#70 and bitcoindevkit/bdk_wallet#235.

Notes to the reviewers

I followed some of Poelstra's previous comments to reduce the number of required matching branches, and also some VM's suggestions on implementing the main GetKey logic for DescriptorSecretKey instead.

Let me know if you have any other suggestions or comments.

Changelog notice

# Added:
 - implemented `GetKey` trait for `KeyMap`
 - implemented `GetKey` trait for `DescriptorSecretKey`

# Changed
 - moved `KeyMap` to it's own `key_map` module, replacing the `BTreeMap` alias.

@tcharding
Copy link
Member

Mad, thanks man. I'm off travelling this week but can help review this when I get a chance if needed. I don't do many reviews in miniscript in general.

@apoelstra
Copy link
Member

In 255a61e:

You need to update Cargo-minimal.lock and Cargo-recent.lock to require bitcoin 0.32.6 since the KeyRequest enum was smaller in 0.32.5 and before.

Relatedly, your unreachable! branch is not actually unreachable. It will be hit by any additions to the KeyRequest enum in the future.

@oleonardolima oleonardolima force-pushed the feat/impl-getkey-for-keymap branch 3 times, most recently from acc2d04 to ab69aac Compare August 28, 2025 14:00
@oleonardolima
Copy link
Contributor Author

In 255a61e:

You need to update Cargo-minimal.lock and Cargo-recent.lock to require bitcoin 0.32.6 since the KeyRequest enum was smaller in 0.32.5 and before.

Relatedly, your unreachable! branch is not actually unreachable. It will be hit by any additions to the KeyRequest enum in the future.

Thanks, I've added a new commit 8e05d85 specifically for this version bump.

I also updated unreachable! to unimplemented! instead.

@apoelstra
Copy link
Member

I also updated unreachable! to unimplemented! instead.

This is not sufficient. The code should not panic on key accesses.

ValuedMammal
ValuedMammal reviewed Aug 29, 2025
View reviewed changes
src/descriptor/key_map.rs
Comment on lines +321 to +322
let path = DerivationPath::from_str("84'/1'/0'/0").unwrap();
let expected_pk = xpriv.xkey.derive_priv(&secp, &path).unwrap().to_priv();
Copy link

Choose a reason for hiding this comment

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

Is this the path derived from the master key or from the tpriv shown above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's from the key origin as per the descriptor above. It's curious because I had to port this test from bdk_wallet specifically because AFAICT it's a scenario that the default KeyRequest::Bip32 implementation can't cover.

Copy link

Choose a reason for hiding this comment

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

I'm confused what the expected key is in this case. If path is to be derived from the origin, don't we only want to derive one additional step from the xkey?

oleonardolima and others added 2 commits September 8, 2025 10:52
@oleonardolima
chore(deps): bump bitcoin to 0.32.6
131cf01 
- it's required in order to have all the added variants for `KeyRequest`
  type.
@tcharding @oleonardolima
refactor(keymap): create KeyMap type
6f345d7 
- creates a new `key_map` module, and `KeyMap` type to replace the
  existing `BTreeMap` alias.
- it's a commit extracted from superseded PR rust-bitcoin#765.
@oleonardolima
Copy link
Contributor Author

I also updated unreachable! to unimplemented! instead.

This is not sufficient. The code should not panic on key accesses.

Alright, thanks! I'll follow the standard that it should return None, as it can't find the key and no error is found.

I was wondering if a new error variant should be added, but I don't think that's the case. Please let me know if I should proceed in a other way.

@oleonardolima
feat: impl GetKey for KeyMap and DescriptorSecretKey
e2d934e 
- add the implementation of GetKey for `KeyMap`, and also implements it
  for the inner type `DescriptorSecretKey` in order to make the match
branching and logic clearer.
@oleonardolima oleonardolima force-pushed the feat/impl-getkey-for-keymap branch from ab69aac to e2d934e Compare September 8, 2025 01:03
@apoelstra
Copy link
Member

CI failure is because the stable compiler broke our CI. I'll fix this separately.

apoelstra
apoelstra approved these changes Sep 8, 2025
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK e2d934e; successfully ran local tests

@tcharding
Copy link
Member

You don't need my review on this do you @apoelstra , its a savage context switch.

@apoelstra
Copy link
Member

Nope! Was just fighting with my local CI setup (had to tweak a bunch of stuff for rust-bitcoin 0.31.x) and forgot to queue the merge here.

Thanks for checking though -- I do usually wait for your review on PRs where you've commented.

@apoelstra apoelstra merged commit 946901e into rust-bitcoin:master Sep 9, 2025
59 of 61 checks passed
@oleonardolima
Copy link
Contributor Author

I've been wondering about this one, 13.0.0 is pretty far away from releasing, right ?
I'll try to find another way to backport these changes into 12.x, if that's fine, ofc.

@apoelstra
Copy link
Member

Yeah, 13.0 is a bit of a ways away.

Backports are welcome. Though this one will be a little tricky because you can't replace the BTreeMap alias in a backport.

@tcharding tcharding mentioned this pull request Sep 23, 2025
Closed
@tcharding tcharding added the summit To review at Rust Bitcoin Summit. label Sep 24, 2025
heap-coder added a commit to heap-coder/rust-miniscript that referenced this pull request Sep 27, 2025
@heap-coder
Merge rust-bitcoin/rust-miniscript#851: feat: implement GetKey for …
9a6dcae 
…`KeyMap` and `DescriptorSecretKey`

e2d934eb9530ffac3a26cb3c822dd5ed9bc29a85 feat: impl `GetKey` for `KeyMap` and `DescriptorSecretKey` (Leonardo Lima)
6f345d75b03d49a1e606d161656067a7f8331cf2 refactor(keymap): create `KeyMap` type (Tobin C. Harding)
131cf01f318fa9ba98c03da8ea9bab62ce7d44eb chore(deps): bump `bitcoin` to `0.32.6` (Leonardo Lima)

Pull request description:

  fixes rust-bitcoin/rust-miniscript#709
  
  ### Description
  
  I've built upon Tobin's work in #765 (see rust-bitcoin/rust-miniscript#765 (comment)) to implement the `GetKey` trait for `KeyMap`, a feature that will enable us to entirely remove the signers API from `bdk_wallet`, see: bitcoindevkit/bdk_wallet#70 and bitcoindevkit/bdk_wallet#235.
  
  ### Notes to the reviewers
  
  I followed some of Poelstra's previous comments to reduce the number of required matching branches, and also some VM's suggestions on implementing the main `GetKey` logic for `DescriptorSecretKey` instead.
  
  Let me know if you have any other suggestions or comments. 
  
  ### Changelog notice
  
  ```md
  # Added:
   - implemented `GetKey` trait for `KeyMap`
   - implemented `GetKey` trait for `DescriptorSecretKey`
  
  # Changed
   - moved `KeyMap` to it's own `key_map` module, replacing the `BTreeMap` alias.
  ```


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


Tree-SHA512: 85168f3d6de0527d077674ff4e988ecda9f6237ae1c15018a1cfeb5760fa5a14b700b58a9e78d79b15cffba12b3f1e8de3a5e82c0822a0506aa7fa926bf8a7d8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

+1 more reviewer

@ValuedMammal ValuedMammal ValuedMammal left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
summit To review at Rust Bitcoin Summit.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement GetKey for KeyMap
4 participants
@oleonardolima @tcharding @apoelstra @ValuedMammal