fix: require distinct outputs for duplicate superblock payments by thepastaclaw · Pull Request #7362 · dashpay/dash

thepastaclaw · 2026-06-13T13:33:15Z

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)

CSuperblock::IsValid matched expected payments against txNew.vout using a forward scan that re-started at the previously matched index. Two adjacent expected payments with identical scriptPubKey and amount could both match the same coinbase output, so a superblock could shortchange payees by emitting only one of two duplicate payouts. Initialize the cursor to -1 and scan from nVoutIndex + 1 so each expected payment consumes a distinct output. Adds a unit-test regression covering both the missing-output and well-formed cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

github-actions · 2026-06-13T13:33:46Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

thepastaclaw · 2026-06-13T13:34:04Z

@coderabbitai review

coderabbitai · 2026-06-13T13:34:10Z

✅ 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 · 2026-06-13T13:37:48Z

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: b09a3108-99fe-477f-b336-cf99d90fcd0f

📥 Commits

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

📒 Files selected for processing (6)

src/governance/superblock.cpp
src/governance/superblock.h
src/masternode/payments.cpp
src/masternode/payments.h
src/test/governance_superblock_tests.cpp
src/validation.cpp

✅ Files skipped from review due to trivial changes (1)

src/validation.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

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

dashpay/dash#7330: Both PRs modify the governance superblock validation path—CSuperblock::IsValid/SuperblockManager::IsValidSuperblock in src/governance/superblock.cpp (this PR adds the is_v24 parameter and vout matching behavior, while the related PR refactors/exposes the superblock validation APIs).

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 clearly and specifically describes the main change: requiring distinct outputs for duplicate superblock payments, which directly addresses the bug fixed in this PR.
Description check	✅ Passed	The description is well-detailed and directly related to the changeset, clearly explaining the issue, the fix, testing approach, and implementation details.
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

_{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.

The stricter rule requiring each expected superblock payment to consume a distinct coinbase output tightens consensus validation. Apply it only once DEPLOYMENT_V24 is active so all nodes flip the rule at the same height and already-validated history keeps the legacy (reuse-allowed) scan, avoiding a chain split during rollout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

thepastaclaw · 2026-06-14T14:11:12Z

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 · 2026-06-14T18:51:04Z

✅ Review complete (commit e5895ab)

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 previously approved these changes Jun 13, 2026

View reviewed changes

thepastaclaw marked this pull request as ready for review June 14, 2026 05:26

UdjinM6 requested changes Jun 14, 2026

View reviewed changes

thepastaclaw dismissed PastaPastaPasta’s stale review via e5895ab June 14, 2026 14:10

UdjinM6 approved these changes Jun 14, 2026

View reviewed changes

thepastaclaw commented Jun 14, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: require distinct outputs for duplicate superblock payments#7362

fix: require distinct outputs for duplicate superblock payments#7362
thepastaclaw wants to merge 2 commits into
dashpay:developfrom
thepastaclaw:fix-superblock-duplicate-payments

thepastaclaw commented Jun 13, 2026

Uh oh!

github-actions Bot commented Jun 13, 2026 •

edited

Loading

Uh oh!

thepastaclaw commented Jun 13, 2026

Uh oh!

coderabbitai Bot commented Jun 13, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jun 13, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

PastaPastaPasta left a comment

Uh oh!

UdjinM6 left a comment

Uh oh!

thepastaclaw commented Jun 14, 2026

Uh oh!

UdjinM6 left a comment

Uh oh!

thepastaclaw commented Jun 14, 2026 •

edited

Loading

Uh oh!

thepastaclaw left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

thepastaclaw commented Jun 13, 2026

Fix duplicate superblock payment matching

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist

Uh oh!

github-actions Bot commented Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ No Merge Conflicts Detected

Uh oh!

thepastaclaw commented Jun 13, 2026

Uh oh!

coderabbitai Bot commented Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Sequence Diagram

Estimated code review effort

Possibly related PRs

Suggested reviewers

❌ Failed checks (1 warning)

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

thepastaclaw commented Jun 14, 2026

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

thepastaclaw commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jun 13, 2026 •

edited

Loading

coderabbitai Bot commented Jun 13, 2026 •

edited

Loading

coderabbitai Bot commented Jun 13, 2026 •

edited

Loading

thepastaclaw commented Jun 14, 2026 •

edited

Loading