fix(rpc): report inactive DIP3 for ProTx RPCs#7342
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR enforces a DIP0003 activation check in SignAndSendSpecialTx: it captures the active chain tip and consensus params, calls DeploymentActiveAfter(DIP0003), and returns a JSONRPCError with activation height/blocks-to-mine if DIP3 is not active. CheckSpecialTx is invoked with the cached tip. CheckSpecialTxInner’s DIP3-inactive rejection message was made explicit. Functional tests now assert register_fund and register RPCs fail with the expected DIP3-inactive error before activation. Sequence Diagram(s)sequenceDiagram
participant Client as RPC Caller
participant RPC as SignAndSendSpecialTx
participant Chain as Chainman/ConsensusParams
participant Validator as CheckSpecialTx
Client->>RPC: Submit ProTx/EvoNode special-tx
RPC->>Chain: Capture active tip & consensus params
RPC->>Chain: DeploymentActiveAfter(DIP0003)
alt DIP0003 not active
Chain-->>RPC: not active (activation height)
RPC-->>Client: JSONRPCError(RPC_VERIFY_ERROR) with blocks-to-mine message
else DIP0003 active
RPC->>Validator: CheckSpecialTx(cached tip)
Validator-->>RPC: validation result (or TX_BAD_SPECIAL with message)
RPC-->>Client: success or validation error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/evo/specialtxman.cppsrc/evo/specialtxman.cpp:5:10: fatal error: 'evo/specialtxman.h' file not found ... [truncated 2200 characters] ... 4784, characters 10-1023 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.
🧹 Nitpick comments (1)
test/functional/feature_dip3_deterministicmns.py (1)
64-79: 💤 Low valueGood coverage for register_fund rejection before DIP3 activation.
The test verifies that
register_fund(internal collateral path) correctly rejects with error code -25 and the expected error message before DIP3 is active.Optional: Consider verifying the complete error message including "current chain height", "next block height", and "Mine X more blocks" to ensure the detailed error reporting works correctly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/feature_dip3_deterministicmns.py` around lines 64 - 79, Extend the assertion on register_fund to check the full DIP3 inactive error text rather than just the short message: after calling before_activation_mn.register_fund(...) verify that the raised RPC error string contains the detailed message including "current chain height", the "next block height" (135), and the guidance "Mine X more blocks" by capturing the exception or returned error payload from register_fund and matching those substrings; reference the existing before_activation_mn.register_fund call and the dip3_inactive_error variable to locate where to add the extra checks, and include assertions that the numeric heights in the message match the expected values derived from the test chain state (e.g., using self.nodes[0].getblockcount() or the known activation height 135) so the full error formatting is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/functional/feature_dip3_deterministicmns.py`:
- Around line 64-79: Extend the assertion on register_fund to check the full
DIP3 inactive error text rather than just the short message: after calling
before_activation_mn.register_fund(...) verify that the raised RPC error string
contains the detailed message including "current chain height", the "next block
height" (135), and the guidance "Mine X more blocks" by capturing the exception
or returned error payload from register_fund and matching those substrings;
reference the existing before_activation_mn.register_fund call and the
dip3_inactive_error variable to locate where to add the extra checks, and
include assertions that the numeric heights in the message match the expected
values derived from the test chain state (e.g., using
self.nodes[0].getblockcount() or the known activation height 135) so the full
error formatting is validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8c239794-a9ea-42a6-a623-40a4c9180feb
📒 Files selected for processing (2)
src/rpc/evo.cpptest/functional/feature_dip3_deterministicmns.py
thepastaclaw
force-pushed
the
protx-dip3-error
branch
from
b4ca6d6 to
87beb3f
Compare
|
@thepastaclaw it appears you didn't use the repos pull request template |
|
Thanks, fixed — I updated the PR body to use the repo pull request template. |
|
✅ Review complete (commit 865791f) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, well-scoped DX improvement that adds an early DIP0003 activation check in SignAndSendSpecialTx so ProTx RPCs return a clear RPC_VERIFY_ERROR (with activation height context) instead of the opaque 'bad-tx-type' from CheckSpecialTx. The pre-check mirrors CheckSpecialTxInner's predicate exactly (DeploymentActiveAfter on DEPLOYMENT_DIP0003 under cs_main), so no consensus or behavioral regression. Functional tests cover register_fund and external-collateral register paths. One minor readability nitpick.
💬 1 nitpick(s)
|
why not just So it will cover not only case of RPC call to create "protx" transaction but also other ways, such as invalid |
|
Thanks — agreed. I moved the message into special tx validation so the clearer inactive-DIP3 reason now applies to all special transaction paths, not just ProTx RPC construction, and kept the ProTx RPC regression coverage updated. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both agent findings are confirmed on the current head. The PR now only changes the shared special transaction validation reject string, while the ProTx RPC path still throws a std::runtime_error that is mapped to RPC_MISC_ERROR (-1), so the PR does not deliver its stated RPC_VERIFY_ERROR (-25) behavior or activation-height context.
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/evo/specialtxman.cpp`:
- [BLOCKING] src/evo/specialtxman.cpp:113: ProTx RPCs still return RPC_MISC_ERROR without activation-height context
This change only expands the validation reject reason returned by CheckSpecialTxInner. The ProTx RPC submission helper still calls CheckSpecialTx from SignAndSendSpecialTx and throws std::runtime_error(state.ToString()) when validation fails; ExecuteCommand maps that exception to RPC_MISC_ERROR (-1). The updated functional test also now expects -1, so the PR locks in the generic RPC error code and still omits the activation height/current height/next block context described in the PR. To deliver the stated behavior, the inactive-DIP3 case needs to be handled in the RPC submission path or the CheckSpecialTx failure needs to be converted there into JSONRPCError(RPC_VERIFY_ERROR, ...) with the activation details.
Note: this is marked as a blocking finding, but GitHub does not allow requesting changes on an author's own PR, so this was posted as a comment review.
|
Follow-up pushed: kept the shared special-tx inactive-DIP3 reject string, and restored the ProTx RPC precheck so the RPC path still returns RPC_VERIFY_ERROR (-25) with activation-height context. Rebuilt src/dashd and reran feature_dip3_deterministicmns.py --descriptors successfully. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
|
||
| if (!DeploymentActiveAfter(pindexPrev, chainman.GetConsensus(), Consensus::DEPLOYMENT_DIP0003)) { | ||
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-tx-type"); | ||
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-tx-type -- DIP-0003 is not active yet, no special transactions are enabled"); |
There was a problem hiding this comment.
@knst @thepastaclaw I think you cannot do this! I believe these strings must be short maybe < 16 chars or so for some reason.
There was a problem hiding this comment.
There are some other long examples in existing code.
Maybe it was limited in past to 16 chars for some reasons, but probably not anymore.
src/validation.cpp: return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(check.GetScriptError())));
src/validation.cpp: return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));
src/validation.cpp: return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool min fee not met", strprintf("%d < %d", package_fee, mempoolRejectFee));
src/validation.cpp: return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "min relay fee not met", strprintf("%d < %d", package_fee, ::minRelayTxFee.GetFee(package_size)));
And even bad-assetlocktx-non-empty-return.
I guess it should be "bad-tx-type (DIP0003 is not active)" then to unify with other similar cases.
|
Addressed @PastaPastaPasta’s review note about short validation reject reasons in e0288b2: the consensus reject reason is back to |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Review complete (commit 865791f)
Automated code review verdict: ship. Current head preserves the ProTx RPC inactive-DIP3 guidance while keeping generic special-tx validation on the concise |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 865791f. The latest delta is a single one-line reject-reason refinement in src/evo/specialtxman.cpp ("bad-tx-type (DIP0003 is not active)") that is correct and aligns with the PR goal. No new findings in the latest delta. One carried-forward nitpick from the prior review (unreachable ternary guard in src/rpc/evo.cpp) remains valid at the current head.
Reviewed commit: 865791f
💬 1 nitpick(s)
1 carried-forward finding remains valid and was already raised on this PR; not re-posting it as a duplicate inline comment.
Carried-forward finding
- nitpick: blocks_to_mine ternary guard is unreachable —
src/rpc/evo.cpp:363-365- Carried-forward prior finding (still valid at 865791f). This branch is only entered when
!DeploymentActiveAfter(tip, consensus_params, Consensus::DEPLOYMENT_DIP0003). For the buried DIP0003 deployment,DeploymentActiveAfterreturns(tip == nullptr ? 0 : tip->nHeight + 1) >= params.DIP0003Height, which is exactlynext_block_height >= activation_height. Inside this branch the negation guaranteesnext_block_height < activation_height, soactivation_height > next_block_heightis always true and the: 0arm of the ternary can never be selected. Drop the guard and use a plain initializer to remove dead code and a misleading fallback.
- Carried-forward prior finding (still valid at 865791f). This branch is only entered when
4 participants
Issue being fixed or feature implemented
ProTx RPCs can return a generic
bad-tx-typeerror with RPC code-1whenused before DIP0003 is active on regtest/devnet. The underlying problem is that
special transaction validation rejects extra payload transactions before DIP3,
but the RPC response does not explain that activation height is the blocker.
This improves the Dash Core developer experience by returning an explicit
inactive-DIP3 RPC verification error with activation height context.
What was done?
before special transaction validation.
RPC_VERIFY_ERRORwith the DIP3 activation height, current chainheight, next block height, and a regtest/devnet hint.
registration paths before DIP3 activation.
How Has This Been Tested?
Tested locally on macOS arm64 with the existing depends prefix and autotools
build.
make -j8 src/dashd src/dash-clitest/functional/feature_dip3_deterministicmns.py --cachedir=/Users/claw/Projects/dash/worktrees/protx-dip3-error/test/cacheshipBreaking Changes
None. This only changes the RPC error returned before DIP0003 activation for
ProTx submission paths.
Checklist: