Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes #29050

stevenroose · 2023-12-11T09:00:40Z

Implementation of OP_TXHASH and OP_CHECKTXHASHVERIFY, as per the draft BIP.

This MR includes a test using the test vectors generated from the reference implementation in the BIP.
The implementation utilizes a caching strategy that hopefully alleviates concerns around resource usage like quadratic hashing.

This MR on purpose does not make any consensus changes yet. Activation of the proposed opcodes will be coordinated in an independent MR, probably combined with other opcodes (like OP_CAT and/or OP_CHECKSIGFROMSTACK).

(I'm not very familiar with checks and lints on here, so I'll try to address problems as they come. I have personally been able to compile and run the tests, but that usually is only the tip of the iceberg.)

DrahtBot · 2023-12-11T09:00:43Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29648 (Remove libbitcoinconsensus by fanquake)
#29491 ([DO NOT MERGE] Schnorr batch verification for blocks by fjahr)
#29280 (Implement OP_CHECKTEMPLATEVERIFY by reardencode)
#29270 (Implement OP_CHECKSIGFROMSTACK(VERIFY) by reardencode)
#29269 (Add OP_INTERNALKEY for Tapscript by reardencode)
#29247 (Reenable OP_CAT by 0xBEEFCAF3)
#29221 (Implement 64 bit arithmetic op codes in the Script interpreter by Christewart)
#28923 (script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks) by theStack)
#28806 (rpc: Add script verification flags to getdeploymentinfo by ajtowns)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#21283 (Implement BIP 370 PSBTv2 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.

stevenroose · 2023-12-13T12:43:33Z

I added a commit that has a few question marks about which contexts should have their own TxHashCache. I'm not entirely understanding which contexts have the PrecomputedTxData either. The MutableTransactionSignatureCreator is the most troubling, because since the TransactionSignatureChecker takes a ptr to TxHashCache, some cache needs to live outside of the MutableTransactionSignatureCreator while actually that cache could be empty on every call. So it seems like we actually might have to add cache variables to all that stuff. I didn't do it until now, though.

Left some todos with questions where I was unsure.

Randy808 · 2024-01-02T14:19:06Z

src/script/interpreter.cpp

