6da6f503a6dd8de85780ca402f5202095aa8b6ea refactor: Let CCoinsViewCache::BatchWrite return void (TheCharlatan)
Pull request description:
CCoinsViewCache::BatchWrite always returns true if called from a backed cache, so just return void instead. Also return void from ::Sync and ::Flush.
This allows for dropping a FatalError condition and simplifying some dead error handling code a bit.
Since we now no longer exercise the "error path" when returning from `CCoinsView::BatchWrite`, make the method clear the cache instead. This should only be exercised by tests and not change production behaviour. This might slightly improve the coins_view fuzz test's ability to generate better coverage.
ACKs for top commit:
l0rinc:
ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea
andrewtoth:
re-ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea
achow101:
ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea
w0xlt:
ACK 6da6f503a6
Tree-SHA512: dfaa325b0cf8108910aebf1b27434aaddb639d10d860e96797c77ea42eca9035a54a7dc1d6a5d4eae2b75fcc9356206d3d5672243d2c906e80d19024c8b95408
CCoinsViewCache::BatchWrite always returns true if called from a backed
cache, so just return void instead. Also return void from ::Sync and
::Flush.
This allows for dropping a FatalError condition and simplifying some
dead error handling code a bit.
Since we now no longer exercise the "error path" when returning from
`CCoinsView::BatchWrite`, make the method clear the cache instead. This
should only be exercised by tests and not change production behaviour.
This might slightly improve the coins_view fuzz test's ability to
generate better coverage.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
This is a minimal behavior change and changes log output from:
[net:error] Something bad happened
[net:warning] Something problematic happened
to either
[error] Something bad happened
[warning] Something problematic happened
or, when -loglevelalways=1 is enabled:
[all:error] Something bad happened
[all:warning] Something problematic happened
Such a behavior change is desired, because all warning and error logs
are written in the same style in the source code and they are logged in
the same format for log consumers.
-BEGIN VERIFY SCRIPT-
sed --regexp-extended --in-place \
's/LogPrintLevel\((BCLog::[^,]*), BCLog::Level::(Error|Warning), */Log\2(/g' \
$( git grep -l LogPrintLevel ':(exclude)src/test/logging_tests.cpp' )
-END VERIFY SCRIPT-
743abbcbde9e5a2db489bca461c98df461eff7d0 refactor: inline constant return value of `BlockTreeDB::WriteBatchSync` and `BlockManager::WriteBlockIndexDB` and `BlockTreeDB::WriteFlag` (Lőrinc)
e030240e909493549e24aa8bcd5b382cab6e2c79 refactor: inline constant return value of `CDBWrapper::Erase` and `BlockTreeDB::WriteReindexing` (Lőrinc)
cdab9480e9e35656f490878f92dab5427b36f21d refactor: inline constant return value of `CDBWrapper::Write` (Lőrinc)
d1847cf5b5af232ad180f5d302361b72334952b2 refactor: inline constant return value of `TxIndex::DB::WriteTxs` (Lőrinc)
50b63a5698e533376ef7a20bc0c440d3d6bf7a9f refactor: inline constant return value of `CDBWrapper::WriteBatch` (Lőrinc)
Pull request description:
Related to https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223587480
### Summary
`WriteBatch` always returns `true` - the errors are handled by throwing `dbwrapper_error` instead.
### Context
This boolean return value of the `Write` methods is confusing because it's inconsistent with `CDBWrapper::Read`, which catches exceptions and returns a boolean to indicate success/failure. It's bad that `Read` returns and `Write` throws - but it's a lot worse that `Write` advertises a return value when it actually communicates errors through exceptions.
### Solution
This PR removes the constant return values from write methods and inlines `true` at their call sites. Many upstream methods had boolean return values only because they were propagating these constants - those have been cleaned up as well.
Methods that returned a constant `true` value that now return `void`:
- `CDBWrapper::WriteBatch`, `CDBWrapper::Write`, `CDBWrapper::Erase`
- `TxIndex::DB::WriteTxs`
- `BlockTreeDB::WriteReindexing`, `BlockTreeDB::WriteBatchSync`, `BlockTreeDB::WriteFlag`
- `BlockManager::WriteBlockIndexDB`
### Note
`CCoinsView::BatchWrite` (and transitively `CCoinsViewCache::Flush` & `CCoinsViewCache::Sync`) were intentionally not changed here. While all implementations return `true`, the base `CCoinsView::BatchWrite` returns `false`. Changing this would cause `coins_view` tests to fail with:
> terminating due to uncaught exception of type std::logic_error: Not all unspent flagged entries were cleared
We can fix that in a follow-up PR.
ACKs for top commit:
achow101:
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
janb84:
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
TheCharlatan:
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
sipa:
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
Tree-SHA512: b2a550bff066216f1958d2dd9a7ef6a9949de518cc636f8ab9c670e0b7a330c1eb8c838e458a8629acb8ac980cea6616955cd84436a7b8ab9096f6d648073b1e
This change updates to IWYU 0.25, which is compatible with Clang 21.
Fixes new "modernize-use-default-member-init" warnings.
The warning in `interpreter.cpp` is a false positive, so it has been
suppressed.
`WriteBatch` can only ever return `true` - its errors are handled by throwing a `throw dbwrapper_error` instead.
The boolean return value is quite confusing, especially since it's symmetric with `CDBWrapper::Read`, which catches the exceptions and returns a boolean instead.
We're removing the constant return value and inlining `true` for its usages.
Remove the manual batch size estimation logic (`SizeEstimate()` method and `size_estimate` member) from `CDBBatch`.
Size is now determined solely by the `ApproximateSize()` method introduced in the previous commit, which delegates to the native LevelDB function.
The manual calculation is no longer necessary as LevelDB now provides this functionality directly, and the previous commit verified that the native function's results matched the manual estimation.
Assertions comparing the two methods are removed from `txdb.cpp`.
Co-authored-by: Wladimir J. van der Laan <laanwj@protonmail.com>
Serialized batch size can be queried via the underlying LevelDB implementation calling the native `leveldb::WriteBatch::ApproximateSize()`.
The previous manual calculation was added in e66dbde6d1 as part of https://github.com/bitcoin/bitcoin/pull/10195. At that time (April 2017), the version of LevelDB used by Bitcoin Core (and even the latest source) lacked a native function for this. LevelDB added this capability in 69e2bd224b, merged later that year.
The old manual estimation method (`SizeEstimate()`) is kept temporarily in this commit, and assertions are added in `txdb.cpp` to verify its results against `ApproximateSize()` during batch writes. This ensures the native function behaves as expected before removing the manual calculation in the subsequent commit.
fae405556d56f6f13ce57f69a06b9ec1e825422b scripted-diff: Rename CBlockTreeDB -> BlockTreeDB (MarcoFalke)
faf63039cce40f5cf8dea5a1d24945773c3433a1 Fixup style of moved code (MarcoFalke)
fa65111b99627289fd47dcfaa5197e0f09b8a50e move-only: Move CBlockTreeDB to node/blockstorage (MarcoFalke)
fa8685597e7302fc136f21b6dd3a4b187fa8e251 index: Drop legacy -txindex check (MarcoFalke)
fa69148a0a26c5054dbccdceeac8e117bf449275 scripted-diff: Use blocks_path where possible (MarcoFalke)
Pull request description:
The only reason for the check was to print a warning about an increase in storage use. Now that 22.x is EOL and everyone should have migrated (or decided to not care about storage use), remove the check.
Also, a move-only commit is included. (Rebased from https://github.com/bitcoin/bitcoin/pull/22242)
ACKs for top commit:
TheCharlatan:
ACK fae405556d56f6f13ce57f69a06b9ec1e825422b, though I lack historical context to really judge the second commit fa8685597e7302fc136f21b6dd3a4b187fa8e251.
stickies-v:
ACK fae405556d56f6f13ce57f69a06b9ec1e825422b
Tree-SHA512: 9da8f48767ae52d8e8e21c09a40c949cc0838794f1856cc5f58a91acd3f00a3bca818c8082242b3fdc9ca5badb09059570bb3870850d3807b75a8e23b5222da1
The block index (CBlockTreeDB) is required to write and read blocks, so
move it to blockstorage. This allows to drop the txdb.h include from
`node/blockstorage.h`.
Can be reviewed with:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
This and the following commit seek to decouple the libbitcoinkernel
library from the shutdown code. As a library, it should it should have
its own flexible interrupt infrastructure without relying on node-wide
globals.
The commit takes the first step towards this goal by de-globalising
`ShutdownRequested` calls in kernel code.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
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.
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.
In certain circumstances, we may want to flush to disk without
emptying `cacheCoins`, which affects performance. UTXO snapshot
activation is one such case.
This method is currently unused and this commit does not
change any behavior.
Incorporates feedback from John Newbery.
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
and mark the inherited CBlockIndex#GetBlockHash public interface member
as deleted, to disallow calling it in the derived CDiskBlockIndex class.
Here is a failing test on master demonstrating the inconsistent behavior of the
current design: calling the same inherited public interface functions on the
same CDiskBlockIndex object should yield identical behavior.
```diff
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 6dc522b421..dac3840f32 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -240,6 +240,15 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
const CBlockIndex* tip = chainman.ActiveTip();
BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx);
+ // CDiskBlockIndex "is a" CBlockIndex, as it publicly inherits from it.
+ // Test that calling the same inherited interface functions on the same
+ // object yields identical behavior.
+ CDiskBlockIndex index{tip};
+ CBlockIndex *pB = &index;
+ CDiskBlockIndex *pD = &index;
+ BOOST_CHECK_EQUAL(pB->GetBlockHash(), pD->GetBlockHash());
+ BOOST_CHECK_EQUAL(pB->ToString(), pD->ToString());
+
```
The GetBlockHash() test assertion only passes on master because the different
methods invoked by the current design happen to return the same result. If one
of the two is changed, it fails like the ToString() assertion does.
Redefining inherited non-virtual functions is well-documented as incorrect
design to avoid inconsistent behavior (see Scott Meyers, "Effective C++", Item
36). Class usage is confusing when the behavior depends on the pointer
definition instead of the object definition (static binding happening where
dynamic binding was expected). This can lead to unsuspected or hard-to-track
bugs.
Outside of critical hot spots, correctness usually comes before optimisation,
but the current design dates back to main.cpp and it may possibly have been
chosen to avoid the overhead of dynamic dispatch. This solution does the same:
the class sizes are unchanged and no vptr or vtbl is added.
There are better designs for doing this that use composition instead of
inheritance or that separate the public interface from the private
implementations. One example of the latter would be a non-virtual public
interface that calls private virtual implementation methods, i.e. the Template
pattern via the Non-Virtual Interface (NVI) idiom.
These methods haven't been used since the chainstate db cache has been
switched from per-tx to per-txout model years ago (PR #10195, commit
d342424301013ec47dc146a4beb49d5c9319d80a).
fa9112aac07dc371bfda437d40eb1b841f36f392 Remove utxo db upgrade code (MarcoFalke)
Pull request description:
It is not possible to upgrade Bitcoin Core pre-segwit (pre-0.13.1) to a recent version without a full IBD from scratch after commit 19a56d1519fb493c3e1bd5cad55360b6b80fa52b (released in version 22.0).
Any Bitcoin Core version with the new database format after commit 1088b02f0ccd7358d2b7076bb9e122d59d502d02 (released in version 0.15), can upgrade to any version that is supported as of today.
This leaves the versions 0.13.1-0.14.x. Even though those versions are unsupported, some users with an existing datadir may want to upgrade to a recent version. However, it seems reasonable to simply ask them to `-reindex` to run a full IBD from scratch. This allows us to remove the utxo db upgrade code.
ACKs for top commit:
Sjors:
re-ACK fa9112aac07dc371bfda437d40eb1b841f36f392
laanwj:
Code review ACK fa9112aac07dc371bfda437d40eb1b841f36f392
Tree-SHA512: 4243bb35df9ac4892f9fad30fe486d338745952bcff4160bcb0937c772d57b13b800647da14695e21e3655e85ee0d95fa3dc7789ee309d59ad84f422297fecb8
Improve readability of code, simplify future scripted diff cleanup PRs, and be
more consistent with naming for GetBoolArg.
This will also be useful for replacing runtime settings type checking
with compile time checking.
-BEGIN VERIFY SCRIPT-
git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g'
-END VERIFY SCRIPT-
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
We can't support a reset of the dbwrapper object when in-memory configuration is used
because it results in the permanent loss of coins. This only affects unittest
configurations (since that's the only place we use in-memory CCoinsViewDB instances).
f19fdd47a6371dcbe0760ef6f3c3c5adb31b1bb4 test: add test for CChainState::ResizeCoinsCaches() (James O'Beirne)
8ac3ef46999ed676ca3775f7b2f461d92f09a542 add ChainstateManager::MaybeRebalanceCaches() (James O'Beirne)
f36aaa6392fdbdac6891d92202d3efeff98754f4 Add CChainState::ResizeCoinsCaches (James O'Beirne)
b223111da2e0e9ceccef75df8a20252b0094b7bc txdb: add CCoinsViewDB::ChangeCacheSize (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
In the assumeutxo implementation draft (#15056), once a UTXO snapshot is loaded, a new chainstate object is created after initialization. This means that we have to reclaim some of the cache that we've allocated to the original chainstate (per `dbcache=`) to repurpose for the snapshot chainstate.
Furthermore, it makes sense to have different cache allocations depending on which chainstate is more active. While the snapshot chainstate is working to get to the network tip (and the background validation chainstate is idle), it makes sense that the snapshot chainstate should have the majority of cache allocation. And contrariwise once the snapshot has reached network tip, most of the cache should be given to the background validation chainstate.
This set of changes (detailed in the commit messages) allows us to dynamically resize the various coins caches. None of the functionality introduced here is used at the moment, but will be in the next AU PR (which introduces `ActivateSnapshot`).
`ChainstateManager::MaybeRebalanceCaches()` defines the (somewhat normative) cache allocations between the snapshot and background validation chainstates. I'd be interested in feedback if anyone has thoughts on the proportions I've set there.
ACKs for top commit:
ajtowns:
weak utACK f19fdd47a6371dcbe0760ef6f3c3c5adb31b1bb4 -- didn't find any major problems, but not super confident that I didn't miss anything
fjahr:
Code review ACK f19fdd4
ryanofsky:
Code review ACK f19fdd47a6371dcbe0760ef6f3c3c5adb31b1bb4. Only change since last review is constructor cleanup (no change in behavior). I think the suggestions here from ajtowns and others are good, but shouldn't delay merging the PR (and hold up assumeutxo)
Tree-SHA512: fffb7847fb6993dd4a1a41cf11179b211b0b20b7eb5f7cf6266442136bfe9d43b830bbefcafd475bfd4af273f5573500594aa41fff03e0ed5c2a1e8562ff9269
We'll need this to dynamically update the cache size of the existing
CCoinsViewDB instance when we create a new one during snapshot activation.
This requires us to keep the CDBWrapper instance as a pointer instead of
a reference so that we're able to destruct it and create a new instance
when the cache size changes.
Also renames `db` to `m_db` since we're already modifying each usage.
Includes feedback from Russ Yanofsky.
