Skip to content

Conversation

@chaitika
Copy link
Contributor

@chaitika chaitika commented Oct 5, 2025

Description
Updating signer implemention for XOnlyPublicKey key request

Fixes #19

All Submissions:

  • I've signed all my commits
  • I followed the conventional commit guidelines
  • I ran cargo +nigthly fmt before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

@ValuedMammal
Copy link
Collaborator

I suggest we do the following:

  1. Use an if let ... to return the PrivateKey only if we found it in the current map entry.
  2. Fix bug introduced by an earlier commit 94089eb that confused xkey.depth with the origin path length.
--- a/src/signer.rs
+++ b/src/signer.rs
@@ -1,6 +1,7 @@
 use alloc::string::ToString;
+use alloc::vec::Vec;
 use std::collections::BTreeMap;
 
 use bitcoin::{
     psbt::{GetKey, GetKeyError, KeyRequest},
     secp256k1::{self, Secp256k1},
@@ -25,11 +26,13 @@ impl GetKey for Signer {
         for entry in &self.0 {
             match entry {
                 (_, DescriptorSecretKey::Single(prv)) => {
                     let map: BTreeMap<_, _> =
                         core::iter::once((prv.key.public_key(secp), prv.key)).collect();
-                    return GetKey::get_key(&map, key_request, secp);
+                    if let Ok(Some(prv)) = GetKey::get_key(&map, key_request.clone(), secp) {
+                        return Ok(Some(prv));
+                    }
                 }
                 (_, desc_sk) => {
                     for desc_sk in desc_sk.clone().into_single_keys() {
                         if let KeyRequest::Bip32((fingerprint, derivation)) = &key_request {
                             if let DescriptorSecretKey::XPrv(k) = desc_sk {
@@ -42,11 +45,15 @@ impl GetKey for Signer {
                                 // The key origin is a strict prefix of the request derivation
                                 if let Some((fp, path)) = &k.origin {
                                     if fingerprint == fp
                                         && derivation.to_string().starts_with(&path.to_string())
                                     {
-                                        let to_derive = &derivation[k.xkey.depth as usize..];
+                                        let to_derive = derivation
+                                            .into_iter()
+                                            .copied()
+                                            .skip(path.len())
+                                            .collect::<Vec<_>>();
                                         let derived = k.xkey.derive_priv(secp, &to_derive)?;
                                         return Ok(Some(derived.to_priv()));
                                     }
                                 }
                             }

@ValuedMammal
Copy link
Collaborator

Here's another test

#[test]
fn get_key_xpriv_with_key_origin() -> anyhow::Result<()> {
    let secp = Secp256k1::new();
    let s = "wpkh([d34db33f/84h/1h/0h]tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)";
    let (_, keymap) = Descriptor::parse_descriptor(&secp, s)?;

    let desc_sk = DescriptorSecretKey::from_str("[d34db33f/84h/1h/0h]tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*")?;
    let desc_xkey = match desc_sk {
        DescriptorSecretKey::XPrv(k) => k,
        _ => panic!(),
    };

    let (fp, _) = desc_xkey.origin.clone().unwrap();
    let path = DerivationPath::from_str("84h/1h/0h/7")?;
    let req = KeyRequest::Bip32((fp, path));

    let exp_prv = desc_xkey
        .xkey
        .derive_priv(&secp, &[ChildNumber::from(7)])?
        .to_priv();

    let res = Signer(keymap).get_key(req, &secp);

    assert!(matches!(
        res,
        Ok(Some(k)) if k == exp_prv,
    ));

    Ok(())
}

@ValuedMammal
Copy link
Collaborator

I tested using BDK test descriptors.

wpkh([e273fe42/84'/1'/0']tprv8g5otvsaxi79T7WyYCAucSgYsa9tcrM2mdZzC5NjcmsRJbgxZtmFsWBjysVerFe3DvKT5tLbJYUssERJUP5miQEtSwV7mcKC263Aikiw8mN/0/*)
wpkh([e273fe42/84'/1'/0']tprv8g5otvsaxi79T7WyYCAucSgYsa9tcrM2mdZzC5NjcmsRJbgxZtmFsWBjysVerFe3DvKT5tLbJYUssERJUP5miQEtSwV7mcKC263Aikiw8mN/1/*)

https://mempool.space/testnet4/tx/d93eb295818b70ca75569169fd188ef3c2ffdf386cdddf4652ae30626b9a0664

@chaitika chaitika force-pushed the feat-update-signer-for-xonlypubkey branch from 23d8cfa to fe83b69 Compare October 6, 2025 13:59
Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK fe83b69

@ValuedMammal ValuedMammal merged commit ac547ce into bitcoindevkit:master Oct 7, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants