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

[theStack]

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:

bitcoin/src/script/sign.cpp

Lines 568 to 570 in daa56f7

	// Test solution
	sigdata.complete = solved && VerifyScript(sigdata.scriptSig, fromPubKey, &sigdata.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, creator.Checker());
	return sigdata.complete;

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 18185f4:

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 18185f4:

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.

[DrahtBot]

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.

Type	Reviewers
ACK	furszy
Stale ACK	achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

[achow101]

Concept ACK

[theStack]

Rebased on master, CI should be green now (there was a silent merge conflict in the benchmark due to a change in the COutPoint ctor interface, see #28922).

[luke-jr]

Does this reduce our safety against memory corruption or similar?

[theStack]

Does this reduce our safety against memory corruption or similar?

I don't think so, at least I don't see how verifying a created signature twice in a row has any benefit over doing it only once.

[furszy]

Would suggest to use FlatSigningProvider instead (see #28307).
It will also save you one extra GetScriptForDestination call per created key.

[theStack]

Have to admit that I'm lacking knowledge here about the concrete differences between FillableSigningProvider and FlatSigningProvider, especially about the possible benefits of the latter in this PR. AFAIR, I chose the fillable provider as it provides a nice interface (.AddKey()), which the flat one doesn't. The following patch works:

diff --git a/src/bench/sign_transaction.cpp b/src/bench/sign_transaction.cpp
index 8cb5e63882..8bd2ecbcd0 100644
--- a/src/bench/sign_transaction.cpp
+++ b/src/bench/sign_transaction.cpp
@@ -23,7 +23,7 @@ static void SignTransactionSingleInput(benchmark::Bench& bench, InputType input_
 {
     ECC_Start();
 
-    FillableSigningProvider keystore;
+    FlatSigningProvider keystore;
     std::vector<CScript> prev_spks;
 
     // Create a bunch of keys / UTXOs to avoid signing with the same key repeatedly
@@ -31,7 +31,9 @@ static void SignTransactionSingleInput(benchmark::Bench& bench, InputType input_
         CKey privkey;
         privkey.MakeNewKey(/*fCompressed=*/true);
         CPubKey pubkey = privkey.GetPubKey();
-        keystore.AddKey(privkey);
+        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;

Is there anything more that could be changed? I don't see how it makes the derivation of the outpoint scriptPubKey (needed for the map of coins) any easier, as I still need the individual GetScriptForDestination calls dependend on the locking script type.

[furszy]

Have to admit that I'm lacking knowledge here about the concrete differences between FillableSigningProvider and FlatSigningProvider, especially about the possible benefits of the latter in this PR.

FillableSigningProvider is just the legacy class and contains legacy scripts limitations (thus #28307). It does not affect the current form of this PR but, because this benchmark uses segwit v0 and v1 outputs, it could cause issues in the future.

Is there anything more that could be changed? I don't see how it makes the derivation of the outpoint scriptPubKey (needed for the map of coins) any easier, as I still need the individual GetScriptForDestination calls dependend on the locking script type.

I never said that it was going to make derivation easier. I Just said that it will also save you an extra GetScriptForDestination call per script.

diff --git a/src/bench/sign_transaction.cpp b/src/bench/sign_transaction.cpp
--- a/src/bench/sign_transaction.cpp	(revision 90469c8b7c1cd1d88134f6837914552792cbef49)
+++ b/src/bench/sign_transaction.cpp	(date 1706283907363)
@@ -23,15 +23,16 @@
 {
     ECC_Start();
 
-    FillableSigningProvider keystore;
-    std::vector<CScript> prev_spks;
+    FlatSigningProvider keystore;
 
     // Create a bunch of keys / UTXOs to avoid signing with the same key repeatedly
     for (int i = 0; i < 32; i++) {
         CKey privkey;
         privkey.MakeNewKey(/*fCompressed=*/true);
         CPubKey pubkey = privkey.GetPubKey();
-        keystore.AddKey(privkey);
+        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;
@@ -40,7 +41,7 @@
         case InputType::P2TR:   prev_spk = GetScriptForDestination(WitnessV1Taproot(XOnlyPubKey{pubkey})); break;
         default: assert(false);
         }
-        prev_spks.push_back(prev_spk);
+        keystore.scripts.insert({CScriptID{prev_spk}, prev_spk});
     }
 
     // Simple 1-input tx with artificial outpoint
@@ -50,15 +51,16 @@
     unsigned_tx.vin.emplace_back(prevout);
 
     // Benchmark.
-    int iter = 0;
+    auto it = keystore.scripts.begin();
     bench.minEpochIterations(100).run([&] {
         CMutableTransaction tx{unsigned_tx};
         std::map<COutPoint, Coin> coins;
-        CScript prev_spk = prev_spks[(iter++) % prev_spks.size()];
+        const CScript& prev_spk = it->second;
         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);
+        if (++it == keystore.scripts.end()) it = keystore.scripts.begin();
     });
 
     ECC_Stop();

[theStack]

FillableSigningProvider is just the legacy class and contains legacy scripts limitations (thus #28307). It does not affect the current form of this PR but, because this benchmark uses segwit v0 and v1 outputs, it could cause issues in the future.

Ok, good to know.

I never said that it was going to make derivation easier. I Just said that it will also save you an extra GetScriptForDestination call per script.

Took me a while now to understand what exactly you mean, after looking deeper in what happens in FillableSigningProvider::AddKey() and realizing that leads to an indirect GetScriptForDestination call. I initially assumed that you talk about needing less GetScriptForDestination calls directly in the benchmark preparation loop (i.e. "save" interpreted in the sense of less code from me, rather than less run-time overhead), which would have surprised me.

I'm still confused on why we would want/need to fill the .scripts of the FlatSigningProvider though. Aren't those only relevant for spends where redeem scripts are involved, i.e. P2(W)SH? Your proposed patch uses the .scripts map of the provider instead of an array to store the prev_spks, but that map is never accessed inside SignTransaction.

I've changed the PR to take use of FlatSigningProvider (and GenerateRandomKey for privkey instantiation, which wasn't available in master back then), but didn't fill the .scripts map as it seems not necessary for the script types tested here.

[achow101]

ACK 3a3ccf0

[furszy]

Code ACK 3a3ccf0

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/20920916767}

[theStack]

Rebased on master, which was necessary due to the removal of ECC_{Start,Stop} from the key header in #29252.
The ECC_Context RAII wrapper is used now instead (like done in commit 28905c1). Re-review should be trivial.

[furszy]

utACK fe92c15