btcsignals: destroy callbacks on disconnect #36011

1. 
   ryanofsky commented at 4:28 PM on August 18, 2026: contributor

   Currently callback functions connected to signals do not get freed when their connection::disconnect method is called or when their associated scoped_connection is destroyed. They live as long as the signal does, even if they will never be called, and can only get garbage collected if new connections are added.

   This behavior is different that previous boost signals behavior and causes a deadlock on GUI shutdown in #10102 because when the node registers signal callbacks (handleInitMessage, etc) that get forwarded to the GUI, the callbacks own mp::EventLoopRef references, and if the callbacks aren't freed, the node process can't shut down.

   This change restores boost signals behavior freeing callbacks when they are disconnected or when scoped_connection objects are destroyed to avoid this problem and make callback lifetimes more predictable.

2. test: characterize btcsignals callback destruction timing

   Add tests pinning down when a callback object (and any state it owns)
   is destroyed. Currently disconnect() only disables the callback: the
   object stays alive until a later connect() call on the same signal
   garbage collects it and every connection handle referencing it has been
   released; with no connection handle held it lives until the signal
   itself is destroyed.
   
   This differs from boost::signals2, which btcsignals aims to be
   api-compatible with: there, disconnecting destroys the slot's function
   object as soon as no emission is using it.
   
   No behavior change; these tests document the status quo so the next
   commit changing the destruction timing has a clear before/after diff.
   
   Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

   a16a941b42
3. btcsignals: destroy callbacks eagerly on disconnect

   Previously a disconnected callback and any state it owned could stay
   alive indefinitely: destruction required both a later connect() call on
   the same signal to garbage collect it and release of every connection
   handle referencing it. That made destruction timing unpredictable for
   callbacks that own resources whose release has side effects, and it
   diverges from boost::signals2, which destroys a disconnected slot's
   function object as soon as no emission is running it. (The concrete
   fallout was in multiprocess bitcoin-gui, where node notification
   callbacks own IPC proxy objects: their deferred destruction kept
   bitcoin-node from exiting and hung interface_gui.py until timeout.)
   
   Make disconnect() destroy the callback, deferring only while a
   concurrent emission is mid-call into it, matching boost::signals2
   semantics. The previous commit's characterization tests are updated;
   their diff shows the ownership change.
   
   Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

   c735a8ec4a
4. btcsignals: fall back to atomic_load/store without __cpp_lib_atomic_shared_ptr

   std::atomic<shared_ptr<T>> is a C++20 library feature, but library
   implementations are not required to ship all C++20 features at once;
   __cpp_lib_atomic_shared_ptr is the feature-test macro that indicates its
   presence. Without it, the generic std::atomic<T> requires trivially copyable
   T — shared_ptr is not, causing build failures on macOS (Xcode 16.2),
   FreeBSD 15.1, and the MSan/TSan custom libc++ builds in CI.
   
   Fall back to the C++14 atomic_load/atomic_store free functions, which are
   specifically overloaded for shared_ptr. The fallback is intended to be
   temporary: those functions are deprecated in C++20 and will eventually be
   removed, so the #else branch can be dropped once all CI platforms carry the
   C++20 specialization.
   
   Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

   6a3d490d82
5. 
   DrahtBot commented at 4:28 PM on August 18, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36011.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

6. fanquake requested review from theuni on Aug 18, 2026
7. DrahtBot added the label CI failed on Aug 18, 2026
8. 
   maflcko commented at 10:26 AM on August 19, 2026: member

   Heh, I really loved the simple garbage collect logic. 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?

9. DrahtBot removed the label CI failed on Aug 19, 2026
10. 
    ryanofsky commented at 12:24 PM on August 19, 2026: contributor

    Yeah no rush, and there are also some potential simplifications this change could allow. This PR is trying to make behavior of btcsignals more predictable and free the external callbacks after they can no longer be called. But internal improvements might be possible too:

    • connection::liveness::m_connected variable could maybe be dropped. It now seems redundant with the m_owner variable.
    • Maybe the garbage collection loop can be dropped and replaced with an m_owner custom deleter that deletes entries from m_connections
    • I'm not sure if m_callback is actually necessary. Maybe the callback pointer could be derived from m_owner with a cast.

Review Requested
theuni