Fix several node initialization issues

sipa commented at 2:08 pm on July 22, 2016: member

There were several issues:

InitBlockIndex was still calling ActivateBestChain (intended so the genesis block would be activated), which caused the full chain activate to occur immediately (before the node was fully started and RPC was enabled). Remove this, as this is dealt with during the background import thread now.
Use a signal and a condition variable to make AppInit2 wait for the genesis block to activate, rather than a sleep-and-test loop (which needs the heavily contended cs_main at that point).
Introduce an extra init message after loading the banlist (which is confusing).
Do a block space check earlier.

If accepted, I propose that the first commit is backported to 0.13. The second needs extra translation, unfortunately.

sipa commented at 2:17 pm on July 22, 2016: member

I guess this does not entirely fix #8117: the main problem I was observing is that after a call with -reindex-chainstate, RPC calls would fail with “error code: -28: error message: Loading banlist…” until the full reindex would complete. That should be fixed by this, though fully activating may be delayed still due to the contention between CNode creation and block processing in the background.

jonasschnelli added the label Refactoring on Jul 23, 2016

in src/init.cpp: in cfa5d1f5f2 outdated

1451-
1452-        if (!fHaveGenesis) {
1453-            MilliSleep(10);
1454+    {
1455+        boost::unique_lock<boost::mutex> lock(cs_GenesisWait);
1456+        while (!fHaveGenesis) {

NicolasDorier commented at 2:41 am on July 24, 2016:

I’m not familiar with this code so I may be wrong. However, it seems you are locking cs_GenesisWait waiting for fHaveGenesis to be true, while at the same time, the code responsible to make fHaveGenesis true at https://github.com/bitcoin/bitcoin/pull/8392/files#diff-c865a8939105e6350a50af02766291b7R521 need the same cs_GenesisWait lock. Seems deadlock ?

sipa commented at 8:22 am on July 24, 2016:

@NicolasDorier Waiting on a condition variable releases the lock temporarily.

Use a signal to continue init after genesis activation 0fd2a33648

Add extra message to avoid a long 'Loading banlist' aa59f2ed3f

Do diskspace check before import thread is started 9d4eb9ad99

sipa force-pushed on Jul 30, 2016

sipa commented at 0:19 am on July 30, 2016: member

Rebased and fixed a bug in unit tests (it needed a call to ActivateBestChain as well now, since removing it from InitBlockIndex).

laanwj commented at 6:34 am on August 2, 2016: member

Going to test this one

laanwj commented at 1:02 pm on August 2, 2016: member

It now does all the catching up at Starting network threads..., which I suppose is better than at Loading banlist... but the underlying problem is still there:

 02016-08-02 12:57:08 init message: Loading banlist...
 12016-08-02 12:57:08 Loaded 0 banned node ips/subnets from banlist.dat  47ms
 22016-08-02 12:57:08 init message: Starting network threads...
 32016-08-02 12:57:08 Added connection peer=0
 42016-08-02 12:58:07 Pre-allocating up to position 0x900000 in rev00559.dat
 52016-08-02 12:58:07 UpdateTip: new best=000000000000000000b4507ddb1dd656f75cf36889931653f83775e69920963c height=418553 version=0x20000001 log2_work=84.910427 tx=139363153 date='2016-06-29 22:02:20' progress=0.982011 cache=10.7MiB(4325tx)
 62016-08-02 12:58:45 UpdateTip: new best=000000000000000000b3fbefdf962686338d9a0b7177adfb21962041f2147b6f height=418554 version=0x20000000 log2_work=84.910463 tx=139363318 date='2016-06-29 21:58:48' progress=0.982010 cache=15.2MiB(6634tx)
 72016-08-02 12:59:11 UpdateTip: new best=00000000000000000247db830d97fd6d6ac97e471b5e0e3062a6b66eec02b5a6 height=418555 version=0x30000001 log2_work=84.910499 tx=139364312 date='2016-06-29 22:06:07' progress=0.982013 cache=17.8MiB(10005tx)
 82016-08-02 12:59:13 UpdateTip: new best=0000000000000000043fdfe42285486df7ac2192d82aad1e1d240fa9ed4f8eba height=418556 version=0x20000001 log2_work=84.910535 tx=139364477 date='2016-06-29 22:06:05' progress=0.982013 cache=18.4MiB(10401tx)
 92016-08-02 12:59:22 UpdateTip: new best=000000000000000003feebd19bb1745ff866e15e3e4f9939ec3dbd5c9f9d6bde height=418557 version=0x30000000 log2_work=84.91057 tx=139365187 date='2016-06-29 22:11:05' progress=0.982014 cache=20.7MiB(11931tx)
102016-08-02 12:59:24 UpdateTip: new best=000000000000000001036a974077ff29582c9504169f286c620d1c86b078b682 height=418558 version=0x20000001 log2_work=84.910606 tx=139365525 date='2016-06-29 22:12:54' progress=0.982015 cache=21.9MiB(12671tx)
112016-08-02 12:59:31 UpdateTip: new best=000000000000000004663f9109b89879caf6d41a671ce17d60c14e6413724df0 height=418559 version=0x20000001 log2_work=84.910642 tx=139366069 date='2016-06-29 22:17:59' progress=0.982017 cache=23.5MiB(13780tx)
122016-08-02 12:59:33 UpdateTip: new best=000000000000000004a15c63f0a457094f608a65f8eab712d33d80baca3922c0 height=418560 version=0x20000001 log2_work=84.910677 tx=139366361 date='2016-06-29 22:20:13' progress=0.982018 cache=23.7MiB(14223tx)
132016-08-02 12:59:33 UpdateTip: new best=000000000000000004867bc3d9e57941bb9df2d883a44d220a255998a0ffcc64 height=418561 version=0x20000001 log2_work=84.910713 tx=139366420 date='2016-06-29 22:20:37' progress=0.982018 cache=23.8MiB(14319tx)
14...

Tried adding a yield in ActivateBestChain. That didn’t work (also found this article on the necessity of explicit yields when unlocking). Adding a boost::this_thread::sleep(boost::posix_time::milliseconds(100)); between every block did however work. But that’s wasteful…

Adding a single 200ms sleep before ActivateBestChain in ThreadImport did the trick too. Adding an arbitrary sleep is quite imprecise/fragile though. Could we launch ThreadImport after the network initialization is done? I guess not as-is, as the poll for the genesis block is expected to be present before that. But some other coordination could maybe work.

In any case ACK on the changes already here.

laanwj merged this on Aug 4, 2016

laanwj closed this on Aug 4, 2016

laanwj referenced this in commit f97d335942 on Aug 4, 2016

in src/init.cpp: in 9d4eb9ad99

1437@@ -1412,26 +1438,20 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
1438         BOOST_FOREACH(const std::string& strFile, mapMultiArgs["-loadblock"])
1439             vImportFiles.push_back(strFile);
1440     }
1441+
1442     threadGroup.create_thread(boost::bind(&ThreadImport, vImportFiles));
1443 
1444     // Wait for genesis block to be processed
1445-    bool fHaveGenesis = false;
1446-    while (!fHaveGenesis && !fRequestShutdown) {

rebroad commented at 9:58 am on August 23, 2016:

This line was only just added in May by Patrick! Is the loss of sensitivity to a shutdown request intentional?

luke-jr referenced this in commit 30eac2d79a on Sep 21, 2016

luke-jr referenced this in commit 3b354d213f on Sep 21, 2016

luke-jr referenced this in commit fc349288cb on Sep 21, 2016

codablock referenced this in commit 0733cb5aa9 on Sep 19, 2017

codablock referenced this in commit 0fc4b45d38 on Dec 29, 2017

codablock referenced this in commit 5e54cf907d on Jan 8, 2018

lateminer referenced this in commit 6fe2142990 on Oct 24, 2018

andvgal referenced this in commit 9de331388e on Jan 6, 2019

furszy referenced this in commit ebbb876740 on Feb 14, 2020

furszy referenced this in commit 6bb09adfbb on Feb 17, 2020

wqking referenced this in commit 4130efa05a on Jul 30, 2020

zkbot referenced this in commit 2d9a9aaa83 on Nov 11, 2020

zkbot referenced this in commit f40121446d on Nov 12, 2020

zkbot referenced this in commit 049951dc45 on Feb 11, 2021

zkbot referenced this in commit b3a6729944 on Feb 16, 2021

zkbot referenced this in commit e85265fbd5 on Feb 17, 2021

zkbot referenced this in commit b4b07a1bbd on Feb 17, 2021

DrahtBot locked this on Sep 8, 2021

Fix several node initialization issues #8392