Skip to content

fix(evo): validate ProUpServTx nType at mempool acceptance - #7488

Draft
PastaPastaPasta wants to merge 2 commits into
dashpay:developfrom
PastaPastaPasta:sec/v051
Draft

fix(evo): validate ProUpServTx nType at mempool acceptance#7488
PastaPastaPasta wants to merge 2 commits into
dashpay:developfrom
PastaPastaPasta:sec/v051

Conversation

@PastaPastaPasta

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

CProUpServTx::IsTriviallyValid() never validates nType, and CheckProUpServTx() never
checks it against the registered masternode. BuildNewListFromBlock() checks both
(specialtxman.cpp:413-417).

That asymmetry is a mempool-vs-block divergence. A ProUpServTx with an out-of-range nType,
or one whose nType disagrees with the masternode it targets, is accepted into the mempool
but can never be mined. Once such a transaction is in a miner's mempool, block assembly
throws and getblocktemplate stops producing templates — for every node that accepted the
transaction, from a single relayed message.

What was done?

Added the two checks that BuildNewListFromBlock() already performs, at mempool acceptance:

  • CProUpServTx::IsTriviallyValid() rejects an out-of-range nType with bad-protx-type.
    This mirrors what CProRegTx::IsTriviallyValid() already does (providertx.cpp:144), so
    the two payload types are now consistent.
  • CheckProUpServTx() rejects nType mismatch against the registered masternode with
    bad-protx-type-mismatch, and re-checks validity with bad-protx-type.

Both reject reasons are deliberately identical to the ones BuildNewListFromBlock() already
uses, so a rejection reads the same regardless of which layer catches it.

Why this is not a consensus change

The block-connect path already rejected exactly these transactions, so nothing that was
previously block-valid becomes block-invalid. The change only moves the rejection earlier,
to the layer that was missing it. No transaction with these properties can exist in any
chain's history, because it could never have been connected.

How Has This Been Tested?

Unit tests added in the commit preceding the fix:

  • src/test/evo_trivialvalidation.cppproupserv_rejects_invalid_ntype, covering
    nType values 42 and 65535.
  • src/test/evo_deterministicmns_tests.cppproupserv_invalid_ntype_basic, which drives
    the transaction through real mempool acceptance and asserts both the rejection and the
    reject reason, then confirms CheckProUpServTx() agrees.

Without the fix these produce 6 and 9 failures respectively; notably the mempool test shows
the transaction being accepted, and the mismatch case reporting protx-dup instead of
bad-protx-type-mismatch. With the fix, evo_trivialvalidation (4 cases) and
evo_dip3_activation_tests (19 cases) both pass.

Built and run on macOS/arm64 against current develop.

Breaking Changes

None. Relaying such a transaction now earns a TX_CONSENSUS rejection, which is the
intended treatment — it could never have been mined.

Known follow-up

Coverage here is C++ unit level only. A functional test driving the divergence through a
real miner's getblocktemplate would be a worthwhile addition; happy to add one in this PR
if reviewers would prefer it before merge.

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

Add unit coverage for the mempool-vs-block consensus divergence: CProUpServTx::IsTriviallyValid and CheckProUpServTx must reject out-of-range and mismatched nType the same way BuildNewListFromBlock already does.
Mirror CProRegTx and BuildNewListFromBlock: reject out-of-range nType in CProUpServTx::IsTriviallyValid, and reject type mismatches in CheckProUpServTx. Closes the mempool/block divergence that let a single ProUpServTx stall getblocktemplate.
@github-actions

Copy link
Copy Markdown

This pull request has conflicts, please rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant