Net: Fixed a race condition when disabling the network. #13212

1. 
   lmanners commented at 5:50 pm on May 10, 2018: contributor

   This change addresses a race condition where setnetworkactive=false wouldn’t always disconnect all peers.

   Before this change, the following could happen:

   1. Thread A – Begins connecting to a node.
   2. Thread B – Sets kNetworkActive=false and disconnects connected nodes.
   3. Thread A – Finishes connecting and adds node to list of connected nodes.

   The node that was connected from Thread A remains connected and active, even though kNetworkActive=false.

   To fix the race, disconnections when kNetworkActive=false are now handled in the main network loop.

   fixes #13038

2. laanwj added the label P2P on May 10, 2018
3. in src/net.cpp:1167 in 8e2e21bf05 outdated

   1162@@ -1163,6 +1163,18 @@ void CConnman::ThreadSocketHandler()
   1163         //
   1164         {
   1165             LOCK(cs_vNodes);
   1166+
   1167+            if (!fNetworkActive)
   

   ---

   ---

   

   promag commented at 4:16 pm on May 12, 2018:

   nit, put { here.
4. in src/net.cpp:1171 in 8e2e21bf05 outdated

   1162@@ -1163,6 +1163,18 @@ void CConnman::ThreadSocketHandler()
   1163         //
   1164         {
   1165             LOCK(cs_vNodes);
   1166+
   1167+            if (!fNetworkActive)
   1168+            {
   1169+                // Disconnect any connected nodes
   1170+                for (CNode* pnode : vNodes) {
   1171+                    if(!pnode->fDisconnect) {
   

   ---

   ---

   

   promag commented at 4:16 pm on May 12, 2018:

   nit, space after if.

   ---

   

   MarcoFalke commented at 4:29 pm on May 12, 2018:

   You can install clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script to preempt whitespace nitpicking.

   ---

   

   lmanners commented at 12:08 pm on May 13, 2018:

   Fixed the formatting using clang-format-diff.py. Thanks!
5. 
   promag commented at 4:19 pm on May 12, 2018: member

   utACK 8e2e21b.

   Now sockets are closed after NotifyNetworkActiveChanged signal is emitted but from what I’ve seen it doesn’t matter.

6. 
   mruddy commented at 12:31 pm on May 21, 2018: contributor

   @lmanners Thanks, this appears to fix the #13038 bug that I reported. Tested ACK

   • I verified that the bug is still easily reproducible with current master branch commit 6738813bcbb730b5a19e67482b6612d568c22699.
   • I applied this patch.
   • I verified that I was not able to reproduce the bug with multiple attempts.

   Tested with:

   /bitcoin/src/qt/bitcoin-qt -datadir=/data/bitcoin-unpruned-txindex -txindex -disablewallet -persistmempool=0 -printtoconsole -debug=net

   I believe this is an example of the race that I was able to capture and see being handled cleanly by this patch:

    02018-05-21T12:20:30Z SetNetworkActive: false
    12018-05-21T12:20:31Z Added connection peer=13
    22018-05-21T12:20:31Z sending version (103 bytes) peer=13
    32018-05-21T12:20:31Z send version message: version 70015, blocks=523490, us=[::]:0, peer=13
    42018-05-21T12:20:31Z Network not active, dropping peer=13
    52018-05-21T12:20:31Z disconnecting peer=13
    62018-05-21T12:20:31Z Cleared nodestate for peer=13
    7
    82018-05-21T12:21:32Z SetNetworkActive: true
    92018-05-21T12:21:33Z trying connection 180.107.22.115:8333 lastseen=36.9hrs
   102018-05-21T12:21:33Z Added connection peer=14
   112018-05-21T12:21:33Z sending version (103 bytes) peer=14
   122018-05-21T12:21:33Z send version message: version 70015, blocks=523490, us=[::]:0, peer=14
   132018-05-21T12:21:34Z trying connection 176.223.201.198:8333 lastseen=0.2hrs
   14
   152018-05-21T12:21:34Z SetNetworkActive: false
   162018-05-21T12:21:34Z Network not active, dropping peer=14
   172018-05-21T12:21:34Z disconnecting peer=14
   182018-05-21T12:21:34Z Cleared nodestate for peer=14
   192018-05-21T12:21:34Z Added connection peer=15
   202018-05-21T12:21:34Z sending version (103 bytes) peer=15
   212018-05-21T12:21:34Z send version message: version 70015, blocks=523490, us=[::]:0, peer=15
   222018-05-21T12:21:34Z Network not active, dropping peer=15
   232018-05-21T12:21:34Z disconnecting peer=15
   242018-05-21T12:21:34Z Cleared nodestate for peer=15
   

7. MarcoFalke added this to the milestone 0.17.0 on May 21, 2018
8. 
   lmanners commented at 4:55 pm on May 21, 2018: contributor

   Thanks for testing this @mruddy!

   Note: when I did my own testing, I added a sleep statement in CConnman::OpenNetworkConnection just after fNetworkActive is checked, to make the race condition reproduce consistently.

9. 
   MarcoFalke commented at 1:54 pm on June 21, 2018: member

   Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
10. Net: Fixed a race condition when disabling the network.

    This change addresses a race condition where setnetworkactive=false wouldn't always disconnect all peers.
    
    Before this change, the following could happen:
    1. Thread A -- Begins connecting to a node.
    2. Thread B -- Sets kNetworkActive=false and disconnects connected nodes.
    3. Thread A -- Finishes connecting and adds node to list of connected nodes.
    
    The node that was connected from Thread A remains connected and active,
    even though kNetworkActive=false.
    
    To fix the race, disconnections when kNetworkActive=false are now handled in the main network loop.
    
    fixes #13038

    793290f940
11. lmanners force-pushed on Jun 21, 2018
12. 
    lmanners commented at 5:28 pm on June 21, 2018: contributor

    Squashed.
13. 
    sipa commented at 2:38 am on June 22, 2018: member

    Concept ACK
14. 
    mruddy commented at 5:08 pm on July 3, 2018: contributor

    Tested ACK. I just retested the squashed commit 793290f940a9af18d4f0292a263d976a066dff65 on top of current master 686e97a0c7358291d628213447cf33e99cde7ce8. LGTM
15. 
    MarcoFalke commented at 11:37 am on July 10, 2018: member

    utACK 793290f940a9af18d4f0292a263d976a066dff65
16. 
    sipa commented at 9:45 pm on July 19, 2018: member

    utACK 793290f940a9af18d4f0292a263d976a066dff65
17. 
    achow101 commented at 9:48 pm on July 19, 2018: member

    utACK 793290f940a9af18d4f0292a263d976a066dff65
18. fanquake added this to the "Mergeable" column in a project

    ---

19. laanwj merged this on Jul 22, 2018
20. laanwj closed this on Jul 22, 2018

    ---

21. laanwj referenced this in commit a6f00ce66f on Jul 22, 2018
22. fanquake removed this from the "Mergeable" column in a project

    ---

23. thohemp referenced this in commit 02d0d0ef34 on Dec 22, 2019
24. thohemp referenced this in commit 34790b669e on Dec 22, 2019
25. nezero referenced this in commit 824407d367 on Feb 18, 2020
26. nezero referenced this in commit 40ad62a0cb on Feb 18, 2020
27. codablock referenced this in commit 1524185462 on Apr 8, 2020
28. codablock referenced this in commit c7fe877924 on Apr 17, 2020
29. codablock referenced this in commit 9a8caf0986 on Apr 17, 2020
30. ckti referenced this in commit f814dc0e7c on Mar 28, 2021
31. ckti referenced this in commit 233d8ab119 on Mar 28, 2021
32. MarcoFalke locked this on Sep 8, 2021

Contributors
lmanners promag MarcoFalke mruddy sipa achow101

Labels
P2P

Milestone
0.17.0