tests: Don’t initialize PrecomputedTransactionData in txvalidationcache tests #18675

1. 
   jnewbery commented at 7:29 pm on April 16, 2020: member

   PrecomputedTransactionData is initialized inside CheckInputScripts(). No need to pre-initialize it before calling into CheckInputScripts().

   Normally, I wouldn’t bother, but we’re making changes to PrecomputedTransactionData in #17977 which would break these tests without removing these constructions. Might as well get these changes out of the way here.

2. [tests] Don't initialize PrecomputedTransactionData in txvalidationcache tests

   PrecomputedTransactionData is initialized inside CheckInputScripts(). No need
   to pre-initialize it before calling into CheckInputScripts().

   3718ae2ef8
3. 
   DrahtBot commented at 7:57 pm on April 16, 2020: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #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.

4. DrahtBot added the label Tests on Apr 16, 2020
5. 
   robot-visions commented at 5:04 pm on April 18, 2020: contributor

   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:

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

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

   0txdata.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?

6. 
   jnewbery commented at 9:43 pm on April 18, 2020: member

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

7. 
   robot-visions commented at 10:41 pm on April 18, 2020: contributor

   ACK 3718ae2ef8dd2559e435bf8d7f5ed5217611ce81

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

8. 
   sipa commented at 11:47 pm on April 18, 2020: member

   utACK 3718ae2ef8dd2559e435bf8d7f5ed5217611ce81
9. MarcoFalke merged this on Apr 19, 2020
10. MarcoFalke closed this on Apr 19, 2020

    ---

11. jnewbery deleted the branch on Apr 19, 2020
12. sidhujag referenced this in commit 729d3bbcee on Apr 20, 2020
13. DrahtBot locked this on Feb 15, 2022

Contributors
jnewbery DrahtBot robot-visions sipa

Labels
Tests