Merge bitcoin/bitcoin#25073: test: Cleanup miner_tests

[DashCoreAutoGuix]

Backports bitcoin#25073

Original commit: 9eaa5db

This cleans up the miner tests:

• Removes duplicate/redundant and thus confusing chainparams object.
• Uses a fresh mempool for each subtest instead of using the "global" one from the testing setup.

Dash-specific adaptations:

• Used Dash's CTxMemPool constructor signature in MakeMempool() since MemPoolOptionsForTest is not yet available
• Kept Dash's BlockAssembler signature (requires m_node and Params())
• Kept Dash's TestSequenceLocks implementation using the refactored API from refactor: Move calculation logic out from CheckSequenceLocksAtTip() bitcoin/bitcoin#23897
• Kept Dash-specific values (500*COIN block subsidy, Now() time, SigOps(20))
• Used MemPoolRemovalReason::CONFLICT instead of REPLACED (not available in Dash)

🤖 Generated with Claude Code

Summary by CodeRabbit

• Tests
  • Refactored mining test infrastructure for improved test isolation and maintainability.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The pull request refactors the miner test infrastructure to use a per-test mempool instead of relying on a global m_node.mempool. A new MakeMempool() helper is introduced, and public test method signatures are updated to explicitly accept and pass around a CTxMemPool& reference, with simplified locking annotations reflecting reduced global state dependencies.

Changes

Cohort / File(s)

Summary

Mempool-centric test refactoring src/test/miner_tests.cpp

Updated TestPackageSelection, TestBasicMining, TestPrioritisedMining, and TestSequenceLocks methods to accept explicit CTxMemPool& parameters instead of CChainParams&, removing m_node.mempool->cs from lock annotations. Added MakeMempool() helper to instantiate per-test mempools. Modified AssemblerForTest() to accept CTxMemPool& instead of CChainParams&. Updated all call sites, mempool operations (addUnchecked, removeRecursive, etc.), and block assembly invocations to use local tx_mempool instead of global mempool, with corresponding locking adjustments.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Specific areas requiring attention:
  • Consistency of CTxMemPool& parameter passing across all updated test methods and their call sites
  • Lock annotation changes and correctness of EXCLUSIVE_LOCKS_REQUIRED(::cs_main) requirements without the mempool cs lock
  • Correct initialization and scope management of MakeMempool() to ensure test isolation
  • Verification that all mempool operations (addUnchecked, removeRecursive, etc.) are properly guarded with LOCK(tx_mempool.cs) instead of previous global lock patterns

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the main change: it's a cleanup of miner_tests that refactors the test to use per-test mempools instead of the global mempool, and removes redundant chainparams parameters.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch backport-0.25-batch-469-pr-25073

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93d8697 and 4a1108c.

📒 Files selected for processing (1)

