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/29491.

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:

• #34184 (mining: add cooldown to createNewBlock() immediately after IBD by Sjors)
• #32575 (refactor: Remove special treatment for single threaded script checking by fjahr)
• #32317 (kernel: Separate UTXO set access from validation functions by sedited)
• #29843 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) by ajtowns)
• #28690 (build: Introduce internal kernel library by sedited)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

• memeory -> memory [spelling error making the word unclear]

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

• secp256k1_batch_add_schnorrsig(..., 32, ...) in src/batchverify.cpp

^{2026-01-31 14:01:32}

🚧 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/22041704016}

I guess this is most interesting to test against the last 50K blocks on mainnet, i.e. since early 2023? Given the high percentage of taproot transactions. Such testing could benefit from a assume utxo snapshot, saving everyone the pain of rewinding. I can bake one, though ideally I'd like #26045 to land first.

Is the current PR already using batches? It's unclear to me by glancing over validation.cpp if and how it's collecting up to max_batch_size{106} signatures before calling batch.Verify().

Maybe add a -schnorrbatchverify startup argument so more people can try it once it gets through review.

I guess this is most interesting to test against the last 50K blocks on mainnet, i.e. since early 2023? Given the high percentage of taproot transactions. Such testing could benefit from a assume utxo snapshot, saving everyone the pain of rewinding. I can bake one, though ideally I'd like #26045 to land first.

Yeah, that would be great. I have to get the tracing part to work in my setup again before I can start.

Is the current PR already using batches? It's unclear to me by glancing over validation.cpp if and how it's collecting up to max_batch_size{106} signatures before calling batch.Verify().

Yes, the rough architecture is as as follows (sorry for not writing a better explanation above but it's still very rough and I expect it to change). So, the batch object is constructed and then handed down from ConnectBlock() to CheckInputScripts() which hands it to the CScriptCheck constructor. When the CScriptCheck instance is executed and the batch object is present BatchingCachingTransactionSignatureChecker is used which only differs from the default CachingTransactionSignatureChecker in that its implementation of VerifySchnorrSignature() and adds the Schnorr sig to the batch object instead of verifying it right there. (Here we have an issue because the function is not doing what the name says anymore but it is a much simpler change this way and I find it hard to predict where we will land with this in the end.) Then back in ConnectBlock() the batch object is verified after all transactions have been iterated over.

The 106 is an implementation detail from secp256k1 that I am not sure needs to be really exposed here. But what matters for the question is: if there are more than 106 sigs added to the batch the verification for the already added sigs will happen when the next sig is added and if the verification fails the adding itself will fail. So, a callback to the last paragraph, every 106 sigs the naming of VerifySchnorrSignature() is actually still correct ;) and the one verify call in the end just verifies the last n % 106 sigs left for that block. I did not put any effort in documenting this properly here yet because the secp256k1 API is not finalized yet and the details on this particular topic might still change, see for example bitcoin-core/secp256k1#1134 (review)

Everything should be conditional on the presence of a batch object which defaults to nullptr and if that is the case all the other functions should work as usual, for example when used outside of the context of a new block.

Maybe add a -schnorrbatchverify startup argument so more people can try it once it gets through review.

Yeah, I have been thinking that or maybe even a compiler argument

@Sjors maybe what confused you is that BatchingCachingTransactionSignatureChecker::VerifySchnorrSignatureBatch() was unused. That was actually a leftover from an earlier design spike and I just forgot to remove it. It's gone now.

if there are more than 106 sigs added to the batch the verification for the already added sigs will happen when the next sig is added and if the verification fails the adding itself will fail.

That clarifies a lot, and should be documented :-)

@willcl-ark and I ran a IBD benchmark vs master for this PR: IBD from a local node to height 840000 and currently only looking at the connect_block duration.

In the current draft implementation:

• master is currently faster
• batch validation is attempted and decreases performance even before taproot activated
• after taproot activation, batch validation is significantly slower than non-batch validation

Used this bpftrace script to collect CSV data:

#!/usr/bin/env bpftrace

usdt:./src/bitcoind:validation:block_connected
{
  $height = (int32) arg1;
  $tx = (uint64) arg2;
  $ins = (int32) arg3;
  $sigops = (int64) arg4;
  $duration = (int64) arg5; // in nanoseconds

  printf("%d,%ld,%d,%ld,%ld\n", $height, $tx, $ins, $sigops, $duration);
}

batch validation is attempted and decreases performance even before taproot activated

From a quick skim of the changes ISTM that we always use the BatchingCachingTransactionSignatureChecker, and there was no switch to the CachingTransactionSignatureChecker, but this could well be because this is only in WIP state. It doesnt account for why it's currently slower in all cases though.

It seems in this approach the m_batch_mutex is held for the entirety of adding, which will cause lock contention. Verify is then a blocking call on the main thread, which negates the multithreading.

I think a better approach would be to have a thread local batch for each worker thread and the main thread, so each thread can add all schnorr sigs without locking, and then each thread can verify their batches in parallel at the end.

🚧 At least one of the CI tasks failed.
_{Task fuzzer,address,undefined,integer: https://github.com/bitcoin/bitcoin/actions/runs/21364278324/job/61491586498
LLM reason (✨ experimental): Fuzz target crash: libFuzzer reported a deadly signal (exit code 77) during fuzz run.}

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