Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic Taproot derivation support for descriptors #22051

Merged
merged 9 commits into from
Jun 3, 2021

Conversation

sipa
Copy link
Member

@sipa sipa commented May 24, 2021

This is a subset of #21365, to aide review.

This adds support tr(KEY) or tr(KEY,SCRIPT) or tr(KEY,{{S1,{{S2,S3},...}},...}) descriptors, describing Taproot outputs with specified internal key, and optionally any number of scripts, in nested groups of 2 inside {/} if there are more than one. While it permits importing tr(KEY), anything beyond that is just laying foundations for more features later.

Missing:

  • Signing support (see Basic Taproot signing support for descriptor wallets #21365)
  • Support for more interesting scripts inside the tree (only pk(KEY) is supported for now). In particular, a multisig policy based on the new OP_CHECKSIGADD opcode would be very useful.
  • Inferring tr() descriptors from outputs (given sufficient information).
  • getaddressinfo support.
  • MuSig support. Standardizing that is still an ongoing effort, and is generally kind of useless without corresponding PSBT support.
  • Convenient ways of constructing descriptors without spendable internal key (especially ones that arent't trivially recognizable as such).

sipa added 9 commits May 24, 2021 12:14
This is just a small simplification to prepare for the follow-up instruction
of a CTxDestination variant for taproot outputs.

In the old code, WITNESS_V1_TAPROOT and WITNESS_UNKNOWN both produced
{version, program} as Solver() output. Change this so that WITNESS_V1_TAPROOT
produces just {program}, like WITNESS_V0_* do.
That results in a much safer interface (making the tweak commit
to the key implicitly using a fixed tag means it can't be used for
unrelated tweaking).
This class functions as a utility for building taproot outputs, from
internal key and script leaves.
This adds a new descriptor with syntax e.g. tr(KEY,{S1,{{S2,S3},S4})
where KEY is a key expression for the internal key and S_i are
script expression for the leaves. They have to be organized in
nested {A,B} groups, with exactly two elements.

tr() only exists at the top level, and inside the script expressions
only pk() scripts are allowed for now.
@Sjors
Copy link
Member

Sjors commented May 25, 2021

utACK 2667366 (based on #21365 (comment) review, plus the new functional test)

Copy link
Contributor

@lsilva01 lsilva01 left a comment

Choose a reason for hiding this comment

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

Tested ACK 2667366

Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

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

This is so exciting 🎉

utACK 2667366

Will test

@achow101
Copy link
Member

achow101 commented Jun 2, 2021

Code Review ACK 2667366

@laanwj laanwj merged commit c7dd9ff into bitcoin:master Jun 3, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 3, 2021
Copy link
Contributor

@fjahr fjahr left a comment

Choose a reason for hiding this comment

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

utACK 2667366

std::holds_alternative<WitnessUnknown>(dest)) {
return OutputType::BECH32;
}
return std::nullopt;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could add a comment that this is for CNoDestination.

@@ -3737,6 +3737,7 @@ class DescribeWalletAddressVisitor
return obj;
}

UniValue operator()(const WitnessV1Taproot& id) const { return UniValue(UniValue::VOBJ); }
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: param seems to be named tap everywhere else

Comment on lines +190 to +201
@staticmethod
def rand_keys(n):
ret = []
idxes = set()
for _ in range(n):
while True:
i = random.randrange(len(KEYS))
if not i in idxes:
break
idxes.add(i)
ret.append(KEYS[i])
return ret
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be replaced with set(random.sample(KEYS, k=n))

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you're right.

meshcollider added a commit that referenced this pull request Jun 17, 2021
458a345 Add support for SIGHASH_DEFAULT in RPCs, and make it default (Pieter Wuille)
c0f0c8e tests: check spending of P2TR (Pieter Wuille)
a238012 Basic Taproot signing logic in script/sign.cpp (Pieter Wuille)
49487bc Make GetInputUTXO safer: verify non-witness UTXO match (Pieter Wuille)
fd3f689 Construct and use PrecomputedTransactionData in PSBT signing (Pieter Wuille)
5cb6502 Construct and use PrecomputedTransactionData in SignTransaction (Pieter Wuille)
5d2e224 Don't nuke witness data when signing fails (Pieter Wuille)
ce93531 Permit full precomputation in PrecomputedTransactionData (Pieter Wuille)
e841fb5 Add precomputed txdata support to MutableTransactionSignatureCreator (Pieter Wuille)
a91d532 Add CKey::SignSchnorr function for BIP 340/341 signing (Pieter Wuille)
e77a283 Use HandleMissingData also in CheckSchnorrSignature (Pieter Wuille)
dbb0ce9 Add TaprootSpendData data structure, equivalent to script map for P2[W]SH (Pieter Wuille)

Pull request description:

  Builds on top of #22051, adding signing support after derivation support.

  Nothing is changed in descriptor features. Signing works for key path and script path spending, through the normal sending functions, and PSBT-based RPCs. However, PSBT usability is rather low as no extensions have been defined to convey Taproot-specific information, so all script information must be known to the signing wallet.

ACKs for top commit:
  achow101:
    re-ACK 458a345
  fjahr:
    Code review ACK 458a345
  Sjors:
    ACK 458a345

Tree-SHA512: 30ed212cf7754763a4a81624ebc084c51727b8322711ac0b390369213c1a891d367ed8b123882ac08c99595320c11ec57ee42304ff22a69afdc3d1a0d55cc711
apoelstra pushed a commit to apoelstra/elements that referenced this pull request Aug 19, 2021
apoelstra added a commit to apoelstra/elements that referenced this pull request Aug 19, 2021
That results in a much safer interface (making the tweak commit
to the key implicitly using a fixed tag means it can't be used for
unrelated tweaking).

bitcoin/bitcoin#22051 (5/9)
We actually preserve the "unrelated tweaking" method so we can
use it in OP_TWEAKVERIFY
apoelstra added a commit to apoelstra/elements that referenced this pull request Aug 19, 2021
bitcoin/bitcoin#22051 (6/9)

Modified to use the old-style Optional rather than std::optional
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 19, 2021
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 25, 2021
bitcoin/bitcoin#22051 (6/9)

Modified to use the old-style Optional rather than std::optional
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 25, 2021
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 25, 2021
That results in a much safer interface (making the tweak commit
to the key implicitly using a fixed tag means it can't be used for
unrelated tweaking).

bitcoin/bitcoin#22051 (5/9)
We actually preserve the "unrelated tweaking" method so we can
use it in OP_TWEAKVERIFY
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 25, 2021
bitcoin/bitcoin#22051 (6/9)

Modified to use the old-style Optional rather than std::optional
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 25, 2021
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 25, 2021
That results in a much safer interface (making the tweak commit
to the key implicitly using a fixed tag means it can't be used for
unrelated tweaking).

bitcoin/bitcoin#22051 (5/9)
We actually preserve the "unrelated tweaking" method so we can
use it in OP_TWEAKVERIFY
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 25, 2021
bitcoin/bitcoin#22051 (6/9)

Modified to use the old-style Optional rather than std::optional
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 25, 2021
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 25, 2021
That results in a much safer interface (making the tweak commit
to the key implicitly using a fixed tag means it can't be used for
unrelated tweaking).

bitcoin/bitcoin#22051 (5/9)
We actually preserve the "unrelated tweaking" method so we can
use it in OP_TWEAKVERIFY
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 25, 2021
bitcoin/bitcoin#22051 (6/9)

Modified to use the old-style Optional rather than std::optional
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
That results in a much safer interface (making the tweak commit
to the key implicitly using a fixed tag means it can't be used for
unrelated tweaking).

bitcoin/bitcoin#22051 (5/9)
We actually preserve the "unrelated tweaking" method so we can
use it in OP_TWEAKVERIFY
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
bitcoin/bitcoin#22051 (6/9)

Modified to use the old-style Optional rather than std::optional
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
That results in a much safer interface (making the tweak commit
to the key implicitly using a fixed tag means it can't be used for
unrelated tweaking).

bitcoin/bitcoin#22051 (5/9)
We actually preserve the "unrelated tweaking" method so we can
use it in OP_TWEAKVERIFY
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
bitcoin/bitcoin#22051 (6/9)

Modified to use the old-style Optional rather than std::optional
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
That results in a much safer interface (making the tweak commit
to the key implicitly using a fixed tag means it can't be used for
unrelated tweaking).

bitcoin/bitcoin#22051 (5/9)
We actually preserve the "unrelated tweaking" method so we can
use it in OP_TWEAKVERIFY
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
bitcoin/bitcoin#22051 (6/9)

Modified to use the old-style Optional rather than std::optional
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
That results in a much safer interface (making the tweak commit
to the key implicitly using a fixed tag means it can't be used for
unrelated tweaking).

bitcoin/bitcoin#22051 (5/9)
We actually preserve the "unrelated tweaking" method so we can
use it in OP_TWEAKVERIFY
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
bitcoin/bitcoin#22051 (6/9)

Modified to use the old-style Optional rather than std::optional
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
That results in a much safer interface (making the tweak commit
to the key implicitly using a fixed tag means it can't be used for
unrelated tweaking).

bitcoin/bitcoin#22051 (5/9)
We actually preserve the "unrelated tweaking" method so we can
use it in OP_TWEAKVERIFY
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 26, 2021
bitcoin/bitcoin#22051 (6/9)

Modified to use the old-style Optional rather than std::optional
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 27, 2021
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 27, 2021
That results in a much safer interface (making the tweak commit
to the key implicitly using a fixed tag means it can't be used for
unrelated tweaking).

bitcoin/bitcoin#22051 (5/9)
We actually preserve the "unrelated tweaking" method so we can
use it in OP_TWEAKVERIFY
sanket1729 pushed a commit to sanket1729/elements that referenced this pull request Aug 27, 2021
bitcoin/bitcoin#22051 (6/9)

Modified to use the old-style Optional rather than std::optional
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
Does the bare minimum to introduce Taproot wallet support with CT; just
adds a CPubKey blinding_pubkey to the taproot destination variant and
updates some visitors.

In future when we define blech32 we will need to make sure we are using
that encoding and using the pubkey.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants