fix: punish invalid dstx messages#7347
Conversation
|
@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 (2)
WalkthroughAdds CCoinJoinBroadcastTx/msg_dstx test wire types, adjusts DSTX validation so missing masternode is treated as non-invalid, updates ProcessMessage to punish peers when ValidateDSTX reports an invalid DSTX early-exit, and adds a P2P functional test (registered in the base suite) that verifies non-punishment for unknown-masternode DSTX and punishment for structurally invalid DSTX. Sequence DiagramsequenceDiagram
participant Peer as P2P Peer
participant ProcessMsg as PeerManagerImpl::ProcessMessage
participant ValidateDSTX as ValidateDSTX
participant Misbehave as Misbehaving
Peer->>ProcessMsg: recv "dstx" message
ProcessMsg->>ValidateDSTX: validate DSTX
alt Missing masternode (no punishment)
ValidateDSTX-->>ProcessMsg: bDoReturn=true, bRet=true
ProcessMsg-->>Peer: ignore without penalty
else Invalid DSTX (punish)
ValidateDSTX-->>ProcessMsg: bDoReturn=true, bRet=false
ProcessMsg->>Misbehave: Misbehaving(pfrom, 10, "invalid dstx")
Misbehave-->>ProcessMsg: score recorded
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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/net_processing.cppsrc/net_processing.cpp:6:10: error: 'net_processing.h' file not found with include; use "quotes" instead ... [truncated 2200 characters] ... __CTrans.CTrans_funct.get_clang_stmt_trans in file "src/clang/cTrans.ml" (inlined), line 5395, characters 4-69 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 |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit da0ea55) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4f814a849
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!bRet) { | ||
| Misbehaving(pfrom.GetId(), 10, "invalid dstx"); |
There was a problem hiding this comment.
When this node is out of IBD but its active tip does not yet include the masternode that signed the DSTX, ValidateDSTX searches only the current tip and 23 ancestors and returns {false, true} from the “Can't find masternode” path. This new !bRet branch therefore increments the peer's misbehavior score for a DSTX that may be valid from the relaying peer's newer chain view, turning a local inability to verify into punishment; consider distinguishing unverifiable/unknown-masternode cases from structural or signature failures before calling Misbehaving.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR correctly applies a small misbehavior score (10) only on the invalid/error early-return paths of ValidateDSTX, leaving benign duplicate and mixing-rate-limited cases untouched. The new functional test exercises the unknown-masternode error path with a matching Python serializer for the dstx message. Two non-blocking observations: the !dmn path can rarely punish honest peers whose MN list is briefly stale, and only one of the three error paths is covered by the test.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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/net_processing.cpp`:
- [SUGGESTION] src/net_processing.cpp:3584-3587: `!dmn` path can punish honest peers when local MN list is briefly stale
This change routes all three `{false, true}` returns from ValidateDSTX through `Misbehaving(..., 10, "invalid dstx")`, including the `!dmn` branch at lines 3584-3587. The 24-block lookback at lines 3565-3582 already mitigates short staleness, but an honest relayer can still trip this case during reorgs, slow validation, or restart catch-up shortly after IBD when the receiving node's deterministic MN list is more than 24 blocks behind the broadcaster's registration. Score 10 keeps the impact bounded (discouragement threshold is 100), but a well-connected peer that forwards multiple valid DSTXes against a stale node could still accumulate score over time. Unlike `!IsValidStructure()` and bad-signature, which are unambiguous protocol violations, missing-MN can be a benign timing artifact. Consider treating `!dmn` like the `!IsValidForMixingTxes` branch (`{true, true}`), or gating the punishment with `m_mn_sync.IsBlockchainSynced()`, so only structural and signature failures are punished.
|
Addressed the automated review feedback in Validation:
@coderabbitai review |
|
✅ Action performedReview finished.
|
1 participant
fix: punish invalid dstx messages
Issue being fixed or feature implemented
Invalid CoinJoin broadcast transaction (
dstx) messages can return early fromvalidation without applying the same peer-accounting consequence used by other
invalid P2P messages.
This hardens DSTX handling by making invalid validation results contribute a low
misbehavior score while preserving benign early returns for duplicate or
otherwise non-error DSTX cases.
What was done?
result.
cases.
dstxmessages.How Has This Been Tested?
Tested on macOS arm64.
git diff --checkpython3 -m py_compile test/functional/p2p_dstx.py test/functional/test_framework/messages.pyconfig.site,--without-gui,--disable-bench,and
--disable-fuzz-binarymake -j8 src/dashdtest/functional/p2p_dstx.py --tmpdir=/tmp/dash_func_dstxtest/functional/p2p_dstx.py --tmpdir=/tmp/dash_func_dstx_2Breaking Changes
None.
Checklist