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

pull theStack wants to merge 1 commits into bitcoin:master from theStack:20191018-refactor-deduplicate_build_transaction_functions changing 7 files +67 −71

theStack commented at 12:23 AM on October 18, 2019: contributor
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()).
fanquake added the label Tests on Oct 18, 2019
MarcoFalke commented at 12:48 PM on October 18, 2019: member

re-run ci
MarcoFalke closed this on Oct 18, 2019
MarcoFalke reopened this on Oct 18, 2019
theStack commented at 10:59 AM on October 19, 2019: contributor

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.
MarcoFalke cross-referenced this on Oct 19, 2019 from issue ci: Cleanup macOS runs by MarcoFalke
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.
theStack force-pushed on Oct 21, 2019
theStack force-pushed on Oct 21, 2019
theStack commented at 8:28 PM on October 21, 2019: contributor

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.

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

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 12:06 AM on October 23, 2019:

Thanks, done. Also added the test/lib/ folder to the Visual Studio build, like in your referenced PR.
theStack force-pushed on Oct 23, 2019
DrahtBot commented at 4:20 AM on October 23, 2019: contributor


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.
MarcoFalke referenced this in commit 8f14d2002b on Oct 23, 2019
MarcoFalke merged this on Oct 23, 2019
MarcoFalke closed this on Oct 23, 2019
MarcoFalke commented at 1:33 PM on October 23, 2019: member

Thanks. ACK
MarkLTZ cross-referenced this on Apr 4, 2020 from issue Bitcoin PR tracking by MarkLTZ
deadalnix referenced this in commit 69616da09d on Apr 22, 2020
ftrader referenced this in commit ebf3d99335 on Aug 17, 2020
theStack deleted the branch on Dec 1, 2020
kwvg cross-referenced this on Oct 12, 2021 from issue merge bitcoin#13219...#15779: benchmarks by kwvg
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#17176 ci: Cleanup macOS runs

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

Conflicts