Syncing of chains with different height depends on “connection direction” #5113

1. 
   domob1812 commented at 7:09 am on October 21, 2014: contributor

   Experimenting with a private -regtest network, I found the following behaviour that seems wrong: I’ve got two nodes, A and B. They are initially disconnected and A has a chain of height 210 while B has one of height 220 - they are forked at block 200. Now I want to connect the nodes and expect that A switches to B’s longer chain.

   When I connect A to B (addnode call on A), this is precisely what happens. However, when I connect B to A (addnode called on B), both keep their chains. This seems very odd to me. I seem to recall that “a few days ago” both alternatives resulted in the nodes syncing to the same chain. (And can try to find out more about this / give more details if necessary.)

2. 
   sipa commented at 7:17 am on October 21, 2014: member

   In which code version do you observe this?
3. 
   domob1812 commented at 7:22 am on October 21, 2014: contributor

   Current Git header: 64ffc995d685cf8a53ef868572e835ce42269ec6

   My code contains also private commits, but they only touch the qa/rpc-tests folder.

4. domob1812 referenced this in commit 2290ed01bc on Oct 24, 2014
5. gavinandresen referenced this in commit e401a2c557 on Oct 24, 2014
6. 
   morcos commented at 1:43 am on October 27, 2014: member

   I ran into this problem as well. The below code demonstrates it by hanging at sync_blocks. If you switch which node connects to which it tends to change whether it works or not. I also noticed that if you use sync_all it sometimes fails syncing the mempools after syncing the blocks. I’m not familar enough with the headers first code, but I think the issue is caused by which peer does a getheaders from the other. This is run from the current master.

    0#!/usr/bin/env python
    1
    2from test_framework import BitcoinTestFramework
    3from bitcoinrpc.authproxy import AuthServiceProxy, JSONRPCException
    4from util import *
    5import time
    6
    7class SyncDir(BitcoinTestFramework):
    8    def setup_network(self):
    9        self.nodes = []
   10        self.nodes.append(start_node(0, self.options.tmpdir, ["-debug"]))
   11        self.nodes.append(start_node(1, self.options.tmpdir, ["-debug"]))
   12        connect_nodes(self.nodes[1], 0)
   13        print("Nodes connected")
   14        self.is_network_split = False
   15        self.sync_all()
   16        random_transaction(self.nodes, Decimal("0.001"), Decimal("0.0001"), 0, 0)
   17        print ("One tx sent")
   18        self.nodes[0].setgenerate(True,1)
   19        print ("Block mined")
   20        self.sync_all()
   21        print ("Chains synced")
   22        stop_node(self.nodes[0],0)
   23        print ("Node stopped")
   24        self.nodes[0] = start_node(3, self.options.tmpdir,["-debug"])
   25        print ("New node created")
   26        connect_nodes(self.nodes[0], 1)  #connect the new node
   27        #connect_nodes(self.nodes[1], 3)  #connect the new node
   28        print ("New node connected....")
   29        #self.sync_all()
   30        sync_blocks(self.nodes)
   31        print ("New node synced")
   32
   33    def run_test(self):
   34        print ("Running test")
   35
   36if __name__ == '__main__':
   37    SyncDir().main()
   

7. 
   TheBlueMatt commented at 1:58 am on October 27, 2014: member

   I believe this is a dup/very related to #5138.
8. 
   domob1812 commented at 6:04 am on October 27, 2014: contributor

   Regarding the sync_all problem: I ran into this myself recently as well. In my case, the problem is that the blocks are synced but the mempools are not. This happens (for me) when transactions are put back into the mempool after reorganising - it seems those are not broadcasted to the nodes that always were on the longer chain.

   Since I introduced sync_all, should I propose another patch? sync_all could be modified to be configurable about what should be synced (blocks and/or mempool), either with an optional argument or with a new member variable. join_network would then only sync blocks and not the mempools. Sounds like a good idea? Or should such a change be introduced only together with actual tests that need the fixed behaviour?

9. laanwj added the label Bug on Jan 8, 2015
10. laanwj added the label Priority High on Jan 8, 2015
11. 
    laanwj commented at 1:25 pm on January 8, 2015: member

    Should be fixed by #5157
12. laanwj closed this on Jan 8, 2015

    ---

13. laanwj referenced this in commit cd737214ce on Sep 16, 2019
14. sidhujag referenced this in commit f9a3ff0065 on Sep 16, 2019
15. MarcoFalke referenced this in commit 59c138d2f1 on Sep 18, 2019
16. random-zebra referenced this in commit 46585f7b62 on Mar 31, 2020
17. reddink referenced this in commit 0c8bc1b50c on May 27, 2020
18. MarcoFalke locked this on Sep 8, 2021

Contributors
domob1812 sipa morcos TheBlueMatt laanwj

Labels
Bug