+                {
+                    // if flags not enabled; treat as a NOP4
+                    if (!(flags & SCRIPT_VERIFY_TXHASH)) {
+                        if (flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS)


nit: Consider adding curly braces here for clarity (since it's so close to the break)

Randy808 · 2024-01-02T14:19:09Z

src/script/interpreter.cpp

+                        if (!memcmp(txhash.begin(), stack.back().data(), 32)) {
+                            return set_error(serror, SCRIPT_ERR_CHECKTXHASHVERIFY);
+                        }
+                        // nothing more to do, leave item on stack


Leaving it on the stack seems inconsistent with the semantics of the other 'VERIFY' opcodes

Randy808 · 2024-01-02T14:26:26Z

src/script/interpreter.cpp

+                        if (stack.back().size() < 32) {
+                            return set_error(serror, SCRIPT_ERR_CHECKTXHASHVERIFY);
+                        }
+                        tx_field_selector = tx_field_selector.subspan(32);


I guess there's nothing wrong with pushing pre-concatenated arguments on the stack to save bytes, but this also seems inconsistent with other opcodes. Not the most important thing in the world but I'm curious what others think

Randy808 · 2024-01-02T14:34:42Z

src/script/txhash.h

+    | TXFS_OUTPUTS
+    | TXFS_CONTROL;
+
+static const unsigned char TXFS_INPUTS_PREVOUTS = 1 << 0;


Good list of options 👍

jonatack · 2024-01-04T18:37:32Z

src/script/bitcoinconsensus.cpp


        if (spentOutputs != nullptr && flags & bitcoinconsensus_SCRIPT_FLAGS_VERIFY_TAPROOT) {
            txdata.Init(tx, std::move(spent_outputs));
        }

-        return VerifyScript(tx.vin[nIn].scriptSig, CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen), &tx.vin[nIn].scriptWitness, flags, TransactionSignatureChecker(&tx, nIn, amount, txdata, MissingDataBehavior::FAIL), nullptr);
+        return VerifyScript(tx.vin[nIn].scriptSig, CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen), &tx.vin[nIn].scriptWitness, flags, TransactionSignatureChecker(&tx, nIn, amount, txdata, &txhash_cache, MissingDataBehavior::FAIL), nullptr);


A working build and green CI may help attract review. The last commit c368d32 doesn't build for me without updating the related fuzz tests.

sample diff

diff --git a/src/test/fuzz/script_assets_test_minimizer.cpp b/src/test/fuzz/script_assets_test_minimizer.cpp index 511b581f606..7478897e170 100644 --- a/src/test/fuzz/script_assets_test_minimizer.cpp +++ b/src/test/fuzz/script_assets_test_minimizer.cpp @@ -7,6 +7,7 @@ #include <primitives/transaction.h> #include <pubkey.h> #include <script/interpreter.h> +#include <script/txhash.h> #include <serialize.h> #include <streams.h> #include <univalue.h> @@ -159,7 +160,8 @@ void Test(const std::string& str) tx.vin[idx].scriptWitness = ScriptWitnessFromJSON(test["success"]["witness"]); PrecomputedTransactionData txdata; txdata.Init(tx, std::vector<CTxOut>(prevouts)); - MutableTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, txdata, MissingDataBehavior::ASSERT_FAIL); + TxHashCache txhash_cache; + MutableTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, txdata, &txhash_cache, MissingDataBehavior::ASSERT_FAIL); for (const auto flags : ALL_FLAGS) { // "final": true tests are valid for all flags. Others are only valid with flags that are // a subset of test_flags. @@ -174,7 +176,8 @@ void Test(const std::string& str) tx.vin[idx].scriptWitness = ScriptWitnessFromJSON(test["failure"]["witness"]); PrecomputedTransactionData txdata; txdata.Init(tx, std::vector<CTxOut>(prevouts)); - MutableTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, txdata, MissingDataBehavior::ASSERT_FAIL); + TxHashCache txhash_cache; + MutableTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, txdata, &txhash_cache, MissingDataBehavior::ASSERT_FAIL); for (const auto flags : ALL_FLAGS) { // If a test is supposed to fail with test_flags, it should also fail with any superset thereof. if ((flags & test_flags) == test_flags) { diff --git a/src/test/fuzz/script_flags.cpp b/src/test/fuzz/script_flags.cpp index accb32f1cc4..d9001fd81a0 100644 --- a/src/test/fuzz/script_flags.cpp +++ b/src/test/fuzz/script_flags.cpp @@ -5,6 +5,7 @@ #include <consensus/amount.h> #include <primitives/transaction.h> #include <script/interpreter.h> +#include <script/txhash.h> #include <serialize.h> #include <streams.h> #include <test/fuzz/fuzz.h> @@ -42,10 +43,11 @@ FUZZ_TARGET(script_flags) } PrecomputedTransactionData txdata; txdata.Init(tx, std::move(spent_outputs)); + TxHashCache txhash_cache; for (unsigned i = 0; i < tx.vin.size(); ++i) { const CTxOut& prevout = txdata.m_spent_outputs.at(i); - const TransactionSignatureChecker checker{&tx, i, prevout.nValue, txdata, MissingDataBehavior::ASSERT_FAIL}; + const TransactionSignatureChecker checker{&tx, i, prevout.nValue, txdata, &txhash_cache, MissingDataBehavior::ASSERT_FAIL}; ScriptError serror; const bool ret = VerifyScript(tx.vin.at(i).scriptSig, prevout.scriptPubKey, &tx.vin.at(i).scriptWitness, verify_flags, checker, &serror); diff --git a/src/test/fuzz/script_sigcache.cpp b/src/test/fuzz/script_sigcache.cpp index 5fdbc9e1069..9cc233fed19 100644 --- a/src/test/fuzz/script_sigcache.cpp +++ b/src/test/fuzz/script_sigcache.cpp @@ -6,6 +6,7 @@ #include <key.h> #include <pubkey.h> #include <script/sigcache.h> +#include <script/txhash.h> #include <test/fuzz/FuzzedDataProvider.h> #include <test/fuzz/fuzz.h> #include <test/fuzz/util.h> @@ -36,7 +37,8 @@ FUZZ_TARGET(script_sigcache, .init = initialize_script_sigcache) const CAmount amount = ConsumeMoney(fuzzed_data_provider); const bool store = fuzzed_data_provider.ConsumeBool(); PrecomputedTransactionData tx_data; - CachingTransactionSignatureChecker caching_transaction_signature_checker{mutable_transaction ? &tx : nullptr, n_in, amount, store, tx_data}; + TxHashCache txhash_cache; + CachingTransactionSignatureChecker caching_transaction_signature_checker{mutable_transaction ? &tx : nullptr, n_in, amount, store, tx_data, &txhash_cache}; if (fuzzed_data_provider.ConsumeBool()) { const auto random_bytes = fuzzed_data_provider.ConsumeBytes<unsigned char>(64); const XOnlyPubKey pub_key(ConsumeUInt256(fuzzed_data_provider));

DrahtBot · 2024-04-01T17:52:03Z

🐙 This pull request conflicts with the target branch and needs rebase.

achow101 · 2024-04-09T14:29:01Z

Seems like this should be a draft

stevenroose mentioned this pull request Dec 11, 2023

Add BIP for OP_TXHASH and OP_CHECKTXHASHVERIFY bitcoin/bips#1500

Open

stevenroose force-pushed the txhash branch from ac4d68f to c469bc4 Compare December 11, 2023 09:35

DrahtBot added the CI failed label Dec 11, 2023

stevenroose force-pushed the txhash branch from c469bc4 to e17ff60 Compare December 11, 2023 11:10

This was referenced Dec 11, 2023

Ephemeral Anchors #29001

Closed

rpc: Add script verification flags to getdeploymentinfo #28806

Closed

build: Introduce internal kernel library #28690

Draft

stevenroose force-pushed the txhash branch from e17ff60 to 970067a Compare December 11, 2023 17:00

DrahtBot mentioned this pull request Dec 11, 2023

refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc #26840

Closed

benthecarman mentioned this pull request Dec 12, 2023

December Topics austinbitdevs/austinbitdevs.com#28

Closed

DrahtBot mentioned this pull request Dec 12, 2023

Remove the legacy wallet and BDB dependency #28710

Draft

This was referenced Dec 13, 2023

script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks) #28923

