-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: bound pending sig share queue #7415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
ec15f90
4666ef3
e2862fe
08d9533
355da45
bdca48c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,11 @@ namespace { | |
| constexpr size_t MAX_SESSIONS_PER_PEER_FACTOR{4}; | ||
| constexpr size_t MIN_SESSIONS_PER_PEER{100}; | ||
|
|
||
| // Incoming QSIGSHARE/QBSIGSHARES traffic is cheap to admit but drains only at BLS verification | ||
| // speed, so unverified shares are bounded and over-cap shares dropped without misbehaviour scoring. | ||
| constexpr size_t MAX_PENDING_SIG_SHARES_PER_NODE{1000}; | ||
| constexpr size_t MAX_PENDING_SIG_SHARES_TOTAL{10000}; | ||
|
|
||
| size_t GetMaxSessionsForPeer(const Consensus::LLMQParams& params) | ||
| { | ||
| return std::max<size_t>(size_t(params.size) * MAX_SESSIONS_PER_PEER_FACTOR, MIN_SESSIONS_PER_PEER); | ||
|
|
@@ -422,7 +427,7 @@ bool CSigSharesManager::ProcessMessageBatchedSigShares(const CNode& pfrom, const | |
| LOCK(cs); | ||
| auto& nodeState = nodeStates[pfrom.GetId()]; | ||
| for (const auto& s : sigSharesToProcess) { | ||
| nodeState.pendingIncomingSigShares.Add(s.GetKey(), s); | ||
| TryAddPendingIncomingSigShare(pfrom.GetId(), nodeState, s); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| } | ||
| return true; | ||
| } | ||
|
|
@@ -467,14 +472,39 @@ bool CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s | |
| } | ||
|
|
||
| auto& nodeState = nodeStates[fromId]; | ||
| nodeState.pendingIncomingSigShares.Add(sigShare.GetKey(), sigShare); | ||
| TryAddPendingIncomingSigShare(fromId, nodeState, sigShare); | ||
| } | ||
|
|
||
| LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- signHash=%s, id=%s, msgHash=%s, member=%d, node=%d\n", __func__, | ||
| signHash.ToString(), sigShare.getId().ToString(), sigShare.getMsgHash().ToString(), sigShare.getQuorumMember(), fromId); | ||
| return true; | ||
| } | ||
|
|
||
| bool CSigSharesManager::TryAddPendingIncomingSigShare(NodeId nodeId, CSigSharesNodeState& nodeState, | ||
| const CSigShare& sigShare) | ||
| { | ||
| AssertLockHeld(cs); | ||
|
|
||
| if (nodeState.banned) { | ||
| return false; | ||
| } | ||
| if (nodeState.pendingIncomingSigShares.Size() >= MAX_PENDING_SIG_SHARES_PER_NODE) { | ||
| LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- per-node pending sig shares cap reached (%d), dropping sigShare. node=%d\n", | ||
| __func__, MAX_PENDING_SIG_SHARES_PER_NODE, nodeId); | ||
| return false; | ||
| } | ||
| size_t total{0}; | ||
| for (const auto& [_, ns] : nodeStates) { | ||
| total += ns.pendingIncomingSigShares.Size(); | ||
| } | ||
| if (total >= MAX_PENDING_SIG_SHARES_TOTAL) { | ||
| LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- global pending sig shares cap reached (%d), dropping sigShare. node=%d\n", | ||
| __func__, MAX_PENDING_SIG_SHARES_TOTAL, nodeId); | ||
| return false; | ||
| } | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Global pending-sig-share cap check is O(peers) per admission TryAddPendingIncomingSigShare iterates every entry in nodeStates while holding cs to compute the global total on every QSIGSHARE/QBSIGSHARES admission. The per-node check is O(1) thanks to CountedBucketMap::Size(), but this loop reintroduces O(peers) work on the hot admission path. Under a flood with many connected peers, cs is held longer per accepted share, partially defeating the O(1) counting refactor and making the backpressure path itself scale with peer count. Maintain a manager-level running total that is updated alongside per-node Add/Erase operations so the global cap remains O(1). Correctness is unaffected, so this is non-blocking. source: ['claude']
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in |
||
| return nodeState.pendingIncomingSigShares.Add(sigShare.GetKey(), sigShare); | ||
| } | ||
|
Comment on lines
+483
to
+508
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Cap/drop/banned enforcement in TryAddPendingIncomingSigShare has no direct unit coverage The PR's stated purpose is bounding pending incoming sig shares, but the enforcement logic in source: ['claude'] |
||
|
|
||
| bool CSigSharesManager::CollectPendingSigSharesToVerify( | ||
| size_t maxUniqueSessions, std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares, | ||
| std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums) | ||
|
|
@@ -1425,6 +1455,7 @@ void CSigSharesManager::MarkAsBanned(NodeId nodeId) | |
| sigSharesRequested.Erase(k); | ||
| }); | ||
| nodeState.requestedSigShares.Clear(); | ||
| nodeState.pendingIncomingSigShares.Clear(); | ||
| nodeState.banned = true; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fresh evidence beyond the earlier queue-cap concern is that
CollectPendingSigSharesToVerify(32, ...)limits unique(nodeId, signHash)pairs, not actual shares; if peers fill the pending maps with many quorum members for the same few sessions, one collected “batch” can contain up to the 10k pending-share cap. This line then allows four such batches to be queued/in flight after they have been erased from the capped pending maps, so flood traffic can still move tens of thousands of BLS shares into verification work despite the intended global cap. Cap the number of shares collected per dispatch or keep queued/in-flight shares charged against the cap.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in
355da45d4a4.CollectPendingSigSharesToVerifynow caps each dispatch by actual shares added (MAX_SHARES_PER_BATCH = 32) instead of unique sessions. With four unverified batches, at most 128 shares can sit in/on the worker pool outside the 10,000-share pending maps; randomized peer round-robin fairness is preserved. Focused LLMQ tests and neighboring suites pass.