fix: require distinct outputs for duplicate superblock payments

[thepastaclaw]

Fix duplicate superblock payment matching

Issue being fixed or feature implemented

CSuperblock::IsValid matched each expected superblock payment by scanning
forward through the coinbase outputs, but the scan restarted at the previously
matched output index. If two adjacent expected payments had the same script and
amount, both expected payments could match the same coinbase output.

That could allow a block to include only one of two identical funded proposal
payouts while still passing validation, because the total payment cap counts
both expected payments.

What was done?

• Advance the superblock payment output cursor past the last matched output, so
  each expected payment must match a distinct coinbase output.
• Add a regression test covering duplicate expected payments with identical
  script and amount:
  • one matching coinbase output is rejected
  • two matching coinbase outputs are accepted
• Register the new governance superblock unit test in the test build.

How Has This Been Tested?

Local macOS arm64 build using the existing depends config:

./autogen.sh
CONFIG_SITE=.../depends/aarch64-apple-darwin/share/config.site \
  ./configure --disable-bench --disable-fuzz --disable-fuzz-binary \
  --disable-gui --with-incompatible-bdb --enable-suppress-external-warnings
make -C src -j$(($(sysctl -n hw.ncpu) - 1)) test/test_dash
./src/test/test_dash --run_test=governance_superblock_tests \
  --log_level=warning
./src/test/test_dash --run_test=governance_validators_tests \
  --log_level=warning

The new regression test was also run against the original buggy scan logic,
where the one-output duplicate-payment case failed as expected, then rerun
after restoring the fix.

Pre-PR review gate:

code-review dashpay/dash upstream/develop fix-superblock-duplicate-payments \
  "Pre-PR review: fix CSuperblock::IsValid so duplicate expected payments \
  require distinct coinbase outputs."

Result: ship.

Breaking Changes

None.

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)

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

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: 3659f312-1319-4f45-865e-d9f0f08e316b

📥 Commits

Reviewing files that changed from the base of the PR and between e5895ab and 0d27678.

📒 Files selected for processing (1)

• src/governance/superblock.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• src/governance/superblock.cpp

---

Walkthrough

The PR extends superblock validation to enforce version-aware distinct output matching for duplicate governance payments. It adds an is_v24 parameter to CSuperblock::IsValid and related APIs, implementing logic that prevents output reuse when adjacent payments have identical scripts and amounts (enforced when is_v24=true via initialization of vout cursor to -1 and forward-only searching). The flag is derived from deployment state in payment validation and threaded through both superblock value and payee checks. CMNPaymentsProcessor::IsBlockValueValid is refactored to accept block index context instead of precomputed height. Comprehensive regression tests validate behavior across three scenarios, and the test is integrated into the build system.

Sequence Diagram

sequenceDiagram
  participant ConnectBlock as CChainState::ConnectBlock
  participant MNPayments as CMNPaymentsProcessor
  participant Deployment as DeploymentActiveAfter
  participant Superblock as CSuperblock::IsValid
  participant Coinbase as Coinbase Tx
  
  ConnectBlock->>MNPayments: IsBlockValueValid(block, pindexPrev, ...)
  activate MNPayments
  MNPayments->>Deployment: Check DEPLOYMENT_V24 active after pindexPrev
  Deployment-->>MNPayments: is_v24 flag
  MNPayments->>Superblock: IsValidSuperblock(..., is_v24)
  activate Superblock
  Superblock->>Coinbase: Search vouts with cursor init at -1
  alt is_v24=true: strict distinct output matching
    Coinbase-->>Superblock: Each payment matched at unique vout index
    Superblock-->>MNPayments: VALID
  else is_v24=false: pre-V24 behavior
    Coinbase-->>Superblock: Payments matched, may reuse vouts
    Superblock-->>MNPayments: VALID
  end
  deactivate Superblock
  MNPayments-->>ConnectBlock: Validation result
  deactivate MNPayments

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• dashpay/dash#7330: Both PRs modify the governance superblock validation path—CSuperblock::IsValid and SuperblockManager::IsValidSuperblock in src/governance/superblock.cpp (this PR adds the is_v24 parameter and vout matching behavior for distinct output enforcement).

