init: Remove auto-import of bootstrap.dat and associated code #17044

pull fjahr wants to merge 1 commits into bitcoin:master from fjahr:pr15954 changing 3 files +6 −16
  1. fjahr commented at 7:00 pm on October 3, 2019: member

    This picks up #15954

    I fixed the code and added at a functional test utilizing the scripts in contrib/linearize as suggested by @MarcoFalke .

  2. fjahr force-pushed on Oct 3, 2019
  3. DrahtBot added the label Docs on Oct 3, 2019
  4. DrahtBot added the label Refactoring on Oct 3, 2019
  5. DrahtBot added the label Tests on Oct 3, 2019
  6. MarcoFalke removed the label Docs on Oct 3, 2019
  7. MarcoFalke removed the label Refactoring on Oct 3, 2019
  8. MarcoFalke removed the label Tests on Oct 3, 2019
  9. MarcoFalke added the label Block storage on Oct 3, 2019
  10. MarcoFalke renamed this:
    refactor: remove old bootstrap relevant code
    remove old bootstrap relevant code
    on Oct 3, 2019
  11. fjahr force-pushed on Oct 3, 2019
  12. in test/functional/feature_loadblock.py:7 in 078d0a5924 outdated 
    0@@ -0,0 +1,87 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2017-2019 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test loadblock option
6+
7+Test the option to start a node with the option loadpath which
    mzumsande commented at 9:31 pm on October 4, 2019:
    loadblock, not loadpath
    fjahr commented at 11:45 am on October 5, 2019:
    fixed
  13. in test/functional/feature_loadblock.py:9 in 078d0a5924 outdated 
    0@@ -0,0 +1,87 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2017-2019 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test loadblock option
6+
7+Test the option to start a node with the option loadpath which
8+loads a serialized blockchain from a file (usually called bootstrap.dat).
9+To generate that file this test uses the helper scripts in available
    mzumsande commented at 9:32 pm on October 4, 2019:
    remove “in”
    fjahr commented at 11:45 am on October 5, 2019:
    fixed
  14. mzumsande commented at 9:49 pm on October 4, 2019: member

    The CIs don’t like the new test yet, probably because the relative paths require one to be in the bitcoin root dir for the scripts to be found.

    Also, the help text for loadblock, “Imports blocks from external blk000??.dat file on startup” could be updated to remove the file name.

  15. in test/functional/feature_loadblock.py:62 in 078d0a5924 outdated 
    57+                              port=node_url.port,
58+                              host=node_url.hostname,
59+                              hashlist=hashlist_path,
60+                              block_dir=block_dir,
61+                              genesis=genesis_block))
62+        cfg.close()
    MarcoFalke commented at 10:00 pm on October 4, 2019:

    Can use https://docs.python.org/3/reference/compound_stmts.html#with instead?

    And the writing it like this, should make the test shorter in line count. https://github.com/bitcoin/bitcoin/blob/7b701fef58f627956d597817a1f9422edd890cdc/test/functional/test_framework/util.py#L306

    fjahr commented at 11:45 am on October 5, 2019:
    Yes, that’s much better, implemented that.
  16. laanwj renamed this:
    remove old bootstrap relevant code
    init: Remove auto-import of `bootstrap.dat` and associated code
    on Oct 5, 2019
  17. fjahr force-pushed on Oct 5, 2019
  18. in test/functional/feature_loadblock.py:72 in eeecdfa27a outdated 
    67+
