Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes #29050

pull stevenroose wants to merge 12 commits into bitcoin:master from stevenroose:txhash changing 44 files +3148 −51
  1. stevenroose commented at 9:00 am on December 11, 2023: contributor

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

  2. DrahtBot commented at 9:00 am on December 11, 2023: contributor

    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.

  3. stevenroose force-pushed on Dec 11, 2023
  4. DrahtBot added the label CI failed on Dec 11, 2023
  5. stevenroose force-pushed on Dec 11, 2023
  6. stevenroose force-pushed on Dec 11, 2023
  7. stevenroose commented at 12:43 pm on December 13, 2023: contributor

    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.

  8. in src/script/interpreter.cpp:600 in c368d32588 outdated
    592@@ -591,7 +593,54 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
    593                     break;
    594                 }
    595 
    596-                case OP_NOP1: case OP_NOP4: case OP_NOP5:
    597+                case OP_TXHASH: case OP_CHECKTXHASHVERIFY:
    598+                {
    599+                    // if flags not enabled; treat as a NOP4
    600+                    if (!(flags & SCRIPT_VERIFY_TXHASH)) {
    601+                        if (flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS)
    


    Randy808 commented at 2:19 pm on January 2, 2024:
    nit: Consider adding curly braces here for clarity (since it’s so close to the break)
  9. in src/script/interpreter.cpp:634 in c368d32588 outdated
    630+
    631+                    if (opcode == OP_CHECKTXHASHVERIFY) {
    632+                        if (!memcmp(txhash.begin(), stack.back().data(), 32)) {
    633+                            return set_error(serror, SCRIPT_ERR_CHECKTXHASHVERIFY);
    634+                        }
    635+                        // nothing more to do, leave item on stack
    


    Randy808 commented at 2:19 pm on January 2, 2024:
    Leaving it on the stack seems inconsistent with the semantics of the other ‘VERIFY’ opcodes
  10. in src/script/interpreter.cpp:614 in c368d32588 outdated
    610+                    Span<const unsigned char> tx_field_selector { stack.back() };
    611+                    if (opcode == OP_CHECKTXHASHVERIFY) {
    612+                        if (stack.back().size() < 32) {
    613+                            return set_error(serror, SCRIPT_ERR_CHECKTXHASHVERIFY);
    614+                        }
    615+                        tx_field_selector = tx_field_selector.subspan(32);
    


    Randy808 commented at 2:26 pm on January 2, 2024:
    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
  11. in src/script/txhash.h:41 in c368d32588 outdated
    38+    | TXFS_CURRENT_INPUT_LAST_CODESEPARATOR_POS
    39+    | TXFS_INPUTS
    40+    | TXFS_OUTPUTS
    41+    | TXFS_CONTROL;
    42+
    43+static const unsigned char TXFS_INPUTS_PREVOUTS = 1 << 0;
    


    Randy808 commented at 2:34 pm on January 2, 2024:
    Good list of options 👍
  12. in src/script/bitcoinconsensus.cpp:115 in c368d32588 outdated
    111         if (spentOutputs != nullptr && flags & bitcoinconsensus_SCRIPT_FLAGS_VERIFY_TAPROOT) {
    112             txdata.Init(tx, std::move(spent_outputs));
    113         }
    114 
    115-        return VerifyScript(tx.vin[nIn].scriptSig, CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen), &tx.vin[nIn].scriptWitness, flags, TransactionSignatureChecker(&tx, nIn, amount, txdata, MissingDataBehavior::FAIL), nullptr);
    116+        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);
    


    jonatack commented at 6:37 pm on January 4, 2024:

    A working build and green CI may help attract review. The last commit https://github.com/bitcoin/bitcoin/pull/29050/commits/c368d32588ff79efc189fbdcfe559d1ca081c36c doesn’t build for me without updating the related fuzz tests.

     0diff --git a/src/test/fuzz/script_assets_test_minimizer.cpp b/src/test/fuzz/script_assets_test_minimizer.cpp
     1index 511b581f606..7478897e170 100644
     2--- a/src/test/fuzz/script_assets_test_minimizer.cpp
     3+++ b/src/test/fuzz/script_assets_test_minimizer.cpp
     4@@ -7,6 +7,7 @@
     5 #include <primitives/transaction.h>
     6 #include <pubkey.h>
     7 #include <script/interpreter.h>
     8+#include <script/txhash.h>
     9 #include <serialize.h>
    10 #include <streams.h>
    11 #include <univalue.h>
    12@@ -159,7 +160,8 @@ void Test(const std::string& str)
    13         tx.vin[idx].scriptWitness = ScriptWitnessFromJSON(test["success"]["witness"]);
    14         PrecomputedTransactionData txdata;
    15         txdata.Init(tx, std::vector<CTxOut>(prevouts));
    16-        MutableTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, txdata, MissingDataBehavior::ASSERT_FAIL);
    17+        TxHashCache txhash_cache;
    18+        MutableTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, txdata, &txhash_cache, MissingDataBehavior::ASSERT_FAIL);
    19         for (const auto flags : ALL_FLAGS) {
    20             // "final": true tests are valid for all flags. Others are only valid with flags that are
    21             // a subset of test_flags.
    22@@ -174,7 +176,8 @@ void Test(const std::string& str)
    23         tx.vin[idx].scriptWitness = ScriptWitnessFromJSON(test["failure"]["witness"]);
    24         PrecomputedTransactionData txdata;
    25         txdata.Init(tx, std::vector<CTxOut>(prevouts));
    26-        MutableTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, txdata, MissingDataBehavior::ASSERT_FAIL);
    27+        TxHashCache txhash_cache;
    28+        MutableTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, txdata, &txhash_cache, MissingDataBehavior::ASSERT_FAIL);
    29         for (const auto flags : ALL_FLAGS) {
    30             // If a test is supposed to fail with test_flags, it should also fail with any superset thereof.
    31             if ((flags & test_flags) == test_flags) {
    32diff --git a/src/test/fuzz/script_flags.cpp b/src/test/fuzz/script_flags.cpp
    33index accb32f1cc4..d9001fd81a0 100644
    34--- a/src/test/fuzz/script_flags.cpp
    35+++ b/src/test/fuzz/script_flags.cpp
    36@@ -5,6 +5,7 @@
    37 #include <consensus/amount.h>
    38 #include <primitives/transaction.h>
    39 #include <script/interpreter.h>
    40+#include <script/txhash.h>
    41 #include <serialize.h>
    42 #include <streams.h>
    43 #include <test/fuzz/fuzz.h>
    44@@ -42,10 +43,11 @@ FUZZ_TARGET(script_flags)
    45         }
    46         PrecomputedTransactionData txdata;
    47         txdata.Init(tx, std::move(spent_outputs));
    48+        TxHashCache txhash_cache;
    49 
    50         for (unsigned i = 0; i < tx.vin.size(); ++i) {
    51             const CTxOut& prevout = txdata.m_spent_outputs.at(i);
    52-            const TransactionSignatureChecker checker{&tx, i, prevout.nValue, txdata, MissingDataBehavior::ASSERT_FAIL};
    53+            const TransactionSignatureChecker checker{&tx, i, prevout.nValue, txdata, &txhash_cache, MissingDataBehavior::ASSERT_FAIL};
    54 
    55             ScriptError serror;
    56             const bool ret = VerifyScript(tx.vin.at(i).scriptSig, prevout.scriptPubKey, &tx.vin.at(i).scriptWitness, verify_flags, checker, &serror);
    57diff --git a/src/test/fuzz/script_sigcache.cpp b/src/test/fuzz/script_sigcache.cpp
    58index 5fdbc9e1069..9cc233fed19 100644
    59--- a/src/test/fuzz/script_sigcache.cpp
    60+++ b/src/test/fuzz/script_sigcache.cpp
    61@@ -6,6 +6,7 @@
    62 #include <key.h>
    63 #include <pubkey.h>
    64 #include <script/sigcache.h>
    65+#include <script/txhash.h>
    66 #include <test/fuzz/FuzzedDataProvider.h>
    67 #include <test/fuzz/fuzz.h>
    68 #include <test/fuzz/util.h>
    69@@ -36,7 +37,8 @@ FUZZ_TARGET(script_sigcache, .init = initialize_script_sigcache)
    70     const CAmount amount = ConsumeMoney(fuzzed_data_provider);
    71     const bool store = fuzzed_data_provider.ConsumeBool();
    72     PrecomputedTransactionData tx_data;
    73-    CachingTransactionSignatureChecker caching_transaction_signature_checker{mutable_transaction ? &tx : nullptr, n_in, amount, store, tx_data};
    74+    TxHashCache txhash_cache;
    75+    CachingTransactionSignatureChecker caching_transaction_signature_checker{mutable_transaction ? &tx : nullptr, n_in, amount, store, tx_data, &txhash_cache};
    76     if (fuzzed_data_provider.ConsumeBool()) {
    77         const auto random_bytes = fuzzed_data_provider.ConsumeBytes<unsigned char>(64);
    78         const XOnlyPubKey pub_key(ConsumeUInt256(fuzzed_data_provider));
    
  13. illesdavid approved
  14. illesdavid approved
  15. illesdavid approved
  16. illesdavid approved
  17. illesdavid approved
  18. ajtowns added the label Consensus on Jan 15, 2024
  19. Add SpanPopFront cfr SpanPopBack d530851f54
  20. uint256: Add getter/setter for big-endian hexadecimal strings 09ca4f2f63
  21. core_io: Add DecodeHexTxOut function c20dd5ece4
  22. script: Add CScript::IsPayToTaproot fast getter 726b66b691
  23. interpreter: Move some constants up to script.h 2eb6347a6c
  24. hash: Expose hash context of HashWriter 374dddcc2e
  25. Implement TxHash calculation ecc5d6dfac
  26. Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes 16986395de
  27. Make bare OP_CHECKTXHASHVERIFY transactions standard eaddb75903
  28. tests: Add txhash_test.cpp with OP_TXHASH test vectors 9ffa9766d4
  29. txhash: Provide TxHashCache to required contexts bebdb3aede
  30. stevenroose force-pushed on Mar 27, 2024
  31. DrahtBot added the label Needs rebase on Apr 1, 2024
  32. DrahtBot commented at 5:52 pm on April 1, 2024: contributor

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

  33. achow101 marked this as a draft on Apr 9, 2024
  34. achow101 commented at 2:29 pm on April 9, 2024: member
    Seems like this should be a draft
  35. txhash: fixup: relax the malleability checks according to BIP 599020daaa
  36. DrahtBot commented at 0:06 am on July 27, 2024: contributor

    ⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
  37. DrahtBot commented at 0:45 am on October 24, 2024: contributor

    ⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me