Open

Implement BIP 370 PSBTv2 #21283

Open

Covenant tools softfork #28550

Closed

DrahtBot mentioned this pull request Dec 22, 2023

Compressed Bitcoin Transactions #29134

Closed

Randy808 reviewed Jan 2, 2024

View reviewed changes

jonatack reviewed Jan 4, 2024

View reviewed changes

illesdavid approved these changes Jan 5, 2024

View reviewed changes

This was referenced Jan 7, 2024

OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE) #29198

Draft

Implement 64 bit arithmetic op codes in the Script interpreter #29221

Draft

DrahtBot mentioned this pull request Jan 15, 2024

Reenable OP_CAT #29247

Open

ajtowns added the Consensus label Jan 15, 2024

Christewart mentioned this pull request Jan 17, 2024

Add SigVersion parameter to IsOpSuccess() #29265

Closed

This was referenced Jan 18, 2024

Implement OP_CHECKSIGFROMSTACK(VERIFY) #29270

Draft

Add OP_INTERNALKEY for Tapscript #29269

Draft

Implement OP_CHECKTEMPLATEVERIFY #29280

Draft

DrahtBot mentioned this pull request Feb 28, 2024

[DO NOT MERGE] Schnorr batch verification for blocks #29491

Draft

5 tasks

This was referenced Mar 13, 2024

Remove libbitcoinconsensus #29648

Merged

wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys #29675

Draft

stevenroose added 11 commits March 27, 2024 10:49

Add SpanPopFront cfr SpanPopBack

d530851

uint256: Add getter/setter for big-endian hexadecimal strings

09ca4f2

core_io: Add DecodeHexTxOut function

c20dd5e

script: Add CScript::IsPayToTaproot fast getter

726b66b

interpreter: Move some constants up to script.h

2eb6347

hash: Expose hash context of HashWriter

374dddc

Implement TxHash calculation

ecc5d6d

Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes

1698639

Make bare OP_CHECKTXHASHVERIFY transactions standard

eaddb75

tests: Add txhash_test.cpp with OP_TXHASH test vectors

9ffa976

txhash: Provide TxHashCache to required contexts

bebdb3a

stevenroose force-pushed the txhash branch from c368d32 to bebdb3a Compare March 27, 2024 23:43

stevenroose mentioned this pull request Mar 28, 2024

Add OP_TXHASH and OP_CHECKTXHASHVERIFY bitcoin-inquisition/bitcoin#51

Open

DrahtBot added the Needs rebase label Apr 1, 2024

achow101 marked this pull request as draft April 9, 2024 14:28

txhash: fixup: relax the malleability checks according to BIP

599020d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes #29050

Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes #29050

stevenroose commented Dec 11, 2023 •

edited

DrahtBot commented Dec 11, 2023 •

edited

stevenroose commented Dec 13, 2023

Randy808 Jan 2, 2024 •

edited

Randy808 Jan 2, 2024

Randy808 Jan 2, 2024 •

edited

Randy808 Jan 2, 2024

jonatack Jan 4, 2024

DrahtBot commented Apr 1, 2024

achow101 commented Apr 9, 2024

Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes #29050

Are you sure you want to change the base?

Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes #29050

Conversation

stevenroose commented Dec 11, 2023 • edited

DrahtBot commented Dec 11, 2023 • edited

Code Coverage

Reviews

Conflicts

stevenroose commented Dec 13, 2023

Randy808 Jan 2, 2024 • edited

Choose a reason for hiding this comment

Randy808 Jan 2, 2024

Choose a reason for hiding this comment

Randy808 Jan 2, 2024 • edited

Choose a reason for hiding this comment

Randy808 Jan 2, 2024

Choose a reason for hiding this comment

jonatack Jan 4, 2024

Choose a reason for hiding this comment

DrahtBot commented Apr 1, 2024

achow101 commented Apr 9, 2024

stevenroose commented Dec 11, 2023 •

edited

DrahtBot commented Dec 11, 2023 •

edited

Randy808 Jan 2, 2024 •

edited

Randy808 Jan 2, 2024 •

edited