refactor: test/bench: dedup Build{Crediting,Spending}Transaction() #17183

1. 
   theStack commented at 0:23 am on October 18, 2019: member

   prototypes used in src/test/script_tests.cpp:

   • CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey, int nValue = 0);
   • CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CScriptWitness& scriptWitness, const CTransaction& txCredit);

   prototypes used in bench/verify_script.cpp:

   • CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey);
   • CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CMutableTransaction& txCredit);

   The more generic versions from the script tests are moved into setup_common.cpp and the calls are adapted accordingly in the verify_script benchmark (passing the nValue of 1 explicitely for BuildCreditingTransaction(), passing empty scriptWitness explicitely and converting txCredit parameter to CTransaction in BuildSpendingTransaction()).

2. fanquake added the label Tests on Oct 18, 2019
3. 
   MarcoFalke commented at 12:48 pm on October 18, 2019: member

   re-run ci
4. MarcoFalke closed this on Oct 18, 2019

   ---

5. MarcoFalke reopened this on Oct 18, 2019

   ---

6. 
   theStack commented at 10:59 am on October 19, 2019: member

   re-run ci

   Thanks. Unfortunately it has failed again, with another reason this time though (Job lint macOS 10.12 (compat)):

   ==> Pouring portable-ruby–2.6.3.mavericks.bottle.tar.gz … No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

7. in src/test/setup_common.cpp:200 in c3b6301df8 outdated

   195@@ -196,3 +196,37 @@ CBlock getBlock13b8a()
   196     stream >> block;
   197     return block;
   198 }
   199+
   200+CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey, int nValue)
   

   ---

   ---

   

   laanwj commented at 3:08 pm on October 21, 2019:

   I don’t think setup_common.cpp is the right place for this (organization-wise, but also so that the boost-test-related stuff doesn’t end up in the bench). Please define a new cpp/h header pair in src/test for transaction utils, or something like that.

   ---

   

   theStack commented at 8:24 pm on October 21, 2019:

   Thanks for the feedback, done.
8. theStack force-pushed on Oct 21, 2019
9. theStack force-pushed on Oct 21, 2019
10. 
    theStack commented at 8:28 pm on October 21, 2019: member

    Still not 100% sure about the name of the new file (currently named transaction_utils.h/cpp). Should it also have the suffix _common to show that it is shared between the unit tests and the benchmarks? Open for any suggestions.
11. refactor: test/bench: dedup Build{Crediting,Spending}Transaction()

    prototypes used in src/test/script_tests.cpp:
    - CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey, int nValue = 0);
    - CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CScriptWitness& scriptWitness, const CTransaction& txCredit);
    
    prototypes used in bench/verify_script.cpp:
    - CMutableTransaction BuildCreditingTransaction(const CScript& scriptPubKey);
    - CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CMutableTransaction& txCredit);
    
    The more generic versions from the script tests are moved into a new file pair
    transaction_utils.cpp/h and the calls are adapted accordingly in the
    verify_script benchmark (passing the nValue of 1 explicitely for
    BuildCreditingTransaction(), passing empty scriptWitness explicitely and
    converting txCredit parameter to CTransaction in BuildSpendingTransaction()).

    a0fc076476
12. in src/Makefile.test.include:60 in 8fdd33b38f outdated

    54@@ -55,7 +55,9 @@ GENERATED_TEST_FILES = $(JSON_TEST_FILES:.json=.json.h) $(RAW_TEST_FILES:.raw=.r
    55 BITCOIN_TEST_SUITE = \
    56   test/main.cpp \
    57   test/setup_common.h \
    58-  test/setup_common.cpp
    59+  test/setup_common.cpp \
    60+  test/transaction_utils.h \
    61+  test/transaction_utils.cpp
    

    ---

    ---

    

    MarcoFalke commented at 5:33 pm on October 22, 2019:

    I think the file should be in a subfolder test/lib/, similar to test/lib/logging (https://github.com/bitcoin/bitcoin/pull/16540/files)

    ---

    

    theStack commented at 0:06 am on October 23, 2019:

    Thanks, done. Also added the test/lib/ folder to the Visual Studio build, like in your referenced PR.
13. theStack force-pushed on Oct 23, 2019
14. 
    DrahtBot commented at 4:20 am on October 23, 2019: 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:

    • #16540 (test: Add ASSERT_DEBUG_LOG to unit test framework by MarcoFalke)
    • #15845 (wallet: Fast rescan with BIP157 block filters by MarcoFalke)

    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.

15. MarcoFalke referenced this in commit 8f14d2002b on Oct 23, 2019
16. MarcoFalke merged this on Oct 23, 2019
17. MarcoFalke closed this on Oct 23, 2019

    ---

18. 
    MarcoFalke commented at 1:33 pm on October 23, 2019: member

    Thanks. ACK
19. deadalnix referenced this in commit 69616da09d on Apr 22, 2020
20. ftrader referenced this in commit ebf3d99335 on Aug 17, 2020
21. theStack deleted the branch on Dec 1, 2020
22. DrahtBot locked this on Feb 15, 2022

Contributors
theStack MarcoFalke laanwj DrahtBot

Labels
Tests