refactor: remove g_txindex from TestChainSetup as optional step#7455
Conversation
|
⛔ Blockers found — Sonnet deferred (commit 198dc72) |
WalkthroughTest transaction helpers now store full Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/evo_deterministicmns_tests.cpp`:
- Around line 139-147: Update GetCollateralOutpoint to assert with BOOST_REQUIRE
when no transaction output matches dmn_types::Regular.collat_amount, instead of
returning a default null COutPoint. Preserve returning the matching COutPoint
for valid transactions and make the missing-collateral invariant fail explicitly
before callers use the result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2eab2b26-efd9-43fd-be07-f1d00baddf54
📒 Files selected for processing (3)
src/test/block_reward_reallocation_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/util/setup_common.cpp
💤 Files with no reviewable changes (1)
- src/test/util/setup_common.cpp
| static COutPoint GetCollateralOutpoint(const CMutableTransaction& tx) | ||
| { | ||
| for (size_t i = 0; i < tx.vout.size(); ++i) { | ||
| if (tx.vout[i].nValue == dmn_types::Regular.collat_amount) { | ||
| return COutPoint(tx.GetHash(), i); | ||
| } | ||
| } | ||
| return COutPoint(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
GetCollateralOutpoint silently returns a null outpoint on miss.
If no output matches collat_amount, the function returns a default COutPoint() (n == 0xFFFFFFFF). This is a silent failure: FuncVerifyDB later does tx_collateral.vout[collateralOutpoint.n] (Line 1343), which becomes an out-of-bounds vector::operator[] access, and other callers assign the null outpoint into proTx.collateralOutpoint, producing an opaque validation failure instead of a clear one. A BOOST_REQUIRE makes the invariant explicit for these consensus-path tests.
🛡️ Proposed guard
static COutPoint GetCollateralOutpoint(const CMutableTransaction& tx)
{
for (size_t i = 0; i < tx.vout.size(); ++i) {
if (tx.vout[i].nValue == dmn_types::Regular.collat_amount) {
return COutPoint(tx.GetHash(), i);
}
}
- return COutPoint();
+ BOOST_REQUIRE_MESSAGE(false, "no collateral output found");
+ return COutPoint(); // unreachable
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static COutPoint GetCollateralOutpoint(const CMutableTransaction& tx) | |
| { | |
| for (size_t i = 0; i < tx.vout.size(); ++i) { | |
| if (tx.vout[i].nValue == dmn_types::Regular.collat_amount) { | |
| return COutPoint(tx.GetHash(), i); | |
| } | |
| } | |
| return COutPoint(); | |
| } | |
| static COutPoint GetCollateralOutpoint(const CMutableTransaction& tx) | |
| { | |
| for (size_t i = 0; i < tx.vout.size(); ++i) { | |
| if (tx.vout[i].nValue == dmn_types::Regular.collat_amount) { | |
| return COutPoint(tx.GetHash(), i); | |
| } | |
| } | |
| BOOST_REQUIRE_MESSAGE(false, "no collateral output found"); | |
| return COutPoint(); // unreachable | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/evo_deterministicmns_tests.cpp` around lines 139 - 147, Update
GetCollateralOutpoint to assert with BOOST_REQUIRE when no transaction output
matches dmn_types::Regular.collat_amount, instead of returning a default null
COutPoint. Preserve returning the matching COutPoint for valid transactions and
make the missing-collateral invariant fail explicitly before callers use the
result.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Independently verified at exact head db95db1. The three-commit stack removes g_txindex/GetTransaction dependencies from TestChainSetup and its two consumer test files, replacing lookups with a Coin-based SimpleUTXOMap threaded through FundTransaction/SignTransaction into script/sign.cpp's SignTransaction(). Verified: no other file under src/test references g_txindex/GetTransaction after this change; the remaining g_txindex use in setup_common.cpp's mineBlocks() is null-guarded and unaffected; UTXO snapshots are captured before consumption in both refactored test files. No correctness, consensus-safety, or build-breaking issues found. Commit-message quality is the only actionable item: two preparatory subjects are 95/104 characters, and the final commit uses a misleading feat: prefix and vague wording for a change that only removes unused test-fixture initialization (sibling commits correctly use refactor:). Note for the record: linux64_tsan-test failed at this exact head on p2p_node_network_limited.py --v2transport, unrelated to any file this PR touches (setup_common.cpp, evo_deterministicmns_tests.cpp, block_reward_reallocation_tests.cpp) — the Codex general agent's claim that CI passed is not fully accurate, though this does not indicate a defect introduced by this PR.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
… tests of deterministicmns
… tests of block-reward-reallocation
g_txindex has own tests and no need in general environment for regression tests
|
rebased due to changes in #7451 |
db95db1 to
198dc72
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/util/setup_common.cpp (2)
337-339: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapture
__func__to avoid printingoperator()in logs.Using
__func__directly inside a lambda evaluates tooperator(), which makes log messages less descriptive. Consider capturing the outer function's name to preserve context.♻️ Proposed refactor
- options.notify_bls_state = [](bool bls_state) { - LogPrintf("%s: bls_legacy_scheme=%d\n", __func__, bls_state); + const char* func = __func__; + options.notify_bls_state = [func](bool bls_state) { + LogPrintf("%s: bls_legacy_scheme=%d\n", func, bls_state); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/util/setup_common.cpp` around lines 337 - 339, Update the lambda assigned to options.notify_bls_state so it captures the enclosing function’s name before the lambda is created, then use that captured name in LogPrintf instead of referencing __func__ inside the lambda. Preserve the existing bls_state value and log format.
627-633: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate stale comment referencing
tx index.Since
g_txindexlifecycle management has been removed from this fixture, specifically citing it as the reason for syncing is slightly misleading. Consider generalizing the comment to refer to background validation tasks.♻️ Proposed refactor
TestChainSetup::~TestChainSetup() { - // Allow tx index to catch up with the block index cause otherwise - // we might be destroying it while scheduler still has some work for it - // e.g. via BlockConnected signal + // Allow background tasks to catch up with the block index cause otherwise + // we might be destroying resources while the scheduler still has some work for them + // e.g. via BlockConnected signal SyncWithValidationInterfaceQueue(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/util/setup_common.cpp` around lines 627 - 633, Update the comment above TestChainSetup::~TestChainSetup and SyncWithValidationInterfaceQueue to remove the stale tx index reference, generalizing the explanation to background validation or scheduler tasks that must finish before fixture destruction. Leave the synchronization behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/test/util/setup_common.cpp`:
- Around line 337-339: Update the lambda assigned to options.notify_bls_state so
it captures the enclosing function’s name before the lambda is created, then use
that captured name in LogPrintf instead of referencing __func__ inside the
lambda. Preserve the existing bls_state value and log format.
- Around line 627-633: Update the comment above TestChainSetup::~TestChainSetup
and SyncWithValidationInterfaceQueue to remove the stale tx index reference,
generalizing the explanation to background validation or scheduler tasks that
must finish before fixture destruction. Leave the synchronization behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3f604969-001c-4c6f-8ffc-88b99830654f
📒 Files selected for processing (5)
src/test/block_reward_reallocation_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/governance_inv_tests.cppsrc/test/util/setup_common.cppsrc/test/validation_chainstatemanager_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/block_reward_reallocation_tests.cpp
- src/test/evo_deterministicmns_tests.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
New latest-delta finding: exact head 198dc72 introduces a confirmed use-after-free during partial GovernanceInvSetup construction, so changes are required. Carried-forward prior finding bd3618963324 remains valid because all three commit subjects are unchanged in the relevant respects; the remaining txindex and Coin-backed signing refactors appear correct.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/governance_inv_tests.cpp`:
- [BLOCKING] src/test/governance_inv_tests.cpp:79-83: Preserve governance cleanup when fixture construction fails
After `m_node.govman` is assigned at line 52, any subsequent `BOOST_REQUIRE` failure throws `boost::execution_aborted`. Because `GovernanceInvSetup` has not finished construction, this destructor is skipped and only the completed base subobjects are destroyed. `TestingSetup::~TestingSetup()` then calls `DashChainstateSetupClose()`, which resets `chain_helper`; later, destruction of the still-live governance manager calls `m_superblocks.Clear()` through its dangling `SuperblockManager` reference. Restore `m_node.govman.reset()` in `TestingSetup::~TestingSetup()` before `DashChainstateSetupClose()` so both normal and partial-construction teardown preserve the required lifetime ordering.
| ~GovernanceInvSetup() { | ||
| // govman holds a reference to chain_helper->superblocks, so it must be reset before chain_helper (matches PrepareShutdown | ||
| // ordering in init.cpp). | ||
| m_node.peerman->RemoveHandlers(); | ||
| m_node.govman.reset(); |
There was a problem hiding this comment.
🔴 Blocking: Preserve governance cleanup when fixture construction fails
After m_node.govman is assigned at line 52, any subsequent BOOST_REQUIRE failure throws boost::execution_aborted. Because GovernanceInvSetup has not finished construction, this destructor is skipped and only the completed base subobjects are destroyed. TestingSetup::~TestingSetup() then calls DashChainstateSetupClose(), which resets chain_helper; later, destruction of the still-live governance manager calls m_superblocks.Clear() through its dangling SuperblockManager reference. Restore m_node.govman.reset() in TestingSetup::~TestingSetup() before DashChainstateSetupClose() so both normal and partial-construction teardown preserve the required lifetime ordering.
source: ['codex']
3 participants
Issue being fixed or feature implemented
g_txindex has own tests and there's no needs in tx-index in the common environment for regression tests
This PR is a blocker for #7462 - particularly bitcoin#25494
What was done?
Removed usages of g_txindex by calling
GetTransactionfrom block_reward_reallocation_tests.cpp, evo_deterministicmns_tests.cppSimplified and removed duplicated code from
evo_deterministicmns_tests.cppHow Has This Been Tested?
Run unit & functional tests.
Breaking Changes
n/a
Checklist: