-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: proactively push recovered signatures; fully connect instantsend quorums for recovered sigs #6967
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
feat: proactively push recovered signatures; fully connect instantsend quorums for recovered sigs #6967
Conversation
|
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6967.f172589d. A new comment will be made when the image is pushed. |
|
This pull request has conflicts, please rebase. |
f172589 to
10b47ca
Compare
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6967.f172589d. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6967.10b47ca5. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6967.10b47ca5. The image should be on dockerhub soon. |
10b47ca to
3c67936
Compare
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6967.3c679369. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6967.3c679369. The image should be on dockerhub soon. |
3c67936 to
2e8ac6a
Compare
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6967.2e8ac6a7. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6967.2e8ac6a7. The image should be on dockerhub soon. |
WalkthroughThis pull request threads a new proactive relay flag through recovered-signature handling. Sequence Diagram(s)sequenceDiagram
participant SM as CSigningManager
participant VS as CMainSignals / ValidationInterface
participant SSM as CSigSharesManager
participant PM as PeerManager
participant ZMQ as ZMQNotifiers
rect rgb(230,240,255)
Note over SM: Recovered sig arrives
SM->>SM: ProcessRecoveredSig(sig, peerman, from)
SM->>SM: compute proactive_relay (from, llmqType)
SM->>VS: NotifyRecoveredSig(sig, id, proactive_relay)
end
rect rgb(240,230,255)
VS->>SSM: NotifyRecoveredSig(sig, proactive_relay)
SSM->>PM: RelayRecoveredSig(sig, proactive_relay)
end
rect rgb(255,240,230)
alt proactive_relay == true
PM->>PM: Proactively broadcast QSIGREC to peers wanting rec sigs
else
PM->>PM: Relay via inventory (MSG_QUORUM_RECOVERED_SIG)
end
end
rect rgb(230,255,240)
PM->>ZMQ: NotifyRecoveredSig(sig, proactive_relay)
ZMQ->>ZMQ: Publish to subscribers (hash/raw as configured)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention during review:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/llmq/utils.cpp (1)
865-877: LGTM! All-members-connected relay path correctly implements full-mesh propagation.The logic correctly populates
relayMemberswith all other quorum members when AMC is enabled, enabling low-latency recovered signature propagation across the full mesh. The fallback toGetQuorumRelayMemberspreserves ring-based relay for non-AMC mode.Note the intentional semantic difference: in AMC mode, all members are included in
relayMembers(bidirectional), whereas non-AMC mode usesonlyOutbound = true(directional ring). This is correct per the comment's goal to "set m_wants_recsigs widely."Optional future refactoring: The AMC iteration pattern (lines 870-874) duplicates similar logic in
GetQuorumConnections(lines 741-744). Consider extracting a shared helper if this pattern grows, though the semantics differ slightly (relay members vs connections), so the current separation is reasonable.src/llmq/signing.h (1)
221-224: ProcessRecoveredSig signature extension is correctAdding
NodeId pFromhere cleanly matches the implementation insigning.cppand preserves the existing lock annotations. From a correctness and API perspective this looks good; if you touch this again, you might optionally renamepFromtofromfor consistency with the implementation.src/llmq/signing.cpp (1)
520-521: End-to-end propagation of originNodeIdandproactive_relaylooks correct
ProcessPendingReconstructedRecoveredSigsnow callsProcessRecoveredSig(..., -1), marking reconstructed/locally-produced recovered sigs as having no originating peer.ProcessPendingRecoveredSigspasses the actualnodeIdper peer, so only locally originated or reconstructed sigs seefrom == -1.CSigningManager::ProcessRecoveredSigusesfrom == -1together with the explicit LLMQ-type exclusion list to computeproactive_relay, and then callsGetMainSignals().NotifyRecoveredSig(recoveredSig, recoveredSig->GetHash().ToString(), proactive_relay);.Given how
NodeId == -1is already used as a sentinel elsewhere, this is a reasonable and consistent way to distinguish local vs. remote sigs for proactive relaying. The TODO about replacing the hard-coded type list with anIsAllMembersConnectedEnabled-style helper is also appropriate but not blocking.If you’d like, I can sketch a small helper (e.g.,
bool ShouldProactivelyRelay(Consensus::LLMQType, NodeId)in llmq/options) to centralize this policy and remove the explicit type list from here.Also applies to: 582-583, 590-639
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/llmq/signing.cpp(4 hunks)src/llmq/signing.h(1 hunks)src/llmq/signing_shares.cpp(3 hunks)src/llmq/signing_shares.h(1 hunks)src/llmq/utils.cpp(1 hunks)src/masternode/active/notificationinterface.cpp(1 hunks)src/masternode/active/notificationinterface.h(1 hunks)src/net_processing.cpp(2 hunks)src/net_processing.h(1 hunks)src/validationinterface.cpp(1 hunks)src/validationinterface.h(2 hunks)src/zmq/zmqabstractnotifier.cpp(1 hunks)src/zmq/zmqabstractnotifier.h(1 hunks)src/zmq/zmqnotificationinterface.cpp(1 hunks)src/zmq/zmqnotificationinterface.h(1 hunks)src/zmq/zmqpublishnotifier.cpp(2 hunks)src/zmq/zmqpublishnotifier.h(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash PR: 6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Applied to files:
src/llmq/signing.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/utils.cpp
📚 Learning: 2025-08-19T15:08:00.835Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.835Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.
Applied to files:
src/llmq/utils.cpp
🧬 Code graph analysis (15)
src/net_processing.h (3)
src/validationinterface.h (16)
void(94-94)void(109-109)void(113-113)void(119-119)void(152-152)void(159-159)void(165-165)void(166-166)void(167-167)void(168-168)void(169-169)void(170-170)void(171-171)void(172-172)void(189-189)void(196-196)src/net_processing.cpp (3)
RelayRecoveredSig(2470-2490)RelayRecoveredSig(2470-2470)sig(630-630)test/functional/test_framework/messages.py (1)
CRecoveredSig(1520-1544)
src/masternode/active/notificationinterface.h (6)
src/llmq/signing_shares.cpp (2)
NotifyRecoveredSig(968-971)NotifyRecoveredSig(968-968)src/masternode/active/notificationinterface.cpp (2)
NotifyRecoveredSig(32-35)NotifyRecoveredSig(32-32)src/validationinterface.cpp (2)
NotifyRecoveredSig(315-321)NotifyRecoveredSig(315-315)src/zmq/zmqabstractnotifier.cpp (2)
NotifyRecoveredSig(71-74)NotifyRecoveredSig(71-71)src/zmq/zmqnotificationinterface.cpp (2)
NotifyRecoveredSig(244-249)NotifyRecoveredSig(244-244)src/zmq/zmqpublishnotifier.cpp (4)
NotifyRecoveredSig(299-306)NotifyRecoveredSig(299-299)NotifyRecoveredSig(465-473)NotifyRecoveredSig(465-465)
src/llmq/signing.h (1)
src/llmq/signing.cpp (2)
ProcessRecoveredSig(590-639)ProcessRecoveredSig(590-590)
src/zmq/zmqpublishnotifier.h (7)
src/llmq/signing_shares.cpp (2)
NotifyRecoveredSig(968-971)NotifyRecoveredSig(968-968)src/masternode/active/notificationinterface.cpp (2)
NotifyRecoveredSig(32-35)NotifyRecoveredSig(32-32)src/validationinterface.cpp (2)
NotifyRecoveredSig(315-321)NotifyRecoveredSig(315-315)src/zmq/zmqabstractnotifier.cpp (2)
NotifyRecoveredSig(71-74)NotifyRecoveredSig(71-71)src/zmq/zmqnotificationinterface.cpp (2)
NotifyRecoveredSig(244-249)NotifyRecoveredSig(244-244)src/zmq/zmqpublishnotifier.cpp (4)
NotifyRecoveredSig(299-306)NotifyRecoveredSig(299-299)NotifyRecoveredSig(465-473)NotifyRecoveredSig(465-465)src/net_processing.cpp (1)
sig(630-630)
src/zmq/zmqabstractnotifier.cpp (5)
src/llmq/signing_shares.cpp (2)
NotifyRecoveredSig(968-971)NotifyRecoveredSig(968-968)src/masternode/active/notificationinterface.cpp (2)
NotifyRecoveredSig(32-35)NotifyRecoveredSig(32-32)src/validationinterface.cpp (2)
NotifyRecoveredSig(315-321)NotifyRecoveredSig(315-315)src/zmq/zmqnotificationinterface.cpp (2)
NotifyRecoveredSig(244-249)NotifyRecoveredSig(244-244)src/zmq/zmqpublishnotifier.cpp (4)
NotifyRecoveredSig(299-306)NotifyRecoveredSig(299-299)NotifyRecoveredSig(465-473)NotifyRecoveredSig(465-465)
src/validationinterface.h (7)
src/llmq/signing_shares.cpp (2)
NotifyRecoveredSig(968-971)NotifyRecoveredSig(968-968)src/masternode/active/notificationinterface.cpp (2)
NotifyRecoveredSig(32-35)NotifyRecoveredSig(32-32)src/validationinterface.cpp (2)
NotifyRecoveredSig(315-321)NotifyRecoveredSig(315-315)src/zmq/zmqabstractnotifier.cpp (2)
NotifyRecoveredSig(71-74)NotifyRecoveredSig(71-71)src/zmq/zmqnotificationinterface.cpp (2)
NotifyRecoveredSig(244-249)NotifyRecoveredSig(244-244)src/zmq/zmqpublishnotifier.cpp (4)
NotifyRecoveredSig(299-306)NotifyRecoveredSig(299-299)NotifyRecoveredSig(465-473)NotifyRecoveredSig(465-465)src/net_processing.cpp (3)
sig(630-630)id(679-679)id(683-683)
src/zmq/zmqnotificationinterface.cpp (6)
src/llmq/signing_shares.cpp (2)
NotifyRecoveredSig(968-971)NotifyRecoveredSig(968-968)src/masternode/active/notificationinterface.cpp (2)
NotifyRecoveredSig(32-35)NotifyRecoveredSig(32-32)src/validationinterface.cpp (2)
NotifyRecoveredSig(315-321)NotifyRecoveredSig(315-315)src/zmq/zmqabstractnotifier.cpp (2)
NotifyRecoveredSig(71-74)NotifyRecoveredSig(71-71)src/zmq/zmqpublishnotifier.cpp (4)
NotifyRecoveredSig(299-306)NotifyRecoveredSig(299-299)NotifyRecoveredSig(465-473)NotifyRecoveredSig(465-465)src/net_processing.cpp (1)
sig(630-630)
src/zmq/zmqabstractnotifier.h (7)
src/llmq/signing_shares.cpp (2)
NotifyRecoveredSig(968-971)NotifyRecoveredSig(968-968)src/masternode/active/notificationinterface.cpp (2)
NotifyRecoveredSig(32-35)NotifyRecoveredSig(32-32)src/validationinterface.cpp (2)
NotifyRecoveredSig(315-321)NotifyRecoveredSig(315-315)src/zmq/zmqabstractnotifier.cpp (2)
NotifyRecoveredSig(71-74)NotifyRecoveredSig(71-71)src/zmq/zmqnotificationinterface.cpp (2)
NotifyRecoveredSig(244-249)NotifyRecoveredSig(244-244)src/zmq/zmqpublishnotifier.cpp (4)
NotifyRecoveredSig(299-306)NotifyRecoveredSig(299-299)NotifyRecoveredSig(465-473)NotifyRecoveredSig(465-465)src/net_processing.cpp (1)
sig(630-630)
src/llmq/signing_shares.h (7)
src/llmq/signing_shares.cpp (2)
NotifyRecoveredSig(968-971)NotifyRecoveredSig(968-968)src/masternode/active/notificationinterface.cpp (2)
NotifyRecoveredSig(32-35)NotifyRecoveredSig(32-32)src/validationinterface.cpp (2)
NotifyRecoveredSig(315-321)NotifyRecoveredSig(315-315)src/zmq/zmqabstractnotifier.cpp (2)
NotifyRecoveredSig(71-74)NotifyRecoveredSig(71-71)src/zmq/zmqnotificationinterface.cpp (2)
NotifyRecoveredSig(244-249)NotifyRecoveredSig(244-244)src/zmq/zmqpublishnotifier.cpp (4)
NotifyRecoveredSig(299-306)NotifyRecoveredSig(299-299)NotifyRecoveredSig(465-473)NotifyRecoveredSig(465-465)src/net_processing.cpp (1)
sig(630-630)
src/zmq/zmqpublishnotifier.cpp (6)
src/llmq/signing_shares.cpp (2)
NotifyRecoveredSig(968-971)NotifyRecoveredSig(968-968)src/masternode/active/notificationinterface.cpp (2)
NotifyRecoveredSig(32-35)NotifyRecoveredSig(32-32)src/validationinterface.cpp (2)
NotifyRecoveredSig(315-321)NotifyRecoveredSig(315-315)src/zmq/zmqabstractnotifier.cpp (2)
NotifyRecoveredSig(71-74)NotifyRecoveredSig(71-71)src/zmq/zmqnotificationinterface.cpp (2)
NotifyRecoveredSig(244-249)NotifyRecoveredSig(244-244)src/net_processing.cpp (1)
sig(630-630)
src/zmq/zmqnotificationinterface.h (7)
src/llmq/signing_shares.cpp (2)
NotifyRecoveredSig(968-971)NotifyRecoveredSig(968-968)src/masternode/active/notificationinterface.cpp (2)
NotifyRecoveredSig(32-35)NotifyRecoveredSig(32-32)src/validationinterface.cpp (2)
NotifyRecoveredSig(315-321)NotifyRecoveredSig(315-315)src/zmq/zmqabstractnotifier.cpp (2)
NotifyRecoveredSig(71-74)NotifyRecoveredSig(71-71)src/zmq/zmqnotificationinterface.cpp (2)
NotifyRecoveredSig(244-249)NotifyRecoveredSig(244-244)src/zmq/zmqpublishnotifier.cpp (4)
NotifyRecoveredSig(299-306)NotifyRecoveredSig(299-299)NotifyRecoveredSig(465-473)NotifyRecoveredSig(465-465)src/validationinterface.h (1)
llmq(35-37)
src/masternode/active/notificationinterface.cpp (6)
src/llmq/signing_shares.cpp (2)
NotifyRecoveredSig(968-971)NotifyRecoveredSig(968-968)src/validationinterface.cpp (2)
NotifyRecoveredSig(315-321)NotifyRecoveredSig(315-315)src/zmq/zmqabstractnotifier.cpp (2)
NotifyRecoveredSig(71-74)NotifyRecoveredSig(71-71)src/zmq/zmqnotificationinterface.cpp (2)
NotifyRecoveredSig(244-249)NotifyRecoveredSig(244-244)src/zmq/zmqpublishnotifier.cpp (4)
NotifyRecoveredSig(299-306)NotifyRecoveredSig(299-299)NotifyRecoveredSig(465-473)NotifyRecoveredSig(465-465)src/net_processing.cpp (1)
sig(630-630)
src/validationinterface.cpp (6)
src/llmq/signing_shares.cpp (2)
NotifyRecoveredSig(968-971)NotifyRecoveredSig(968-968)src/masternode/active/notificationinterface.cpp (2)
NotifyRecoveredSig(32-35)NotifyRecoveredSig(32-32)src/zmq/zmqabstractnotifier.cpp (2)
NotifyRecoveredSig(71-74)NotifyRecoveredSig(71-71)src/zmq/zmqnotificationinterface.cpp (2)
NotifyRecoveredSig(244-249)NotifyRecoveredSig(244-244)src/zmq/zmqpublishnotifier.cpp (4)
NotifyRecoveredSig(299-306)NotifyRecoveredSig(299-299)NotifyRecoveredSig(465-473)NotifyRecoveredSig(465-465)src/net_processing.cpp (3)
sig(630-630)id(679-679)id(683-683)
src/llmq/signing.cpp (1)
src/validationinterface.cpp (2)
GetMainSignals(121-124)GetMainSignals(121-121)
src/llmq/signing_shares.cpp (6)
src/masternode/active/notificationinterface.cpp (2)
NotifyRecoveredSig(32-35)NotifyRecoveredSig(32-32)src/validationinterface.cpp (2)
NotifyRecoveredSig(315-321)NotifyRecoveredSig(315-315)src/zmq/zmqabstractnotifier.cpp (2)
NotifyRecoveredSig(71-74)NotifyRecoveredSig(71-71)src/zmq/zmqnotificationinterface.cpp (2)
NotifyRecoveredSig(244-249)NotifyRecoveredSig(244-244)src/zmq/zmqpublishnotifier.cpp (4)
NotifyRecoveredSig(299-306)NotifyRecoveredSig(299-299)NotifyRecoveredSig(465-473)NotifyRecoveredSig(465-465)src/net_processing.cpp (1)
sig(630-630)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/signing_shares.h
[error] 1-1: Clang format differences found in src/llmq/signing_shares.h. Run the clang-format-diff tool or clang-format to fix formatting changes.
src/masternode/active/notificationinterface.cpp
[error] 1-1: Clang format differences found in src/masternode/active/notificationinterface.cpp. Run the clang-format-diff tool or clang-format to fix formatting changes.
src/llmq/signing.cpp
[error] 1-1: Clang format differences found in src/llmq/signing.cpp. Run the clang-format-diff tool or clang-format to fix formatting changes.
🔇 Additional comments (16)
src/zmq/zmqnotificationinterface.h (1)
39-39: NotifyRecoveredSig override signature looks consistentThe new
NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& sig, bool proactive_relay)override matches the updatedCValidationInterfaceAPI and the implementation inzmqnotificationinterface.cpp. No issues from an interface or correctness perspective.src/masternode/active/notificationinterface.h (1)
29-29: ActiveNotificationInterface override correctly forwards proactive_relayThe updated
NotifyRecoveredSigoverride matches the new validation interface signature and simply forwardsproactive_relaytoshareman, which is exactly what we want at this layer.src/zmq/zmqabstractnotifier.h (1)
78-78: ZMQ notifier API extension is consistentExtending
NotifyRecoveredSigwithbool proactive_relayhere matches the implementation and all known overrides. It keeps the API coherent with the rest of the recovered‑sig pipeline; subclasses can start using the flag when needed.src/zmq/zmqabstractnotifier.cpp (1)
71-73: Base NotifyRecoveredSig implementation correctly updatedThe base
CZMQAbstractNotifier::NotifyRecoveredSignow matches the new signature and keeps the previous no‑op behavior. This is a clean, compatible update.src/zmq/zmqnotificationinterface.cpp (1)
244-248: Recovered‑sig ZMQ notifications correctly propagate proactive_relayThe updated
NotifyRecoveredSigimplementation cleanly threadsproactive_relaythrough to each ZMQ notifier usingTryForEachAndRemoveFailed. The captures and call pattern are correct and maintain previous behavior for existing notifiers.src/net_processing.h (1)
107-107: RelayRecoveredSig interface change is coherent with implementationSwitching
RelayRecoveredSigto takeconst llmq::CRecoveredSig& sig, bool proactive_relaylines up with the implementation innet_processing.cppand with how the flag is computed upstream. The forward declaration viavalidationinterface.his sufficient for this header; just ensure the cpp includes the fullllmq::CRecoveredSigdefinition (e.g., viallmq/signing.h), which appears to be the case from the provided snippet.src/masternode/active/notificationinterface.cpp (1)
32-34: LGTM: Parameter threading is correct.The
proactive_relayparameter is correctly threaded through to the share manager's notification interface. The implementation properly delegates the call with both parameters.src/zmq/zmqpublishnotifier.cpp (1)
299-306: LGTM: Unused parameter is intentional.Both ZMQ notifier implementations correctly add the
proactive_relayparameter to match the updated interface signature. The parameter is intentionally unused because ZMQ notifiers publish recovered signatures unconditionally regardless of relay type. This is consistent with the base class pattern inCZMQAbstractNotifier::NotifyRecoveredSig, which marks both parameters as unused.Also applies to: 465-473
src/net_processing.cpp (1)
630-631: Recovered-sig relay change is sound; proactive_relay correctly limited to local recoveryThe new
RelayRecoveredSig(const llmq::CRecoveredSig& sig, bool proactive_relay)API and the proactive/non-proactive split are correct:
- Proactive path directly sends
QSIGREConly to peers that have explicitly setm_wants_recsigs, avoiding theinv→getdataround-trip.- Non-proactive path preserves the existing behavior by constructing
MSG_QUORUM_RECOVERED_SIGfromsig.GetHash()and usingPushInvguarded bym_wants_recsigs.Locking and lifetimes are sound (no
m_peer_mutexheld at call-sites,GetPeerRefguarded internally,sigcaptured by reference only within the synchronousForEachNodecall).Verification confirms that
proactive_relayis only settruewhenfrom == -1(local recovery), not on reprocessing of already-known signatures, so the deduplication concern does not materialize in practice.Two optional consistency improvements remain valid if desired:
Consistent peer iteration — Using
ForEachNodefor proactive butm_peer_mapfor non-proactive is not incorrect but creates conceptual asymmetry. Aligning both to a single iteration pattern would improve maintainability.Deduplication semantics — Proactive path bypasses
m_tx_inventory_known_filter, so direct sends may repeat if the samesigis ever encountered twice. Given thefrom == -1guard limits proactive sends to first-time local recovery, this is low-risk and acceptable as-is.src/validationinterface.cpp (1)
315-321: NotifyRecoveredSig correctly threadsproactive_relaythrough validation callbacksThe lambda capture and call into
CValidationInterface::NotifyRecoveredSig(sig, proactive_relay)look correct and maintain existing enqueue/log semantics while adding the new flag. No further changes needed here.src/zmq/zmqpublishnotifier.h (2)
81-85: Recovered-sig ZMQ hash notifier signature matches updated base interfaceThe added
bool proactive_relayparameter aligns this override withCZMQAbstractNotifier::NotifyRecoveredSig. It’s fine that the flag is unused in the implementation for now.
150-154: Recovered-sig ZMQ raw notifier signature updated consistentlyThe raw recovered-sig notifier now correctly overrides the
(sig, bool proactive_relay)variant, matching the base class and other call sites.src/llmq/signing_shares.cpp (2)
836-841: Passingfrom = -1cleanly distinguishes locally recovered signaturesUpdating both single-member and regular recovery paths to call
ProcessRecoveredSig(..., -1)is consistent with the existing use ofNodeId == -1as a “no peer / local” sentinel and allowsCSigningManagerto enable proactive relay only for locally reconstructed sigs. This looks correct.Also applies to: 870-871
968-971: NotifyRecoveredSig now correctly forwards proactive relay intent to PeerManagerThe new
CSigSharesManager::NotifyRecoveredSig(sig, proactive_relay)simply asserts the pointer and callsm_peerman.RelayRecoveredSig(*sig, proactive_relay), which is exactly what we want for propagating the flag through to networking. No issues seen.src/validationinterface.h (1)
171-172: AllNotifyRecoveredSigoverrides have been properly updated to the new signatureVerification confirms all derived classes that override
NotifyRecoveredSighave been updated to match the new 2-parameter signature(const std::shared_ptr<const llmq::CRecoveredSig>&, bool proactive_relay).Found and verified:
CZMQNotificationInterface::NotifyRecoveredSig✓ActiveNotificationInterface::NotifyRecoveredSig✓CZMQAbstractNotifier::NotifyRecoveredSig✓CZMQPublishHashRecoveredSigNotifier::NotifyRecoveredSig✓CZMQPublishRawRecoveredSigNotifier::NotifyRecoveredSig✓CSigSharesManager::NotifyRecoveredSig✓Classes that don't override this method (e.g.,
CDSNotificationInterface,PeerManager, test utilities) correctly use the base class no-op default. No old 1-parameter signatures found anywhere in the codebase.src/llmq/signing.cpp (1)
11-11: Verify clang-format CI failure insrc/llmq/signing.cppThe review comment references CI formatting differences, but visual inspection of the code at line 11 (the new include) and lines 633-638 (the multi-line
proactive_relayexpression) shows standard, properly aligned formatting. No clang-format directives are present that would exempt these sections.Since the sandbox environment cannot execute clang-format or access CI logs, please verify:
- Does the CI pipeline actually report formatting violations for this file?
- If yes, run
clang-format -i src/llmq/signing.cpplocally and commit the changes.- If no, this review comment may be outdated and can be dismissed.
| bool allowDiffMsgHashSigning = false) EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingSigns, !cs); | ||
|
|
||
| void NotifyRecoveredSig(const std::shared_ptr<const CRecoveredSig>& sig) const EXCLUSIVE_LOCKS_REQUIRED(!cs); | ||
| void NotifyRecoveredSig(const std::shared_ptr<const CRecoveredSig>& sig, bool proactive_relay) const EXCLUSIVE_LOCKS_REQUIRED(!cs); |
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.
Recovered‑sig notification wiring is correct; fix clang‑format to unbreak CI
The new NotifyRecoveredSig(const std::shared_ptr<const CRecoveredSig>& sig, bool proactive_relay) const EXCLUSIVE_LOCKS_REQUIRED(!cs) overload is consistent with its implementation in signing_shares.cpp and with the locking strategy (no cs held while calling PeerManager::RelayRecoveredSig). Semantics look good.
CI is currently failing on src/llmq/signing_shares.h due to a clang‑format diff, though. Please run the project’s clang‑format tooling on this file (or at least around this declaration) and commit the resulting formatting changes so the pipeline passes.
🤖 Prompt for AI Agents
In src/llmq/signing_shares.h around line 452, the new overload declaration is
correctly implemented but its formatting doesn't match the project's
clang-format rules and is causing CI failures; run the project's clang-format
tool (or apply the repo's clang-format style) to reformat this file or at least
the vicinity of this declaration, update the header with the formatted result,
and commit that change so the CI clang-format check passes.
| void ActiveNotificationInterface::NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& sig, bool proactive_relay) | ||
| { | ||
| m_active_ctx.shareman->NotifyRecoveredSig(sig); | ||
| m_active_ctx.shareman->NotifyRecoveredSig(sig, proactive_relay); |
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.
Fix clang-format issues.
The CI pipeline reports formatting differences in this file. Please run clang-format to resolve the formatting issues before merging.
Run the following command to fix formatting:
#!/bin/bash
# Format the file according to project style
clang-format -i src/masternode/active/notificationinterface.cpp🤖 Prompt for AI Agents
In src/masternode/active/notificationinterface.cpp around lines 32 to 34, the
file fails CI due to clang-format style differences; run clang-format -i on that
file to reformat it (or apply your project's clang-format settings) so the
NotifyRecoveredSig function and surrounding whitespace/indentation match project
style, then re-run clang-format or the CI linter and commit the updated file.
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6967.e404e679. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6967.e404e679. The image should be on dockerhub soon. |
e404e67 to
7e782ff
Compare
…ho performed recovery
… connections for all members
7e782ff to
a57a811
Compare
|
Resolved! Forgot to push that final change |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6967.def66c9c. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6967.def66c9c. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6967.ca6369bd. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6967.ca6369bd. The image should be on dockerhub soon. |
UdjinM6
left a comment
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.
what are all these "chore: empty" commits about?
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6967.8747a552. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6967.8747a552. The image should be on dockerhub soon. |
8747a55 to
f18a12e
Compare
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6967.f18a12e8. A new comment will be made when the image is pushed. |
UdjinM6
left a comment
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.
utACK f18a12e
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6967.f18a12e8. The image should be on dockerhub soon. |
kwvg
left a comment
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.
Conceptual ACK, see suggestions
| } | ||
|
|
||
| void CZMQNotificationInterface::NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& sig) | ||
| void CZMQNotificationInterface::NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& sig, bool /*proactive_relay*/) |
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.
| void CZMQNotificationInterface::NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& sig, bool /*proactive_relay*/) | |
| void CZMQNotificationInterface::NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& sig, [[maybe_unused]] bool proactive_relay) |
| GetMainSignals().NotifyRecoveredSig(recoveredSig, recoveredSig->GetHash().ToString()); | ||
| // TODO refactor to use a better abstraction analogous to IsAllMembersConnectedEnabled | ||
| auto proactive_relay = from == -1 && | ||
| llmqType != Consensus::LLMQType::LLMQ_100_67 && |
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.
Can we document why these LLMQs are excluded from proactive relay? Either by placing them in a dedicated function or by comment? Similar exclusions are present in llmq/options.cpp (see below) but aren't immediately clear why.
Lines 28 to 30 in 199ac79
| if (spork_value == 1 && llmqType != Consensus::LLMQType::LLMQ_100_67 && llmqType != Consensus::LLMQType::LLMQ_400_60 && llmqType != Consensus::LLMQType::LLMQ_400_85) { | |
| return true; | |
| } |
Co-authored-by: Kittywhiskers Van Gogh <[email protected]>
|
Guix Automation has failed due to the HEAD commit not being signed by an authorized core-team member. Please rebase and sign or push a new empty signed commit to allow Guix build to happen. |
UdjinM6
left a comment
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.
utACK a58f1bb
kwvg
left a comment
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.
utACK a58f1bb
Issue being fixed or feature implemented
Currently; recovered sigs are send to other peers in a manner as described in https://github.com/dashpay/dips/blob/master/dip-0006.md#intra-quorum-communication
After the implementation of Concentrated Recovery for small quorums, this is no longer ideal. for cases where latency is of paramount importance, we can reuse the connections that are maintained to perform concentrated recovery for recovered signature propagation.
Additionally; we can proactively send recovered signatures to interested peers we are connected to when we are the member who performed recovery. For LLMQs that use concentrated recovery, typically only one peer will perform initial recovery, so sending out proactively makes sense.
What was done?
How Has This Been Tested?
Has been running on testnet for a while.
Breaking Changes
None
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.