68+        # use a new empty blocks dir to make sure blocks are loaded from bootstrap file
69+        shutil.rmtree(self.nodes[0].datadir)
70+        initialize_datadir(self.options.tmpdir, 0, self.chain)
71+        blocksdir_path = os.path.join(self.options.tmpdir, 'blocksdir')
72+        os.mkdir(blocksdir_path)
    MarcoFalke commented at 5:19 pm on October 7, 2019:

    What is the point of blocksdir_path (new empty blocks dir), when the whole datadir is cleaned?

    I’d even go further and not remove the whole datadir tree. Since that also deletes the debug log for debugging purposes.

    You could spin up two nodes, and call setnetworkactive(False) on the second one. Then reboot the second one with the -loadblock arg here.

    fjahr commented at 9:09 pm on October 8, 2019:
    I was weighing different options initially and thought using just one node might be more economical. But I did not think about the logs. I am now using a second node.
  19. fjahr force-pushed on Oct 8, 2019
  20. fjahr force-pushed on Oct 8, 2019
  21. jonasschnelli commented at 6:31 am on October 9, 2019: contributor
    Concept ACK
  22. fjahr force-pushed on Oct 9, 2019
  23. fjahr force-pushed on Oct 9, 2019
  24. fjahr force-pushed on Oct 9, 2019
  25. in ci/test/06_script_b.sh:12 in 2ac1637787 outdated 
     7@@ -8,6 +8,9 @@ export LC_ALL=C.UTF-8
 8 
 9 cd "build/bitcoin-$HOST" || (echo "could not enter distdir build/bitcoin-$HOST"; exit 1)
10 
11+mkdir -p contrib
12+cp -R ../../contrib/ contrib/
    MarcoFalke commented at 1:20 pm on October 9, 2019:

    I think you can’t run these with the travis user, only with the docker root user.

    I.e.

    0DOCKER_EXEC cp -R ../../contrib/ contrib/
    MarcoFalke commented at 1:22 pm on October 9, 2019:
    Also, if those files are missing because they are not in the distdir, I’d prefer to add them instead of copying them manually in the ci
    fjahr commented at 4:27 pm on October 9, 2019:
    Yes, that seems cleaner, doing that instead now
  26. fanquake added the label Waiting for author on Oct 9, 2019
  27. in test/functional/feature_loadblock.py:59 in 2ac1637787 outdated 
    54+        base_dir = self.config["environment"]["SRCDIR"]
55+        linearize_dir = os.path.join(base_dir, "contrib", "linearize")
56+
57+        self.log.info("Run linearization of block hashes")
58+        linearize_hashes_path = os.path.join(linearize_dir, "linearize-hashes.py")
59+        os.system(linearize_hashes_path + " " + cfg_path + " > " + hashlist_path)
    MarcoFalke commented at 1:23 pm on October 9, 2019:
    Shouldn’t you check the return code?
    MarcoFalke commented at 1:24 pm on October 9, 2019:
    Also, does the > work on all OS flavors? It seems that appveyor windows is passing, so it seems fine!?
    laanwj commented at 1:25 pm on October 9, 2019:
    also, please use subprocess instead of os.system to invoke other processes from python
    fjahr commented at 4:26 pm on October 9, 2019:

    Also, does the > work on all OS flavors? It seems that appveyor windows is passing, so it seems fine!?

    I did a brief check and did not find any issues concerning windows. I am on macOS and it’s fine.

    fjahr commented at 4:26 pm on October 9, 2019:

    also, please use subprocess instead of os.system to invoke other processes from python

    done

    Shouldn’t you check the return code?

    done

  28. fjahr force-pushed on Oct 9, 2019
  29. DrahtBot commented at 3:51 pm on October 9, 2019: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16981 (Improve runtime performance of –reindex by LarryRuane)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  30. MarcoFalke removed the label Waiting for author on Oct 9, 2019
  31. fjahr force-pushed on Oct 9, 2019
  32. fjahr commented at 11:00 pm on October 9, 2019: member
    As the test got much larger than the initial commit and also fixes a separate issue #17019 it is now split into its own PR at #17091
  33. in src/init.cpp:1707 in d617ff75c2 outdated 
    1704+
