Skip to content

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

Merged
laanwj merged 1 commit intobitcoin:masterfrom
achow101:disallow-tr-privkey-import
Jun 12, 2021
Merged

Allow tr() import only when Taproot is active#22156
laanwj merged 1 commit intobitcoin:masterfrom
achow101:disallow-tr-privkey-import

Conversation

@achow101
Copy link
Member

@achow101 achow101 commented Jun 5, 2021 •
edited
Loading

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 5, 2021 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

This was referenced Jun 5, 2021
Merged
Closed
@instagibbs
Copy link
Member

instagibbs commented Jun 6, 2021

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 achow101 changed the title Only allow tr() import to privkey wallets when Taproot is active Allow tr() import to privkey wallets only when Taproot is active Jun 6, 2021
@meshcollider meshcollider added this to the 22.0 milestone Jun 9, 2021
@sipa
Copy link
Member

sipa commented Jun 10, 2021

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

@achow101
Allow tr() import only when Taproot is active
fbf485c 
To avoid issues around fund loss, only allow descriptor wallets
to import tr() descriptors after taproot has activated.
@achow101 achow101 force-pushed the disallow-tr-privkey-import branch from c3875fa to fbf485c Compare June 10, 2021 19:45
@achow101
Copy link
Member Author

achow101 commented Jun 10, 2021 •
edited
Loading

Updated to disallow import into watch only wallets too.

@achow101 achow101 changed the title Allow tr() import to privkey wallets only when Taproot is active Allow tr() import only when Taproot is active Jun 10, 2021
@michaelfolkson
Copy link

michaelfolkson commented Jun 10, 2021

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
Copy link
Member Author

achow101 commented Jun 10, 2021

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
Copy link
Member

sipa commented Jun 10, 2021

utACK fbf485c

@fjahr
Copy link
Contributor

fjahr commented Jun 10, 2021

Code review ACK fbf485c

ghost
ghost reviewed Jun 10, 2021
View reviewed changes
test/functional/wallet_taproot.py
Comment on lines -175 to +177
self.num_nodes = 2
self.num_nodes = 3
self.setup_clean_chain = True
self.extra_args = [['-keypool=100'], ['-keypool=100']]
self.extra_args = [['-keypool=100'], ['-keypool=100'], ["-vbparams=taproot:1:1"]]
Copy link

@ghost ghost Jun 10, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@achow101 achow101 Jun 10, 2021

Choose a reason for hiding this comment

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

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

ghost
ghost approved these changes Jun 10, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

utACK fbf485c

@ghost
Copy link

ghost commented Jun 12, 2021

  1. Compiled successfully on Fedora 34.
  2. Created a descriptor wallet: bitcoin-cli createwallet "D1" false false "" true true
  3. Tried importing a descriptor and got the error as expected:
$ bitcoin-cli -rpcwallet=D1 importdescriptors '[{"desc" : "tr(c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5,{pk(fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a1460297556),pk(e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd13)})#2rqrdjrh", "timestamp" : "now"}]'
[
  {
    "success": false,
    "error": {
      "code": -4,
      "message": "Cannot import tr() descriptor when Taproot is not active"
    }
  }
]

Few questions

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

@laanwj
Copy link
Member

laanwj commented Jun 12, 2021

Code review ACK fbf485c

@achow101
Copy link
Member Author

achow101 commented Jun 12, 2021

  1. 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.

  1. Which other RPCs support importing keys using descriptors?

Just importmulti and importdescriptors.

@laanwj laanwj merged commit b0e5fbf into bitcoin:master Jun 12, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 13, 2021
@laanwj
Merge bitcoin#22156: Allow tr() import only when Taproot is active
670372a 
fbf485c Allow tr() import only when Taproot is active (Andrew Chow)

Pull request description:

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

ACKs for top commit:
  sipa:
    utACK fbf485c
  fjahr:
    Code review ACK fbf485c
  laanwj:
    Code review ACK fbf485c
  prayank23:
    utACK bitcoin@fbf485c

Tree-SHA512: 83c43376515eea523dbc89bc5a0fde53e54aec492e49a40c2a33d80fc94aac459e232ae07b024b4bd75b58078c8d090bc7a2d69541c5d3d4834d2f4cfc9c8208
@Sjors
Copy link
Member

Sjors commented Jun 15, 2021

Post merge concept ACK.

@ryanofsky ryanofsky mentioned this pull request Jun 15, 2021
Draft
@Sjors Sjors mentioned this pull request Jun 16, 2021
Merged
@ajtowns ajtowns mentioned this pull request Jun 28, 2021
Merged
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge b0e5fbf into merged_master (Bitcoin PR bitcoin/bitcoin#22156)
d4d5a35
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

interfaces RPC/REST/ZMQ Wallet

Projects

None yet

Milestone

22.0

Development

Successfully merging this pull request may close these issues.

9 participants

@achow101 @DrahtBot @instagibbs @sipa @michaelfolkson @fjahr @laanwj @Sjors @meshcollider