chore: merge master into develop#7397
Conversation
… crashes Harden the pushed DKG message path (QCONTRIB/QCOMPLAINT/QJUSTIFICATION/QPCOMMITMENT): - Require the sending peer to be MNAuth-verified (qwatch is unauthenticated and does not bypass this). - Reject oversized DKG payloads (per-type MaxDKGMessageSize) before deserialization/retention. - Structurally pre-validate (param-only checks, on a copy) before retention. - Never feed an invalid BLS signature to CBLSSignature::AggregateInsecure(): the batch verifier skips invalid sigs and per-type PreVerifyMessage rejects them.
…ector A validly-signed DKG self-complaint + justification can reach CBLSWorker::AsyncVerifyContributionShare() with a null BLSVerificationVectorPtr (when the member's contribution was never received), which the singular overload dereferenced with no null check, crashing observers/non-member masternodes. Treat a null vvec as a failed verification, matching the plural path. Includes a bls_tests regression case.
Inbound ISDLOCK messages from any peer are deserialized, hashed and retained before BLS signature verification, with three unbounded dimensions an attacker could exploit. Oversized lock: NetInstantSend::ProcessMessage now rejects locks whose input count exceeds InstantSendLock::MAX_INPUTS immediately after deserialization -- before ::SerializeHash() and the O(n) dedup in TriviallyValid() -- so an oversized lock no longer forces a second full serialize+hash before being scored. MAX_INPUTS is derived as MaxBlockSize() / 41 (~48,780): a consensus-valid transaction must fit in a block and each CTxIn is >=41 bytes on the wire, so no valid transaction -- and therefore no valid islock -- can exceed it. Deriving from MaxBlockSize() (now constexpr) keeps the cap correct if the maximum block size ever changes. The cap is also kept in TriviallyValid() as defense-in-depth. Unbounded received queue: EnqueueInstantSendLock() drops new locks once pendingInstantSendLocks reaches MAX_PENDING_INSTANTSEND_LOCKS (1024). Unbounded awaiting-tx queue: pendingNoTxInstantSendLocks holds BLS-verified locks whose transaction has not yet arrived; a peer with quorum-signing power could fill it with locks for transactions that never appear. It is now bounded by the same ceiling. Honest islocks are re-relayed, so dropping under flood is not fatal.
The per-object MNGOVERNANCESYNC vote-sync path passes a peer-supplied CBloomFilter to GetSyncableVoteInvs(), which calls filter.contains() on every cached vote; contains() loops nHashFuncs times. nHashFuncs is deserialized without bounds, so a ~43-byte request with nHashFuncs=0xFFFFFFFF forces ~4.3 billion MurmurHash3 evaluations per cached vote while g_msgproc_mutex is held, stalling all P2P message processing. Reject filters that violate CBloomFilter::IsWithinSizeConstraints() (vData <= 36000 bytes, nHashFuncs <= 50) right after deserialization, with Misbehaving(100), mirroring the existing filterload handler. Legitimate filters are constructed within these bounds, so this only rejects abusive requests.
ThreadSocketHandler uses a Dash-specific edge-triggered scheme: when mapReceivableNodes is non-empty it polls with a zero timeout (the only_poll fast path) instead of waiting SELECT_TIMEOUT_MILLISECONDS, so sockets that still have buffered data keep getting drained. A node enters mapReceivableNodes when its socket signals readable (fHasRecvData=true) and only leaves once SocketRecvData() drains it and clears fHasRecvData. A receive-paused peer (fPauseRecv, set once its queued message bytes exceed -maxreceivebuffer, ~5 MB by default) is skipped by both GenerateWaitSockets() and SocketHandlerConnected() and is therefore never drained, so fHasRecvData stays set and it lingers in mapReceivableNodes. only_poll then stays true and ThreadSocketHandler spins at 100% CPU for the entire pause window. A remote, unauthenticated peer can trigger and sustain this by sending data faster than the node processes it (resending periodically to stay paused and to reset the 20-minute inactivity timer), pinning the single socket-handler thread and degrading network I/O for all peers. Pull the receivable-node check into HasUnpausedReceivableNode() and count a receivable node as work only when it is not paused; while only paused nodes remain receivable, fall back to the normal select/poll wait. The paused peer is still drained promptly once the message handler pops enough messages to clear fPauseRecv. This finishes the partial backport of bitcoin#27981 (commit 8c986d6), which was noted to need revisiting alongside bitcoin#24356. Add net_tests/socket_handler_skips_receive_paused_nodes covering the predicate directly: a lone receive-paused node must not count as work (the regression), while genuinely drainable nodes still do. The busy-loop only manifests under sustained receive-pause, which requires the message-handler thread to fall behind the receive path; that is not reliably reproducible from a functional test without expensive or ban-triggering traffic, so the regression is guarded at the unit level.
c93513b Merge bitcoin#30633: Fixes for GCC 15 compatibility (merge-script) Pull request description: ## Issue being fixed or feature implemented Dash Core is failed to get compiled with warnings enabled on gcc-16 ## What was done? Backport bitcoin#30633 ## How Has This Been Tested? Build passes _these_ relevant errors ## Breaking Changes N/A ## Checklist: - [x] 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 - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK c93513b Tree-SHA512: c9b5537ff911b7fbc3eacd0517e7c67ccac907ce2d6983a1671c3a28196e460234d87e4b947536b71b310b44a1cbeb0341c3dc36c3bc6b98075c41de58b92238
…b subtree 75b19b3 fix: compilation error with modern gcc for leveldb subtree (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Dash Core's build fails with gcc-16: In copy constructor 'constexpr leveldb::DBImpl::CompactionState::Output::Output(const leveldb::DBImpl::CompactionState::Output&)', inlined from 'constexpr _Tp* std::construct_at(_Tp*, _Args&& ...) [with _Tp = leveldb::DBImpl::CompactionState::Output; _Args = {const leveldb::DBImpl::CompactionState::Output&}]' at /usr/include/c++/16/bits/stl_construct.h:110:9, inlined from 'static constexpr void std::allocator_traits<std::allocator<_Up> >::construct(allocator_type&, _Up*, _Args&& ...) [with _Up = leveldb::DBImpl::CompactionState::Output; _Args = {const leveldb::DBImpl::CompactionState::Output&}; _Tp = leveldb::DBImpl::CompactionState::Output]' at /usr/include/c++/16/bits/alloc_traits.h:716:21, inlined from 'constexpr void std::vector<_Tp, _Alloc>::_M_realloc_append(_Args&& ...) [with _Args = {const leveldb::DBImpl::CompactionState::Output&}; _Tp = leveldb::DBImpl::CompactionState::Output; _Alloc = std::allocator<leveldb::DBImpl::CompactionState::Output>]' at /usr/include/c++/16/bits/vector.tcc:594:26, inlined from 'constexpr void std::vector<_Tp, _Alloc>::push_back(const value_type&) [with _Tp = leveldb::DBImpl::CompactionState::Output; _Alloc = std::allocator<leveldb::DBImpl::CompactionState::Output>]' at /usr/include/c++/16/bits/stl_vector.h:1417:21, inlined from 'leveldb::Status leveldb::DBImpl::OpenCompactionOutputFile(CompactionState*)' at leveldb/db/db_impl.cc:808:31: leveldb/db/db_impl.cc:57:10: error: 'out.leveldb::DBImpl::CompactionState::Output::file_size' may be used uninitialized [-Werror=maybe-uninitialized] 57 | struct Output { | ^~~~~~ leveldb/db/db_impl.cc: In member function 'leveldb::Status leveldb::DBImpl::OpenCompactionOutputFile(CompactionState*)': leveldb/db/db_impl.cc:804:29: note: 'out' declared here 804 | CompactionState::Output out; ## What was done? Original leveldb's commit is ad9b1c989380538bfb19c5c65df0d3f72d8ed62b Backported to Bitcoin Core by bitcoin#34470 Though, that too many unrelated changes meanwhile. That's temporary fix until leveldb's version won't be properly updated. ## How Has This Been Tested? Together with dashpay#7392 I finally compile dash core with gcc 16 again. ## Breaking Changes N/A ## Checklist: - [x] 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 75b19b3 Tree-SHA512: 985042f04a4df7b20aaa773d302401af948099ae8c29d6669f0441e654b9c8847942e75574c52d1652411e321b80b1861f93c163792636d1246bea9edc99c249
Resolves conflicts caused by master's DKG intake hardening and instantsend
queue-cap fixes landing while develop had refactored the same code paths
into src/llmq/net_dkg.cpp and src/instantsend/net_instantsend.cpp.
- src/llmq/dkgsession.h: adopt master's split, include llmq/dkgmessages.h
instead of inlining the four DKG message classes.
- src/llmq/dkgmessages.h: use util/std23.h / std23::to_underlying to match
develop's convention (master used util/underlying.h).
- src/active/dkgsessionhandler.cpp, src/llmq/dkgsessionmgr.cpp: keep
develop's layout (the ProcessMessage / BatchVerify / HandleDKGRound path
now lives in net_dkg.cpp).
- src/llmq/net_dkg.cpp: port master's hardening into develop's layout:
* BatchVerifyMessageSigs skips messages with invalid signatures before
AggregateInsecure(), bails when no valid pubkeys were accumulated,
and defends the single-verification fallback against null members
and invalid sigs.
* NetDKG::ProcessMessage rejects non-MNAuth-verified peers, oversized
payloads (MaxDKGMessageSize) and structurally-invalid payloads
(CheckDKGMessageStructure) before retention.
- src/instantsend/instantsend.cpp: keep develop's PreVerifyIsLock, and
extend AddPendingISLock with the pendingNoTxInstantSendLocks queue cap
from master.
- src/test/bls_tests.cpp: combine both sides' new includes.
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 2dd6190) |
|
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)
WalkthroughThis PR updates Dash Core version metadata to v23.1.7, refreshes man pages and release notes, and adds a new v23.1.5 release-notes document. It also introduces DKG message types in a new header and adds validation for BLS verification inputs, governance bloom filters, InstantSend lock sizes, DKG message size/structure/signatures, and receive-paused socket handling. New unit and functional tests cover the added validation paths. Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Peer
participant NetDKG
participant CDKGSession
Peer->>NetDKG: push DKG message
NetDKG->>NetDKG: verify GetVerifiedProRegTxHash()
NetDKG->>NetDKG: check size vs MaxDKGMessageSize
NetDKG->>NetDKG: CheckDKGMessageStructure(msg)
NetDKG->>CDKGSession: PreVerifyMessage(msg)
CDKGSession->>CDKGSession: validate sig.IsValid()
CDKGSession-->>NetDKG: accept or mark peer bad
sequenceDiagram
participant Peer
participant NetInstantSend
participant CInstantSendManager
Peer->>NetInstantSend: ISDLOCK message
NetInstantSend->>NetInstantSend: check inputs.size() > MAX_INPUTS
NetInstantSend-->>Peer: reject and misbehave if oversized
NetInstantSend->>CInstantSendManager: enqueue lock
CInstantSendManager->>CInstantSendManager: check pending queue size
CInstantSendManager-->>NetInstantSend: drop or store
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
🧹 Nitpick comments (2)
test/functional/p2p_govsync_bloom.py (1)
48-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid coverage of the
nHashFuncsboundary; consider also coveringvDatasize.The test validates the
nHashFuncsbound but not thevData(36000-byte) bound also enforced byIsWithinSizeConstraints(). Optional addition for completeness.🤖 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/p2p_govsync_bloom.py` around lines 48 - 70, The GovsyncBloomCapTest currently checks only the nHashFuncs limit in msg_govsync/IsWithinSizeConstraints(), but it does not cover the vData 36000-byte limit. Extend run_test in GovsyncBloomCapTest to add a case that sends a govsync request with an oversized vData bloom filter payload and verify the peer is disconnected, keeping the existing good_peer and bad_peer boundary checks intact.test/functional/feature_llmq_dkg_intake.py (1)
107-119: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd the
QWATCH-then-push regression here.The new gate in
src/llmq/net_dkg.cppexplicitly says unauthenticatedQWATCHmust not bypass MN verification, but this test only covers a plain unverified sender. A peer that sendsQWATCHfirst and then pushesqcontrib/qpcommitis the edge case most likely to regress this hardening, so it should be asserted alongside this case.🤖 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_llmq_dkg_intake.py` around lines 107 - 119, Add a regression check in test_unverified_sender_rejected for the QWATCH-first path: alongside the existing unverified peer loop, use the same P2PInterface peer to send an unauthenticated QWATCH before any push message, then verify subsequent DKG_PUSH_TYPES still trigger the “DKG message from non-verified peer” debug log and ban score increments. Keep the assertion in the same test helper flow so the coverage explicitly exercises the new net_dkg.cpp gate against QWATCH followed by qcontrib/qpcommit pushes.
🤖 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.
Inline comments:
In `@src/instantsend/instantsend.h`:
- Around line 57-63: The current MAX_PENDING_INSTANTSEND_LOCKS cap in
InstantSend only limits item count, but each pending lock can carry many
outpoints, so memory can still grow far beyond the intended bound. Update the
pending-queue limit logic in the InstantSend codepath (using
MAX_PENDING_INSTANTSEND_LOCKS and the
pendingInstantSendLocks/pendingNoTxInstantSendLocks handling) to enforce a
total-byte budget instead of, or in addition to, a simple count. Make the
rejection/drain logic account for InstantSendLock::MAX_INPUTS so a single queue
cannot retain near-gigabyte outpoint storage under flood conditions.
In `@src/llmq/dkgsession.h`:
- Line 8: The dkgsession header is no longer self-contained because
ReceiveMessageState and the std::optional<CInv> APIs still rely on CInv, but the
current include set does not guarantee that type. Restore the direct protocol.h
include in src/llmq/dkgsession.h alongside llmq/dkgmessages.h so the
declarations that use CInv remain valid regardless of transitive includes.
---
Nitpick comments:
In `@test/functional/feature_llmq_dkg_intake.py`:
- Around line 107-119: Add a regression check in test_unverified_sender_rejected
for the QWATCH-first path: alongside the existing unverified peer loop, use the
same P2PInterface peer to send an unauthenticated QWATCH before any push
message, then verify subsequent DKG_PUSH_TYPES still trigger the “DKG message
from non-verified peer” debug log and ban score increments. Keep the assertion
in the same test helper flow so the coverage explicitly exercises the new
net_dkg.cpp gate against QWATCH followed by qcontrib/qpcommit pushes.
In `@test/functional/p2p_govsync_bloom.py`:
- Around line 48-70: The GovsyncBloomCapTest currently checks only the
nHashFuncs limit in msg_govsync/IsWithinSizeConstraints(), but it does not cover
the vData 36000-byte limit. Extend run_test in GovsyncBloomCapTest to add a case
that sends a govsync request with an oversized vData bloom filter payload and
verify the peer is disconnected, keeping the existing good_peer and bad_peer
boundary checks intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8e1878a2-215b-4781-a210-4fe522da45a9
📒 Files selected for processing (32)
configure.accontrib/flatpak/org.dash.dash-core.metainfo.xmldoc/man/dash-cli.1doc/man/dash-qt.1doc/man/dash-tx.1doc/man/dash-util.1doc/man/dash-wallet.1doc/man/dashd.1doc/release-notes.mddoc/release-notes/dash/release-notes-23.1.5.mdsrc/Makefile.amsrc/bls/bls_worker.cppsrc/consensus/consensus.hsrc/governance/net_governance.cppsrc/instantsend/instantsend.cppsrc/instantsend/instantsend.hsrc/instantsend/lock.cppsrc/instantsend/lock.hsrc/instantsend/net_instantsend.cppsrc/llmq/dkgmessages.hsrc/llmq/dkgsession.cppsrc/llmq/dkgsession.hsrc/llmq/dkgsessionmgr.cppsrc/llmq/net_dkg.cppsrc/net.cppsrc/net.hsrc/test/bls_tests.cppsrc/test/evo_islock_tests.cppsrc/test/net_tests.cpptest/functional/feature_llmq_dkg_intake.pytest/functional/p2p_govsync_bloom.pytest/functional/test_runner.py
💤 Files with no reviewable changes (1)
- src/llmq/dkgsessionmgr.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean release merge-back of v23.1.7 from master into develop. Both agents independently confirmed the conflict resolution is coherent: DKG hardening ported into develop's split net_dkg.cpp/net_instantsend.cpp layout, dkgmessages.h wired into Makefile.am, functional tests registered in test_runner.py, no conflict markers, and no consensus semantics altered.
Two new CodeRabbit inline findings were reviewed and found out of scope: (1) the item-count MAX_PENDING_INSTANTSEND_LOCKS cap in instantsend.h landed on master via commit 0a681db and shipped in v23.1.7 — a byte-budget refinement critiques already-released master design, not the merge resolution; (2) the dkgsession.h include swap to <llmq/dkgmessages.h> was master's own change (dkgmessages.h on master likewise omits <protocol.h>) and CInv reaches the header transitively — a header-hygiene nit inherited from a shipped release, not a build-breaking merge defect.
Posted as a COMMENT review because GitHub does not allow APPROVE on my own PR.
|
CodeRabbit's two inline findings are already withdrawn after the merge-back scope check. The two remaining review-level items are useful test-coverage ideas, but they both target code/tests that landed on |
|
Correction to my earlier triage: CI proved the Fixed in @coderabbitai review |
|
(๑•̀ㅂ•́)و good catch confirmed! ✏️ Learnings added
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta is a one-line #include <protocol.h> in src/llmq/dkgsession.h, which is required because the header exposes CInv as a member and in four ReceiveMessage return signatures. This fixes the previously out-of-scope CodeRabbit self-containment note. No new defects were introduced; the merge-back correctly preserves master's DKG intake hardening, InstantSend queue caps, BLS null-vvec guard, governance bloom bound, and socket-handler busy-loop fix in develop's refactored layout.
Posted as a COMMENT review because GitHub does not allow approving my own PR.
Prior review reconciliation: the previous automated review at 11cb440f had no in-scope findings to carry forward. The earlier out-of-scope CodeRabbit dkgsession.h self-containment note is now fixed/outdated by the direct #include <protocol.h> restoration in this head, and CodeRabbit has no active inline findings to react to on 2dd61907.
No in-scope findings.
|
The red I pulled the
|
3 participants
chore: merge master into develop
Issue being fixed or feature implemented
Merge the v23.1.7 release branch history from
masterback intodevelopafter the release.
What was done?
upstream/master(01fc719f90,chore: prepare v23.1.7 release) intoupstream/develop(547bf5eee2).InstantSend queue bounding, and BLS test coverage.
present on
master.test coverage;
CDKGPrematureCommitment::GetSignHash()definition after
dkgmessages.hintroduced inline message helpers;worker count;
leaving exact reduced-quorum member counts to the DKG session.
How Has This Been Tested?
git diff --check HEAD^1..HEADrg '^(<<<<<<<|>>>>>>>)'code-review dashpay/dash upstream/develop chore/merge-master-into-develop-20260701No local compile was run because this worktree has no configured build directory.
Breaking Changes
None expected. This is a release merge-back from
mastertodevelop.Checklist
code-owners and collaborators only)