1705+    if (gArgs.IsArgSet("-loadblock")) {
1706+        const std::vector<std::string> vLoadBlockArgs = gArgs.GetArgs("-loadblock");
1707+
1708+        // Regard the '-noloadblock' argument
1709+        if (!vLoadBlockArgs.empty()) {
    promag commented at 3:35 pm on October 10, 2019:
    You could remove this check, iterating an empty array is fine.
    fjahr commented at 6:25 pm on October 12, 2019:
    done
  34. promag commented at 3:35 pm on October 10, 2019: member
    Concept ACK
  35. fjahr force-pushed on Oct 12, 2019
  36. luke-jr commented at 6:46 pm on October 12, 2019: member
    Concept ACK
  37. laanwj referenced this in commit 4258fd7377 on Oct 23, 2019
  38. in src/init.cpp:1706 in 04aeb5d4ab outdated 
    1703-        vImportFiles.push_back(strFile);
1704+
1705+    if (gArgs.IsArgSet("-loadblock")) {
1706+        const std::vector<std::string> vLoadBlockArgs = gArgs.GetArgs("-loadblock");
1707+
1708+        // Regard the '-noloadblock' argument
    MarcoFalke commented at 2:27 pm on October 23, 2019:
    what does this comment mean? GetArgs takes care of the -noloadblock option
    MarcoFalke commented at 2:27 pm on October 23, 2019:
    Also, why is this code block changed at all?
    fjahr commented at 9:34 pm on October 31, 2019:
    True, after implementing the feedback this doesn’t do anything different aside from using AbsPathForConfigVal() which does not seem to be critical here.
  39. in src/init.cpp:377 in 04aeb5d4ab outdated 
    373@@ -374,7 +374,7 @@ void SetupServerArgs()
374     gArgs.AddArg("-debuglogfile=<file>", strprintf("Specify location of debug log file. Relative paths will be prefixed by a net-specific datadir location. (-nodebuglogfile to disable; default: %s)", DEFAULT_DEBUGLOGFILE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
375     gArgs.AddArg("-feefilter", strprintf("Tell other nodes to filter invs to us by our mempool min fee (default: %u)", DEFAULT_FEEFILTER), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);
376     gArgs.AddArg("-includeconf=<file>", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
377-    gArgs.AddArg("-loadblock=<file>", "Imports blocks from external blk000??.dat file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
378+    gArgs.AddArg("-loadblock=<file>", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    MarcoFalke commented at 2:32 pm on October 23, 2019:
    why is this removed? I’d rather add a test for importing blk000.dat files.
    mzumsande commented at 9:01 pm on October 31, 2019:
    @MarcoFalke: I suggested removing that above, because it seemed to imply a non-existing naming convention for the file (might as well load from bootstrap.dat or whatever your file is named).
    MarcoFalke commented at 8:18 pm on November 1, 2019:

    See for example this test:

     0diff --git a/test/functional/feature_loadblock.py b/test/functional/feature_loadblock.py
 1index bf2a4ff61f..a532a83946 100755
 2--- a/test/functional/feature_loadblock.py
 3+++ b/test/functional/feature_loadblock.py
 4@@ -25,7 +25,7 @@ from test_framework.util import assert_equal, wait_until
 5 class LoadblockTest(BitcoinTestFramework):
 6     def set_test_params(self):
 7         self.setup_clean_chain = True
 8-        self.num_nodes = 2
 9+        self.num_nodes = 3
10 
11     def run_test(self):
12         self.nodes[1].setnetworkactive(state=False)
13@@ -71,13 +71,16 @@ class LoadblockTest(BitcoinTestFramework):
14         subprocess.run([sys.executable, linearize_data_file, cfg_file],
15                        check=True)
16 
17-        self.log.info("Restart second, unsynced node with bootstrap file")
18+        self.log.info("Restart unsynced nodes with bootstrap file or raw block file")
19         self.stop_node(1)
20         self.start_node(1, ["-loadblock=" + bootstrap_file])
21-        wait_until(lambda: self.nodes[1].getblockcount() == 100)
22+        self.stop_node(2)
23+        self.start_node(2, ["-loadblock=" + os.path.join(blocks_dir, 'blk00000.dat')])
24+        for i in [1, 2]:
25+            wait_until(lambda: self.nodes[i].getblockcount() == 100)
26 
27-        assert_equal(self.nodes[1].getblockchaininfo()['blocks'], 100)
28-        assert_equal(self.nodes[0].getbestblockhash(), self.nodes[1].getbestblockhash())
29+            assert_equal(self.nodes[i].getblockchaininfo()['blocks'], 100)
30+            assert_equal(self.nodes[0].getbestblockhash(), self.nodes[i].getbestblockhash())
31 
32 
33 if __name__ == '__main__':
    laanwj commented at 2:42 pm on November 4, 2019:
    Agree with removing this. blkXXXXX.dat are the block files and the way to (re)index them is -reindex, it has nothing to do with -loadblock.
    MarcoFalke commented at 2:49 pm on November 4, 2019:
    Isn’t the point that you can import the block.dat files from another datadir with -loadblock=blk001.dat -loadblock=blk002.dat -loadblock=...
    laanwj commented at 2:58 pm on November 4, 2019:

    I don’t think that’s the point. In that case it’s less hassle to copy over all the blk files and do a reindex.

    I think the idea behind -loadblock is to import the output from linearize; huge consecutive ‘block archives’ (there used to be torrents for this). But I might be wrong. I’m not sure what it’s used for nowadays.

    MarcoFalke commented at 3:00 pm on November 4, 2019:
    Oh, so it is multi-arg to support a split-up bootstrap.dat?
    fjahr commented at 3:52 pm on November 5, 2019:
    Yeah, that was my understanding as well. But I did see it spelled out anywhere explicitly.
    laanwj commented at 4:42 pm on November 5, 2019:

    Oh, so it is multi-arg to support a split-up bootstrap.dat?

    I think so; linearize splits it into 1GB files by default.

  40. MarcoFalke commented at 2:32 pm on October 23, 2019: member
    some questions
  41. MarcoFalke deleted a comment on Oct 23, 2019
  42. sidhujag referenced this in commit 3e3f53b757 on Oct 26, 2019
  43. fjahr force-pushed on Oct 31, 2019
  44. MarcoFalke commented at 8:18 pm on November 1, 2019: member
    ACK 5453e2ddf5f9c432d59ba64315a239792136526d
  45. laanwj commented at 11:31 am on November 2, 2019: member

    nit:

     0diff --git a/doc/developer-notes.md b/doc/developer-notes.md
 1index 1a7ce91ca67d47903a3b97d45944fb2df6ea2fa1..08004778e3e13284bf1de95efd24a62202847bfc 100644
 2--- a/doc/developer-notes.md
 3+++ b/doc/developer-notes.md
 4@@ -384,7 +384,7 @@ Threads
 5 
 6 - ThreadScriptCheck : Verifies block scripts.
 7 
 8-- ThreadImport : Loads blocks from blk*.dat files or bootstrap.dat.
 9+- ThreadImport : Loads blocks from `blk*.dat` files or `-loadblock=<file>`.
10 
11 - StartNode : Starts other threads.
12
  46. remove old bootstrap relevant code 
    - only load blockfiles when we have paths
- add release notes for modified bootstrap functionality
- amend documentation on ThreadImport
    104f7de593
  47. fjahr force-pushed on Nov 5, 2019
  48. fjahr commented at 3:50 pm on November 5, 2019: member
    @laanwj good catch, added that.
  49. MarcoFalke commented at 5:01 pm on November 5, 2019: member
    ok, fine LGTM
  50. laanwj commented at 6:24 pm on November 5, 2019: member
    ACK 104f7de5934f13b837fcf21f6d6b2559799eabe2
  51. laanwj referenced this in commit d35b12107e on Nov 5, 2019
  52. laanwj merged this on Nov 5, 2019
  53. laanwj closed this on Nov 5, 2019
  54. jasonbcox referenced this in commit d7421f8eef on Nov 4, 2020
  55. MarcoFalke locked this on Dec 16, 2021


fjahr mzumsande MarcoFalke jonasschnelli laanwj DrahtBot promag luke-jr

Labels
Block storage

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me