Suggested reviewers

• PastaPastaPasta
• UdjinM6

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.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 accurately and specifically describes the main change: a bug fix for superblock payment validation that now requires distinct coinbase outputs for duplicate payments.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, clearly explaining the issue, the fix implemented, testing methodology, and the impact of the changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[PastaPastaPasta]

utACK e9a2560

[UdjinM6]

This is a breaking change that could cause chain split during network update I think. Should be gated behind v24 imo, pls see 488f432.

[thepastaclaw]

Thanks — agreed, this tightens consensus. Applied the V24 gating from 488f432 in e5895ab so pre-V24 validation keeps the legacy inclusive scan and the distinct-output requirement only activates with DEPLOYMENT_V24.\n\nValidated with:\n\n\nsrc/test/test_dash --run_test=governance_superblock_tests\n

[UdjinM6]

utACK e5895ab

[thepastaclaw]

✅ Review complete (commit 64e0d7d)

[thepastaclaw]

Code Review

Tight, well-scoped fix for a superblock validation bug where two identical expected payments could match the same coinbase output. The change is correctly gated behind DEPLOYMENT_V24 to preserve historical validation, pre-V24 semantics are exactly preserved via std::max(nVoutIndex, 0), and a focused unit test covers the failing duplicate case, the valid two-output case, and the pre-V24 compatibility path. No in-scope issues.

[PastaPastaPasta]

A few things; that we may end up needing changes for;

1. make sure good test coverage for BOTH the pre-hf and post-hf cases.
2. make sure old logic and new pre-hf logic are the same
3. let's consider adding a TODO etc, that when we harden this fork, we can simplify this code with the new logic assuming that no old blocks ever hit the bugged case.
4. Since this tightens consensus, it could be acceptable to do this as a "soft fork" where if we do hit this bugged case, the intend is to fork off, but I think it's fine to keep it behind HF

[thepastaclaw]

Thanks, agreed on all four.

• The current unit test covers the post-HF failure case (one duplicate output rejected), the post-HF valid case (two distinct duplicate outputs accepted), and the pre-HF compatibility case (the historical single-output behavior accepted with is_v24=false).
• I rechecked the legacy path against the base implementation: pre-HF now starts at std::max(nVoutIndex, 0), which preserves the old inclusive scan from initial vout 0 and then the previously matched vout.
• Added 0d27678e38 with the TODO to simplify this once V24 is hardened/finalized and historical duplicate-output blocks cannot be encountered.
• Re-ran:

src/test/test_dash --run_test=governance_superblock_tests

[PastaPastaPasta]

utACK 0d27678

[thepastaclaw]

Code Review

Cumulative PR fixes a duplicate-superblock-payment matching bug by advancing the vout cursor past the previously matched output, gated behind V24 to preserve consensus for historical blocks. The latest delta (e5895ab → 0d27678) is purely documentation — a TODO note describing future simplification once V24 is finalized. No prior carried-forward findings (prior review was empty). Both Claude and Codex independently found no issues in the cumulative PR; verified the only delta is the TODO comment in src/governance/superblock.cpp.

[thepastaclaw]

Code Review

New-push review at 64e0d7d. The latest delta is an empty 'ci: rerun after flaky governance test' commit with no code changes. The prior review at 0d27678 found no issues; there are no carried-forward prior findings. Cumulative re-verification of the PR head confirms the V24-gated distinct-output check is correctly plumbed (CSuperblock::IsValid, IsValidSuperblock, IsBlockValueValid, IsBlockPayeeValid all take is_v24 derived from pindexPrev via DeploymentActiveAfter), and the new unit test exercises V24-rejection of a duplicate-matching-vout, V24-acceptance of two distinct matching vouts, and pre-V24 backward compatibility.

Note: GitHub does not allow PastaClaw to approve their own PR, so this clean automated review is posted as a comment review.