fix: punish invalid dstx messages - #7347
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
More reviews will be available in 16 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between f32588aa7850dcfcb5be9396f6bb36c86f1d52c6 and 4329a4f. 📒 Files selected for processing (4)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds 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 Unknown masternode (score=1, do_return=true)
ValidateDSTX-->>ProcessMsg: {1, true}
ProcessMsg->>Misbehave: Misbehaving(pfrom, 1, "invalid dstx")
Misbehave-->>ProcessMsg: penalty recorded
else Invalid DSTX structure (score=10, do_return=true)
ValidateDSTX-->>ProcessMsg: {10, true}
ProcessMsg->>Misbehave: Misbehaving(pfrom, 10, "invalid dstx")
Misbehave-->>ProcessMsg: penalty recorded
else Already have (score=0, do_return=true)
ValidateDSTX-->>ProcessMsg: {0, true}
ProcessMsg-->>Peer: return without penalty
else Valid (score=0, do_return=false)
ValidateDSTX-->>ProcessMsg: {0, false}
ProcessMsg->>ProcessMsg: continue to mempool acceptance
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 4329a4f) |
![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.
|
I think this would still allow an attacker to waste our CPU / potentially DoS us? no? IMO we should punish for unknown MNs, but make it a small punishment, so that someone would need to send like 50 or 100 invalid / bad mn dstx before we ban? |
|
Agreed — ignoring unknown-MN DSTX entirely leaves a cheap CPU/validation spam lane. I pushed
Validation: The test now covers the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/net_processing.cpp (1)
3542-3545: ⚡ Quick winUse a strong result type for DSTX validation scores.
The scoring logic looks correct, but
std::pair<int, bool>keeps this security-sensitive contract positional; old-style returns like{false, true}would still compile as a zero-penalty early return. A small named result type makes future edits safer.Suggested refactor
-// Returned score is the misbehavior penalty to apply to the relaying peer; 0 means no penalty. -// do_return signals the caller to stop further processing of the DSTX. -std::pair<int /*misbehavior_score*/, bool /*do_return*/> static ValidateDSTX(CDeterministicMNManager& dmnman, CDSTXManager& dstxman, ChainstateManager& chainman, - CMasternodeMetaMan& mn_metaman, CTxMemPool& mempool, CCoinJoinBroadcastTx& dstx, uint256 hashTx) +enum class DSTXMisbehaviorScore : int { + NONE = 0, + UNKNOWN_MASTERNODE = 1, + INVALID = 10, +}; + +struct DSTXValidationResult { + DSTXMisbehaviorScore misbehavior_score{DSTXMisbehaviorScore::NONE}; + bool do_return{false}; +}; + +static DSTXValidationResult ValidateDSTX(CDeterministicMNManager& dmnman, CDSTXManager& dstxman, ChainstateManager& chainman, + CMasternodeMetaMan& mn_metaman, CTxMemPool& mempool, CCoinJoinBroadcastTx& dstx, uint256 hashTx) @@ - return {10, true}; + return {DSTXMisbehaviorScore::INVALID, true}; @@ - return {0, true}; // not an error + return {DSTXMisbehaviorScore::NONE, true}; // not an error @@ - return {1, true}; + return {DSTXMisbehaviorScore::UNKNOWN_MASTERNODE, true}; @@ - return {0, true}; + return {DSTXMisbehaviorScore::NONE, true}; @@ - return {10, true}; + return {DSTXMisbehaviorScore::INVALID, true}; @@ - return {0, false}; + return {DSTXMisbehaviorScore::NONE, false}; @@ - const auto& [misbehavior_score, bDoReturn] = ValidateDSTX(*m_dmnman, m_dstxman, m_chainman, m_mn_metaman, m_mempool, dstx, hashTx); - if (bDoReturn) { - if (misbehavior_score > 0) { - Misbehaving(pfrom.GetId(), misbehavior_score, "invalid dstx"); + const auto dstx_validation = ValidateDSTX(*m_dmnman, m_dstxman, m_chainman, m_mn_metaman, m_mempool, dstx, hashTx); + if (dstx_validation.do_return) { + if (dstx_validation.misbehavior_score != DSTXMisbehaviorScore::NONE) { + Misbehaving(pfrom.GetId(), static_cast<int>(dstx_validation.misbehavior_score), "invalid dstx"); } return; }Also applies to: 3551-3555, 3588-3591, 3594-3604, 3610-3610, 4675-4679
🤖 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 `@src/net_processing.cpp` around lines 3542 - 3545, Replace the std::pair<int, bool> return type with a named result type (such as a struct with clearly named fields like misbehavior_score and do_return) to make the security-sensitive DSTX validation contract explicit and type-safe. Update the ValidateDSTX function signature at line 3542-3545 to use this new named type, then update all return statements within ValidateDSTX (around lines 3551-3555, 3588-3591, 3594-3604, 3610-3610) to construct instances of the new type using named initialization instead of positional pair construction. Finally, update all call sites of ValidateDSTX (around line 4675-4679) to use named field access instead of .first and .second accessors.
🤖 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 `@src/net_processing.cpp`:
- Around line 3542-3545: Replace the std::pair<int, bool> return type with a
named result type (such as a struct with clearly named fields like
misbehavior_score and do_return) to make the security-sensitive DSTX validation
contract explicit and type-safe. Update the ValidateDSTX function signature at
line 3542-3545 to use this new named type, then update all return statements
within ValidateDSTX (around lines 3551-3555, 3588-3591, 3594-3604, 3610-3610) to
construct instances of the new type using named initialization instead of
positional pair construction. Finally, update all call sites of ValidateDSTX
(around line 4675-4679) to use named field access instead of .first and .second
accessors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 161d5d50-ef31-421a-990b-b31c105d4c7e
📥 Commits
Reviewing files that changed from the base of the PR and between da0ea55f6d77d68f0d746979f0db834af5f0539e and a8346482ab7aecf1516a0a5b8cfae1928e72d8ef.
📒 Files selected for processing (2)
src/net_processing.cpptest/functional/p2p_dstx.py
|
Addressed CodeRabbit’s named-result nitpick in |
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both agents independently found no in-scope defects in PR #7347 at a8346482, and the prior review at da0ea55f also had no findings to carry forward. The PR cleanly converts DSTX validation to a graduated misbehavior score, applies a small +1 penalty for unverifiable (unknown-MN) DSTXes to discourage floods while tolerating transient MN-removal races, and adds a focused functional test covering all three outcomes including the cumulative discouragement path. CodeRabbit had zero inline findings.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (a8346482ab..f32588aa) is a pure readability refactor in src/net_processing.cpp: ValidateDSTX now returns a typed DSTXValidationResult struct with an enum class DSTXValidationScore replacing std::pair<int,bool>. Score values (NONE=0, UNKNOWN_MASTERNODE=1, INVALID=10) match the prior literals, and the caller correctly compares against NONE and casts to int for Misbehaving — behavior is identical to the prior commit. Prior review at a8346482ab found no issues and there are no carried-forward findings; cumulative current-head inspection (DSTX validation, peer misbehavior accounting, Python DSTX serialization, functional test coverage of unknown-MN / structurally invalid / discouragement-flood) also surfaced none.
Note: GitHub does not allow PastaClaw to approve their own PR, so this clean automated review is posted as a comment review.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest push (7b8a80f9) is a CI retrigger with no code changes since f32588aa, which had a clean prior review. No carried-forward findings; no new findings in the latest delta. Cumulative PR state (DSTX validation scoring with named result type, low-score penalty for unknown-masternode relays, and the focused functional test) remains clean. GitHub does not allow PastaClaw to approve its own PR, so this is posted as a clean comment review rather than an approval.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
looks good but you should drop the empty "ci retrigger" commit
Previously, DSTX messages whose masternode we couldn't find in the deterministic MN list were silently dropped with no peer penalty. This let a malicious peer flood us with unverifiable DSTXes indefinitely. Apply a small misbehavior score (1) on that path so 100 such messages from one peer cross the discouragement threshold, while keeping the existing stronger penalty (10) for clearly malformed DSTXes and bad-signature DSTXes. The ValidateDSTX return value is changed from a bool to the misbehavior score itself; the call site now applies that score directly. Extend p2p_dstx.py to cover all three observable cases: small penalty for unknown-MN, stronger penalty for malformed structure, and the cumulative discouragement of a peer flooding unknown-MN DSTXes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the positional std::pair<int, bool> return with a small DSTXValidationResult struct and a DSTXValidationScore enum (NONE=0, UNKNOWN_MASTERNODE=1, INVALID=10). Behavior is unchanged; the call site reads named fields and casts the enum for Misbehaving. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7b8a80f to
4329a4f
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both agents independently found no in-scope issues in this PR. The latest delta (f32588a → 4329a4f) is empty (rebase), and the cumulative review of the DSTX validation scoring change (named result type with tiered DSTXValidationScore enum) shows a well-formed correctness fix with appropriate functional test coverage. No prior findings to reconcile.
Reviewed commit: 4329a4f1.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Done — I dropped the empty CI-retrigger commit. Current head is |
|
This pull request has conflicts, please rebase. |
4329a4f refactor: use named result type and score enum for ValidateDSTX (PastaClaw) 7d073b9 fix: penalize unknown-masternode dstx relays with a small score (PastaClaw) 9c68646 fix: avoid punishing unverifiable dstx relays (PastaClaw) c25cf3d fix: punish invalid dstx messages (PastaClaw) Pull request description: # fix: punish invalid dstx messages ## Issue being fixed or feature implemented Invalid CoinJoin broadcast transaction (`dstx`) messages can return early from validation 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? - Apply a low misbehavior score when DSTX validation returns an invalid/error result. - Preserve existing behavior for successful validation and benign early-return cases. - Add P2P test framework serialization for `dstx` messages. - Add a focused functional test covering invalid DSTX peer accounting. ## How Has This Been Tested? Tested on macOS arm64. - `git diff --check` - `python3 -m py_compile test/functional/p2p_dstx.py test/functional/test_framework/messages.py` - Configured with depends `config.site`, `--without-gui`, `--disable-bench`, and `--disable-fuzz-binary` - `make -j8 src/dashd` - `test/functional/p2p_dstx.py --tmpdir=/tmp/dash_func_dstx` - `test/functional/p2p_dstx.py --tmpdir=/tmp/dash_func_dstx_2` ## Breaking Changes None. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] 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 ACKs for top commit: UdjinM6: utACK 4329a4f Tree-SHA512: 79ac5b9bf9d359d68abf9b8a890fd09719c642635f199568efe4cb61c9979aa8cf89df48e6229c5c807b12b4ae127be701aef95ec04f4518fcabcdde305ce9b0 (cherry picked from commit a59ad9e)
4329a4f refactor: use named result type and score enum for ValidateDSTX (PastaClaw) 7d073b9 fix: penalize unknown-masternode dstx relays with a small score (PastaClaw) 9c68646 fix: avoid punishing unverifiable dstx relays (PastaClaw) c25cf3d fix: punish invalid dstx messages (PastaClaw) Pull request description: # fix: punish invalid dstx messages ## Issue being fixed or feature implemented Invalid CoinJoin broadcast transaction (`dstx`) messages can return early from validation 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? - Apply a low misbehavior score when DSTX validation returns an invalid/error result. - Preserve existing behavior for successful validation and benign early-return cases. - Add P2P test framework serialization for `dstx` messages. - Add a focused functional test covering invalid DSTX peer accounting. ## How Has This Been Tested? Tested on macOS arm64. - `git diff --check` - `python3 -m py_compile test/functional/p2p_dstx.py test/functional/test_framework/messages.py` - Configured with depends `config.site`, `--without-gui`, `--disable-bench`, and `--disable-fuzz-binary` - `make -j8 src/dashd` - `test/functional/p2p_dstx.py --tmpdir=/tmp/dash_func_dstx` - `test/functional/p2p_dstx.py --tmpdir=/tmp/dash_func_dstx_2` ## Breaking Changes None. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] 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 ACKs for top commit: UdjinM6: utACK 4329a4f Tree-SHA512: 79ac5b9bf9d359d68abf9b8a890fd09719c642635f199568efe4cb61c9979aa8cf89df48e6229c5c807b12b4ae127be701aef95ec04f4518fcabcdde305ce9b0 (cherry picked from commit a59ad9e)
24920a0 chore: prepare v23.1.8 release (pasta) 2194248 Merge #7348: fix: penalize oversized notfound messages (pasta) f5c72c3 Merge #7347: fix: punish invalid dstx messages (pasta) 550caf7 Merge #7465: fix(qt): handle pixel-sized fonts when scaling widgets (pasta) e203710 Merge #7419: fix(net): bound CoinJoin message vector intake (pasta) 5f5b960 Merge #7418: fix(net): bound signing message vector intake (Pasta) 7cc2cca Merge #7450: test: make governance vote fixtures wire-valid (Pasta) f011c80 Merge #7440: fix(net): bound governance vote signature deserialization (Pasta) 4b4d96a Merge #7442: fix(net): authorize governance inv responses via the net-layer per-peer request tracker (Pasta) f855b13 Merge #7444: fix(net): bound bloom message vectors before allocation (Pasta) 5b5c6fb Merge #7415: fix: bound pending sig share queue (Pasta) 9bbe808 Merge #7416: fix(net): bound quorum data response vectors (Pasta) da42f50 Merge #7424: fix: bound ChainLock seen cache (Pasta) 5b310df Merge #7438: fix: bound SPORK signature deserialization (Pasta) e118d0c Merge #7259: fix: dangling point to cj client (Pasta) 9921621 Merge #7439: refactor: add bounded vector deserialization (Pasta) 89bdf7c Merge #7414: fix(net): throttle per-object governance vote sync requests (Pasta) 44c396d Merge #7402: fix: bound pending recovered sig queue to prevent remote OOM (Pasta) 05cfe27 Merge #7351: fix: limit signing share sessions per peer (pasta) 3ef3a5b Merge #7408: fix: bound DKG contribution blob intake (pasta) 0ea6532 Merge #7387: test: migrate governance inv cache coverage to unit tests (Pasta) 8ffdf7f Merge #7398: backport: compact block relay hardening (bitcoin#26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) (Pasta) 2915142 backport: bitcoin#27608 - p2p: Avoid prematurely clearing download state for other peers (PastaClaw) 90b5473 Merge #7396: fix: run of circular-dependencies with python3.15 (Pasta) b003cdc Merge #7395: ci: update GitHub Actions pins for Node 24 (pasta) 97c3dd1 Merge #7394: fix: stabilize par help text in manpages (pasta) 8f8616b Merge #7372: backport: bitcoin#32693: depends: fix cmake compatibility error for freetype (pasta) 48f72be Merge #7360: fix: empty platformP2PPort deprecated field in protx listdiff results (pasta) a8cccff Merge #7298: fix(qt): keep PoSe score visible when hiding banned masternodes (pasta) Pull request description: Release PR for Dash Core v23.1.8, a patch release on top of v23.1.7. Fast-forwards from `v23.1.x` (currently at `chore: prepare v23.1.7 release`), 29 commits, no merge commits, no conflicts. ## Contents Backports of PRs already reviewed and merged on `develop`: `#7259` `#7347` `#7348` `#7351` `#7298` `#7360` `#7372` `#7387` `#7394` `#7395` `#7396` `#7398` `#7402` `#7408` `#7414` `#7415` `#7416` `#7418` `#7419` `#7424` `#7438` `#7439` `#7440` `#7442` `#7444` `#7450` `#7465` Plus `backport: bitcoin#27608`, a single commit taken from Dash #7237 because #7398's compact-block hardening depends on it. The rest of that v0.26 batch is intentionally not included on v23.1.x. The commit is byte-identical to its reviewed counterpart inside #7237. And release preparation: version bump, regenerated man pages, release notes, archived 23.1.7 notes. ## Note for reviewers: this branch was rebuilt An earlier revision of this PR was discarded and the branch rebuilt from scratch. Review comments on the previous revision point at commits that no longer exist, though the feedback itself was carried over (see below). The reason: several commits titled `Merge #NNNN` in the earlier revision contained substantial code that exists nowhere upstream — apparently written from a description of each PR rather than ported from its diff. For example, `feature_llmq_simplepose.py` is byte-identical between v23.1.7 and `develop`, yet the earlier `Merge #7408` rewrote 66 lines of it; `test/functional/p2p_governance_invs.py` does not exist on `develop` at all, yet had grown from 62 to 148 lines. That mislabeling matters because a commit titled `Merge #NNNN` invites less scrutiny, not more. It also had consequences: the earlier revision was **missing #7440 entirely**, and contained eleven consecutive commits that did not compile (code written against newer upstream APIs this branch does not have — `Misbehaving(Peer&)`, and `PeerIsBanned` used five commits before it was declared). Every commit on this branch has now been diffed against its upstream merge commit. Where a backport differs, it is because v23.1.x predates an upstream refactor and the change had to be applied to the pre-refactor file — for example #7418 and #7438 patch `signing_shares.cpp` / `spork.cpp` where upstream patches `net_signing.cpp` / `net_processing.cpp`. ## Dropped from this branch - **#7350** (`net: don't lock cs_main while reading blocks`) — dropped on review feedback. It is a 110-line lock-structure refactor of `ProcessGetBlockData` with no measured benefit, and it would add avoidable churn to the eventual master→develop merge-back. Nothing on this branch depends on it: #7398's compact-block work precedes it, and the remaining 14 commits replay with zero conflicts once it is removed. Thanks @knst. ## Added after the initial review pass - **#7351** (`fix: limit signing share sessions per peer`) — cherry-picked as a single commit and placed before #7402, matching upstream's merge order. The include block additionally carries `<ranges>`: upstream's diff adds only `<algorithm>` because develop already had it, whereas v23.1.x did not and the backported `GetSessionCount()` / `GetAnnouncementSessionCount()` use `std::ranges::count_if`. - **#7465** (`fix(qt): handle pixel-sized fonts when scaling widgets`) — cherry-picked from the five upstream commits. `optiontests.cpp` additionally includes `qt/guiutil_font.h`, because `fontsLoaded()` and `updateFonts()` are declared there on v23.1.x while develop declares them in `qt/guiutil.h`, which is all the upstream test includes. Two further backports were added later and applied without any adaptation -- their diffs are byte-for-byte identical to upstream: - **#7347** (`fix: punish invalid dstx messages`) - **#7348** (`fix: penalize oversized notfound messages`) ## Adaptations worth flagging - **#7360** — upstream gates `platformP2PPort` / `platformHTTPPort` in `protx listdiff` behind `IsServiceDeprecatedRPCEnabled()`. On 23.x those deprecated fields are deliberately not enforced through gating (see `bbcd9d543e6`), so shipping the gate as-is would silently drop two fields that v23.1.7 always returned. Changed to `if (true)` with a comment, per review feedback, keeping the block aligned with `develop`. The substantive fix from #7360 — reading the live port from `netInfo` instead of the always-zero scalar — is retained. - **#7415** — the pending-map caps (`MAX_PENDING_SIG_SHARES_PER_NODE`, `MAX_PENDING_SIG_SHARES_TOTAL`) are backported. The additional bound upstream places on batches awaiting verification is not, because it guards a condition that does not exist here: upstream's dispatcher pushes one task per batch inside an inner loop, whereas v23.1.x pushes a single looping worker per 10 ms tick. There is no unbounded task queue to bound. - **Man pages** — regenerated without the `lock` debug category, which only exists under `DEBUG_LOCKCONTENTION` and so is absent from release binaries. Thanks @UdjinM6 for catching this. ## Known CI failure macOS jobs are expected to fail. `actions/upload-artifact@v6` rejects filenames containing `:`, and the Xcode SDK ships Perl man pages with `::` in the name. A release-branch-only workaround existed on the earlier revision but was dropped as it corresponds to no upstream PR. This is accepted for this release. ## Testing - Every commit through #7465 compiles individually (verified for 27 of the 29; the three additions below were verified at the tip) — verified individually, not just at the tip. - Full build clean; no new warnings. - Unit tests pass. - Functional tests pass: `feature_llmq_signing` (both variants), `feature_llmq_chainlocks`, `feature_llmq_dkgerrors`, `feature_llmq_is_cl_conflicts`, `p2p_instantsend`, `feature_dip3_deterministicmns` (both wallet types), `rpc_coinjoin`. - Qt unit tests pass (32 cases, run under the `cocoa` platform plugin so the pixel-sized font regression from #7465 actually executes rather than self-skipping). - Lint: one pre-existing `lint-cppcheck-dash` failure, identical on v23.1.7, in files this branch does not touch. Top commit has no ACKs. Tree-SHA512: 0fa469c9a33820aa85fbb8b90c5877409d09490f746f1300b05ceda470a600765f900bec42e5aad5d90a2d46b44e28c20e44dc4a8fe719553a072298069eaec4
Upstream landed its own "release: prepare v23.1.8" (dashpay#7493) plus parallel merges of several PRs this branch had already backported, so most conflicts are two adaptations of the same change rather than divergent intent. Resolutions: - Release artifacts (manpages, flatpak metainfo, release notes, configure.ac): took upstream's published v23.1.8 text as the base, then re-added the CoinJoin/wallet, GUI, and credits content that upstream's copy lacked. - LLMQ/net/test files where upstream carried a refined superset (dashpay#7351, dashpay#7347, dashpay#7348, dashpay#7465): took upstream. - CoinJoin client lifetime, overviewpage mixing-state, dmnstate platform ports, and coinjoin_tests: kept this branch's versions. - test/functional/p2p_governance_invs.py: accepted upstream's removal in favor of the src/test/governance_inv_tests.cpp migration (dashpay#7387). Auto-merge produced two duplicate definitions that would not compile (IsSyncableObject in governance.cpp, UnserializeBatchedSigShares in signing_shares.cpp); both were reduced to a single definition. Co-Authored-By: Claude <noreply@anthropic.com>
728f505 doc: record the v23.1.8 critical crash fixes in the release notes (pasta) ca56af8 fix(llmq): reject parentless quorum base blocks instead of terminating (pasta) a801d3f fix(llmq): reject unregistered LLMQ types from qsigshare before quorum lookup (pasta) 6c1f611 fix: skip already-removed conflicts when a ProTx key change clears the mempool (pasta) 24920a0 chore: prepare v23.1.8 release (pasta) 2194248 Merge #7348: fix: penalize oversized notfound messages (pasta) f5c72c3 Merge #7347: fix: punish invalid dstx messages (pasta) 550caf7 Merge #7465: fix(qt): handle pixel-sized fonts when scaling widgets (pasta) e203710 Merge #7419: fix(net): bound CoinJoin message vector intake (pasta) 5f5b960 Merge #7418: fix(net): bound signing message vector intake (Pasta) 7cc2cca Merge #7450: test: make governance vote fixtures wire-valid (Pasta) f011c80 Merge #7440: fix(net): bound governance vote signature deserialization (Pasta) 4b4d96a Merge #7442: fix(net): authorize governance inv responses via the net-layer per-peer request tracker (Pasta) f855b13 Merge #7444: fix(net): bound bloom message vectors before allocation (Pasta) 5b5c6fb Merge #7415: fix: bound pending sig share queue (Pasta) 9bbe808 Merge #7416: fix(net): bound quorum data response vectors (Pasta) da42f50 Merge #7424: fix: bound ChainLock seen cache (Pasta) 5b310df Merge #7438: fix: bound SPORK signature deserialization (Pasta) e118d0c Merge #7259: fix: dangling point to cj client (Pasta) 9921621 Merge #7439: refactor: add bounded vector deserialization (Pasta) 89bdf7c Merge #7414: fix(net): throttle per-object governance vote sync requests (Pasta) 44c396d Merge #7402: fix: bound pending recovered sig queue to prevent remote OOM (Pasta) 05cfe27 Merge #7351: fix: limit signing share sessions per peer (pasta) 3ef3a5b Merge #7408: fix: bound DKG contribution blob intake (pasta) 0ea6532 Merge #7387: test: migrate governance inv cache coverage to unit tests (Pasta) 8ffdf7f Merge #7398: backport: compact block relay hardening (bitcoin#26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) (Pasta) 2915142 backport: bitcoin#27608 - p2p: Avoid prematurely clearing download state for other peers (PastaClaw) 90b5473 Merge #7396: fix: run of circular-dependencies with python3.15 (Pasta) b003cdc Merge #7395: ci: update GitHub Actions pins for Node 24 (pasta) 97c3dd1 Merge #7394: fix: stabilize par help text in manpages (pasta) 8f8616b Merge #7372: backport: bitcoin#32693: depends: fix cmake compatibility error for freetype (pasta) 48f72be Merge #7360: fix: empty platformP2PPort deprecated field in protx listdiff results (pasta) a8cccff Merge #7298: fix(qt): keep PoSe score visible when hiding banned masternodes (pasta) Pull request description: ## Issue being fixed or feature implemented Merges master (v23.1.8) back into develop so develop carries the v23.1.8 release and, most importantly, the three remotely reachable crash fixes shipped on top of it. Without this, develop-based nodes remain vulnerable to all three crashes. ## What was done? Merged `upstream/master` into `develop`. The genuinely new payload is: - **Three remote-crash fixes** (with their regression tests): - skip already-removed conflicts when a ProTx key change clears the mempool (`txmempool.cpp`) - reject unregistered LLMQ types from `qsigshare` before quorum lookup (`llmq/blockprocessor.cpp`, `llmq/quorumsman.cpp`) - reject parentless quorum base blocks instead of terminating (`llmq/commitment.cpp`, `llmq/utils.cpp`, `validation.{cpp,h}`) - **Release bookkeeping**: v23.1.8 release notes, archived 23.1.7 notes, flatpak release entry, regenerated v23.1.8 man pages, and the `configure.ac` version bump (develop keeps `IS_RELEASE=false` and its own configure flags). Everything else on master since the last merge (the v23.1.x security-hardening train, #7259–#7465 and #7398) was dual-merged and already exists on develop as its own merge commits, so all conflicts from those files resolve to develop's side. Deliberate adaptations, itemized in the merge commit message: - The `qsigshare` LLMQ-type gate is ported into develop's `net_signing.cpp` and the parentless-base DKG check into develop's `net_dkg.cpp` (develop moved message processing out of `signing_shares.cpp`/`dkgsessionmgr.cpp`). - The defence-in-depth `find()` lookup is additionally applied to `CQuorumManager::GetCachedMutableQuorum()`, a develop-only method reached with a wire-supplied LLMQ type from the `QDATA` handler. - The new mempool regression test is adapted to develop's `CreateProRegTx`/`CreateProUpServTx`/`CreateProUpRevTx` helper signatures and `MemPoolOptionsForTest`. - `src/active/quorums.cpp` (deleted by develop's ActiveContext refactor) stays deleted; develop's #7416 equivalent already covers the new layout. ## How Has This Been Tested? Built with `--enable-debug` on macOS (arm64). Ran the new/extended suites — `evo_deterministicmns_tests`, `llmq_invalid_type_tests`, `evo_utils_tests` — plus adjacent ones (`llmq_signing_tests`, `llmq_dkg_tests`, `llmq_blockprocessor_tests`, `llmq_commitment_tests`, `llmq_utils_tests`, `mempool_tests`, `validation_tests`): all pass. The three regression tests abort the process on unpatched code. ## Breaking Changes None. `ChainstateManager::IsQuorumTypeEnabled()` loosens its `pindexPrev` parameter from `gsl::not_null` to a plain pointer (null now returns false instead of aborting); all existing callers are unaffected. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK e15bb64 Tree-SHA512: 0897f0e1267c4083ef505127ceb18309e310caf36a066674ea298d66836afb86c762532e7cc68d6c0761142b2e5b291b497496b7e82d0f7ce60549761c0ebdd8
3 participants
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