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

fe92c15 script/sign: avoid duplicated signature verification after signing (Sebastian Falbesoner)
0800895 bench: add benchmark for `SignTransaction` (Sebastian Falbesoner)

Pull request description:

  This PR is a small performance improvement on the `SignTransaction` function, which is used mostly by the wallet (obviously) and the `signrawtransactionwithkey` RPC. The lower-level function `ProduceSignature` already calls `VerifyScript` internally as last step in order to check whether the signature data is complete:
  https://github.com/bitcoin/bitcoin/blob/daa56f7f665183bcce3df146f143be37f33c123e/src/script/sign.cpp#L568-L570

  If and only if that is the case, the `complete` field of the `SignatureData` is set to `true` accordingly and there is no need then to verify the script after again, as we already know that it would succeed.

  This leads to a rough ~20% speed-up for `SignTransaction` for single-input ECDSA or Taproot transactions, according to the newly introduced `SignTransaction{ECDSA,Taproot}` benchmarks:

  ```
  $ ./src/bench/bench_bitcoin --filter=SignTransaction.*
  ```

  without commit 18185f4f578b8795fdaa75926630a691e9c8d0d4:

  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |          185,597.79 |            5,388.00 |    1.6% |      0.22 | `SignTransactionECDSA`
  |          141,323.95 |            7,075.94 |    2.1% |      0.17 | `SignTransactionSchnorr`

  with commit 18185f4f578b8795fdaa75926630a691e9c8d0d4:

  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |          149,757.86 |            6,677.45 |    1.4% |      0.18 | `SignTransactionECDSA`
  |          108,284.40 |            9,234.94 |    2.0% |      0.13 | `SignTransactionSchnorr`

  Note that there are already signing benchmarks in the secp256k1 library, but `SignTransaction` does much more than just the cryptographical parts, i.e.:
  * calculate the unsigned tx's `PrecomputedTransactionData` if necessary
  * apply Solver on the prevout scriptPubKey, fetch the relevant keys from the signing provider
  * perform the actual signing operation (for ECDSA signatures, that could be more than once due to low-R grinding)
  * verify if the signatures are correct by calling `VerifyScript` (more than once currently, which is fixed by this PR)

  so it probably makes sense to also have benchmarks from that higher-level application perspective.

ACKs for top commit:
  achow101:
    ACK fe92c15
  furszy:
    utACK fe92c15
  glozow:
    light review ACK fe92c15

Tree-SHA512: b7225ff9e8a640ca5222dea5b2a463a0f9b9de704e4330b5b9a7bce2d63a1f4620575c474a8186f4708d7d9534eab55d000393d99db79c0cfc046b35f0a4a778

Author: achow101

src/Makefile.bench.include

@@ -54,6 +54,7 @@ bench_bench_bitcoin_SOURCES = \
   bench/rollingbloom.cpp \
   bench/rpc_blockchain.cpp \
   bench/rpc_mempool.cpp \
+  bench/sign_transaction.cpp \
   bench/streams_findbyte.cpp \
   bench/strencodings.cpp \
   bench/util_time.cpp \

src/bench/sign_transaction.cpp

@@ -0,0 +1,70 @@
+// Copyright (c) 2023 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <bench/bench.h>
+#include <addresstype.h>
+#include <coins.h>
+#include <key.h>
+#include <primitives/transaction.h>
+#include <pubkey.h>
+#include <script/interpreter.h>
+#include <script/script.h>
+#include <script/sign.h>
+#include <uint256.h>
+#include <util/translation.h>
+
+enum class InputType {
+    P2WPKH, // segwitv0, witness-pubkey-hash (ECDSA signature)
+    P2TR,   // segwitv1, taproot key-path spend (Schnorr signature)
+};
+
+static void SignTransactionSingleInput(benchmark::Bench& bench, InputType input_type)
+{
+    ECC_Context ecc_context{};
+
+    FlatSigningProvider keystore;
+    std::vector<CScript> prev_spks;
+
+    // Create a bunch of keys / UTXOs to avoid signing with the same key repeatedly
+    for (int i = 0; i < 32; i++) {
+        CKey privkey = GenerateRandomKey();
+        CPubKey pubkey = privkey.GetPubKey();
+        CKeyID key_id = pubkey.GetID();
+        keystore.keys.emplace(key_id, privkey);
+        keystore.pubkeys.emplace(key_id, pubkey);
+
+        // Create specified locking script type
+        CScript prev_spk;
+        switch (input_type) {
+        case InputType::P2WPKH: prev_spk = GetScriptForDestination(WitnessV0KeyHash(pubkey)); break;
+        case InputType::P2TR:   prev_spk = GetScriptForDestination(WitnessV1Taproot(XOnlyPubKey{pubkey})); break;
+        default: assert(false);
+        }
+        prev_spks.push_back(prev_spk);
+    }
+
+    // Simple 1-input tx with artificial outpoint
+    // (note that for the purpose of signing with SIGHASH_ALL we don't need any outputs)
+    COutPoint prevout{/*hashIn=*/Txid::FromUint256(uint256::ONE), /*nIn=*/1337};
+    CMutableTransaction unsigned_tx;
+    unsigned_tx.vin.emplace_back(prevout);
+
+    // Benchmark.
+    int iter = 0;
+    bench.minEpochIterations(100).run([&] {
+        CMutableTransaction tx{unsigned_tx};
+        std::map<COutPoint, Coin> coins;
+        CScript prev_spk = prev_spks[(iter++) % prev_spks.size()];
+        coins[prevout] = Coin(CTxOut(10000, prev_spk), /*nHeightIn=*/100, /*fCoinBaseIn=*/false);
+        std::map<int, bilingual_str> input_errors;
+        bool complete = SignTransaction(tx, &keystore, coins, SIGHASH_ALL, input_errors);
+        assert(complete);
+    });
+}
+
+static void SignTransactionECDSA(benchmark::Bench& bench)   { SignTransactionSingleInput(bench, InputType::P2WPKH); }
+static void SignTransactionSchnorr(benchmark::Bench& bench) { SignTransactionSingleInput(bench, InputType::P2TR);   }
+
+BENCHMARK(SignTransactionECDSA, benchmark::PriorityLevel::HIGH);
+BENCHMARK(SignTransactionSchnorr, benchmark::PriorityLevel::HIGH);

src/script/sign.cpp

@@ -831,7 +831,7 @@ bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
         }
 
         ScriptError serror = SCRIPT_ERR_OK;
-        if (!VerifyScript(txin.scriptSig, prevPubKey, &txin.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount, txdata, MissingDataBehavior::FAIL), &serror)) {
+        if (!sigdata.complete && !VerifyScript(txin.scriptSig, prevPubKey, &txin.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount, txdata, MissingDataBehavior::FAIL), &serror)) {
             if (serror == SCRIPT_ERR_INVALID_STACK_OPERATION) {
                 // Unable to sign input and verification failed (possible attempt to partially sign).
                 input_errors[i] = Untranslated("Unable to sign input, invalid stack size (possibly missing key)");