PoC: tooling for OP_TEMPLATEHASH#108
Conversation
|
Windows CI failure seems unrelated: |
cafd902 to
c064b18
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
We use the 's' property in two ways: 1. To reason about malleability, as per the Miniscript specifications; 2. To sanity check that a top-level Miniscript fragment requires a transaction signature to be spent. This is fine as long as the only way to fix the transaction is by using a signature, and as long as all signatures are over the transaction. But in the following commits we are going to introduce fragments that either fix the transaction but do not require access to a private key (hence should not be 's' when reasoning about malleability), or allow to sign messages that are no the transaction itself and therefore should not be considered for the sanity check. Therefore, separate the two roles into two properties. Keep 's' for reasoning about malleability, and introduce a 't' property that determines whether the fragment's satisfaction fixes the transaction.
This is a bit convoluted since no other fragment relied on the Taproot internal key yet, and it needs to be passed from the context, and therefore all places where we create a context: in signing, descriptor parsing / inference, unit tests and fuzz tests. The approach taken is to only query the internal key once at parsing / inference time and store it in the fragment's keys vector. This way it naturally integrates with the existing code for pk_k(), such for serialization, signing and duplicate key checks. Care was taken in fuzz harnesses to not invalidate existing seeds, but also use a meaningful key in TestNode() (i.e. in miniscript_smart and miniscript_stable targets). A new optional internal_key parameter is adding down the call chain as an "out" parameter: the first time the fuzzer generates a pk_i() fragment (if any) the internal key is set and reused for all pk_i() fragments. This way we can roundtrip ser/parsing, make it available to the satisfier to sign, etc..
Likewise pk() and pkh(), this is syntactic sugar for the common case of an internal public key signature check.
This fragment checks the spending transaction's template hash. Because it commits to the spending transaction (with more malleable fields than SIGHASH_ALL, but less than SIGHASH_SINGLE/NONE) then we give it the 's' property. Note this breaks the invariant that an 's' fragment always contains at least one key. In the miniscript_smart and miniscript_stable fuzz harnesses we use the message hash for dummy signatures as the template hash. We sometimes create th() fragments with this hash (making them satisfiable), and sometimes not.
The satisfaction algorithm checks for signatures when going over key fragments, as a signature-checking fragment may have more than one key fragment as "descendant" where not all of them are available or some are more desirable to use than others. The key fragments have therefore no knowledge of the message to be signed to satisfy their signature-checking "ancestor" fragment. This was not an issue up until now since Miniscript only supported transaction signature checking, and therefore signatures had implicitly to be provided for the transaction itself. But we are about to introduce a Miniscript fragment that check signatures for arbitrary messages in the upcoming commits. Therefore in preparation make key fragments aware of the message their "ancestor" signature-checking fragment expect them to be signing.
…trary message A cms() fragment that takes as inputs a key expression and a message to check the signature against. The message is forwarded to key expressions and the satisfier made aware of a potential custom message to sign in place of the customary sighash. The chosen order of arguments did not require introduce more state to the parser, but did require introducing more to the decoder (where previously it was only necessary for thresh()).
This fragment is the equivalent of 'c:' but for rebindable signatures. It is a specialization of the 'cms()' fragment for a specific message that is the TEMPLATEHASH of the spending transaction.
This field allows a verifier to validate the transaction template(s) committed to in a transaction output. One caveat is that transaction are Bitcoin-serialized, which includes some field not committed to in a template hash.
The existing PSBT output field for a Taproot internal key is not keyed, which makes it so only a single Taproot internal key may be specified. This makes sense since there may be at most a single one per Taproot output, but since we introduce the capability of committing to the template of a spending transaction, it is often useful for a verifier to validate the outputs of the spending transaction.
We are going to reuse them in the following commit.
The rationale here is the same as for the additional Taproot internal keys, be able to inspect the outputs of transaction templates committed to in this output.
This is a specially crafted PSBT of a transaction that pays to a Taproot with a leaf with a TEMPLATEHASH equality check for a transaction that pays to 2 Taproot outputs. This highlights the use of all introduced fields, as well as existing ones (BIP32 derivations).
c064b18 to
4430066
Compare
|
Rebased after #100 was merged. This is technically a pull request, but this is mostly to make this demonstration available. I am happy to keep it around as a draft if you prefer that. |
ajtowns
left a comment
There was a problem hiding this comment.
Sigh. I thought it might be fun to ask claude for a review before just blindly pushing changes to areas I'm largely unfamiliar with, but then it went and found actual bugs.
Beyond the notes below which seem like real bugs to me and worth fixing, it also suggests:
- PSBT deserializer should verify m_committed_txs[h] actually templatehashes to h.[psbt.h:946-960] accepts arbitrary tx for any claimed hash. One-line addition closes the "malicious PSBT producer puts unrelated tx" abuse path.
- PSBT combiner silently keeps first on conflict for the new maps. psbt.cpp:286-288 uses
std::map::insertof a range, which doesn't replace on conflict. For the new fields specifically, conflicting values are either bugs or attacks; silent first-wins is the worst behavior. Raising at combine time is the safer default. (Combined with the above, the COMMITTED_TXS case becomes self-policing.) - cms(K, m) silently rejects messages other than 32 bytes. sign.cpp:340-345. The script-level fragment accepts arbitrary-length messages, and the parser will happily accept them, but Sign() returns Availability::NO for non-32-byte. So cms(pk_i(),ab21) (the test descriptor in commit 79f420f) is parseable but unsignable. Worth flagging as either a parse-time rejection or a documented limitation.
- Commit message for the th() commit (c6220f8) is stale — says "we give it the 's' property" but code gives t. Author may want to amend on next push.
|
|
||
| //! Template hash satisfaction. | ||
| bool CheckTemplateHash(const std::vector<unsigned char>& hash) const { | ||
| return hash.data() == GetTemplateHash().data(); |
There was a problem hiding this comment.
This compares two unsigned char* not the contents of what's pointed to. std::ranges::equal(hash, GetTemplateHash()) might be what's wanted.
| //! Check whether this script always needs a signature. | ||
| bool NeedsSignature() const { return GetType() << "s"_mst; } | ||
| //! Check whether this script always needs a transaction signature. | ||
| bool NeedsSignature() const { return GetType() << "t"_mst; } |
There was a problem hiding this comment.
Maybe consider renaming this to NeedsTxSignature as well as the extra comment?
| * | ||
| * An additional type property helps reasoning about "sanity": | ||
| * - "t" Transaction signed: | ||
| * - Satisfactions (if any) for this expression always involve at least one signature. |
There was a problem hiding this comment.
Should this be "t - Transaction commitment -- Satisfaction for this expression commits to the tx" ? I don't see how you have "non-malleability" without this, so not sure splitting it out from that section makes sense?
| WRAP_C, //!< [X] OP_CHECKSIG | ||
| WRAP_D, //!< OP_DUP OP_IF [X] OP_ENDIF | ||
| WRAP_V, //!< [X] OP_VERIFY (or -VERIFY version of last opcode in X) | ||
| WRAP_R, //!< [X] OP_TEMPLATEHASH OP_SWAP OP_CHECKSIGVERIFY |
| WRAP_R, //!< [X] OP_TEMPLATEHASH OP_SWAP OP_CHECKSIGVERIFY | ||
| WRAP_J, //!< OP_SIZE OP_0NOTEQUAL OP_IF [X] OP_ENDIF | ||
| WRAP_N, //!< [X] OP_0NOTEQUAL | ||
| CMS, //!< [X] <m> OP_SWAP OP_CHECKSIGVERIFY |
There was a problem hiding this comment.
I think "CMS" isn't a good name here -- it sounds like "check multisig" to me.
Also OP_CSFS not OP_CSV
| if (!key_lookup.emplace(key).second) { | ||
| throw std::ios_base::failure("Duplicate Key, additional output Taproot internal key already provided"); | ||
| } else if (key.size() != 33) { | ||
| throw std::ios_base::failure("Additional output Taproot internal key key is not 33 bytes"); |
There was a problem hiding this comment.
"output Taproot tree" not "internal key" here.
| } | ||
| case Fragment::TH: { | ||
| if (ctx.CheckTemplateHash(node.data)) { | ||
| return {INVALID, InputStack{}.SetWithSig()}; |
There was a problem hiding this comment.
(SetWithSig seems like it needs a rename too)
| sig.resize(64); | ||
| // Use uint256{} as aux_rnd for now. | ||
| if (!key.SignSchnorr(hash, sig, merkle_root, {})) return false; | ||
| if (nHashType) sig.push_back(nHashType); |
There was a problem hiding this comment.
If I am understanding correctly, the hash_type byte must not be appended to a CSFS/custom-message signature.
- if (nHashType) sig.push_back(nHashType);
+ if (!custom_msg.has_value() && nHashType) sig.push_back(nHashType);| // Write the OP_TEMPLATEHASH-committed transactions | ||
| for (const auto& [hash, tx]: m_committed_txs) { | ||
| SerializeToVector(s, PSBT_OUT_COMMITTED_TXS, hash); | ||
| s << TX_WITH_WITNESS(tx); |
There was a problem hiding this comment.
SerializeToVector(...) writes the CompactSize value length before the actual value.
diff --git a/src/psbt.h b/src/psbt.h
index b84eaaf62a..8928a8d998 100644
--- a/src/psbt.h
+++ b/src/psbt.h
@@ -831,13 +831,13 @@ struct PSBTOutput
// Write the OP_TEMPLATEHASH-committed transactions
for (const auto& [hash, tx]: m_committed_txs) {
SerializeToVector(s, PSBT_OUT_COMMITTED_TXS, hash);
- s << TX_WITH_WITNESS(tx);
+ SerializeToVector(s, TX_WITH_WITNESS(tx));
}
// Write the additional Taproot internal keys
for (const auto& [output_key, internal_key]: m_tap_internal_keys) {
SerializeToVector(s, PSBT_OUT_TAP_INTERNAL_KEYS, output_key);
- s << internal_key;
+ SerializeToVector(s, internal_key);
}
// Write the additional Taproot trees
@@ -955,7 +955,8 @@ struct PSBTOutput
throw std::ios_base::failure("Output committed transaction key is not 33 bytes");
}
uint256 hash{std::span<uint8_t>{key.begin() + 1, key.end()}};
- CMutableTransaction tx{deserialize, TX_WITH_WITNESS, s};
+ CMutableTransaction tx;
+ UnserializeFromVector(s, TX_WITH_WITNESS(tx));
m_committed_txs.emplace(std::move(hash), std::move(tx));
break;
}
@@ -967,7 +968,7 @@ struct PSBTOutput
throw std::ios_base::failure("Additional output Taproot internal key key is not 33 bytes");
}
XOnlyPubKey output_key{std::span(key).last(32)}, internal_key;
- s >> internal_key;
+ UnserializeFromVector(s, internal_key);
m_tap_internal_keys.emplace(std::move(output_key), std::move(internal_key));
break;
}| break; | ||
| } | ||
| } | ||
| if (last - in >= 3 && in[0].first == OP_EQUAL && in[1].first == OP_TEMPLATEHASH) { |
There was a problem hiding this comment.
It would also accept <31-byte push> OP_TEMPLATEHASH OP_EQUAL and construct an invalid TH node.
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 765df39cce..452f3318ff 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -2518,7 +2518,7 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
break;
}
}
- if (last - in >= 3 && in[0].first == OP_EQUAL && in[1].first == OP_TEMPLATEHASH) {
+ if (last - in >= 3 && in[0].first == OP_EQUAL && in[1].first == OP_TEMPLATEHASH && in[2].second.size() == 32) {
if (!IsTapscript(ctx.MsContext())) return {};
constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::TH, in[2].second));
in += 3;
diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index 60cd148bb3..4e4807abcc 100644
--- a/src/test/miniscript_tests.cpp
+++ b/src/test/miniscript_tests.cpp
@@ -799,6 +799,11 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
ms_th += HexStr(TestData::MESSAGE_HASH) + "))";
Test(ms_th, "", "?", TESTMODE_VALID | TESTMODE_NONMAL | TESTMODE_NEEDSIG | TESTMODE_P2WSH_INVALID);
Test(ms_th, "?", "?", TESTMODE_VALID | TESTMODE_NONMAL | TESTMODE_NEEDSIG | TESTMODE_P2WSH_INVALID);
+ for (const size_t hash_size : {31U, 33U}) {
+ const std::vector<unsigned char> invalid_hash(hash_size, 0x42);
+ const CScript invalid_th_script{CScript{} << invalid_hash << OP_TEMPLATEHASH << OP_EQUAL};
+ BOOST_CHECK(!miniscript::FromScript(invalid_th_script, tap_converter));
+ }
Test("cms(pk_k(02e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd13),ec4916dd28fc4c10d78e287ca5d9cc51ee1ae73cbfde08c6b37324cbfaac8bc5)", "20e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd1320ec4916dd28fc4c10d78e287ca5d9cc51ee1ae73cbfde08c6b37324cbfaac8bc57ccc", "20e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd1320ec4916dd28fc4c10d78e287ca5d9cc51ee1ae73cbfde08c6b37324cbfaac8bc57ccc", TESTMODE_VALID | TESTMODE_NONMAL | TESTMODE_P2WSH_INVALID);
Test("or_i(and_b(hash160(20195b5a3d650c17f0f29f91c33f8f6335193d07),a:cms(pk_k(02e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd13),ec4916dd28fc4c10d78e287ca5d9cc51ee1ae73cbfde08c6b37324cbfaac8bc5)),and_v(v:older(42),pkh(025cbdf0646e5db4eaa398f365f2ea7a0e3d419b7e0330e39ce92bddedcac4f9bc)))", "6382012088a91420195b5a3d650c17f0f29f91c33f8f6335193d07876b20e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd1320ec4916dd28fc4c10d78e287ca5d9cc51ee1ae73cbfde08c6b37324cbfaac8bc57ccc6c9a67012ab26976a9141a7ac36cfa8431ab2395d701b0050045ae4a37d188ac68", "6382012088a91420195b5a3d650c17f0f29f91c33f8f6335193d07876b20e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd1320ec4916dd28fc4c10d78e287ca5d9cc51ee1ae73cbfde08c6b37324cbfaac8bc57ccc6c9a67012ab26976a9141a7ac36cfa8431ab2395d701b0050045ae4a37d188ac68", TESTMODE_VALID | TESTMODE_NONMAL | TESTMODE_P2WSH_INVALID);
Test("or_i(and_b(hash160(20195b5a3d650c17f0f29f91c33f8f6335193d07),a:cms(pk_k(02e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd13),424242babaffec4916dd28fc4c10d78e287ca5d9cc51ee1ae73cbfde08c6b37324cbfaac8bc5)),and_v(v:older(42),pkh(025cbdf0646e5db4eaa398f365f2ea7a0e3d419b7e0330e39ce92bddedcac4f9bc)))", "6382012088a91420195b5a3d650c17f0f29f91c33f8f6335193d07876b20e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd1326424242babaffec4916dd28fc4c10d78e287ca5d9cc51ee1ae73cbfde08c6b37324cbfaac8bc57ccc6c9a67012ab26976a9141a7ac36cfa8431ab2395d701b0050045ae4a37d188ac68", "6382012088a91420195b5a3d650c17f0f29f91c33f8f6335193d07876b20e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd1326424242babaffec4916dd28fc4c10d78e287ca5d9cc51ee1ae73cbfde08c6b37324cbfaac8bc57ccc6c9a67012ab26976a9141a7ac36cfa8431ab2395d701b0050045ae4a37d188ac68", TESTMODE_VALID | TESTMODE_NONMAL | TESTMODE_P2WSH_INVALID);
I guess now i have to actually clean this up 🥲. I remember cutting some corners since it was just a demo as a companion for my ML post. It will take me some time to swap context back in to go and see what needs to be reworked besides fixing the bugs you guys have found. |
|
Converted to draft since it's not ready yet |
This is a PoC implementation of extensions to standard tooling (PSBT and Miniscript descriptors) to support our "Taproot-native (re-)bindable transactions" proposal.
See this mailing list post for details and rationale on the various design choices. In short this PR introduces 4 new Miniscript fragments, and 3 new PSBT output fields. The Miniscript fragments are the following:
Xrequired typepk_iOP_INTERNALKEYth(h)<h> OP_TEMPLATEHASH OP_EQUALcms(X,m)[X] <m> OP_SWAP OP_CHECKSIGFROMSTACKr:X[X] OP_TEMPLATEHASH OP_SWAP OP_CHECKSIGFROMSTACKThe 't' property is not currently described in the BIP or on the Miniscript website. This is a new property i had to introduce to split two related properties "a spending path needs a signature" and "the spending transaction is encumbered". See commit a3e3ad1 for details.
The PSBT fields introduced are the following:
<keytype><keydata><valuedata>PSBT_OUT_COMMITTED_TXS = 0x0b<32 bytes of template hash><bytes of Bitcoin-serialized transaction committed>PSBT_OUT_TAP_INTERNAL_KEYS = 0x0c<bytes taproot output key><32-byte internal key>PSBT_OUT_TAP_TREES = 0x0d<bytes taproot output key>{<8-bit uint depth> <8-bit uint leaf version> <compact size uint scriptlen> <bytes script>}*This is PoC quality, but should still give some reasonable assurance given the unit and end-to-end sanity checks, and especially the integration of the new fragments in the existing thorough Miniscript fuzz targets.