Skip to content

fix: missing shareman->RegisterRecoveryInterface() in ActiveContext::Start #7243

Closed
#7244
Closed
fix: missing shareman->RegisterRecoveryInterface() in ActiveContext::Start#7243
#7244

Description

@thepastaclaw
thepastaclaw
opened on Mar 22, 2026

Bug

ActiveContext::Start() registers recovery interfaces for cl_signer and is_signer but not shareman:

// src/active/context.cpp — current code
cl_signer->RegisterRecoveryInterface();
is_signer->RegisterRecoveryInterface();
// shareman->RegisterRecoveryInterface(); ← MISSING

Without this, completed sig share sessions are only cleaned up via the 5-second Cleanup() interval instead of promptly via HandleNewRecoveredSig. The corresponding UnregisterRecoveryInterface() call in Stop() is also missing.

Impact

  • Delayed cleanup of completed sig share sessions (up to 5s instead of immediate)
  • Under CI load with frozen mocktime, this manifests as flaky test failures in feature_llmq_signing.py and interface_zmq_dash.py where InstantSend locks don't arrive within expected timeouts

Root Cause

The registration was likely dropped during the sig share refactoring that split share management into a separate shareman object. The cl_signer and is_signer registrations survived but shareman was missed.

Fix

// In ActiveContext::Start():
cl_signer->RegisterRecoveryInterface();
is_signer->RegisterRecoveryInterface();
+shareman->RegisterRecoveryInterface();

// In ActiveContext::Stop():
+shareman->UnregisterRecoveryInterface();
is_signer->UnregisterRecoveryInterface();
cl_signer->UnregisterRecoveryInterface();

Discovery

Found during CI triage of dash#7242 (wallet deprecation backport). The fix was originally committed as a80c46c438 on the backport branch but has been stripped out since it's unrelated to that PR's scope.

See also: dash#7233 (flaky LLMQ signing test fixes — related symptoms, different root cause).

Activity

  1. thepastaclaw commented on Mar 22, 2026

    @thepastaclaw
    thepastaclaw
    on Mar 22, 2026
    CollaboratorAuthor

    Additional CI failure from this root cause:

    The PR only touches RPC/wallet code (src/rpc/*.cpp, src/wallet/rpc/*.cpp) and adds deprecatedrpc=create_bdb to test config — zero relation to LLMQ signing. Confirms this is the pre-existing flakiness described in this issue.

  2. knst commented on Mar 23, 2026

    @knst
    knst
    on Mar 23, 2026
    Collaborator

    @thepastaclaw check this function:

    void PeerManagerImpl::AddExtraHandler(std::unique_ptr<NetHandler>&& handler)
{
    assert(handler != nullptr);
    if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) {
        RegisterValidationInterface(i);
    } 
    m_handlers.emplace_back(std::move(handler));
}

    is it sufficient?

  3. thepastaclaw commented on Mar 23, 2026

    @thepastaclaw
    thepastaclaw
    on Mar 23, 2026
    CollaboratorAuthor

    Good question. No, AddExtraHandler is not sufficient for this case — it only auto-registers CValidationInterface:

    void PeerManagerImpl::AddExtraHandler(std::unique_ptr<NetHandler>&& handler)
{
    if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) {
        RegisterValidationInterface(i);
    }
    m_handlers.emplace_back(std::move(handler));
}

    RegisterRecoveryInterface() is a different registration path — it calls sigman.RegisterRecoveredSigsListener(this) to register as a CRecoveredSigsListener, not a CValidationInterface. AddExtraHandler has no knowledge of CRecoveredSigsListener or CSigningManager.

    Additionally, shareman is owned by ActiveContext, not by PeerManager — it is not added via AddExtraHandler at all. (The NetSigning handler that is added via AddExtraHandler holds a raw pointer to shareman but only implements CValidationInterface itself.)

    So the explicit RegisterRecoveryInterface() / UnregisterRecoveryInterface() calls in ActiveContext::Start() / Stop() are the right place for this — matching how cl_signer and is_signer are already registered there.

    That said, extending AddExtraHandler with a similar dynamic_cast<CRecoveredSigsListener*> pattern could be a nice follow-up to make the registration more systematic, but it would need a reference to CSigningManager which PeerManager does not currently hold, and it would not cover ActiveContext-owned objects like shareman anyway.

  4. PastaPastaPasta
    closed this as completedin 5506d0aon Mar 25, 2026
  5. PastaPastaPasta
    added a commit that references this issue on Mar 25, 2026

    Merge #7244: fix: register shareman recovery interface in ActiveContext

    3863c88
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions