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

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.)

In which code version do you observe this?

Current Git header: 64ffc995d685cf8a53ef868572e835ce42269ec6

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

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.

#!/usr/bin/env python

from test_framework import BitcoinTestFramework
from bitcoinrpc.authproxy import AuthServiceProxy, JSONRPCException
from util import *
import time

class SyncDir(BitcoinTestFramework):
    def setup_network(self):
        self.nodes = []
        self.nodes.append(start_node(0, self.options.tmpdir, ["-debug"]))
        self.nodes.append(start_node(1, self.options.tmpdir, ["-debug"]))
        connect_nodes(self.nodes[1], 0)
        print("Nodes connected")
        self.is_network_split = False
        self.sync_all()
        random_transaction(self.nodes, Decimal("0.001"), Decimal("0.0001"), 0, 0)
        print ("One tx sent")
        self.nodes[0].setgenerate(True,1)
        print ("Block mined")
        self.sync_all()
        print ("Chains synced")
        stop_node(self.nodes[0],0)
        print ("Node stopped")
        self.nodes[0] = start_node(3, self.options.tmpdir,["-debug"])
        print ("New node created")
        connect_nodes(self.nodes[0], 1)  #connect the new node
        #connect_nodes(self.nodes[1], 3)  #connect the new node
        print ("New node connected....")
        #self.sync_all()
        sync_blocks(self.nodes)
        print ("New node synced")

    def run_test(self):
        print ("Running test")

if __name__ == '__main__':
    SyncDir().main()

I believe this is a dup/very related to #5138.

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?

Should be fixed by #5157