fix: avoid calling `GetCoin` and `SignTransaction()` inside of `assert(...)` in tests #29572

pull UdjinM6 wants to merge 2 commits into bitcoin:master from UdjinM6:fix_sign_in_assert changing 1 files +3 −3

UdjinM6 commented at 1:37 AM on March 6, 2024: contributor

Technically, it's not possible to compile without assertions at the moment because of this check. However, if it would be removed one day then txpackage_tests will start to fail for such builds.

Running txpackage_tests compiled with the check removed and CPPFLAGS="-DNDEBUG": master: *** 21 failures are detected in the test module "Bitcoin Core Test Suite" this PR: *** No errors detected
fix: avoid calling `GetCoin` and `SignTransaction()` inside of `assert(...)` in tests 92b3d564de
DrahtBot commented at 1:37 AM on March 6, 2024: contributor


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



Code Coverage

For detailed information about the code coverage, see the test coverage report.



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:
- #30849 (refactor: migrate bool GetCoin to return optional<Coin> by l0rinc)
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.
maflcko commented at 11:06 AM on March 12, 2024: member

If you are worried about assert, you can use Assert, which does not have the "problem".
suggestion: use `Assert()` instead 0ea815bfeb
DrahtBot commented at 12:31 AM on September 8, 2024: contributor



There hasn't been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

in src/test/util/setup_common.cpp:384 in 0ea815bfeb

 380 | @@ -381,16 +381,14 @@ std::pair<CMutableTransaction, CAmount> TestChain100Setup::CreateValidTransactio
 381 |      for (const auto& outpoint_to_spend : inputs) {
 382 |          // - Use GetCoin to properly populate utxo_to_spend,
 383 |          Coin utxo_to_spend;
 384 | -        bool has_utxo = coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend);

l0rinc commented at 1:50 PM on September 9, 2024:

This was addressed in https://github.com/bitcoin/bitcoin/pull/30849/files#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R454 without an assert/Assert

l0rinc commented at 1:54 PM on September 9, 2024: contributor

I'd prefix the PR with test: to obviate that it doesn't touch production code. And the PR description isn't convincing enough to me.
UdjinM6 closed this on Sep 9, 2024
bitcoin locked this on Sep 9, 2025

Contributors

fix: avoid calling `GetCoin` and `SignTransaction()` inside of `assert(...)` in tests #29572

Code Coverage

Reviews

Conflicts