refactor: remove old bootstrap relevant code

tryphe commented at 2:32 am on May 5, 2019: contributor

Just a cleanup PR:

Remove bootstrap file check from ThreadImport() which is called via the ’loadblk’ thread during startup by default.
’loadblk’ thread will only spawn during AppInitMain() if -loadblock is set with arguments, and correctly regards -noloadblock.

Additionally:

-loadblock arguments now resolve into absolute paths.

One implication here is that copying something to $datadir/bootstrap.dat and starting with -loadblock won’t work anymore. You’ll have to explicitly specify the file now. Since bootstrapping is gone from all documentation and most of the codebase (this is the last I could find), I think this is a decent change.

fanquake added the label Refactoring on May 5, 2019

laanwj commented at 3:59 pm on May 5, 2019: member

Concept ACK, might want to add a short mention to the release notes in case someone was still relying on the old behavior.

tryphe commented at 6:28 pm on May 5, 2019: contributor

@laanwj, thanks! Release notes created.

fanquake commented at 2:41 am on May 6, 2019: member

Concept ACK

Can you squash 9de6512 -> 72275fc into a single commit. (i.e doc: add release notes for [#15954](/bitcoin-bitcoin/15954/)).

Can you also checkout the Travis build logs, and fix (and squash into the first commit) any errors thrown by the linter. i.e This diff appears to have added new lines with trailing whitespace..

tryphe force-pushed on May 6, 2019

in doc/release-notes-15954.md:4 in e721200e6f outdated

0@@ -0,0 +1,4 @@
1+Configuration option changes
2+-----------------------------
3+
4+Importing blocks upon startup via the `bootstrap.dat` file no longer occurs by default. The file must now be specified with `-loadblocks=<file>`.

promag commented at 1:35 pm on May 6, 2019:

-loadblock

in src/init.cpp:1717 in e721200e6f outdated

1713@@ -1728,12 +1714,15 @@ bool AppInitMain(InitInterfaces& interfaces)
1714     if (gArgs.IsArgSet("-blocknotify"))
1715         uiInterface.NotifyBlockTip_connect(BlockNotifyCallback);
1716 
1717-    std::vector<fs::path> vImportFiles;
1718-    for (const std::string& strFile : gArgs.GetArgs("-loadblock")) {
1719-        vImportFiles.push_back(strFile);
1720-    }
1721+    if (gArgs.IsArgSet("-loadblock")) {

promag commented at 1:50 pm on May 6, 2019:

Note that it is possible to run with -noloadblock. I suggest to add the condition before spawning the thread:

0if (!vImportFiles.empty()) {
1    // Spawn 'loadblk' thread to run ThreadImport()
2    threadGroup.create_thread(std::bind(&ThreadImport, vImportFiles));
3}

promag commented at 9:40 pm on May 7, 2019:

See https://github.com/bitcoin/bitcoin/blob/b2a6b0216192b6e8428f1a80b47f5178fccb961a/src/util/system.cpp#L407-L412

Basically you can invert options and so if you run with -noloadblock then gArgs.IsArgSet("-loadblock") == true but gArgs.GetArgs("-loadblock") gives an empty array. The result is that the import thread is spawned for no good reason.

tryphe commented at 11:46 pm on May 7, 2019:

Thanks! Let me know if you see anything else that needs changing.

promag commented at 1:53 pm on May 6, 2019: member

One implication here is that copying something to $datadir/bootstrap.dat and starting with -loadblock won’t work. You’ll have to explicitly specify the file now

A solution is to wrap each path with AbsPathForConfigVal?

Concept ACK.

remove old bootstrap relevant code

- remove bootstrap file check from ThreadImport() which is called via the 'loadblk' thread during startup by default
- 'loadblk' thread should only spawn if argument -loadblock is set

Create release-notes-15954.md

add release notes for modified bootstrap functionality

changed markdown formatting of title

changed semantics to reflect 0.18 notes

changed semantics to reflect 0.18 notes rather than 0.17

[doc] clarify explanation

remove whitespace

fix documentation typo

only load blockfiles when we have paths

use absolute paths for -loadblock and regard the -noloadblock argument

54ec140fdf

tryphe force-pushed on May 7, 2019

tryphe commented at 11:44 pm on May 7, 2019: contributor

@promag nice catches, thanks! Squashed.

in src/init.cpp:1728 in 54ec140fdf

1728+                vImportFiles.push_back(AbsPathForConfigVal(strFile));
1729+            }
1730 
1731-    threadGroup.create_thread(std::bind(&ThreadImport, vImportFiles));
1732+            // Spawn 'loadblk' thread to run ThreadImport()
1733+            threadGroup.create_thread(std::bind(&ThreadImport, vImportFiles));

MarcoFalke commented at 10:19 pm on May 9, 2019:

The thread must be called currently (even if nothing is imported) to activate the best chain.

This explains the test failures, I believe

tryphe commented at 11:56 pm on May 9, 2019:

That’s interesting. This compiled and runs fine for me, but I guess since I’m already synced I didn’t reproduce these test issues?

tryphe commented at 0:01 am on May 10, 2019:

Also, this is the only failure I see, but I assumed it was unrelated? FAIL qt/test/test_bitcoin-qt (exit status: 134)

I can’t seem to find the source of the 134

tryphe commented at 0:51 am on May 10, 2019:

There’s also this in appveyor but I wasn’t totally sure what it meant: AssertionError: [node 0] Error: no RPC connection via File "C:\projects\bitcoin/test/functional/create_cache.py", line 28, in <module> I guess that’s it?

MarcoFalke commented at 12:13 pm on May 10, 2019: member

Could add a functional test to mine some blocks, create a boostrap.dat with the helper script, then load it with the command line arg.

fanquake renamed this:
~~remove old bootstrap relevant code~~
refactor: remove old bootstrap relevant code
on Jun 3, 2019

fanquake commented at 2:34 pm on June 3, 2019: member

@tryphe Are you interested in writing a test for this change?

fanquake added the label Up for grabs on Aug 15, 2019

fanquake closed this on Aug 15, 2019

fanquake removed the label Up for grabs on Oct 3, 2019

laanwj referenced this in commit d35b12107e on Nov 5, 2019

MarcoFalke locked this on Dec 16, 2021

refactor: remove old bootstrap relevant code #15954