Move g_signals.SetBestChain(..) below SyncWithWallets #4798

I believe that the setBestChain-signal should be triggered After we sync the txs. I tested in regtest, if we write the chainstate, trigger setBestChain, and then crash before syncing the txs, we dont rescan them. The same test after this patch showed "Rescanning..." in the log file.

Looking at old code this was always the case, but has been changed due to refactorings. Also after this patch, the signal is not triggered for disconnectTip(..) anymore. I think it doesnt make much sense to trigger there, wasnt the case in old code either.

untested ACK; can you add a comment about why the order is important?

@sipa done.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4798_d920f7dcf876f5bb5c525a55c56028fcf65cbdf1/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

Looks very reasonable to me :-)

Shouldnt you call the SetBestChain in DisconnectTip too?

@TheBlueMatt I cant think of a scenario where this makes sense, can you?

In old code the signal was emitted after all the disconnects and connects. Just look at a main.cpp from the beginning of the year.

@TheBlueMatt for symmetry, I guess. But it would be extremely unlikely to matter. The locator written has exponentially-further-back block hashes, and disconnects shouldn't really be doing 1000s of blocks deep reorgs (which is the frequency at which these locators are written).

Untested ACK

ut ACK