Allow tr() import only when Taproot is active #22156

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:disallow-tr-privkey-import changing 4 files +45 −2

achow101 commented at 1:33 am on June 5, 2021: member

To avoid issues around fund loss, only allow descriptor wallets to import tr() descriptors after taproot has activated.
DrahtBot added the label interfaces on Jun 5, 2021
DrahtBot added the label RPC/REST/ZMQ on Jun 5, 2021
DrahtBot added the label Wallet on Jun 5, 2021
DrahtBot commented at 5:23 am on June 5, 2021: member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:
- #21365 (Basic Taproot signing support for descriptor wallets by sipa)
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.
instagibbs commented at 9:13 am on June 6, 2021: member

slight reword suggestion because I was confused thinking you’d only allow tr() descriptors and not wsh() or anything else

“Allow tr() import to privkey wallets only when Taproot is active”
achow101 renamed this:
~~Only allow tr() import to privkey wallets when Taproot is active~~
Allow tr() import to privkey wallets only when Taproot is active
on Jun 6, 2021
meshcollider added this to the milestone 22.0 on Jun 9, 2021
sipa commented at 7:06 pm on June 10, 2021: member

Hmm, any reason to restrict this protection to private key wallets? A watch-only taproot wallet giving out addresses is just as dangerous.

Allow tr() import only when Taproot is active

To avoid issues around fund loss, only allow descriptor wallets
to import tr() descriptors after taproot has activated.

fbf485c9b2

achow101 force-pushed on Jun 10, 2021
achow101 commented at 7:46 pm on June 10, 2021: member

Updated to disallow import into watch only wallets too.
achow101 renamed this:
~~Allow tr() import to privkey wallets only when Taproot is active~~
Allow tr() import only when Taproot is active
on Jun 10, 2021
michaelfolkson commented at 7:49 pm on June 10, 2021: contributor

Concept ACK. This appears to be at least one wallet restriction that should be in place prior to Taproot being active (I haven’t figured out if there are potentially others)

This PR doesn’t allow tr() import to Signet wallets either right? Ideally no restrictions would be applied to the Signet wallet.
achow101 commented at 7:51 pm on June 10, 2021: member

This PR doesn’t allow tr() import to Signet wallets either right? Ideally no restrictions would be applied to the Signet wallet.

tr() can be imported into signet wallets as taproot is activated on signet. This is not a blanket disallow, it checks for activation.
sipa commented at 7:53 pm on June 10, 2021: member

utACK fbf485c9b2bf1d056bfea77345a15cf56a9cd786
fjahr commented at 9:28 pm on June 10, 2021: member

Code review ACK fbf485c9b2bf1d056bfea77345a15cf56a9cd786

in test/functional/wallet_taproot.py:177 in fbf485c9b2

171@@ -172,9 +172,9 @@ class WalletTaprootTest(BitcoinTestFramework):
172     """Test generation and spending of P2TR address outputs."""
173 
174     def set_test_params(self):
175-        self.num_nodes = 2
176+        self.num_nodes = 3
177         self.setup_clean_chain = True
178-        self.extra_args = [['-keypool=100'], ['-keypool=100']]
179+        self.extra_args = [['-keypool=100'], ['-keypool=100'], ["-vbparams=taproot:1:1"]]

unknown commented at 10:28 pm on June 10, 2021:

nit: self.nodes[1] is not used in this test so can we keep 2 nodes and change self.extra_args for one?

achow101 commented at 10:30 pm on June 10, 2021:

Node 1 will be used in #21365. I’d like to avoid conflicts with that as much as possible.

unknown approved
unknown commented at 10:28 pm on June 10, 2021: none

utACK https://github.com/bitcoin/bitcoin/pull/22156/commits/fbf485c9b2bf1d056bfea77345a15cf56a9cd786

ghost commented at 8:15 am on June 12, 2021: none

Compiled successfully on Fedora 34.
Created a descriptor wallet: bitcoin-cli createwallet "D1" false false "" true true
Tried importing a descriptor and got the error as expected:

0$ bitcoin-cli -rpcwallet=D1 importdescriptors '[{"desc" : "tr(c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5,{pk(fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a1460297556),pk(e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd13)})#2rqrdjrh", "timestamp" : "now"}]'
1[
2  {
3    "success": false,
4    "error": {
5      "code": -4,
6      "message": "Cannot import tr() descriptor when Taproot is not active"
7    }
8  }
9]

Few questions

Why is it only for descriptor wallets? importmulti can be used in legacy wallets to import using descriptors
Which other RPCs support importing keys using descriptors?

laanwj commented at 12:26 pm on June 12, 2021: member

Code review ACK fbf485c9b2bf1d056bfea77345a15cf56a9cd786
achow101 commented at 2:27 pm on June 12, 2021: member
Why is it only for descriptor wallets? importmulti can be used in legacy wallets to import using descriptors
#22154 implements generic blocking of bech32m addresses (so tr() descriptors and any future segwit versions) in legacy wallets. For legacy wallets, importing bech32m addresses is completely disallowed.
Which other RPCs support importing keys using descriptors?
Just importmulti and importdescriptors.
laanwj merged this on Jun 12, 2021
laanwj closed this on Jun 12, 2021
sidhujag referenced this in commit 670372a23a on Jun 13, 2021
Sjors commented at 4:12 pm on June 15, 2021: member

Post merge concept ACK.
gwillen referenced this in commit d4d5a35e7a on Jun 1, 2022
DrahtBot locked this on Aug 16, 2022

Contributors
achow101 DrahtBot instagibbs sipa michaelfolkson fjahr ghost laanwj Sjors

Labels
Wallet RPC/REST/ZMQ interfaces

Milestone
22.0

Allow tr() import only when Taproot is active #22156

Conflicts