This is a commit in preparation for the next few commits. The functions
are moved to methods to avoid their re-declaration for the purpose of
passing in BlockManager options.
The functions that were now moved into the BlockManager should no longer
use the params as an argument, but instead use the member variable.
In the moved ReadBlockFromDisk and UndoReadFromDisk, change
the function signature to accept a reference to a CBlockIndex instead of
a raw pointer. The pointer is expected to be non-null, so reflect that
in the type.
To allow for the move of functions to BlockManager methods all call
sites require an instantiated BlockManager, or a callback to one.
This commit effectively moves the definition of these constants
out of the chainparamsbase to their own file.
Using the ChainType enums provides better type safety compared to
passing around strings.
The commit is part of an ongoing effort to decouple the libbitcoinkernel
library from the ArgsManager and other functionality that should not be
part of the kernel library.
8f14fc8622 test: cover fastprune with excessive block size (Matthew Zipkin)
271c23e87f blockstorage: Adjust fastprune limit if block exceeds blockfile size (Martin Zumsande)
Pull request description:
The debug-only `-fastprune` option used in several tests is not always safe to use:
If a `-fastprune` node receives a block larger than the maximum blockfile size of `64kb` bad things happen: The while loop in `BlockManager::FindBlockPos` never terminates, and the node runs oom because memory for `m_blockfile_info` is allocated in each iteration of the loop.
The same would happen if a naive user used `-fastprune` on anything other than regtest (so this can be tested by syncing on signet for example, the first block that crashes the node is at height 2232).
Change the approach by raising the blockfile size to the size of the block, if that block otherwise wouldn't fit (idea by TheCharlatan).
ACKs for top commit:
ryanofsky:
Code review ACK 8f14fc8622. Added new assert, test, and comment since last review
TheCharlatan:
ACK 8f14fc8622
pinheadmz:
ACK 8f14fc8622
Tree-SHA512: df2fea30613ef9d40ebbc2416eacb574f6d7d96847db5c33dda22a29a2c61a8db831aa9552734ea4477e097f253dbcb6dcb1395d43d2a090cc0588c9ce66eac3
If the added block exceeds the blockfile size in test-only
-fastprune mode, the node would get stuck in an infinite loop and
run out of memory.
Avoid this by raising the blockfile size to the size of the added block
in this situation.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
00e9b97f37 refactor: Move fs.* to util/fs.* (TheCharlatan)
106b46d9d2 Add missing fs.h includes (TheCharlatan)
b202b3dd63 Add missing cstddef include in assumptions.h (TheCharlatan)
18fb36367a refactor: Extract util/fs_helpers from util/system (Ben Woosley)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". This commit was originally authored by empact and is taken from its parent PR #25152.
#### Context
There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel library (https://github.com/bitcoin/bitcoin/pull/25290, https://github.com/bitcoin/bitcoin/pull/25487, https://github.com/bitcoin/bitcoin/pull/25527, https://github.com/bitcoin/bitcoin/pull/25862, https://github.com/bitcoin/bitcoin/pull/26177, and https://github.com/bitcoin/bitcoin/pull/27125). The `ArgsManager` is defined in `system.h`. A similar pull request extracting functionality from `system.h` has been merged in https://github.com/bitcoin/bitcoin/pull/27238.
#### Changes
Next to providing better code organization, this PR removes some reliance of the tree of libbitcoinkernel header includes on `system.h` (and thus the `ArgsManager` definition) by moving filesystem related functions out of the `system.*` files.
There is already a pair of `fs.h` / `fs.cpp` in the top-level `src/` directory. They were not combined with the files introduced here, to keep the patch cleaner and more importantly because they are often included without the utility functions. The new files are therefore named `fs_helpers` and the existing `fs` files are moved into the util directory.
Further commits splitting more functionality out of `system.h` are still in #25152 and will be submitted in separate PRs once this PR has been processed.
ACKs for top commit:
hebasto:
ACK 00e9b97f37
Tree-SHA512: 31422f148d14ba3c843b99b1550a6fd77c77f350905ca324f93d4f97b652246bc58fa9696c64d1201979cf88733e40be02d262739bb7d417cf22bf506fdb7666
The fs.* files are already part of the libbitcoin_util library. With the
introduction of the fs_helpers.* it makes sense to move fs.* into the
util/ directory as well.
b3e78dc91d refactor: Don't use global chainparams in chainstatemanager method (TheCharlatan)
382b692a50 Split non/kernel chainparams (Carl Dong)
edabbc78a3 Add factory functions for Main/Test/Sig/Reg chainparams (Carl Dong)
d938098398 Remove UpdateVersionBitsParameters (Carl Dong)
84b85786f0 Decouple RegTestChainParams from ArgsManager (Carl Dong)
76cd4e7c96 Decouple SigNetChainParams from ArgsManager (Carl Dong)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". dongcarl is the original author of this patchset, these commits were taken from https://github.com/dongcarl/bitcoin/tree/2022-03-libbitcoinkernel-chainparams-args-only.
#### Context
The bitcoin kernel library currently relies on code containing user configurations through the `ArgsManager`. This is not optimal, since as a stand-alone library it should not rely on bitcoind's argument parsing logic. Instead, its interfaces should accept control and options structs that control the kernel library's desired configuration.
Similar work towards decoupling the `ArgsManager` from the kernel has been done in
https://github.com/bitcoin/bitcoin/pull/25290, https://github.com/bitcoin/bitcoin/pull/25487, https://github.com/bitcoin/bitcoin/pull/25527 and https://github.com/bitcoin/bitcoin/pull/25862.
#### Changes
By moving the `CChainParams` class definition into the kernel and giving it new factory functions `CChainParams::{RegTest,SigNet,Main,TestNet}`it can be constructed without an `ArgsManager` reference, unlike the current factory function `CreateChainParams`.
The first few commits remove uses of `ArgsManager` within `CChainParams`. Then the `CChainParams` definition is moved to a new file in the `kernel/` subdirectory.
ACKs for top commit:
MarcoFalke:
re-ACK b3e78dc91d🛁
ryanofsky:
Code review ACK b3e78dc91d. Only changes since last review were recent review suggestions.
ajtowns:
ACK b3e78dc91d
Tree-SHA512: 3835aca1d3e3c75cc3303dd584bab3a77e58f6c678724a5e359fe4b0e17e0763a00931ee6191f516b9fde50496f59cc691f0709c0254206db3863bbf7ab2cacd
Moves chainparams code not using the ArgsManager to the kernel.
Subsequently use the kernel chainparams header now where possible in
order to further decouple chainparams call sites from gArgs.
error is a low-level function with a sole dependency on LogPrintf, which
is defined in logging.h
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving the function out of system.h allows including it from a separate
source file without including the ArgsManager definitions from system.h.
fa1b4e5c32 Use steady clock in FlushStateToDisk (MarcoFalke)
1111e2f8b4 Use steady clock in SeedStrengthen and FindBestImplementation (MarcoFalke)
Pull request description:
There may be a theoretical deadlock for the duration of the offset when the system clock is adjusted into a past time while executing `SeedStrengthen`.
Fix this by using steady clock.
Do the same in `FindBestImplementation`, which shouldn't be affected, because it discards outlier measurements. However, doing the same there for consistency seems fine.
Do the same in `FlushStateToDisk`, which should make the flushes more steady, if the system clock is adjusted by a large offset.
ACKs for top commit:
john-moffett:
ACK fa1b4e5c32
willcl-ark:
ACK fa1b4e5c3
Tree-SHA512: cc625e796b186accd53222bd64eb57d0512bc7e588312d254349b542bbc5e5daac348ff2b3b3f7dc5ae0bbbae2ec11fdbf3022cf2164211633765a4b0108e83e
2b373fe49d docs: update assumeutxo.md (James O'Beirne)
87a1108c81 test: add snapshot completion unittests (James O'Beirne)
d70919a88f refactor: make MempoolMutex() public (James O'Beirne)
7300ced9de log: add LoadBlockIndex() message for assumedvalid blocks (James O'Beirne)
d96c59cc5c validation: add ChainMan logic for completing UTXO snapshot validation (James O'Beirne)
f2a4f3376f move-only-ish: init: factor out chainstate initialization (James O'Beirne)
637a90b973 add Chainstate::HasCoinsViews() (James O'Beirne)
c29f26b47b validation: add CChainState::m_disabled and ChainMan::isUsable (James O'Beirne)
5ee22cdafd add ChainstateManager.GetSnapshot{BaseHeight,BaseBlock}() (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: https://github.com/bitcoin/bitcoin/pull/15606)
Part two of replacing https://github.com/bitcoin/bitcoin/pull/24232.
---
When a user activates a snapshot, the serialized UTXO set data is used to create an "assumed-valid" chainstate, which becomes active in an attempt to get the node to network tip as quickly as possible. Simultaneously in the background, the already-existing chainstate continues "conventional" IBD to both accumulate full block data and serve as a belt-and-suspenders to validate the assumed-valid chainstate.
Once the background chainstate's tip reaches the base block of the snapshot used, we set `m_stop_use` on that chainstate and immediately take the hash of its UTXO set; we verify that this matches the assumeutxo value in the source code. Note that while we ultimately want to remove this background chainstate, we don't do so until the following initialization process, when we again check the UTXO set hash of the background chainstate, and if it continues to match, we remove the (now unnecessary) background chainstate, and move the (previously) assumed-valid chainstate into its place. We then reinitialize the chainstate in the normal way.
As noted in previous comments, we could do the filesystem operations "inline" immediately when the background validation completes, but that's basically just an optimization that saves disk space until the next restart. It didn't strike me as worth the risk of moving chainstate data around on disk during runtime of the node, though maybe my concerns are overblown.
The final result of this completion process is a fully-validated chain, where the only evidence that the user synced using assumeutxo is the existence of a `base_blockhash` file in the `chainstate` directory.
ACKs for top commit:
achow101:
ACK 2b373fe49d
Tree-SHA512: a204e1d6e6932dd83c799af3606b01a9faf893f04e9ee1a36d63f2f1ccfa9118bdc1c107d86976aa0312814267e6a42074bf3e2bf1dead4b2513efc6d955e13d
Trigger completion when a background validation chainstate reaches the
same height as a UTXO snapshot, and handle cleaning up the chainstate
on subsequent startup.
This is only used in the current PR to avoid ugly
`strprintf(Untranslated("%s:\n%s"), str, MakeUnorderedList(details)`
boilerplate in init code. But in the future the function could be extended and
more widely used to include more details in GUI error messages or display them
in a more readable way, see code comment.
3141eab9c6 test: add functional test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
e252909e56 test: add unit test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
77557dda4a prune: scan and unlink already pruned block files on startup (Andrew Toth)
Pull request description:
There are a few cases where we can mark a block and undo file as pruned in our block index, but not actually remove the files from disk.
1. If we call `FindFilesToPrune` or `FindFilesToPruneManual` and crash before `UnlinkPrunedFiles`.
2. If on Windows there is an open file handle to the file somewhere else when calling `fs::remove` in `UnlinkPrunedFiles` (https://en.cppreference.com/w/cpp/filesystem/remove, https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-deletefilew#remarks). This could be from another process, or if we are calling `ReadBlockFromDisk`/`ReadRawBlockFromDisk` without having a lock on `cs_main` (which has been allowed since ccd8ef65f9).
This PR mitigates this by scanning all pruned block files on startup after `LoadBlockIndexDB` and unlinking them again.
ACKs for top commit:
achow101:
ACK 3141eab9c6
pablomartin4btc:
re-ACK with added functional test 3141eab9c6.
furszy:
Code review ACK 3141eab9
theStack:
Code-review ACK 3141eab9c6
Tree-SHA512: 6c73bc57838ad1b7e5d441af3c4d6bf4c61c4382e2b86485e57fbb74a61240710c0ceeceb8b4834e610ecfa3175c6955c81ea4b2285fee11ca6383f472979d8d
0af16e7134 doc: add release note for #25574 (Martin Zumsande)
57ef2a4812 validation: report if pruning prevents completion of verification (Martin Zumsande)
0c7785bb25 init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache (Martin Zumsande)
d6f781f1cf validation: return VerifyDBResult::INTERRUPTED if verification was interrupted (Martin Zumsande)
6360b5302d validation: Change return value of VerifyDB to enum type (Martin Zumsande)
Pull request description:
`VerifyDB()` can fail to complete due to insufficient dbcache at the level 3 checks. This PR improves the error handling in this case in the following ways:
- The rpc `-verifychain` now returns false if the check can't be completed due to insufficient cache
- During init, we only log a warning if the default values for `-checkblocks` and `-checklevel` are taken and the check doesn't complete. However, if the user actively specifies one of these args, we return with an InitError if we can't complete the check.
This PR also changes `-verifychain` RPC to return `false` if the verification didn't finish due to missing block data (pruning) or due to being interrupted by the node being shutdown.
Previously, this PR also included a fix for a possible assert during verification - this was done in #27009 (now merged).
ACKs for top commit:
achow101:
ACK 0af16e7134
ryanofsky:
Code review ACK 0af16e7134. Only small suggested changes since the last review, like renaming some of the enum values. I did leave more suggestions, but they are not very important and could be followups
john-moffett:
ACK 0af16e7134
MarcoFalke:
lgtm re-ACK 0af16e7134 🎚
Tree-SHA512: 84b4f767cf9bfbafef362312757c9bf765b41ae3977f4ece840e40c52a2266b1457832df0cdf70440be0aac2168d9b58fc817238630b0b6812f3836ca950bc0e
Moves chainstate initialization into its own function. This is
necessary to later support a more readable way of handling
background-validation chainstate cleanup during init, since the
chainstate initialization functions may need to be repeated after
moving leveldb filesystem content around.
This commit isn't strictly necessary, but the alternative is to (ab)use
the `while` loop in init.cpp with a `continue` on the basis of a
specific ChainstateLoadingError return value from LoadChainstate. Not
only is this harder to read, but it can't be unittested.
The approach here lets us consolidate background-validation cleanup to
LoadChainstate, and therefore exercise it within tests.
This commit is most easily reviewed with
git diff --color-moved=dimmed-zebra
--color-moved-ws=ignore-space-change
6a5e88e5cf miner: don't re-apply default Options value if argument is unset (stickies-v)
ea72c3d9d5 refactor: avoid duplicating BlockAssembler::Options members (stickies-v)
cba749a9b7 refactor: rename local gArgs to args (stickies-v)
Pull request description:
Two follow-ups for #26695, both refactoring and no observed (*) behaviour change:
- Rename `gArgs` to `args` because it's not actually a global
- Add `BlockAssembler::Options` as a (private) member to `BlockAssembler` to avoid having to assign all the options individually, essentially duplicating them
Reduces LoC and makes the code more readable, in my opinion.
---
(*) as [pointed out by ajtowns](https://github.com/bitcoin/bitcoin/pull/26883#discussion_r1068247937), this PR changes the interface of `ApplyArgsManOptions()`, making this not a pure refactoring PR. In practice, `ApplyArgsManOptions()` is never called in such a way that this leads to observed behaviour change. Regardless, I've carved out the potential behaviour change into a separate commit and would be okay with dropping it, should it turn out to be controversial.
ACKs for top commit:
glozow:
ACK 6a5e88e5cf
TheCharlatan:
Light code review ACK 6a5e88e5cf
Tree-SHA512: 15c30442ff0e070b1a58dc4c9615550d619ce35b4a2596b2c0a9d790259bbf987cab708f7cbb1057a8cf8b4c3226f3ad981282d3499ac442094806492a5f68ce
The rpc command verifychain now fails if the dbcache was not sufficient
to complete the verification at the specified level and depth.
In the same situation, the VerifyDB check during Init will now fail (and lead to
an early shutdown) if the user has explicitly specified -checkblocks or
-checklevel but the check couldn't be executed because of the limited
cache. If the user didn't change any of the two and is using the defaults, log a warning
but don't prevent the node from starting up.
This does not change behavior. It is in preparation for
special handling of the case where VerifyDB doesn't finish
for various reasons, but doesn't fail.
Use DBParams struct to remove ArgsManager uses from txdb.
To reduce size of this commit, this moves references to gArgs variable out of
txdb.cpp to calling code in chainstate.cpp. But these moves are temporary. The
gArgs references in chainstate.cpp are moved out to calling code in init.cpp in
later commits.
This commit does not change behavior.
Add CoinsViewOptions struct to remove ArgsManager uses from txdb.
To reduce size of this commit, this moves references to gArgs variable out of
txdb.cpp to calling code in validation.cpp. But these moves are temporary. The
gArgs references in validation.cpp are moved out to calling code in init.cpp in
later commits.
This commit does not change behavior.
Add DBParams and DBOptions structs to remove ArgsManager uses from dbwrapper.
To reduce size of this commit, this moves references to gArgs variable out of
dbwrapper.cpp to calling code in txdb.cpp. But these moves are temporary. The
gArgs references in txdb.cpp are moved out to calling code in init.cpp in later
commits.
This commit does not change behavior.
faba08b5b4 refactor: Remove stray cs_main redundant declaration (MarcoFalke)
fa02591edf doc: Export threadsafety.h from sync.h (MarcoFalke)
Pull request description:
Looks like this was forgotten when introducing kernel/cs_main ?
Also, there is a commit to export threadsafety.h from sync.h.
ACKs for top commit:
hebasto:
ACK faba08b5b4
Tree-SHA512: 0aa58e7693b6fcd504f9da7339f8baa463a6407f67b27f68002db705f4642321ac3765f16c3d906c925ee24085591b79160a62fa5f4aaf6f2e5dcc788411800d
eeee61065f Use AutoFile and HashVerifier where possible (MarcoFalke)
fa961141f7 Add HashVerifier (MarcoFalke)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `AutoFile` and `HashVerifier`. `CAutoFile` and `CHashVerifier` remain in places where it is not yet possible.
ACKs for top commit:
stickies-v:
ACK eeee61065f
Tree-SHA512: 93786778c309ecfdc1ed43552d24ff9d966954d69a47f66faaa6de24daacd25c651f3f62bde5abbb362700298fb3c04ffbd3207a0dd13d0bd8bff7fd6d07dcf8
ApplyArgsManOptions does not need to set default values for missing
arguments, these are already defined in the BlockAssembler::Options.
This commit changes the interface of ApplyArgsManOptions(). If
ApplyArgsManOptions() is called again after a option is changed,
this option will no longer be reset to the default value.
There is no observed behaviour change due to how
ApplyArgsManOptions() is currently used, and the new interface is
consistent with e.g. ValidationCacheSizes and MemPoolLimits.
Add Options as a member to BlockAssembler to avoid having to assign
all the options individually.
Additionally brings the struct more in line with how we typically
define default and ArgManager values, as e.g. with
ChainstateManager::Options and and CTxMemPool::Options
