This reworks/revives #15848 to add a check for low disk space on first startup and issue a warning if disk space is below the expected space required to accommodate the blocks.
This PR was fashioned by a team of developers at the bitcoin++ conference workshop: "Let's contribute to Bitcoin Core"
Fixes #15813
Confirmed this in an underpowered VM. I know it was discussed to use a warning instead of an error. I think that a warning, however, should probably not terminate the process. So perhaps it would be better to change this function to return true after emitting the warning.
2022-06-09T11:42:03Z init message: Verifying blocks…
2022-06-09T11:42:03Z block index 463ms
2022-06-09T11:42:03Z Warning: Disk space for "/home/litch/.bitcoin/blocks" may not accommodate the block files
Warning: Disk space for "/home/litch/.bitcoin/blocks" may not accommodate the block files
2022-06-09T11:42:03Z Shutdown: In progress...
2022-06-09T11:42:03Z scheduler thread exit
2022-06-09T11:42:03Z Shutdown: done
litch@bitcoind-test:~/bitcoin$
So perhaps it would be better to change this function to return true after emitting the warning.
Well thanks for testing - in this case we don't need to return at all, as the intention is to note the issue and carry on. Removed the return statement.
1645 | + : chainparams.AssumedBlockchainSize() * GIB_BYTES; 1646 | + } 1647 | + } 1648 | + if (!CheckDiskSpace(gArgs.GetBlocksDirPath(), additional_bytes_needed)) { 1649 | + InitWarning(strprintf(_("Disk space for %s may not accommodate the block files"), 1650 | + fs::quoted(fs::PathToString(gArgs.GetBlocksDirPath())))
would be nice to use args (the local reference) over gArgs (the global) for new code
👍
...
2022-06-09T15:43:48Z Initializing databases...
2022-06-09T15:43:48Z Opening LevelDB in /home/litch/.bitcoin/chainstate
2022-06-09T15:43:49Z Opened LevelDB successfully
2022-06-09T15:43:49Z Wrote new obfuscate key for /home/litch/.bitcoin/chainstate: 5da37d724c848cd8
2022-06-09T15:43:49Z Using obfuscation key for /home/litch/.bitcoin/chainstate: 5da37d724c848cd8
2022-06-09T15:43:49Z init message: Verifying blocks…
2022-06-09T15:43:49Z block index 1014ms
2022-06-09T15:43:49Z Warning: Disk space for "/home/litch/.bitcoin/blocks" may not accommodate the block files. Approximately 460 GB of data will be stored in this directory.
Warning: Disk space for "/home/litch/.bitcoin/blocks" may not accommodate the block files. Approximately 460 GB of data will be stored in this directory.
2022-06-09T15:43:49Z loadblk thread start
2022-06-09T15:43:49Z UpdateTip: new best=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f height=0 version=0x00000001 log2_work=32.000022 tx=1 date='2009-01-03T18:15:05Z' progr
ess=0.000000 cache=0.0MiB(0txo)
2022-06-09T15:43:49Z Failed to open mempool file from disk. Continuing anyway.
2022-06-09T15:43:49Z loadblk thread exit
2022-06-09T15:43:49Z block tree size = 1
...
When running on a VM with a small HDD, this prints the warning as expected.
After downloading a number of blocks, then re-starting bitcoind, the warning is not re-emitted.
1632 | @@ -1633,6 +1633,25 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) 1633 | return false; 1634 | } 1635 | 1636 | + uint64_t additional_bytes_needed = 0; 1637 | + if (!fReindex) {
I think the reason for this is that fReindex sets back the height to 0 without deleting blocks (to rebuild the chain later in init), so that it would be wrong to give a warning that doesn't account for the already downloaded blocks.
But in that case, I think that this should also be skipped for fReindexChainState, for the same reason.
While I'm not able to confirm your point, disabling the check in that case seems prudent as this check is intended for the common case.
Disabled the warning on fReindexChainState, and reworked it to avoid duplicative check in the cases where additional_bytes_needed would be 0.
Moved the check further down initialization, to the point where chain_active_height was already being read, so we don't need to do any new locking - a question to test is: will the height be the appropriate value at this time in the startup?
Concept ACK (as a warning only).
I don't think the current location in initialization is correct. ~50 lines above, we start the thread for importing blocks from an external file. This thread will be copying the blocks from the external file to our datadir and then put them all into the block index. It additionally does not do ActivateBestChain until after all of the blocks have been loaded, so we could end up in a similar scenario as a reindex - all the blocks are written but the height still remains 0.
We need to be checking the disk space before -loadblk is handled so that we can know whether we have enough disk space before we begin loading blocks from the external file(s).
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | achow101, aureleoules, willcl-ark |
| Concept ACK | hernanmarino, luke-jr, pablomartin4btc |
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
I agree with @achow101 I believe this should be done earlier.
As @hernanmarino and @achow101, perhaps we could move that validation after the other 2 that also check disk space? (around line 1635)
As suggested, moved the check back to being alongside the other disk space checks.
Code review ACK bba667e
Will need some accommodation for the tests - ignoring the message perhaps?:
test 2022-08-08T06:47:18.274000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 133, in main self.run_test() File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_crosschain.py", line 53, in run_test self.stop_nodes() File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 567, in stop_nodes node.stop_node(wait=wait, wait_until_stopped=False) File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 350, in stop_node raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr)) AssertionError: Unexpected stderr Warning: Disk space for "/tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20220808_063508/wallet_crosschain_80/node1/testnet3/blocks" may not accommodate the block files. Approximately 40 GB of data will be stored in this directory. !=
Core review ACK.
And it works as expected, only displaying the warning the first time before we start loading blocks.
2022-08-08T19:01:43Z Opening LevelDB in /tmp/btc/chainstate
2022-08-08T19:01:43Z Opened LevelDB successfully
2022-08-08T19:01:43Z Wrote new obfuscate key for /tmp/btc/chainstate: f4392c646d173853
2022-08-08T19:01:43Z Using obfuscation key for /tmp/btc/chainstate: f4392c646d173853
2022-08-08T19:01:43Z init message: Verifying blocks…
2022-08-08T19:01:43Z block index 82ms
2022-08-08T19:01:43Z Warning: Disk space for "/tmp/btc/blocks" may not accommodate the block files. Approximately 460 GB of data will be stored in this directory.
2022-08-08T19:02:24Z loadblk thread start
2022-08-08T19:02:24Z UpdateTip: new best=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f height=0 version=0x00000001 log2_work=32.000022 tx=1 date='2009-01-03T18:15:05Z' progress=0.000000 cache=0.0MiB(0txo)
2022-08-08T19:02:24Z block tree size = 1
2022-08-08T19:02:24Z nBestHeight = 0
2022-08-08T19:02:24Z Failed to open mempool file from disk. Continuing anyway.
2022-08-08T19:02:24Z loadblk thread exit
Also on the UI:
@Empact, I also get these 2 funtional tests failing when I run them locally (they pass on the master):
Concept and Code Review ACK bba667e0701090180c0c3b7d2903a8a74e9875f7. I also verified that the warning is shown.
CI fails because the tests p2p_dos_header_tree.py and wallet_crosschain.py try to spawn a testnet chain, which would be too big for the CI and creates an output on stderr which should not happen.
<details> <summary>This fix works on my machine but I haven't tested in CI: </summary>
diff --git a/test/functional/p2p_dos_header_tree.py b/test/functional/p2p_dos_header_tree.py
index fde1e4bfa..922e65036 100755
--- a/test/functional/p2p_dos_header_tree.py
+++ b/test/functional/p2p_dos_header_tree.py
@@ -22,6 +22,7 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework):
self.setup_clean_chain = True
self.chain = 'testnet3' # Use testnet chain because it has an early checkpoint
self.num_nodes = 2
+ self.extra_args = [['-prune=550']] * self.num_nodes
def add_options(self, parser):
parser.add_argument(
@@ -62,7 +63,7 @@ class RejectLowDifficultyHeadersTest(BitcoinTestFramework):
self.log.info("Feed all fork headers (succeeds without checkpoint)")
# On node 0 it succeeds because checkpoints are disabled
- self.restart_node(0, extra_args=['-nocheckpoints'])
+ self.restart_node(0, extra_args=['-nocheckpoints', '-prune=550'])
peer_no_checkpoint = self.nodes[0].add_p2p_connection(P2PInterface())
peer_no_checkpoint.send_and_ping(msg_headers(self.headers_fork))
assert {
diff --git a/test/functional/wallet_crosschain.py b/test/functional/wallet_crosschain.py
index b6d0c8798..c0047542e 100755
--- a/test/functional/wallet_crosschain.py
+++ b/test/functional/wallet_crosschain.py
@@ -21,7 +21,7 @@ class WalletCrossChain(BitcoinTestFramework):
# Switch node 1 to testnet before starting it.
self.nodes[1].chain = 'testnet3'
- self.nodes[1].extra_args = ['-maxconnections=0'] # disable testnet sync
+ self.nodes[1].extra_args = ['-maxconnections=0', '-prune=550'] # disable testnet sync
with open(self.nodes[1].bitcoinconf, 'r', encoding='utf8') as conf:
conf_data = conf.read()
with open (self.nodes[1].bitcoinconf, 'w', encoding='utf8') as conf:
@@ -51,7 +51,7 @@ class WalletCrossChain(BitcoinTestFramework):
if not self.options.descriptors:
self.log.info("Override cross-chain wallet load protection")
self.stop_nodes()
- self.start_nodes([['-walletcrosschain']] * self.num_nodes)
+ self.start_nodes([['-walletcrosschain', '-prune=550']] * self.num_nodes)
self.nodes[0].loadwallet(node1_wallet)
self.nodes[1].loadwallet(node0_wallet)
</details>
EDIT: it works in CI as well
To accommodate the expected blocks data.
Co-authored-by: Antoine Poinsot <darosior@protonmail.com>
Co-authored-by: benthecarman <benthecarman@live.com>
Co-authored-by: Justin Litchfield <litch@me.com>
Co-authored-by: Liran Cohen <c.liran.c@gmail.com>
Co-authored-by: Ryan Loomba <ryan.loomba@gmail.com>
Co-authored-by: Buck Perley <bucko.perley@gmail.com>
Co-authored-by: bajjer <bajjer@bajjer.xyz>
Co-authored-by: Suhail Saqan <suhail.saqan@gmail.com>
Co-authored-by: Christopher Sweeney <sweeney.chris@gmail.com>
Co-authored-by: Alyssa <orbitalturtle@protonmail.com>
Co-authored-by: Ben Schroth <ben@styng.social>
Co-authored-by: Jason Hester <mail@jason-hester.me>
Co-authored-by: Matt Clough <Matt.clough@pm.me>
Co-authored-by: Elise Schedler <eliseschedler@gmail.com>
Co-authored-by: ghander <cen254@gmail.com>
Co-authored-by: PopeLaz <btclz@fastmail.com>
Co-authored-by: Aurèle Oulès <hello@aureleoules.com>
ACK 6630a1e8448c633e4abaa8f5903f11cac6f433a7
ACK 6630a1e8448c633e4abaa8f5903f11cac6f433a7
tACK 6630a1e8448c633e4abaa8f5903f11cac6f433a7 rebased on master. Warning shows on first start but not on restart after some blocks have been downloaded.
Thanks to all the reviewers and contributors. For posterity, here's the original session in which this was written: https://youtu.be/tRXIkqM8gSI