refactor: route per-LLMQ-type caches through PerLlmqTypeCache - #7516
refactor: route per-LLMQ-type caches through PerLlmqTypeCache#7516PastaPastaPasta wants to merge 1 commit into
Conversation
|
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:
WalkthroughThe change adds Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant NetSigning
participant LLMQTypeRegistry
Peer->>NetSigning: Send QSIGSHARE
NetSigning->>LLMQTypeRegistry: Validate LLMQ type
LLMQTypeRegistry-->>NetSigning: Type is unregistered
NetSigning->>Peer: Log, ban, and stop processing
Possibly related PRs
Suggested reviewers: 🚥 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 |
|
✅ Final review complete — no blockers (commit 40d714a) |
46a4012 to
9abbc9b
Compare
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
e464127 to
35a4bc9
Compare
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If these PRs merge firstThis PR will likely need a rebase:
|
dc2d5cf to
84847d7
Compare
45c7343 to
d8f8044
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Carried forward from the prior review, the network-facing invalid-QSIGSHARE rejection and 100-point peer-scoring branch remains untested; no correctness blocker was found in the implementation. In the latest delta, squashing resolves the earlier corrective-follow-up history issue but newly removes the two-commit separation that the PR description explicitly identifies as necessary for minimal backports.
Source: reviewers codex/gpt-5.6-sol (general and dash-core-commit-history) and claude/claude-sonnet-5 (general and dash-core-commit-history; the first specialist attempt was unparseable and a fresh retry succeeded); verifier codex/gpt-5.6-sol. The openclaw-agent coordinator is orchestration-only.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 2 suggestion(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/llmq/net_signing.cpp`:
- [SUGGESTION] src/llmq/net_signing.cpp:83-87: Add coverage for the QSIGSHARE validation and ban path itself
The new unit suite tests unsupported types only through `PerLlmqTypeCache`, `HasMinedCommitment`, `GetQuorum`, and `GetCachedMutableQuorum`; it never sends an invalid QSIGSHARE through `NetSigning::ProcessMessage`. The existing functional QSIGSHARE case uses a registered type. This leaves the new trust-boundary behavior unprotected: if this gate were removed, the hardened cache would safely miss, `ProcessMessageSigShare` would return true when `GetQuorum` returns null (`signing_shares.cpp` lines 437-440), and the sender would be silently dropped without reaching the fallback `BanNode` call. Add a functional P2P case with SPORK_21 active that sends a `CSigShare` carrying an unregistered type and verifies the rejection log and 100-point peer score; the whitelisted-peer and deprecated-banscore pattern in `feature_llmq_dkg_intake.py` keeps that score observable.
In `<commit:d8f8044>`:
- [SUGGESTION] <commit:d8f8044>:1: Single squashed commit undermines the stated backport-separability goal
The PR description says this is structured as two commits: a standalone QSIGSHARE gate small enough to backport, followed by the broader cache refactor and its tests. Current head instead contains exactly one commit whose patch combines the handler gate with the new cache abstraction, eight cache migrations, build wiring, and unit tests. That removes the independently cherry-pickable security fix and conflicts with the stated backport plan. Split the history into a minimal gate commit and a separate `PerLlmqTypeCache` refactor-and-tests commit; this also keeps the broad structural hardening independently reviewable and bisectable.
| if (!Params().GetLLMQ(sigShare.getLlmqType()).has_value()) { | ||
| LogPrint(BCLog::LLMQ_SIGS, "NetSigning::%s -- rejected %s from peer=%d: invalid llmqType [%d]\n", | ||
| __func__, msg_type, pfrom.GetId(), std23::to_underlying(sigShare.getLlmqType())); | ||
| BanNode(pfrom.GetId()); | ||
| return; |
There was a problem hiding this comment.
🟡 Suggestion: Add coverage for the QSIGSHARE validation and ban path itself
The new unit suite tests unsupported types only through PerLlmqTypeCache, HasMinedCommitment, GetQuorum, and GetCachedMutableQuorum; it never sends an invalid QSIGSHARE through NetSigning::ProcessMessage. The existing functional QSIGSHARE case uses a registered type. This leaves the new trust-boundary behavior unprotected: if this gate were removed, the hardened cache would safely miss, ProcessMessageSigShare would return true when GetQuorum returns null (signing_shares.cpp lines 437-440), and the sender would be silently dropped without reaching the fallback BanNode call. Add a functional P2P case with SPORK_21 active that sends a CSigShare carrying an unregistered type and verifies the rejection log and 100-point peer score; the whitelisted-peer and deprecated-banscore pattern in feature_llmq_dkg_intake.py keeps that score observable.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Add coverage for the QSIGSHARE validation and ban path itself no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
d8f8044 to
c722221
Compare
|
Good catch, added in c722221. Agreed that the gate was unpinned. I confirmed the mechanism you describe: I implemented it as a unit case rather than a functional one. Verified it actually pins the behaviour rather than just passing: with the gate deleted the test fails with One clarification on scope, since the phrasing suggests otherwise: what was untested is the ban, not the crash. The 🤖 Posted autonomously by Claude on behalf of pasta. |
|
This pull request has conflicts, please rebase. |
… QGETDATA b05403e fix: score peers requesting an unregistered LLMQType via QGETDATA (pasta) Pull request description: ## Issue being fixed or feature implemented The `QGETDATA` handler validates the requested `Consensus::LLMQType`, but treats an unregistered one as a routine miss. `sendQDATA` groups `QUORUM_TYPE_INVALID` with `QUORUM_BLOCK_NOT_FOUND`, `QUORUM_NOT_FOUND` and `MASTERNODE_IS_NO_MEMBER`, all of which are scored only when the request limiter is also exceeded: ```cpp case (CQuorumDataRequest::Errors::QUORUM_TYPE_INVALID): case (CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND): case (CQuorumDataRequest::Errors::QUORUM_NOT_FOUND): case (CQuorumDataRequest::Errors::MASTERNODE_IS_NO_MEMBER): misbehave = request_limit_exceeded; ``` That grouping is right for the others: a peer hits them innocently when it is ahead of us, or when we lack data it reasonably expected us to have. An unregistered LLMQ type is not in that category. No quorum of a type this chain does not register can exist, so there is nothing for a well-behaved peer to ask about, and every sibling handler already treats one as a protocol violation -- QSIGREC, the DKG messages and QFCOMMITMENT all score 100. The practical exposure is small: `QGETDATA` already requires a ProRegTx-verified masternode or a qwatch connection, and each request passes through `RegisterDataRequest`, so the traffic is rate-limited even when unscored. This is a consistency fix, not a fix for a live attack. ## What was done? Send the `QUORUM_TYPE_INVALID` reply exactly as before, so a confused peer still learns why its request failed, then score the sender 100. `p2p_quorum_data.py` exercises every error code on one connection. Its invalid-type case moves to the end of that block, because the peer is now dropped once it sends one; the test asserts the disconnect. Spotted while reviewing #7516, which fixes an abort reachable through the same class of unvalidated `LLMQType`. This change is independent of it -- it does not depend on that PR and can land in either order. ## How Has This Been Tested? Built and run on macOS/arm64 against develop: - Full build clean. - `p2p_quorum_data.py`: passes. ## Breaking Changes A peer that requests quorum data for an LLMQ type this chain does not register is now scored to the ban threshold instead of being answered indefinitely. Only requests that were already answered with `QUORUM_TYPE_INVALID` are affected. A new LLMQ type is introduced with a deployment, and no quorum of that type exists on chain before activation, so an honest peer has nothing to request until every node recognises the type. This should not misfire across a version boundary. ## 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 Top commit has no ACKs. Tree-SHA512: 4a3636aa3964de8c61eef2cf6cbf6311647473c6ebf09b389465c97294e993f588441b42234812866aaf91681de4aca75a1fffe41d1fac9e3ef03c23c02e509e
Consensus::LLMQType is a uint8_t enum that arrives verbatim over the wire, and the LRU caches keyed by it are only pre-seeded with the types in Params().GetConsensus().llmqs. Indexing one of those std::maps with operator[] on an unregistered key default-constructs an unordered_lru_cache with MaxSize = 0, whose constructor asserts; util/check.h makes compiling with NDEBUG a hard #error, so that assert is live in release builds. The QSIGSHARE handler gate and find()-guarded lookups this branch originally carried landed separately in v23.1.8 (a801d3f) and reached develop via the master back-merge, so this is now the follow-up hardening rather than the fix itself. That fix guarded only the two lookups a QSIGSHARE could reach; every other map keyed by LLMQ type still used operator[]. Replace them all with PerLlmqTypeCache, which owns one LRU per registered type and answers unregistered types as misses, so no caller can size a zero-capacity cache into existence. This covers mapHasMinedCommitmentCache, m_qc_hashes_lru, mapQuorumsCache, scanQuorumsCache, cleanupQuorumsCache, indexed_quorums_cache, mapQuorumMembers and mapIndexedQuorumMembers, and lets InitQuorumsCache go away. Extend llmq_invalid_type_tests to pin the cache contract itself plus the QSIGSHARE ban. The ban needs its own test because no cache-level assertion can catch its removal: ProcessMessageSigShare returns true when GetQuorum yields nullptr, so without the gate the peer is dropped silently and never scored.
c722221 to
40d714a
Compare
PastaPastaPasta
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR replaces eight ad-hoc per-LLMQ-type caches with a single PerLlmqTypeCache abstraction in one coherent commit (40d714a), preserving capacities, lock scopes, and lazy-init semantics at every converted call site, and adds a genuine end-to-end test (qsigshare_unknown_llmq_type_bans_peer) that drives NetSigning::ProcessMessage with a hand-serialized unregistered-type QSIGSHARE message and asserts the peer misbehavior score moves 0→100. Both fresh Sonnet reviewer lanes and the fresh Codex checkpoint independently confirm zero new findings at head 40d714a, and I independently verified: (1) the new test at src/test/llmq_invalid_type_tests.cpp:170-201 pins the handler gate and BanNode's 100-point scoring, and (2) git log --oneline fff866495dc..40d714a8778 -- src/llmq/net_signing.cpp returns empty while git merge-base --is-ancestor a801d3f3b72 fff866495dc confirms the QSIGSHARE point fix already shipped via v23.1.8/#7534, so this branch carries only the structural refactor with no point fix left to separate. No blocking, suggestion, or nitpick findings apply to the current head.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
2 participants
Issue being fixed or feature implemented
Note on scope after rebase: this PR originally carried the QSIGSHARE fix itself. That fix shipped independently in v23.1.8 (
a801d3f3b72) and reacheddevelopthrough the master back-merge in #7534, so the handler gate and the twofind()-guarded lookups are already present. What remains here is the structural follow-up, retitled accordingly.Consensus::LLMQTypeis auint8_tenum serialized verbatim over the wire with no range check. The LLMQ LRU caches keyed by it arestd::map<LLMQType, unordered_lru_cache<...>>, seeded only with the types registered for the active chain.operator[]on an unregistered key default-constructs the cache withMaxSize = 0, andunordered_lru_cache's constructor runsassert(_maxSize != 0). Sincesrc/util/check.hmakes compiling withNDEBUGa hard#error, that assert is live in release builds — so any suchoperator[]is a deterministic abort waiting for a caller that has not validated the type.The v23.1.8 fix closed the reported vector by gating the QSIGSHARE handler and switching the two lookups a
QSIGSHAREcould reach (HasMinedCommitment,GetQuorum/GetCachedMutableQuorum) tofind(). The remaining caches still useoperator[], and nothing in the type system stops a future caller from reaching one with an unvalidated type.What was done?
Remove the hazard structurally rather than gating each lookup. A new
PerLlmqTypeCache<Value, Key>(src/llmq/cache.h) owns the map, holds one LRU per registered type, and answers for any other type as a miss with writes dropped — so no caller can size a zero-capacity cache into existence.All eight caches are converted (
mapHasMinedCommitmentCache,m_qc_hashes_lru,mapQuorumsCache,scanQuorumsCache,mapQuorumMembers,mapIndexedQuorumMembers,indexed_quorums_cache,cleanupQuorumsCache), the ad-hocfind()guards added in v23.1.8 are folded into it, andInitQuorumsCacheis deleted.Per-type cache capacities, lock scopes,
GUARDED_BYannotations and the lazyif (empty()) Init(...)seeding are preserved at every converted site.One deliberate behaviour change worth calling out:
HasMinedCommitmenton an unregistered type now falls through to the EvoDB probe and returns false, wheredevelopcurrently returns false before touching the DB. That probe is the same one any cache miss performs, and the handlers reject unregistered types before reaching it, so this is not remotely reachable — but it is a step back from the early return and is easy to restore if reviewers prefer.llmq_invalid_type_testsis extended to pin thePerLlmqTypeCachecontract itself (unregistered types read as misses, writes dropped, capacity 0) alongside the existingHasMinedCommitment/GetQuorum/GetCachedMutableQuorumcoverage, and adds a test for the QSIGSHARE ban. That ban needs its own test because no cache-level assertion can catch its removal:ProcessMessageSigSharereturnstruewhenGetQuorumyieldsnullptr, so without the gate the peer is dropped silently and never scored — every other test in the file would stay green.How Has This Been Tested?
Locally, on macOS/arm64, rebased onto
developatfff866495dc:src/test/test_dash --run_test=llmq_invalid_type_tests,llmq_utils_tests: 19 test cases, no errors.Breaking Changes
None. Only lookups naming an LLMQ type that is not registered for the active chain are affected, and those were never valid.
Checklist: