Add XOnlyPublicKey support for PSBT key retrieval and improve Taproot signing #4238

erickcestari · 2025-03-14T13:33:55Z

The bip32_sign_schnorr function was previously only attempting to retrieve private keys using KeyRequest::Bip32, which limited the ability to sign Taproot inputs with key maps that don't support BIP32 derivation paths.

Changes

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

These improvements enable wallet implementations to store keys indexed by either PublicKey or XOnlyPublicKey and successfully sign PSBTs.

Closes #4150

coveralls · 2025-03-14T14:30:47Z

Pull Request Test Coverage Report for Build 13994039737

Details

116 of 134 (86.57%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.5%) to 83.633%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
bitcoin/src/psbt/mod.rs	109	127	85.83%

Totals
Change from base Build 13992036628:	0.5%
Covered Lines:	22264
Relevant Lines:	26621

💛 - Coveralls

Kixunil · 2025-03-14T20:09:46Z

bitcoin/src/psbt/mod.rs

+                sk
+            } else if let Ok(Some(sk)) = k
+                .get_key(&KeyRequest::Pubkey(xonly.public_key(secp256k1::Parity::Odd).into()), secp)
+            {


This is unfortunate. I wouldn't mind this in a backport but how about we modify GetKey trait to also have get_by_x_only?

I'd also prefer to provide HashMap<XOnlyPublicKey, PrivateKey> and do the retrieval by ignoring the parity (because both keys can be computed).

This is unfortunate. I wouldn't mind this in a backport but how about we modify GetKey trait to also have get_by_x_only?

I'd also prefer to provide HashMap<XOnlyPublicKey, PrivateKey> and do the retrieval by ignoring the parity (because both keys can be computed).

What about adding a new KeyRequest variant for XOnlyPubKey? I think this would better follow the established pattern for adding new key retrieval methods:

pub enum KeyRequest { /// Request a private key using the associated public key. Pubkey(PublicKey), /// Request a private key using BIP-32 fingerprint and derivation path. Bip32(KeySource), /// Request a private key using the associated x-only public key. XOnlyPubkey(XOnlyPublicKey), }

This approach seems cleaner than modifying the GetKey trait to add a separate get_by_x_only method, and it maintains consistency with how other key types are handled.

Oh, yes, I forgot that get_key takes an enum (which is somewhat weird and possibly slightly slower than necessary but not really broken).

Still, I think having a map data structure that ignores parity when retrieving keys would make a lot of sense.

Kixunil

Cool, aside from the missing negation bug it looks great!

Kixunil · 2025-03-17T18:53:25Z

bitcoin/src/psbt/mod.rs

+            KeyRequest::XOnlyPubkey(xonly) => Ok(self.get(xonly).cloned()),
+            KeyRequest::Pubkey(pk) => {
+                let xonly = XOnlyPublicKey::from(pk.inner);
+                Ok(self.get(&xonly).cloned())


There's one slight issue: after retrieving the key we need to check the parity of pk and negate the private key if it's Odd.

Kixunil · 2025-03-18T16:56:00Z

bitcoin/src/psbt/mod.rs

+                    };
+
+                    return Ok(Some(negated_priv_key));
+                }


IIRC the parity doesn't matter for x-only keys because it's checked when signing and flipped on-demand. But I like this code conceptually because it may lead to less surprising result if anyone uses it.

Kixunil · 2025-03-18T16:56:27Z

bitcoin/src/psbt/mod.rs

+                }
+
+                let pubkey_odd = PublicKey::new(xonly.public_key(secp256k1::Parity::Odd));
+                if let Some(priv_key) = self.get(&pubkey_odd).cloned() {


.copied would've been a bit nicer but not a big deal.

bitcoin/src/psbt/mod.rs

Kixunil · 2025-03-18T17:03:26Z

bitcoin/src/psbt/mod.rs

+                return Ok(None);
+            },
+            KeyRequest::Pubkey(pk) => {
+                let xonly = XOnlyPublicKey::from(pk.inner);


Sadly, this is still incorrect - if pk has odd parity this will return even parity. We need to do the check here against pk's parity.

Also thinking about it, it'd be nicer to have a private fn make_parity(&self, parity: Parity) -> Self on PrivateKey since it's already used in two places.

Oh, now I think I got it. We need to account for the parity of the public key in the request. When we retrieve a private key from the map using just the x-only part, we must verify if that private key generates a public key with the same parity as the original request. If the parity doesn't match, we need to negate the private key before returning it.

My fault, for the misunderstanding.

Sadly, this is still incorrect - if pk has odd parity this will return even parity. We need to do the check here against pk's parity.

Also thinking about it, it'd be nicer to have a private fn make_parity(&self, parity: Parity) -> Self on PrivateKey since it's already used in two places.

Instead of a make_parity function that requires checking the current parity again, maybe we could just add a simple negate function to PrivateKey? Since we already know when we need to negate the key in our code, this would be more direct and avoid redundant public key generation and parity checks.

Oh, sorry, I forgot parity is implied. I have a horrible day today.

No worries! Sorry to hear you're having a rough day. Hope things improve!

Kixunil

ACK d2f55a6

Kixunil · 2025-03-18T19:57:33Z

bitcoin/src/crypto/key.rs

+    /// but with the opposite y-coordinate parity. This is useful for ensuring compatibility 
+    /// with specific public key formats and BIP-340 requirements.
+    #[inline]
+    pub fn negate(&self) -> Self {


I wanted to make it private but given it doesn't require secp256k1 context, I'm not too worried about making it public.

Kixunil · 2025-03-18T20:10:57Z

bitcoin/src/psbt/mod.rs

+        pubkey_map.insert(pk, priv_key);
+
+        let req_result = pubkey_map.get_key(&KeyRequest::XOnlyPubkey(xonly), &secp).unwrap();
+        assert!(req_result.is_some(), "Should have found a key for odd parity XOnlyPubkey request");


This is not needed since you're unwrapping below.

Kixunil

ACK 3d87885

apoelstra

ACK 3d87885; successfully ran local tests

apoelstra · 2025-03-21T13:37:05Z

bitcoin/src/psbt/mod.rs

+        let mut psbt = Psbt::from_unsigned_tx(tx).unwrap();
+        psbt.inputs[0].tap_internal_key = Some(internal_key);
+        psbt.inputs[0].witness_utxo = Some(transaction::TxOut {
+            value: Amount::from_sat_unchecked(10),


In 3d87885:

Sorry, you need to rebase because of this (the merge commit does not pass my local CI). from_sat_unchecked was removed. You can use from_sat_u32 instead.

apoelstra · 2025-03-21T13:37:14Z

bitcoin/src/psbt/mod.rs

+        let mut psbt = Psbt::from_unsigned_tx(tx).unwrap();
+        psbt.inputs[0].tap_internal_key = Some(internal_key);
+        psbt.inputs[0].witness_utxo = Some(transaction::TxOut {
+            value: Amount::from_sat_unchecked(10),


In 3d87885:

Same here.

… 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

Kixunil

ACK 069d2fd

apoelstra

ACK 069d2fd; successfully ran local tests

jaonoctus · 2025-03-24T20:37:08Z

Well done @erickcestari 🥂

…val 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: - #3356 - #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 #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

github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Mar 14, 2025

Kixunil reviewed Mar 14, 2025

View reviewed changes

erickcestari force-pushed the hashmap-sign-taproot-psbt branch from e36f06c to 9193b00 Compare March 17, 2025 13:01

erickcestari changed the title ~~Fix HashMap<PublicKey, PrivateKey> signing with Taproot inputs~~ Add XOnlyPublicKey support for PSBT key retrieval and improve Taproot signing Mar 17, 2025

erickcestari requested a review from Kixunil March 17, 2025 14:27

Kixunil requested changes Mar 17, 2025

View reviewed changes

erickcestari force-pushed the hashmap-sign-taproot-psbt branch 2 times, most recently from 6ac1002 to 4f28d1c Compare March 18, 2025 15:36

Kixunil requested changes Mar 18, 2025

View reviewed changes

erickcestari force-pushed the hashmap-sign-taproot-psbt branch 2 times, most recently from 05c2f45 to d2f55a6 Compare March 18, 2025 18:10

erickcestari requested a review from Kixunil March 18, 2025 18:10

Kixunil previously approved these changes Mar 18, 2025

View reviewed changes

erickcestari dismissed Kixunil’s stale review via 3d87885 March 18, 2025 23:43

erickcestari force-pushed the hashmap-sign-taproot-psbt branch from d2f55a6 to 3d87885 Compare March 18, 2025 23:43

Kixunil previously approved these changes Mar 19, 2025

View reviewed changes

apoelstra previously approved these changes Mar 20, 2025

View reviewed changes

apoelstra reviewed Mar 21, 2025

View reviewed changes

erickcestari dismissed stale reviews from apoelstra and Kixunil via 069d2fd March 21, 2025 14:29

erickcestari force-pushed the hashmap-sign-taproot-psbt branch from 3d87885 to 069d2fd Compare March 21, 2025 14:29

Kixunil approved these changes Mar 21, 2025

View reviewed changes

apoelstra approved these changes Mar 21, 2025

View reviewed changes

apoelstra merged commit 8788995 into rust-bitcoin:master Mar 24, 2025
24 checks passed

apoelstra mentioned this pull request May 3, 2025

[backport 0.32.x] bip32: return error when attempting to derive past maximum depth #4434

Merged

tcharding mentioned this pull request May 5, 2025

backport: Add XOnlyPublicKey support for PSBT key retrieval and improve Taproot signing #4443

Merged

tcharding mentioned this pull request May 5, 2025

Tag a new release 0.32.6 #4444

Closed

This was referenced May 14, 2025

chore(deps): bump bitcoin to 0.32.6 alpenlabs/alpen#804

Merged

chore(deps): bump bitcoin to 0.32.6 alpenlabs/strata-bridge#144

Merged

tcharding mentioned this pull request Jun 5, 2025

Implement GetKey for KeyMap rust-bitcoin/rust-miniscript#765

Open

Add XOnlyPublicKey support for PSBT key retrieval and improve Taproot signing #4238

Add XOnlyPublicKey support for PSBT key retrieval and improve Taproot signing #4238

Uh oh!

Conversation

erickcestari commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

coveralls commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 13994039737

Details

💛 - Coveralls

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!

Kixunil left a comment

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!

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!

Kixunil left a comment

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!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

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!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaonoctus commented Mar 24, 2025

Uh oh!

Uh oh!

erickcestari commented Mar 14, 2025 •

edited

Loading

coveralls commented Mar 14, 2025 •

edited

Loading