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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• [tests] Reduced number of validations in tx_validationcache_tests #13533 ([tests] Reduced number of validations in tx_validationcache_tests by lucash-dev)

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.

Thanks for doing this cleanup, tests passed for me!

Before I finish my review, there's something I'm hoping to understand.

The refactoring change #18401 adds the following to validation.cpp#CheckInputScripts:

if (!txdata.m_ready) {
    txdata.Init(tx);
}

However, the Schnorr/taproot/tapscript change #17977 instead does the following (no txdata.m_ready check):

txdata.Init(tx, std::move(spent_outputs));

What's the motivation for updating the tests to start with uninitialized PrecomputedTransactionData, as opposed to keeping the m_ready check in the Schnorr/taproot/tapscript change?

Thanks for the review @robot-visions

What's the motivation for updating the tests to start with uninitialized PrecomputedTransactionData

Great question! I added the if (!txdata.m_ready) to CheckInputScripts() and assert(!m_ready) to PrecomputedTransactionData::Init() following the conversation here: #18401 (comment) to ensure that a PrecomputedTransactionData object is only initialized once. That means that the subsequent commits in the Taproot PR need to be changed slightly to ensure that Init() isn't called more than once. I've done that in a branch here: https://github.com/jnewbery/bitcoin/commits/pr17977.2. One of the changes needed is to not initialize these PrecomputedTransactionData objects in the tests, which is what this PR is doing.

ACK 3718ae2

Thanks for clarifying! That makes sense—it looks like pr17977.2 includes both the if (!txdata.m_ready) check and the test update.

utACK 3718ae2