[PastaPastaPasta]

Issue being fixed or feature implemented

CProUpServTx::IsTriviallyValid() never validates nType, and CheckProUpServTx() never
checks it against the registered masternode. RebuildListFromBlock() checks both
(specialtxman.cpp:395-425).

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, at mempool acceptance, the checks the block-connect path already performs:

• CProUpServTx::IsTriviallyValid() rejects an out-of-range nType with bad-protx-type.
  This mirrors what CProRegTx::IsTriviallyValid() already does (providertx.cpp:114), so
  the two payload types are now consistent.
• CheckProUpServTx() rejects nType mismatch against the registered masternode with
  bad-protx-type-mismatch. No separate range check is needed there: GetValidatedPayload()
  runs IsTriviallyValid() before the body, so an out-of-range nType is already gone by
  that point.

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

Additionally, the block path's own IsValidMnType() check was moved ahead of the
nType != dmn->nType mismatch check in RebuildListFromBlock(). In its old position it was
unreachable — dmn->nType is always in range, so an out-of-range payload nType always
tripped the mismatch check first and was reported as bad-protx-type-mismatch. Block
acceptance is unchanged; the same transactions are rejected either way. The reject reason is
now the accurate one, and it matches what IsTriviallyValid() reports for the same payload.

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.cpp — proupserv_rejects_invalid_ntype, covering
  nType values 42 and 65535.
• src/test/evo_deterministicmns_tests.cpp — proupserv_invalid_ntype_basic, four cases:
  1. out-of-range nType fails trivial validation and never enters the mempool;
  2. valid-but-mismatched nType (Evo claimed for a Regular MN) fails contextual validation
     and mempool acceptance, with the TxValidationResult pinned so the peer punishment
     level cannot change unnoticed;
  3. the block path rejects a hand-built block for both reasons — bad-protx-type for the
     out-of-range value, bad-protx-type-mismatch for the valid-but-wrong one;
  4. a matching nType still reaches the mempool, guarding against over-rejection.

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, the full evo_* set (50 cases, including
evo_trivialvalidation and evo_dip3_activation_tests) passes.

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

[github-actions]

This pull request has conflicts, please rebase.

[thepastaclaw]

✅ Final review complete — no blockers (commit 7956e24)

[coderabbitai]

Walkthrough

Provider service-update transactions now reject invalid masternode types during trivial validation. Contextual validation also rejects types that differ from the registered masternode. Regression tests cover valid types, out-of-range values, mismatches, mempool rejection, invalid blocks, and provider registration behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Transaction
  participant CProUpServTx
  participant CheckProUpServTx
  participant RegisteredMasternode
  Transaction->>CProUpServTx: Run IsTriviallyValid
  CProUpServTx->>CProUpServTx: Check IsValidMnType
  CProUpServTx-->>Transaction: Return bad-protx-type for invalid type
  Transaction->>CheckProUpServTx: Run contextual validation
  CheckProUpServTx->>RegisteredMasternode: Compare nType
  RegisteredMasternode-->>CheckProUpServTx: Return registered masternode type
  CheckProUpServTx-->>Transaction: Reject mismatch or invalid type

Loading

Possibly related PRs

• dashpay/dash#7437: Both changes modify masternode provider-transaction validation in the same implementation files.

Suggested reviewers: knst

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Title check	✅ Passed	The title clearly and concisely identifies the primary change: validating ProUpServTx nType during mempool acceptance.
Description check	✅ Passed	The description directly explains the validation bug, implementation, tests, and lack of consensus changes.

✨ 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.}

[thepastaclaw]

Final validation — Codex + Sonnet

The PR correctly closes the ProUpServTx mempool-versus-block validation gap and adds focused coverage for invalid, mismatched, and valid nType cases. The proposed dead-code and commit-order comments do not require author action: the former is a harmless structural mirror of the block-connect checks, while the latter is expressly permitted by the repository's disclosed regression-test-first exception.
Source: reviewers gpt-5.6-sol (Codex) and claude-sonnet-5 (Sonnet); final verifier gpt-5.6-sol. Coordinator openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: gpt-5.6-sol — final-verifier (fallback)
• Sonnet reviewers: claude-sonnet-5 — general (completed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — dash-core-commit-history (completed)

[UdjinM6]

pls see 7956e24

[thepastaclaw]

Final validation — Codex + Sonnet

Carried-forward prior findings: none; the prior final review at 51846b0 had no live findings, and current-head inspection plus reconciliation of the updated PR body, UdjinM6's exact-head reply, the superseded rebase notice, and current threads found no silently lost issue. New latest-delta findings: none; commit 7956e24 correctly removes an unreachable contextual range check, returns precise block-path rejection reasons, and strengthens regression coverage, while the proposed history rewrite is optional rather than actionable. Source: successful reviewers Codex gpt-5.6-sol and Claude claude-sonnet-5; final verifier Codex gpt-5.6-sol, acting as the explicit fallback for the Sonnet verifier. Coordinator openclaw-agent is orchestration-only.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: gpt-5.6-sol — final-verifier (fallback)
• Sonnet reviewers: claude-sonnet-5 — general (failed), claude-sonnet-5 — dash-core-commit-history (completed), claude-sonnet-5 — general (failed), claude-sonnet-5 — general (failed), claude-sonnet-5 — general (completed)