fix: reject unvalidated LLMQType in QSIGSHARE before quorum lookup - #7516
fix: reject unvalidated LLMQType in QSIGSHARE before quorum lookup#7516PastaPastaPasta wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
WalkthroughThe change adds Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
|
🔍 Review in progress — actively reviewing now (commit 45c7343) |
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 this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
dc2d5cf to
84847d7
Compare
Consensus::LLMQType is a uint8_t enum serialized verbatim in CSigBase/CSigShare with no range check. ProcessMessage fed each CSigShare from a QSIGSHARE message straight into ProcessMessageSigShare -> GetQuorum -> HasMinedCommitment, which indexed mapHasMinedCommitmentCache[llmqType]. That map holds only the types registered for the active chain, and operator[] on any other key default-constructs an LRU with MaxSize 0, whose constructor asserts -- a deterministic abort from a single ~100-byte message, reachable by any inbound peer past the version handshake. Every sibling handler (QSIGREC, QSIGSESANN, QGETDATA, QFCOMMITMENT) already gated on Params().GetLLMQ(...).has_value(); QSIGSHARE was the gap. QBSIGSHARES and QSIGSHARESINV/QGETSIGSHARES carry no type on the wire and inherit a session type that QSIGSESANN validated, so they need no gate of their own.
The LLMQ caches were std::map<LLMQType, unordered_lru_cache<...>> seeded by InitQuorumsCache with the registered types only. operator[] on any other key default-constructs an LRU whose MaxSize template argument defaults to 0, and unordered_lru_cache's constructor asserts on that, so every unguarded lookup was an abort waiting for a caller that had not validated the type. The previous commit closes the reported path; six more operator[] uses had the same shape and relied on their callers gating first. PerLlmqTypeCache owns the map instead: it holds a cache per registered type and answers for every other type as a miss, dropping the writes, so the hazard is unreachable rather than merely unreached. InitQuorumsCache is deleted. Converted: mapHasMinedCommitmentCache, m_qc_hashes_lru, mapQuorumsCache, scanQuorumsCache, mapQuorumMembers, mapIndexedQuorumMembers, indexed_quorums_cache, cleanupQuorumsCache. Per-type capacities, lock scopes, GUARDED_BY annotations and the lazy seeding are preserved at every site. llmq_invalid_type_tests covers the cache contract and the two lookup paths a wire-supplied type reaches: HasMinedCommitment directly, and GetQuorum via HasQuorum. One behaviour change: HasMinedCommitment on an unregistered type now falls through to the EvoDB probe and returns false rather than short-circuiting; that is the same probe any cache miss performs.
84847d7 to
45c7343
Compare
2 participants
Issue being fixed or feature implemented
Consensus::LLMQTypeis auint8_tenum serialized verbatim inCSigBase/CSigSharewith no range check. Before this change,llmq::NetSigning::ProcessMessagefed eachCSigSharefrom aQSIGSHAREmessage straight intoCSigSharesManager::ProcessMessageSigShare->CQuorumManager::GetQuorum->CQuorumBlockProcessor::HasMinedCommitment, which indexedmapHasMinedCommitmentCache[llmqType].That map is seeded only with the LLMQ types registered for the active chain.
operator[]on an unregistered key default-constructsUint256LruHashMap<bool>withMaxSize = 0, andunordered_lru_cache's constructor runsassert(_maxSize != 0). Sincesrc/util/check.hmakes compiling withNDEBUGa hard#error, this assert is live in release builds.The result is a deterministic abort from a single ~100-byte P2P message. It is reachable by any inbound peer past the version handshake with no masternode authentication and no quorum membership. The victim must be running as a masternode and have
SPORK_21_QUORUM_ALL_CONNECTEDactive, so the practical effect is degraded quorum availability, ChainLocks and InstantSend.Every sibling handler (QSIGREC, QSIGSESANN, QGETDATA, QFCOMMITMENT) already gated on
Params().GetLLMQ(...).has_value(). QSIGSHARE was the gap. QBSIGSHARES and QSIGSHARESINV/QGETSIGSHARES need no gate of their own: they carry no type on the wire and inherit one from a session that QSIGSESANN already validated.What was done?
LLMQTypein the QSIGSHARE handler before any quorum lookup, and score the sender. This closes the reported vector.std::map<LLMQType, unordered_lru_cache<...>>seeded byInitQuorumsCache, so everyoperator[]on them was an abort waiting for a caller that had not validated the type -- five uses outside the reported path had the same shape. A newPerLlmqTypeCache<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. All eight caches are converted (mapHasMinedCommitmentCache,m_qc_hashes_lru,mapQuorumsCache,scanQuorumsCache,mapQuorumMembers,mapIndexedQuorumMembers,indexed_quorums_cache,cleanupQuorumsCache) 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:HasMinedCommitmenton an unregistered type now falls through to the EvoDB probe and returns false, rather than returning early. That is the same probe any cache miss performs, and the handlers reject unregistered types before reaching it.How Has This Been Tested?
Two commits: the handler gate on its own (small enough to backport), then the cache refactor with the tests that guard it.
Locally, on macOS/arm64:
src/test/test_dash: 795 test cases, no errors. The newllmq_invalid_type_testssuite covers thePerLlmqTypeCachecontract plusHasMinedCommitment,GetQuorumandGetCachedMutableQuorumwith unregistered types.feature_llmq_signing.py,feature_llmq_rotation.py,feature_llmq_connections.py,feature_llmq_data_recovery.py,p2p_quorum_data.py: all pass.test/lint/all-lint.py: clean apart from pre-existing cppcheck warnings in files this PR does not touch.Breaking Changes
None. Only messages carrying an LLMQ type that is not registered for the active chain are affected, and those were never valid.
Checklist: