feat: use CheckBlock / ConnectBlock distinction properly #6989
Conversation
WalkthroughThis pull request introduces context-free basic validation functions for various special transaction types in the codebase. Specific basic validators are added for asset unlock transactions, coinbase transactions, ProTx variants (ProReg, ProUpServ, ProUpReg, ProUpRev), MNHF transactions, and LLMQ commitments. Each validates transaction type, payload presence, and version ranges. A dispatcher function Sequence DiagramsequenceDiagram
participant CheckBlock
participant CheckSpecialTxBasic
participant SpecificBasicCheck as CheckXxxBasic()
participant TxValidationState
CheckBlock->>CheckBlock: for each tx in block.vtx
CheckBlock->>CheckSpecialTxBasic: CheckSpecialTxBasic(tx, state)
CheckSpecialTxBasic->>CheckSpecialTxBasic: check tx has extra_payload
alt has extra payload
CheckSpecialTxBasic->>TxValidationState: Invalid (TX_CONSENSUS)
CheckSpecialTxBasic-->>CheckBlock: false
else no extra payload
CheckSpecialTxBasic->>CheckSpecialTxBasic: dispatch by tx.nType
CheckSpecialTxBasic->>SpecificBasicCheck: CheckCbTxBasic/CheckProRegTxBasic/etc.
SpecificBasicCheck->>SpecificBasicCheck: validate tx type
SpecificBasicCheck->>SpecificBasicCheck: validate payload presence
SpecificBasicCheck->>SpecificBasicCheck: validate version
alt all valid
SpecificBasicCheck-->>CheckSpecialTxBasic: true
CheckSpecialTxBasic-->>CheckBlock: true
else validation failed
SpecificBasicCheck->>TxValidationState: set error state
SpecificBasicCheck-->>CheckSpecialTxBasic: false
CheckSpecialTxBasic-->>CheckBlock: false
end
end
alt CheckSpecialTxBasic failed
CheckBlock->>CheckBlock: assert TX_CONSENSUS
CheckBlock->>CheckBlock: return BLOCK_CONSENSUS invalid
else CheckSpecialTxBasic passed
CheckBlock->>CheckBlock: continue block processing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/evo/mnhftx.cpp (1)
114-131:CheckMNHFTxBasicis well‑scoped; consider reusing it inCheckMNHFTxThe new basic validator correctly enforces MNHF type, payload presence, and version range in a context‑free way with consensus errors (
bad-mnhf-type,bad-mnhf-payload,bad-mnhf-version), which fits the newCheckSpecialTxBasicflow.You now have the same three checks duplicated at the top of
CheckMNHFTx; as a future cleanup, you could delegate toCheckMNHFTxBasic(or factor the common bits) to avoid drift if these rules ever evolve.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/evo/assetlocktx.cpp(1 hunks)src/evo/assetlocktx.h(1 hunks)src/evo/cbtx.cpp(1 hunks)src/evo/cbtx.h(1 hunks)src/evo/deterministicmns.cpp(4 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/mnhftx.cpp(1 hunks)src/evo/mnhftx.h(1 hunks)src/evo/specialtx.cpp(1 hunks)src/evo/specialtx.h(1 hunks)src/llmq/commitment.cpp(1 hunks)src/llmq/commitment.h(1 hunks)src/validation.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/evo/cbtx.cppsrc/evo/cbtx.hsrc/evo/deterministicmns.hsrc/validation.cppsrc/evo/deterministicmns.cppsrc/evo/mnhftx.cppsrc/evo/specialtx.cpp
📚 Learning: 2025-07-09T15:02:26.899Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Applied to files:
src/evo/deterministicmns.hsrc/validation.cppsrc/evo/deterministicmns.cppsrc/evo/mnhftx.cppsrc/evo/specialtx.cpp
📚 Learning: 2025-11-13T20:02:55.480Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6969
File: src/evo/deterministicmns.h:441-479
Timestamp: 2025-11-13T20:02:55.480Z
Learning: In `src/evo/deterministicmns.h`, the `internalId` field in `CDeterministicMN` and the `mnInternalIdMap` in `CDeterministicMNList` are non-deterministic and used only for internal bookkeeping and efficient lookups. Different nodes can assign different internalIds to the same masternode depending on their sync history. Methods like `IsEqual()` intentionally ignore internalId mappings and only compare consensus-critical deterministic fields (proTxHash, collateral, state, etc.).
Applied to files:
src/evo/deterministicmns.cpp
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
Repo: dashpay/dash PR: 6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/evo/deterministicmns.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/commitment.cpp
🧬 Code graph analysis (12)
src/evo/cbtx.cpp (1)
src/validation.h (1)
state(169-194)
src/evo/specialtx.h (2)
src/evo/specialtx.cpp (2)
CheckSpecialTxBasic(17-47)CheckSpecialTxBasic(17-17)src/validation.h (1)
state(169-194)
src/evo/mnhftx.h (1)
src/evo/mnhftx.cpp (2)
CheckMNHFTxBasic(114-131)CheckMNHFTxBasic(114-114)
src/evo/assetlocktx.cpp (1)
src/validation.h (1)
state(169-194)
src/evo/assetlocktx.h (1)
src/evo/assetlocktx.cpp (2)
CheckAssetUnlockTxBasic(114-140)CheckAssetUnlockTxBasic(114-114)
src/evo/cbtx.h (1)
src/evo/cbtx.cpp (2)
CheckCbTxBasic(22-44)CheckCbTxBasic(22-22)
src/llmq/commitment.h (1)
src/llmq/commitment.cpp (2)
CheckLLMQCommitmentBasic(216-244)CheckLLMQCommitmentBasic(216-216)
src/evo/deterministicmns.h (1)
src/evo/deterministicmns.cpp (14)
CheckProRegTxBasic(1093-1106)CheckProRegTxBasic(1093-1093)CheckProRegTx(1108-1239)CheckProRegTx(1108-1108)CheckProUpServTxBasic(1241-1254)CheckProUpServTxBasic(1241-1241)CheckProUpServTx(1256-1330)CheckProUpServTx(1256-1256)CheckProUpRegTxBasic(1332-1345)CheckProUpRegTxBasic(1332-1332)CheckProUpRegTx(1347-1415)CheckProUpRegTx(1347-1347)CheckProUpRevTxBasic(1417-1430)CheckProUpRevTxBasic(1417-1417)
src/validation.cpp (1)
src/evo/specialtx.cpp (2)
CheckSpecialTxBasic(17-47)CheckSpecialTxBasic(17-17)
src/evo/deterministicmns.cpp (1)
src/validation.h (1)
state(169-194)
src/evo/specialtx.cpp (5)
src/evo/deterministicmns.cpp (8)
CheckProRegTxBasic(1093-1106)CheckProRegTxBasic(1093-1093)CheckProUpServTxBasic(1241-1254)CheckProUpServTxBasic(1241-1241)CheckProUpRegTxBasic(1332-1345)CheckProUpRegTxBasic(1332-1332)CheckProUpRevTxBasic(1417-1430)CheckProUpRevTxBasic(1417-1417)src/evo/cbtx.cpp (2)
CheckCbTxBasic(22-44)CheckCbTxBasic(22-22)src/llmq/commitment.cpp (2)
CheckLLMQCommitmentBasic(216-244)CheckLLMQCommitmentBasic(216-216)src/evo/mnhftx.cpp (2)
CheckMNHFTxBasic(114-131)CheckMNHFTxBasic(114-114)src/evo/assetlocktx.cpp (4)
CheckAssetLockTx(44-95)CheckAssetLockTx(44-44)CheckAssetUnlockTxBasic(114-140)CheckAssetUnlockTxBasic(114-114)
src/llmq/commitment.cpp (1)
src/validation.h (1)
state(169-194)
🔇 Additional comments (15)
src/evo/assetlocktx.h (1)
158-158: LGTM!The function declaration is correctly placed and consistent with the existing validation functions. The implementation in assetlocktx.cpp performs appropriate context-free validation.
src/llmq/commitment.h (1)
167-167: LGTM!The function declaration is correctly placed and follows the established pattern for basic validation functions. The implementation performs appropriate context-free checks.
src/evo/deterministicmns.h (1)
770-777: LGTM!All four Basic validation function declarations are correctly added and follow a consistent pattern. Each is appropriately placed before its corresponding full validation function. The implementations perform context-free validation checking transaction type and payload presence.
src/llmq/commitment.cpp (1)
216-244: LGTM!The implementation of
CheckLLMQCommitmentBasiccorrectly performs context-free validation:
- Transaction type verification
- Payload presence check
- LLMQ type validation
- Version range enforcement
- Size validation for non-null commitments
All checks are appropriately context-free and error messages are consistent with the codebase.
src/evo/mnhftx.h (1)
172-172: LGTM!The function declaration is correctly placed and consistent with other Basic validation functions in the PR. The implementation performs appropriate context-free validation.
src/evo/cbtx.cpp (1)
22-44: LGTM!The implementation of
CheckCbTxBasiccorrectly performs context-free validation:
- Coinbase transaction verification
- Transaction type check
- Payload presence validation
- Version range enforcement
All checks are appropriately context-free and consistent with other Basic validation functions in this PR.
src/evo/specialtx.cpp (1)
8-47: Centralized basic special-tx precheck looks consistent and context-free
CheckSpecialTxBasiccleanly gates onHasExtraPayloadField()and then dispatches bytx.nTypeto the appropriate*Basicfunction (orCheckAssetLockTx), with a conservative default of rejecting unknown types via"bad-tx-type". The implementation stays context-free and mirrors the per-type basic validators added elsewhere, which fits the intended CheckBlock-vs-ConnectBlock split.src/evo/cbtx.h (1)
71-71: Basic CBTx validator declaration matches usageThe
CheckCbTxBasicdeclaration is correctly typed, aligned with the implementation incbtx.cpp, and placed coherently next toCheckCbTx, keeping the public validation API consistent.src/evo/assetlocktx.cpp (1)
114-140: Asset unlock basic validator mirrors full checks; confirm result-category choice
CheckAssetUnlockTxBasicintentionally mirrors the structural checks inCheckAssetUnlockTx(type, no inputs, max outputs, payload presence, version range) while staying context-free, which fits the new “Basic” validation layer. The main behavioral difference is the use ofTxValidationResult::TX_CONSENSUShere vsTX_BAD_SPECIALin the full check for the same error strings—if that distinction is meaningful for how callers classify failures, it’d be good to double‑check that this is the intended split.src/evo/deterministicmns.cpp (4)
1093-1106: CheckProRegTxBasic provides minimal, context-free ProReg validationThis basic ProReg validator keeps to type + payload presence checks with consensus error codes and no chainstate dependencies, matching the pattern used by other ProTx validation helpers and making it safe to call from
CheckBlock.
1241-1254: CheckProUpServTxBasic aligns with the ProTx basic-validation patternThe service-update basic check mirrors
CheckProRegTxBasic: it enforces the expected special-tx type and payload presence only, deferring all stateful and version-change logic to the fullCheckProUpServTxpath. This is appropriate for a context-free precheck.
1332-1345: CheckProUpRegTxBasic cleanly splits structural from stateful checksFor ProUpReg, the basic validator correctly limits itself to
tx.nTypeand payload existence checks and reuses the standard"bad-protx-type"/"bad-protx-payload"errors, keeping behavior consistent with the other ProTx flows while enabling early rejection inCheckBlock.
1417-1430: CheckProUpRevTxBasic completes the ProTx basic-validator setThe revocation basic check follows the same minimal, context-free pattern as the other ProTx basic validators, giving
CheckBlocka uniform way to reject malformed ProUpRev transactions before touching masternode state.src/validation.cpp (1)
4029-4039: Good separation of context‑free special‑tx checks intoCheckBlockRunning
CheckSpecialTxBasicoverblock.vtxhere cleanly extends the existing context‑freeCheckBlocksemantics and correctly maps any basic special‑tx failure to aBLOCK_CONSENSUSinvalid, with an assertion guarding that onlyTX_CONSENSUSresults are produced.Please double‑check that all
Check*TxBasiccallees reachable fromCheckSpecialTxBasicremain strictly context‑free (no chain/mempool access) and only ever markTxValidationResult::TX_CONSENSUS, so the new assertion and error mapping stay valid over time.src/evo/specialtx.h (1)
17-20: Header API forCheckSpecialTxBasiclooks consistentForward‑declaring
TxValidationStateand exposingCheckSpecialTxBasic(const CTransaction&, TxValidationState&)here cleanly matches the implementation and keeps dependencies minimal.
1 participant
Please remove the italicized help prompts before submitting or merging
Provide a general summary of your changes in the Title above
Pull requests without a rationale and clear improvement may be closed
immediately.
Please provide clear motivation for your patch and explain how it improves
Dash Core user experience or Dash Core developer experience
significantly:
src/test/) orfunctional tests (see
test/). Contributors should note which tests covermodified code. If no tests exist for a region of modified code, new tests
should accompany the change.
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Dash Core, if possible.
Issue being fixed or feature implemented
What was done?
Describe your changes in detail
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.
Breaking Changes
Please describe any breaking changes your code introduces
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.