Skip to content

fix: resolve flaky verify_db_legacy test and skip unspendable outputs in test UTXO map#7266

Open
UdjinM6 wants to merge 2 commits intodashpay:developfrom
UdjinM6:fix/evo-test-flakiness
Open

fix: resolve flaky verify_db_legacy test and skip unspendable outputs in test UTXO map#7266
UdjinM6 wants to merge 2 commits intodashpay:developfrom
UdjinM6:fix/evo-test-flakiness

Conversation

@UdjinM6
Copy link
Copy Markdown

@UdjinM6 UdjinM6 commented Apr 3, 2026

Issue being fixed or feature implemented

  • Sync txindex after mining tx_collateral in FuncVerifyDB before it's looked up via GetTransaction/g_txindex in CreateProUpRevTx. Unlike coinbase txns (indexed during initial chain setup), this non-coinbase tx is only indexed asynchronously, causing intermittent critical check txFrom has failed failures. Example: https://github.com/dashpay/dash/actions/runs/23958304920/job/69882226393?pr=7265.
  • Skip OP_RETURN and other unspendable outputs in BuildSimpleUtxoMap in both evo_deterministicmns_tests and block_reward_reallocation_tests. Dash coinbases can include OP_RETURN outputs (credit pool payments when V20+MN_RR are active) which should never be selected as transaction inputs.

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

UdjinM6 and others added 2 commits April 3, 2026 23:58
FuncVerifyDB mines tx_collateral in a block via ProcessNewBlock, then
later CreateProUpRevTx looks it up via GetTransaction/g_txindex.
Unlike coinbase txns (indexed during initial chain setup), tx_collateral
is only indexed asynchronously by the txindex background thread. Without
an explicit IndexWaitSynced, the lookup can race and intermittently fail
with "critical check txFrom has failed".

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dash coinbase transactions can include OP_RETURN outputs (e.g. credit
pool payments when V20+MN_RR are active). BuildSimpleUtxoMap was adding
these to the UTXO map, allowing SelectUTXOs to pick them as transaction
inputs which would then fail signature verification.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@UdjinM6 UdjinM6 added this to the 24 milestone Apr 3, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 3, 2026

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@thepastaclaw
Copy link
Copy Markdown

thepastaclaw commented Apr 3, 2026

✅ Review complete (commit 74056e9)

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 3, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2d5efb41-1bf6-48bf-9340-2c58583906fd

📥 Commits

Reviewing files that changed from the base of the PR and between 2ca9da3 and 74056e9.

📒 Files selected for processing (2)
  • src/test/block_reward_reallocation_tests.cpp
  • src/test/evo_deterministicmns_tests.cpp

Walkthrough

Two test files were modified to improve test stability and correctness. The BuildSimpleUtxoMap function in both test files was updated to filter out unspendable outputs, preventing them from being selected as funding inputs during test execution. Additionally, evo_deterministicmns_tests.cpp was enhanced with includes for transaction index utilities and a synchronization checkpoint in the FuncVerifyDB function to ensure the transaction index is fully synchronized before dependent operations execute.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 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 (2 passed)
Check name Status Explanation
Title check ✅ Passed The PR title accurately summarizes the two main changes: fixing the flaky verify_db_legacy test by syncing txindex and skipping unspendable outputs in the test UTXO map.
Description check ✅ Passed The PR description clearly explains both issues being addressed: the FuncVerifyDB txindex race condition and the need to skip unspendable outputs in BuildSimpleUtxoMap.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

Copy link
Copy Markdown

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Clean test fix addressing two independent flakiness issues. (1) BuildSimpleUtxoMap now skips OP_RETURN unspendable outputs that were polluting the UTXO map in both block_reward_reallocation_tests and evo_deterministicmns_tests — these outputs would cause later spend attempts to fail with 'missing inputs'. (2) FuncVerifyDB adds IndexWaitSynced(*g_txindex) after ProcessNewBlock to ensure the txindex has indexed the collateral transaction before CreateProUpRevTx looks it up via GetTransaction. Both fixes are correct and minimal. No issues found.

Reviewed commit: 74056e9

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.

2 participants