#36011 btcsignals: destroy callbacks on disconnect

full analysis

https://github.com/bitcoin/bitcoin/pull/36011 · ryanofsky · +115/-9 in 2 files, 3 commits · labels: none

Goal

  • Destroy disconnected signal callbacks immediately instead of deferring cleanup
  • Prevent shutdown hangs and deadlocks when callbacks hold resources with release side effects

Updates `btcsignals` so that calling `disconnect()` or destroying a `scoped_connection` promptly destroys the registered callback function object, rather than deferring destruction until the entire signal is destroyed or a future `connect()` call runs garbage collection. It falls back to C++14 atomic shared pointer operations on platforms lacking C++20 `std::atomic<std::shared_ptr<T>>`.

Problem: Disconnected callbacks in `btcsignals` remain alive indefinitely if no subsequent `connect()` calls are made to trigger garbage collection. When callbacks own resources with side effects upon destruction (such as `mp::EventLoopRef` in multiprocess Bitcoin), this deferred destruction causes shutdown deadlocks.

Category: Utilities (logging, arguments, libraries) (#22 of 66)

P3 · bug fix

  • P3 because it fixes shutdown deadlocks and unpredictable resource retention in utility signals
  • Essential for multiprocess GUI support, though deferrable for the current release

Fixes unpredictable callback lifetime semantics in btcsignals, which currently fails to free disconnected callbacks until subsequent connections occur. While this unblocks a clean shutdown in the multiprocess bitcoin-gui PR (#10102), the author and maintainers agree there is no immediate release urgency ('Yeah no rush').

Membership: Modifies btcsignals under src/util/btcsignals.h, a shared utility primitive.

Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 1

Reviewability: Ready

  • Ready to review now with passing CI and new characterization tests

The code and unit tests are complete and passing CI. Although the author noted possible internal simplifications, none invalidate a review of this patch.

Author status: active; noted potential simplifications that could follow this change

Agreement: Crickets

  • General agreement on the need for the change without objections
  • Acknowledged as necessary to fix multiprocess GUI shutdown issues (maflcko)
  • Agreed that merging is not urgent for the upcoming release (maflcko, ryanofsky)

Crickets: no formal reviews yet; maflcko noted there is no rush for the next major release

Only one comment has been posted, asking if the change can slip the next release, which the author confirmed. No reviewers have reviewed the code or formally evaluated the concept.

  • 2026-08-19 maflcko: 'Too bad it doesn't work for the mp-gui. I guess this can slip the next major release, as there is no rush, right?'
  • 2026-08-19 ryanofsky: 'Yeah no rush, and there are also some potential simplifications this change could allow.'

Objections: none enumerated.

Participants: maflcko (neutral)

State derived from the lists: no substantive comment either way

Review verdicts (DrahtBot): 0

Dependencies

Enables:

Files

46 lines under test/bench/ci.

  • src/util/btcsignals.h +69/-9
  • src/test/btcsignals_tests.cpp +46/-0

Card

This PR modifies btcsignals in src/util/btcsignals.h to eagerly destroy callback objects when disconnected, matching boost::signals2 semantics. Under the current implementation, callbacks persist until future connect() calls garbage-collect them, which causes a shutdown hang in multiprocess bitcoin (#10102) where callbacks hold IPC event loop references. The PR has had no formal reviews yet and the author agreed with maflcko that there is no rush to merge it for the next release.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T21:46, confidence high, input hash fcfe0bc21b52b506