• src/test/miner_tests.cpp (19 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

C++20 codebase should be placed under src/

Files:

• src/test/miner_tests.cpp

src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

• src/test/miner_tests.cpp

**

⚙️ CodeRabbit configuration file

**: # CodeRabbit AI Review Instructions for Dash Backports

Your Role

You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.

Critical Validation Rules

1. File Operations Must Match (AUTO-REJECT if violated)

• If Bitcoin modifies an existing file → Dash MUST modify (not create new)
• If Bitcoin creates a new file → Dash creates
• If Bitcoin deletes a file → Dash deletes
• Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys

2. Size Ratio Check (80-150% of Bitcoin)

• Count functional lines changed (exclude comments/whitespace)
• Dash changes should be 80-150% of Bitcoin's size
• Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash

3. No Scope Creep

• Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
• No unrelated refactoring or style changes
• Only Bitcoin's intended changes + minimal Dash adaptations

4. Bitcoin-Specific Code Detection

• Auto-reject witness/segwit code: msg_wtxidrelay, MSG_WTX, witness imports
• Auto-reject RBF (replace-by-fee) functionality
• Note: PSBT is supported in Dash (don't flag)

5. Mandatory Adaptations Only

• bitcoin → dash in strings/paths
• Bitcoin → Dash in user-facing text
• Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
• Hardcoded test values specific to Dash
• No other changes unless absolutely required

6. Completeness Check

• All files changed in Bitcoin must be present
• Extra files need clear justification (Dash-specific compatibility)
• Missing files = incomplete backport

Review Process

1. First: Check file operations match exactly
2. Second: Calculate size ratio
3. Third: Scan for scope creep patterns
4. Fourth: Detect Bitcoin-specific code
5. Fifth: Verify all changes are minimal adaptations

Output Format

VALIDATION: [PASS/FAIL]

File Operatio...

Files:

• src/test/miner_tests.cpp

🧠 Learnings (5)

📓 Common learnings

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T19:54:21.426Z
Learning: In Dash backports from Bitcoin Core, including necessary compilation fixes (such as API compatibility changes like UniValue get_int() → getInt<int>()) alongside the core backport is standard and expected practice. These compatibility fixes ensure the backported code compiles in Dash's evolved codebase while preserving Bitcoin's original functionality and intent.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: When backporting Bitcoin Core commits that use Python's textwrap.dedent() function in test files, the textwrap import statement needs to be explicitly added if it's missing in the Dash test file.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: In Dash backports from Bitcoin Core, when the DIFFICULTY_ADJUSTMENT_INTERVAL constant is missing, it should be defined as 24 for Dash (different from Bitcoin's value), as seen in the getnetworkhashps RPC backport fix.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-27T22:35:10.176Z
Learning: In Dash backports, src/dashbls files are vendored dependencies that should not be modified during Bitcoin Core backports unless there is specific justification. Unauthorized modifications to vendored dependencies should be removed to maintain code integrity.

📚 Learning: 2025-07-28T22:03:12.364Z

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.

Applied to files:

• src/test/miner_tests.cpp

📚 Learning: 2025-07-28T20:34:29.061Z

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.

Applied to files:

• src/test/miner_tests.cpp

📚 Learning: 2025-07-29T21:29:32.827Z

Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.

Applied to files:

• src/test/miner_tests.cpp

📚 Learning: 2025-07-28T23:09:09.522Z

Learnt from: CR
Repo: DashCoreAutoGuix/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Applied to files:

• src/test/miner_tests.cpp

🧬 Code graph analysis (1)

src/test/miner_tests.cpp (2)

src/test/util/txmempool.h (1)

• spendsCoinbase (19-32)

src/test/util/setup_common.h (1)

• HasReason (232-243)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: x86_64-w64-mingw32 / Build depends
• GitHub Check: x86_64-apple-darwin / Build depends
• GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
• GitHub Check: arm-linux-gnueabihf / Build depends
• GitHub Check: x86_64-pc-linux-gnu / Build depends
• GitHub Check: Lint / Run linters

🔇 Additional comments (4)

src/test/miner_tests.cpp (4)

1-641: Backport validation for miner_tests cleanup: PASS

VALIDATION: PASS
File Operations: MATCH – only src/test/miner_tests.cpp is modified, as in the Bitcoin commit.
Size Ratio: Within expected 80–150% – changes closely mirror upstream, with only small Dash-specific constructor/signature adaptations.
Scope Creep: NONE – no new tests or unrelated refactors; all edits are about using a per-test mempool and updating the helpers accordingly.
Bitcoin-Specific Code: NONE – no witness/RBF/MSG_WTX/wtxid additions.
Completeness: COMPLETE for this commit – all behavior changes described in the PR summary are present here and nothing extra.

This looks like a faithful Dash backport of bitcoin#25073 with only the necessary Dash adaptations (CTxMemPool ctor, BlockAssembler params, 500*COIN, Now(), SigOps(20), MemPoolRemovalReason::CONFLICT). As per coding guidelines, this stays well within scope.

---

6-7: Helper/API refactor (TestSequenceLocks, MakeMempool, AssemblerForTest) is consistent and safe

• Adding <coins.h> is required for CCoinsViewMemPool used in TestSequenceLocks.
• TestSequenceLocks(const CTransaction&, CTxMemPool&) now builds a CCoinsViewMemPool over the passed tx_mempool and uses CalculateLockPointsAtTip/CheckSequenceLocksAtTip, matching the refactored API and avoiding direct dependence on m_node.mempool.
• MakeMempool() resets m_node.mempool and returns a reference to the new CTxMemPool, which is then immediately locked and only used within the corresponding scope; no reference survives across subsequent MakeMempool() calls.
• AssemblerForTest(CTxMemPool&) now explicitly wires the chosen mempool into BlockAssembler while preserving Dash’s constructor shape (ActiveChainstate(), m_node, &tx_mempool, Params(), options).

These changes correctly decouple the helpers from the global mempool while keeping the Dash-specific API intact.

Also applies to: 39-61, 68-75

---

113-219: Per-test mempool usage in package/prioritised selection tests matches upstream behavior

• Both TestPackageSelection and TestPrioritisedMining now start with CTxMemPool& tx_mempool{MakeMempool()}; LOCK(tx_mempool.cs);, ensuring a clean mempool per subtest and explicit locking rather than relying on m_node.mempool.
• All addUnchecked, removeRecursive, and PrioritiseTransaction calls have been switched from the global mempool to tx_mempool with no behavior changes to fee logic, ancestor packages, or prioritisation semantics.
• The Dash-specific adaptation to use MemPoolRemovalReason::CONFLICT in removeRecursive is appropriate, as the tests only care that the conflicting entry is removed, not about the exact reason enum.
• CreateNewBlock invocations all go through AssemblerForTest(tx_mempool), so each test’s template is built purely from its local mempool content.

This cleanly implements the intended “fresh mempool per subtest” behavior without altering the economic logic of the tests.

Also applies to: 174-181, 194-216, 499-575

---

221-497: Basic mining and sequence-lock tests correctly migrated to local mempool and integrate with the fixture

• TestBasicMining now uses MakeMempool() in each logical block, with CTxMemPool& tx_mempool{MakeMempool()}; LOCK(tx_mempool.cs); so sigops, size, orphan, coinbase, double-spend, and P2SH validation subtests all operate on isolated mempools.
• The BLOCKSUBSIDY of 500 * COIN, Now<NodeSeconds>() timestamps, and SigOps(20) usage are preserved as Dash-specific behavior while only swapping m_node.mempool for tx_mempool.
• The sequence-lock section uses a single tx_mempool instance for all relative/absolute height/time scenarios and now calls TestSequenceLocks(tx, tx_mempool) everywhere, which is consistent with the new helper signature and keeps logic unchanged.
• In CreateNewBlock_validity, const CChainParams& chainparams = Params(); replaces the redundant chainparams argument, and the initial CTxMemPool& tx_mempool{*m_node.mempool}; is only used to build pemptyblocktemplate before any MakeMempool() calls, avoiding any dangling-reference issues after mempool resets.
• All calls to TestBasicMining, TestPackageSelection, and TestPrioritisedMining are made under a single LOCK(cs_main);, preserving the original lock ordering (cs_main then tx_mempool.cs) in the new structure.

Overall, the refactor maintains the original test intent while cleaning up mempool state handling in a way that aligns with the upstream commit.

Also applies to: 389-497, 392-478, 581-588, 630-641

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[DashCoreAutoGuix]

Verification Agent - Fixes Applied

Issue Found

The backport incorrectly scoped the LOCK(cs_main) statement, causing it to be released before calling TestPackageSelection and TestPrioritisedMining. This resulted in thread-safety analysis errors on macOS:

test/miner_tests.cpp:637:5: error: calling function 'TestPackageSelection' requires holding mutex 'cs_main' exclusively
test/miner_tests.cpp:642:5: error: calling function 'TestPrioritisedMining' requires holding mutex 'cs_main' exclusively

Root Cause

In Bitcoin commit 9eaa5dbc81, the LOCK(cs_main) is acquired once and held for all three test function calls (TestBasicMining, TestPackageSelection, TestPrioritisedMining).

The Dash backport incorrectly added braces around the lock scope:

{
    LOCK(cs_main);
    TestBasicMining(scriptPubKey, txFirst, baseheight);
}  // Lock released here - INCORRECT!

TestPackageSelection(scriptPubKey, txFirst);  // Called without lock
TestPrioritisedMining(scriptPubKey, txFirst);  // Called without lock

Fix Applied

Removed the incorrect braces to match Bitcoin's structure:

LOCK(cs_main);

TestBasicMining(scriptPubKey, txFirst, baseheight);

m_node.chainman->ActiveChain().Tip()->nHeight--;
SetMockTime(0);

TestPackageSelection(scriptPubKey, txFirst);

m_node.chainman->ActiveChain().Tip()->nHeight--;
SetMockTime(0);

TestPrioritisedMining(scriptPubKey, txFirst);

Validation

• ✅ Build succeeds (incremental build)
• ✅ miner_tests pass locally
• ✅ Structure now matches Bitcoin commit 9eaa5db exactly

CI will re-run to confirm the fix resolves the macOS build failure.

Commit: 4a1108c