Skip to content

Comments

test: use BOOST_REQUIRE for fatal preconditions in coinjoin_tests#7164

Merged
PastaPastaPasta merged 1 commit intodashpay:developfrom
thepastaclaw:test/coinjoin-require-not-check
Feb 23, 2026
Merged

test: use BOOST_REQUIRE for fatal preconditions in coinjoin_tests#7164
PastaPastaPasta merged 1 commit intodashpay:developfrom
thepastaclaw:test/coinjoin-require-not-check

Conversation

@thepastaclaw
Copy link

@thepastaclaw thepastaclaw commented Feb 22, 2026

Issue

Refs #6696

Description

The CTransactionBuilderTest test intermittently crashes with a hard assert() failure in AddTxToChain():

assertion: it != wallet->mapWallet.end()
file: wallet/test/coinjoin_tests.cpp, line: 171

This happens because BOOST_CHECK(txBuilder.Commit(strResult)) is non-fatal — when Commit() fails intermittently (e.g., under UBSAN/sanitizer builds), execution continues and AddTxToChain() is called with an invalid/empty txid, hitting the hard assert().

Solution

Replace assert() and BOOST_CHECK() with BOOST_REQUIRE() at all points where subsequent code depends on the check result:

  • assert(it != wallet->mapWallet.end())BOOST_REQUIRE(...) in AddTxToChain()
  • BOOST_CHECK(txBuilder.Commit(strResult))BOOST_REQUIRE(...) (both call sites)
  • BOOST_CHECK(dest_opt) and BOOST_CHECK(res)BOOST_REQUIRE(...) in GetTallyItem() where the result is immediately dereferenced
  • assert(tallyItem.outpoints.size() == vecAmounts.size())BOOST_REQUIRE_EQUAL(...) in GetTallyItem()

BOOST_REQUIRE aborts the test case on failure, producing a clean failure message instead of a process abort via assert(). This prevents the cascading crash that makes debugging the root cause harder.

Validation

What was tested:

  • Full build and test suite including wallet/test/coinjoin_tests.cpp
  • UBSAN build to verify the previously flaky assert path now produces a clean BOOST_REQUIRE failure instead of a process abort

Results:

  • CI full suite: all applicable checks passed
    • linux64-build / Build source — pass
    • linux64-test / Test source — pass
    • linux64_ubsan-build / Build source — pass
    • linux64_ubsan-test / Test source — pass
    • mac-build / Build source — pass
    • win64-build / Build source — pass

Environment: GitHub Actions CI (linux64 + ubsan + mac + win64 matrix)

Replace assert() and BOOST_CHECK() with BOOST_REQUIRE() where subsequent
code depends on the check result. When Commit() or CreateTransaction()
fails intermittently (e.g. under sanitizer builds), BOOST_CHECK logs the
failure but continues execution, causing a cascading hard assert() crash
in AddTxToChain() when the transaction is not found in mapWallet.

BOOST_REQUIRE aborts the test case on failure, giving a clean failure
message instead of a process abort.

Fixes dashpay#6696
@github-actions
Copy link

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Feb 22, 2026

Walkthrough

This pull request modifies the coinjoin test file to replace weaker runtime assertions (assert/BOOST_CHECK) with stricter test assertions (BOOST_REQUIRE). The changes affect wallet test setup, destination reservation, transaction creation, and commit operations. By enforcing preconditions more strictly, failed assertions now halt test execution immediately rather than allowing the test to continue with potentially invalid state.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: replacing assert/BOOST_CHECK with BOOST_REQUIRE for fatal preconditions in the coinjoin_tests test file.
Linked Issues check ✅ Passed The PR directly addresses all objectives from issue #6696: replacing non-fatal checks (BOOST_CHECK/assert) with BOOST_REQUIRE at precondition points to prevent cascading crashes and ensure clean test failures.
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to replacing assert/BOOST_CHECK with BOOST_REQUIRE in the coinjoin_tests.cpp file where preconditions are checked, directly addressing the linked issue.
Description check ✅ Passed The PR description comprehensively explains the issue, solution, and validation, directly matching the changeset's replacement of runtime checks with BOOST_REQUIRE.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@thepastaclaw
Copy link
Author

Updated issue linkage: switched from Closes #6696 to Refs #6696. This PR improves test failure behavior (avoids assert-abort cascades) but does not eliminate the underlying flake root cause.

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 4f9cc3f

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 4f9cc3f

@PastaPastaPasta PastaPastaPasta merged commit 8c2c188 into dashpay:develop Feb 23, 2026
45 of 48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants