Fix signing of multi_a and rawtr scripts with wallets that only have corresponding keys

achow101 commented at 0:11 am on October 29, 2022: member

A user reported on stackexchange that they were unable to sign for a multi_a script using a wallet that only had the corresponding keys (i.e. it did not have the multi_a() descriptor). This PR fixes this issue.

Additionally, wallet_taproot.py is modified to test for this scenario by having another wallet in do_test_psbt which contains descriptors that only have the keys involved in the descriptor being tested. wallet_taproot.py was also modified to create new wallets for each test case rather than sharing wallets throughout as the sharing could result in the signing wallet having the keys in a different descriptor and accidentally result in failing to detect a test failure.

The changes to the test also revealed a similar issue with rawtr() descriptors, which has also been fixed by checking if a descriptor can produce a SigningProvider for the Taproot output pubkey.

sign: Fill in taproot pubkey info for all script path sigs

Taproot pubkey info was not being added for multi_a signing. The filling
of this info is moved into the common function CreateTaprootScriptSig so
that any signing of taproot scripts will include the pubkey info.

323890d0d7

psbt: Include output pubkey in additional pubkeys to sign

In addition to the pubkeys in hd_keypaths and tap_bip32_keypaths, also
see if the descriptor can produce a SigningProvider for the output
pubkey.

Also slightly refactors this area to reduce code duplication.

8781a1b6bb

tests: Use new wallets for each test in wallet_taproot.py

To avoid a wallet potentially being able to sign a transaction using
keys from descriptors imported in previous tests, make new wallets for
each test case rather than sharing them.

6efcdf6b7f

tests: Test Taproot PSBT signing with keys in other descriptor

Test that the same keys included in other descriptors will still be able
to sign a PSBT that requires those keys.

0de30ed509

achow101 added the label Wallet on Oct 29, 2022

achow101 added the label Tests on Oct 29, 2022

achow101 added the label PSBT on Oct 29, 2022

achow101 added the label Needs backport (24.x) on Oct 29, 2022

achow101 added this to the milestone 24.0 on Oct 29, 2022

DrahtBot commented at 7:25 am on October 31, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26257 (script, test: python linter fixups and updates by jonatack)
#23502 (wallet: Avoid underpaying transaction fees when signing taproot spends by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake requested review from darosior on Oct 31, 2022

fanquake requested review from Sjors on Oct 31, 2022

fanquake requested review from instagibbs on Nov 3, 2022

instagibbs commented at 2:13 pm on November 3, 2022: member

crACK https://github.com/bitcoin/bitcoin/pull/26418/commits/0de30ed509a9969cb254e00097671625c9e107d2

darosior approved

darosior commented at 9:23 am on November 4, 2022: member

ACK 0de30ed509a9969cb254e00097671625c9e107d2

fanquake merged this on Nov 4, 2022

fanquake closed this on Nov 4, 2022

fanquake referenced this in commit 754eefd21c on Nov 4, 2022

fanquake referenced this in commit 2159676b6e on Nov 4, 2022

fanquake referenced this in commit 0a5ea2aa84 on Nov 4, 2022

fanquake referenced this in commit 6e4d87e696 on Nov 4, 2022

fanquake removed the label Needs backport (24.x) on Nov 4, 2022

fanquake commented at 3:59 pm on November 4, 2022: member

Backported to 24.x in #26452.

sidhujag referenced this in commit fdb5d1b92a on Nov 5, 2022

in src/wallet/scriptpubkeyman.cpp:2514 in 8781a1b6bb outdated

2516+                pubkeys.push_back(pk);
2517+            }
2518+
2519+            // Taproot output pubkey
2520+            std::vector<std::vector<unsigned char>> sols;
2521+            if (Solver(script, sols) == TxoutType::WITNESS_V1_TAPROOT) {

S3RK commented at 8:32 am on November 7, 2022:

q: Do I understand correctly that this part is needed to fix rawtr() descriptors?

achow101 commented at 3:30 pm on November 7, 2022:

Yes, this is for rawtr.

in src/script/sign.cpp:153 in 323890d0d7 outdated

145@@ -146,6 +146,16 @@ static bool CreateSig(const BaseSignatureCreator& creator, SignatureData& sigdat
146 
147 static bool CreateTaprootScriptSig(const BaseSignatureCreator& creator, SignatureData& sigdata, const SigningProvider& provider, std::vector<unsigned char>& sig_out, const XOnlyPubKey& pubkey, const uint256& leaf_hash, SigVersion sigversion)
148 {
149+    KeyOriginInfo info;
150+    if (provider.GetKeyOriginByXOnly(pubkey, info)) {
151+        auto it = sigdata.taproot_misc_pubkeys.find(pubkey);
152+        if (it == sigdata.taproot_misc_pubkeys.end()) {
153+            sigdata.taproot_misc_pubkeys.emplace(pubkey, std::make_pair(std::set<uint256>({leaf_hash}), info));

S3RK commented at 8:37 am on November 7, 2022:

not related to the PR, but I’m confused taproot_misc_pubkeys will be serialised into PSBT_IN_TAP_BIP32_DERIVATION, but according to bip-0174

Keys within each scope should never be duplicated

Can you help me understand how it works?

achow101 commented at 3:31 pm on November 7, 2022:

PSBT_IN_TAP_BIP32_DERIVATION fields have keys that are composed of the type and the pubkey. As each pubkey is different, each key of this type will be different as well so there is no duplication. If a key appears in multiple tapscripts, we take the else path which only updates the set of leaf hashes.

S3RK commented at 8:40 am on November 7, 2022: contributor

Post merge ACK 0de30ed509a9969cb254e00097671625c9e107d2 @achow101 do I understand correctly that 323890d0d7db2628f9dc6eaeba6e99ce0a12e1f5 is enough to fix multi_a and 8781a1b6bbd0af3cfdf1421fd18de5432494619a is needed only to fix rawtr?

also left a few questions for better understanding

fanquake referenced this in commit 2a5d9818ed on Nov 9, 2022

in test/functional/wallet_taproot.py:8 in 0de30ed509

4@@ -5,6 +5,7 @@
5 """Test generation and spending of P2TR addresses."""
6 
7 import random
8+import uuid

Sjors commented at 9:52 am on November 11, 2022:

This triggers a segfault on my Ubuntu 22.10 machine (Python 3.6.15 installed via PyEnv, Kernel 6.0.6). I’m able to work around it:

0CC=clang pyenv install 3.6.15

bitcoin locked this on Nov 11, 2023

Fix signing of multi_a and rawtr scripts with wallets that only have corresponding keys #26418

Conflicts