← Watch feed
Merge bitcoin/bitcoin#35477: test: exercise Schnorr signature cache in txvalidationcache_tests.cpp
Public commit record
96/100 · Strong
What the developer wrote
Authored by merge-script
Merge bitcoin/bitcoin#35477: test: exercise Schnorr signature cache in txvalidationcache_tests.cpp
3ba1bbfa3f85fae32c0b6614db9bb1a1b2db8ce7 test: exercise Schnorr signature cache in txvalidationcache_tests.cpp (Sebastian Falbesoner)
198b36bc85d54792af82ea9b9e58554608573cb6 test: respect "TAPROOT requires WITNESS" rule in `ValidateCheckInputsForAllFlags` (Sebastian Falbesoner)
e78a2a0d00cf67637eae4caf4db1735f110aff14 test: refactor: simplify tx vin/vout creation in txvalidationcache_tests.cpp (Sebastian Falbesoner)
Pull request description:
The Schnorr verification path of the signature cache is currently never hit in the unit tests, i.e. with the following patch they still pass:
```diff
diff --git a/src/script/sigcache.cpp b/src/script/sigcache.cpp
index c6fcc8f8eb..87688c1049 100644
--- a/src/script/sigcache.cpp
+++ b/src/script/sigcache.cpp
@@ -44,6 +44,7 @@ void SignatureCache::ComputeEntryECDSA(uint256& entry, const uint256& hash, cons
void SignatureCache::ComputeEntrySchnorr(uint256& entry, const uint256& hash, std::span<const unsigned char> sig, const XOnlyPubKey& pubkey) const
{
+ assert(false);
CSHA256 hasher = m_salted_hasher_schnorr;
hasher.Write(hash.begin(), 32).Write(pubkey.data(), pubkey.size()).Write(sig.data(), sig.size()).Finalize(entry.begin());
}
```
This PR adds missing coverage for that by adding a Taproot key-path spend to `checkinputs_test` in `txvalidationcache_tests.cpp`. Same as for the already-existing ECDSA spends, the caching is tested across a large number of flag combinations (using `ValidateCheckInputsForAllFlags`), both with an invalid Schnorr signature (-> should only fail if `SCRIPT_VERIFY_TAPROOT` is set) and a valid one (-> should pass for all flag combinations).
ACKs for top commit:
Bortlesboat:
tACK 3ba1bbfa3f
sedited:
ACK 3ba1bbfa3f85fae32c0b6614db9bb1a1b2db8ce7
instagibbs:
ACK 3ba1bbfa3f85fae32c0b6614db9bb1a1b2db8ce7
Tree-SHA512: e43f7077d9e9ab6f8b5e9e70f0187767d65f686ce24350ce5d61cc4cdf07d5eebdf5e4327ce665c212b7cc02be1e8632a6a9fbcf6be2916f8e058993e5fb2650
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Mentions testing or verification✓ Links an issue, advisory, or supporting reference✓ Names security-relevant behavior explicitly
What changed, and why it matters
This commit only adds and refactors unit tests for Bitcoin Core's transaction validation cache. It does not change any production code that runs on the live Bitcoin network. The new tests specifically exercise the Schnorr signature verification path used by Taproot transactions, ensuring that part of the caching logic is actually tested. There is no security vulnerability being fixed here.
Recommended action
No action required. This is a test-coverage improvement and can be treated as routine QA.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The commit merges PR #35477, which is a pure test-only change in src/test/txvalidationcache_tests.cpp. It adds a Taproot key-path spend test case to exercise the Schnorr branch of the signature cache (ComputeEntrySchnorr), updates ValidateCheckInputsForAllFlags to enforce that SCRIPT_VERIFY_TAPROOT implies SCRIPT_VERIFY_WITNESS, and refactors vin/vout creation to use initializer lists. No consensus, validation, or networking code is modified.
Changed components
src/test/txvalidationcache_tests.cppInspect captured patch +54 / −54
### src/test/txvalidationcache_tests.cpp
@@ -49,12 +49,8 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup)
for (int i = 0; i < 2; i++)
{
spends[i].version = 1;
- spends[i].vin.resize(1);
- spends[i].vin[0].prevout.hash = m_coinbase_txns[0]->GetHash();
- spends[i].vin[0].prevout.n = 0;
- spends[i].vout.resize(1);
- spends[i].vout[0].nValue = 11*CENT;
- spends[i].vout[0].scriptPubKey = scriptPubKey;
+ spends[i].vin = {CTxIn{m_coinbase_txns[0]->GetHash(), 0}};
+ spends[i].vout = {CTxOut{11*CENT, scriptPubKey}};
// Sign:
std::vector<unsigned char> vchSig;
@@ -118,8 +114,6 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup)
// CHECKLOCKTIMEVERIFY (or CHECKSEQUENCEVERIFY), but the script does contain
// OP_CHECKLOCKTIMEVERIFY (or OP_CHECKSEQUENCEVERIFY), then script execution
// should fail.
-// Capture this interaction with the upgraded_nop argument: set it when evaluating
-// any script flag that is implemented as an upgraded NOP code.
static void ValidateCheckInputsForAllFlags(const CTransaction &tx, script_verify_flags failing_flags, bool add_to_cache, CCoinsViewCache& active_coins_tip, ValidationCache& validation_cache) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
{
PrecomputedTransactionData txdata;
@@ -138,6 +132,10 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, script_verify
// script/interpreter.cpp
test_flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS;
}
+ if ((test_flags & SCRIPT_VERIFY_TAPROOT)) {
+ // TAPROOT requires WITNESS
+ test_flags |= SCRIPT_VERIFY_WITNESS;
+ }
if ((test_flags & SCRIPT_VERIFY_WITNESS)) {
// WITNESS requires P2SH
test_flags |= SCRIPT_VERIFY_P2SH;
@@ -172,6 +170,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
CScript p2sh_scriptPubKey = GetScriptForDestination(ScriptHash(p2pk_scriptPubKey));
CScript p2pkh_scriptPubKey = GetScriptForDestination(PKHash(coinbaseKey.GetPubKey()));
CScript p2wpkh_scriptPubKey = GetScriptForDestination(WitnessV0KeyHash(coinbaseKey.GetPubKey()));
+ CScript p2tr_scriptPubKey = GetScriptForDestination(WitnessV1Taproot(XOnlyPubKey(coinbaseKey.GetPubKey())));
FillableSigningProvider keystore;
BOOST_CHECK(keystore.AddKey(coinbaseKey));
@@ -184,18 +183,14 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
CMutableTransaction spend_tx;
spend_tx.version = 1;
- spend_tx.vin.resize(1);
- spend_tx.vin[0].prevout.hash = m_coinbase_txns[0]->GetHash();
- spend_tx.vin[0].prevout.n = 0;
- spend_tx.vout.resize(4);
- spend_tx.vout[0].nValue = 11*CENT;
- spend_tx.vout[0].scriptPubKey = p2sh_scriptPubKey;
- spend_tx.vout[1].nValue = 11*CENT;
- spend_tx.vout[1].scriptPubKey = p2wpkh_scriptPubKey;
- spend_tx.vout[2].nValue = 11*CENT;
- spend_tx.vout[2].scriptPubKey = CScript() << OP_CHECKLOCKTIMEVERIFY << OP_DROP << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
- spend_tx.vout[3].nValue = 11*CENT;
- spend_tx.vout[3].scriptPubKey = CScript() << OP_CHECKSEQUENCEVERIFY << OP_DROP << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
+ spend_tx.vin = {CTxIn{m_coinbase_txns[0]->GetHash(), 0}};
+ spend_tx.vout = {
+ CTxOut{11*CENT, p2sh_scriptPubKey},
+ CTxOut{11*CENT, p2wpkh_scriptPubKey},
+ CTxOut{11*CENT, CScript() << OP_CHECKLOCKTIMEVERIFY << OP_DROP << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG},
+ CTxOut{11*CENT, CScript() << OP_CHECKSEQUENCEVERIFY << OP_DROP << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG},
+ CTxOut{11*CENT, p2tr_scriptPubKey},
+ };
// Sign, with a non-DER signature
{
@@ -246,12 +241,8 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
{
CMutableTransaction invalid_under_p2sh_tx;
invalid_under_p2sh_tx.version = 1;
- invalid_under_p2sh_tx.vin.resize(1);
- invalid_under_p2sh_tx.vin[0].prevout.hash = spend_tx.GetHash();
- invalid_under_p2sh_tx.vin[0].prevout.n = 0;
- invalid_under_p2sh_tx.vout.resize(1);
- invalid_under_p2sh_tx.vout[0].nValue = 11*CENT;
- invalid_under_p2sh_tx.vout[0].scriptPubKey = p2pk_scriptPubKey;
+ invalid_under_p2sh_tx.vin = {CTxIn{spend_tx.GetHash(), 0}};
+ invalid_under_p2sh_tx.vout = {CTxOut{11*CENT, p2pk_scriptPubKey}};
std::vector<unsigned char> vchSig2(p2pk_scriptPubKey.begin(), p2pk_scriptPubKey.end());
invalid_under_p2sh_tx.vin[0].scriptSig << vchSig2;
@@ -263,13 +254,8 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
CMutableTransaction invalid_with_cltv_tx;
invalid_with_cltv_tx.version = 1;
invalid_with_cltv_tx.nLockTime = 100;
- invalid_with_cltv_tx.vin.resize(1);
- invalid_with_cltv_tx.vin[0].prevout.hash = spend_tx.GetHash();
- invalid_with_cltv_tx.vin[0].prevout.n = 2;
- invalid_with_cltv_tx.vin[0].nSequence = 0;
- invalid_with_cltv_tx.vout.resize(1);
- invalid_with_cltv_tx.vout[0].nValue = 11*CENT;
- invalid_with_cltv_tx.vout[0].scriptPubKey = p2pk_scriptPubKey;
+ invalid_with_cltv_tx.vin = {CTxIn{spend_tx.GetHash(), 2, {}, /*nSequenceIn=*/0}};
+ invalid_with_cltv_tx.vout = {CTxOut{11*CENT, p2pk_scriptPubKey}};
// Sign
std::vector<unsigned char> vchSig;
@@ -291,13 +277,8 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
{
CMutableTransaction invalid_with_csv_tx;
invalid_with_csv_tx.version = 2;
- invalid_with_csv_tx.vin.resize(1);
- invalid_with_csv_tx.vin[0].prevout.hash = spend_tx.GetHash();
- invalid_with_csv_tx.vin[0].prevout.n = 3;
- invalid_with_csv_tx.vin[0].nSequence = 100;
- invalid_with_csv_tx.vout.resize(1);
- invalid_with_csv_tx.vout[0].nValue = 11*CENT;
- invalid_with_csv_tx.vout[0].scriptPubKey = p2pk_scriptPubKey;
+ invalid_with_csv_tx.vin = {CTxIn{spend_tx.GetHash(), 3, {}, /*nSequenceIn=*/100}};
+ invalid_with_csv_tx.vout = {CTxOut{11*CENT, p2pk_scriptPubKey}};
// Sign
std::vector<unsigned char> vchSig;
@@ -322,12 +303,8 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
{
CMutableTransaction valid_with_witness_tx;
valid_with_witness_tx.version = 1;
- valid_with_witness_tx.vin.resize(1);
- valid_with_witness_tx.vin[0].prevout.hash = spend_tx.GetHash();
- valid_with_witness_tx.vin[0].prevout.n = 1;
- valid_with_witness_tx.vout.resize(1);
- valid_with_witness_tx.vout[0].nValue = 11*CENT;
- valid_with_witness_tx.vout[0].scriptPubKey = p2pk_scriptPubKey;
+ valid_with_witness_tx.vin = {CTxIn{spend_tx.GetHash(), 1}};
+ valid_with_witness_tx.vout = {CTxOut{11*CENT, p2pk_scriptPubKey}};
// Sign
SignatureData sigdata;
@@ -342,19 +319,42 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
ValidateCheckInputsForAllFlags(CTransaction(valid_with_witness_tx), SCRIPT_VERIFY_WITNESS, true, m_node.chainman->ActiveChainstate().CoinsTip(), m_node.chainman->m_validation_cache);
}
+ // Test a Taproot (witness v1) key-path spend, to exercise the Schnorr branch of the signature cache.
+ {
+ CMutableTransaction tr_tx;
+ tr_tx.vin = {CTxIn{spend_tx.GetHash(), 4}};
+ tr_tx.vout = {CTxOut{11*CENT, p2pk_scriptPubKey}};
+
+ // Sign P2TR output for key-path spending (i.e. add Schnorr signature to witness stack)
+ FlatSigningProvider tr_keystore;
+ tr_keystore.keys.emplace(coinbaseKey.GetPubKey().GetID(), coinbaseKey);
+ const std::map<COutPoint, Coin> coins{
+ {tr_tx.vin[0].prevout, Coin(spend_tx.vout[4], /*nHeightIn=*/0, /*fCoinBaseIn=*/false)}
+ };
+ std::map<int, bilingual_str> input_errors;
+ BOOST_REQUIRE(SignTransaction(tr_tx, &tr_keystore, coins, {.sighash_type = SIGHASH_DEFAULT}, input_errors));
+ auto& witness_stack = tr_tx.vin[0].scriptWitness.stack;
+ BOOST_REQUIRE(witness_stack.size() == 1 && witness_stack[0].size() == 64);
+
+ // Invalidate signature; an invalid Taproot key-path spend is only invalid if SCRIPT_VERIFY_TAPROOT is set
+ witness_stack[0][63] ^= 0x01; // damage signature
+ ValidateCheckInputsForAllFlags(CTransaction(tr_tx), SCRIPT_VERIFY_TAPROOT, true, m_node.chainman->ActiveChainstate().CoinsTip(), m_node.chainman->m_validation_cache);
+ witness_stack[0][63] ^= 0x01; // repair signature
+
+ // A valid Taproot key-path spend is valid under all flags
+ ValidateCheckInputsForAllFlags(CTransaction(tr_tx), 0, true, m_node.chainman->ActiveChainstate().CoinsTip(), m_node.chainman->m_validation_cache);
+ }
+
{
// Test a transaction with multiple inputs.
CMutableTransaction tx;
tx.version = 1;
- tx.vin.resize(2);
- tx.vin[0].prevout.hash = spend_tx.GetHash();
- tx.vin[0].prevout.n = 0;
- tx.vin[1].prevout.hash = spend_tx.GetHash();
- tx.vin[1].prevout.n = 1;
- tx.vout.resize(1);
- tx.vout[0].nValue = 22*CENT;
- tx.vout[0].scriptPubKey = p2pk_scriptPubKey;
+ tx.vin = {
+ CTxIn{spend_tx.GetHash(), 0},
+ CTxIn{spend_tx.GetHash(), 1},
+ };
+ tx.vout = {CTxOut{22*CENT, p2pk_scriptPubKey}};
// Sign
for (int i = 0; i < 2; ++i) {Risk score
Our methodology →Why this scored 15/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.