test: exercise Schnorr signature cache in txvalidationcache

theStack commented at 2:03 AM on June 7, 2026: contributor

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

test: refactor: simplify tx vin/vout creation in txvalidationcache_tests.cpp e78a2a0d00

test: respect "TAPROOT requires WITNESS" rule in `ValidateCheckInputsForAllFlags`

This is preparatory for the next commit, which introduces Taproot spends
that would fail without this rule being enforced.

While touching the function, also remove an outdated comment that refers
to a parameter that doesn't exist anymore (`upgraded_nop` was removed
in commit 01013f5d2fbe3fa86565c927bf7bb8ec0f525073, PR #10699).

198b36bc85

test: exercise Schnorr signature cache in txvalidationcache_tests.cpp 3ba1bbfa3f

DrahtBot added the label Tests on Jun 7, 2026

DrahtBot commented at 2:03 AM on June 7, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35477.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	Bortlesboat
Concept ACK	w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

ValidateCheckInputsForAllFlags(CTransaction(tr_tx), SCRIPT_VERIFY_TAPROOT, true, m_node.chainman->ActiveChainstate().CoinsTip(), m_node.chainman->m_validation_cache) in src/test/txvalidationcache_tests.cpp

w0xlt commented at 6:35 AM on June 7, 2026: contributor

Concept ACK

Bortlesboat commented at 9:22 PM on July 19, 2026: contributor

tACK 3ba1bbfa3f

Built this branch and ran test_bitcoin --run_test=txvalidationcache_tests, no errors. To check the coverage claim I also re-applied the assert(false) from the description to ComputeEntrySchnorr on top of the branch: the suite then dies on the assertion, and passes again once removed, so the new spend really does reach the Schnorr side of the cache.

The refactor commit looks behavior-preserving to me (the CTxIn ctor defaults nSequence to SEQUENCE_FINAL, same as what resize() gave before). And the TAPROOT->WITNESS implication in the second commit is load-bearing, not just cosmetic: without it a random draw of TAPROOT-without-WITNESS would evaluate the P2TR output as a bare OP_1 <32B> push under legacy rules, the damaged-signature tx would validate, and the expected-failure check would trip.

in src/test/txvalidationcache_tests.cpp:341 in 3ba1bbfa3f

 336 | +        auto& witness_stack = tr_tx.vin[0].scriptWitness.stack;
 337 | +        BOOST_REQUIRE(witness_stack.size() == 1 && witness_stack[0].size() == 64);
 338 | +
 339 | +        // Invalidate signature; an invalid Taproot key-path spend is only invalid if SCRIPT_VERIFY_TAPROOT is set
 340 | +        witness_stack[0][63] ^= 0x01; // damage signature
 341 | +        ValidateCheckInputsForAllFlags(CTransaction(tr_tx), SCRIPT_VERIFY_TAPROOT, true, m_node.chainman->ActiveChainstate().CoinsTip(), m_node.chainman->m_validation_cache);

Bortlesboat commented at 9:22 PM on July 19, 2026:

Took me a minute to convince myself the random flag draw can't break this one: combos that include DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM but not TAPROOT also have to pass here (and in the valid run below, where nothing may fail). They do, because for a 32-byte v1 program VerifyWitnessProgram returns success at the !(flags & SCRIPT_VERIFY_TAPROOT) early-exit before the discourage check is reached, so that flag can only fire for other witness versions/sizes. Fine as is, though half a sentence in the comment above wouldn't hurt.

Bortlesboat commented at 9:22 PM on July 19, 2026:

Took me a minute to convince myself the random flag draw can't break this one: combos that include DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM but not TAPROOT also have to pass here (and in the valid run below, where nothing may fail). They do, because for a 32-byte v1 program VerifyWitnessProgram returns success at the !(flags & SCRIPT_VERIFY_TAPROOT) early-exit before the discourage check is reached, so that flag can only fire for other witness versions/sizes. Fine as is, though half a sentence in the comment above wouldn't hurt.

Bortlesboat commented at 9:22 PM on July 19, 2026: contributor

tACK 3ba1bbfa3f

Built this branch and ran test_bitcoin --run_test=txvalidationcache_tests, no errors. To check the coverage claim I also re-applied the assert(false) from the description to ComputeEntrySchnorr on top of the branch: the suite then dies on the assertion, and passes again once removed, so the new spend really does reach the Schnorr side of the cache.

The refactor commit looks behavior-preserving to me (the CTxIn ctor defaults nSequence to SEQUENCE_FINAL, same as what resize() gave before). And the TAPROOT->WITNESS implication in the second commit is load-bearing, not just cosmetic: without it a random draw of TAPROOT-without-WITNESS would evaluate the P2TR output as a bare OP_1 <32B> push under legacy rules, the damaged-signature tx would validate, and the expected-failure check would trip.

test: exercise Schnorr signature cache in txvalidationcache_tests.cpp #35477

Code Coverage & Benchmarks

Reviews

LLM Linter (✨ experimental)