btcsignals: delete broken scoped_connection move assignment #35120

1. 
   thomasbuilds commented at 12:07 PM on April 20, 2026: contributor

   The defaulted move-assignment operator for scoped_connection overwrites m_conn without first calling disconnect(). Since disconnection is signaled via the liveness flag (which is never cleared) the old callback remains registered in the signal and keeps firing, violating the RAII contract:

   int val{0};
   btcsignals::scoped_connection sc0 = sig.connect(IncrementCallback);
   btcsignals::scoped_connection sc1 = sig.connect(SquareCallback);
   sc0 = std::move(sc1);
   val = 3; sig(val);
   // Expected: 9 (only SquareCallback)
   // Actual:  16 (both callbacks fire, old connection leaked)
   

   Move assignment is unused in the codebase, so following the review discussion this deletes the broken operator instead of fixing it. A correct implementation can be added if a use case arises.

   Earlier versions of this PR fixed the operator and added a default constructor to enable the member-variable assignment pattern; that was dropped in favor of removing the unused operation.

2. 
   DrahtBot commented at 12:07 PM on April 20, 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/35120.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	sedited, maflcko

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   No conflicts as of last run.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

3. 
   sedited commented at 12:14 PM on April 20, 2026: contributor

   cc @theuni

4. 
   maflcko commented at 12:58 PM on April 20, 2026: member

   heh, I guess at least this on in particular is my fault due to suggesting it in #34495 (review).

   It is unused in the codebase right now, so there is no observable bug.

   I wonder what the use-case for move assignment is. I presume, back when I suggested to default this, I assumed that scoped_connection() exists to create an empty connection. However, it does not exist.

   I guess the use-case would be something like:

   struct Wallet{
     scoped_connection a{};
     scoped_connection b{};
   }
   

   Where those are later move-assigned? However, the constructor does not exist to create those, so I think it would make sense to either enable both or none.

   Moreover, if going strictly after https://www.boost.org/doc/libs/latest/doc/html/boost/signals2/scoped_connection.html#id-1_3_34_6_2_1_1_2_3_6-bb and https://www.boost.org/doc/libs/latest/doc/html/boost/signals2/connection.html#id-1_3_34_6_2_1_1_1_5-bb, it is also possible to re-seat a non-scoped connection, which is not fixed by this pull request either, so it seems incomplete.

   I don't know what to do here, I just wanted to mention that there seems to be context on the larger picture missing from the pull description.

5. thomasbuilds force-pushed on Apr 20, 2026
6. 
   thomasbuilds commented at 1:11 PM on April 20, 2026: contributor

   Thanks for the context! Updated the PR to address your points

7. 
   maflcko commented at 1:15 PM on April 20, 2026: member

   I don't think they are addressed. It is still unclear to me what we want to achieve here. Do we want fully boost-compatibility, in which case this pull is still incomplete, as per my last comment. Do we want a subset? If yes, why that subset?

8. thomasbuilds renamed this:
   btcsignals: disconnect old connection on scoped_connection move-assignment
   btcsignals: fix scoped_connection move-assignment and add default constructor
   on Apr 20, 2026
9. 
   thomasbuilds commented at 1:24 PM on April 20, 2026: contributor

   Ok my reasoning was the move-assignment is broken, and fixing it without a default constructor makes it useless (no way to create an empty scoped_connection to assign into), so I added both together as the minimal set to make it work.

   But you're right that this creates an arbitrary subset, it fixes one thing while leaving out operator=(connection&&) without a clear rationale for where to draw the line.

   So I see two paths

   1. Delete the broken move-assignment. It's unused, so removing it is safe. If a use case comes up later, the full set of operations can be added correctly at that point 2. Go for full boost-compatibility: fix move-assignment, add default constructor, add operator=(connection&&), etc

   What do you think?

10. DrahtBot added the label CI failed on Apr 20, 2026
11. 
    maflcko commented at 1:57 PM on April 20, 2026: member

    operator=(connection&&) for re-seating from a raw connection (as in boost) is left out of scope — can be added separately if needed.

    Ah sorry, I was wrong to indicate this was missing. Whatever the underlying shared_ptr is doing is already correct here, IIUC. It is fine for you to use an LLM, but please still try to think critically. Otherwise, this may lead to more issues, where a misunderstanding is amplified/repeated instead of corrected.

12. 
    thomasbuilds commented at 2:08 PM on April 20, 2026: contributor

    yes you're right went too fast there

13. DrahtBot removed the label CI failed on Apr 20, 2026
14. maflcko approved
15. 
    maflcko commented at 3:17 PM on April 20, 2026: member

    seems fine. I guess the alternative would be to go back to =delete.

16. maflcko added this to the milestone 32.0 on Apr 20, 2026
17. 
    sedited commented at 2:07 PM on June 7, 2026: contributor

    seems fine. I guess the alternative would be to go back to =delete.

    My feeling is now that we have landed our own implementation we shouldn't be beholden to conventions outside of it, so deleting dead code seems like the better alternative?

18. btcsignals: delete broken scoped_connection move assignment

    The defaulted move assignment overwrites m_conn without disconnecting
    it first, so the previous callback stays registered with the signal and
    keeps firing, violating the RAII contract:
    
        btcsignals::scoped_connection sc0 = sig.connect(IncrementCallback);
        btcsignals::scoped_connection sc1 = sig.connect(SquareCallback);
        sc0 = std::move(sc1);
        val = 3; sig(val); // both callbacks fire: 16 instead of 9
    
    Move assignment is unused in the codebase, so delete it rather than
    fixing it. It can be implemented properly if a use case arises.

    b83a999b14
19. thomasbuilds force-pushed on Jun 11, 2026
20. thomasbuilds renamed this:
    btcsignals: fix scoped_connection move-assignment and add default constructor
    btcsignals: delete broken scoped_connection move assignment
    on Jun 11, 2026
21. thomasbuilds requested review from maflcko on Jun 11, 2026
22. sedited approved
23. 
    sedited commented at 10:29 AM on June 11, 2026: contributor

    ACK b83a999b1449c0b0b2eac3173359539323438fde

24. 
    maflcko commented at 10:32 AM on June 11, 2026: member

    lgtm ACK b83a999b1449c0b0b2eac3173359539323438fde

25. fanquake merged this on Jun 11, 2026
26. fanquake closed this on Jun 11, 2026

    ---

27. 
    theuni commented at 2:21 PM on June 11, 2026: member

    Thanks for addressing, post-merge ACK.

Contributors

thomasbuildsDrahtBotseditedmaflckotheuni

Review Requested
maflcko

Milestone
32.0