Preparation for a future commit where kernel's dependency
on logging.cpp is removed completely.
Replace usage of logging\.h with util/log\.h where it
suffices, and fix wrong includes according to iwyu.
5646e6c0d3581f12419913b88745f51c7a3161b9 index: restrict index helper function to namespace (Martin Zumsande)
032f3503e3fe8e144640904670867afc18a4bbbf index, refactor: deduplicate LookUpOne (Martin Zumsande)
a67d3eb91d5ef840fc49167d7048a33872ecddf8 index: deduplicate Hash / Height handling (Martin Zumsande)
Pull request description:
The logic for `DBHashKey` / `DBHeightKey` handling and lookup of entries is shared by `coinstatsindex` and `blockfilterindex`, leading to many lines of duplicated code. De-duplicate this by moving the logic to `index/db_key.h` (using templates for the index-specific `DBVal`).
ACKs for top commit:
fjahr:
re-ACK 5646e6c0d3581f12419913b88745f51c7a3161b9
furszy:
utACK 5646e6c0d3581f12419913b88745f51c7a3161b9
sedited:
ACK 5646e6c0d3581f12419913b88745f51c7a3161b9
Tree-SHA512: 6f41684d6a9fd9bb01239e9f2e39a12837554f247a677eadcc242f0c1a2d44a79979f87249c4e0305ef1aa708d7056e56dfc40e1509c6d6aec2714f202fd2e09
The code was largely duplicated between coinstatsindex
and blockfilterindex.
Deduplicate it by moving it to a shared file.
slight change in behavior: the index name is no longer
part of the error msg in case of (un)serialization errors.
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
The std::move in coinstatsindex was not necessary since it was passed as a const reference argument.
The other change in the utxo supply fuzz test changes a line that seems to have triggered a false alarm.
The coinstatsindex currently looks for block data at a hash key if the prev block in CustomAppend is different than expected. This is not needed since base index should always prevent us ending up in this scenario since it should rewind the index before calling CustomAppend in this case. But even if we run into this and our belt-and-suspenders code is getting hit, the index could not recover properly from the hash key index data so it can be removed without any real impact.
This is practically irrelevant due to the unlikeliness of a re-org
reaching so deep that it would drop the BIP30 blocks from the chain
(91842 and 91880). However this serves as documentation and ensures that
the functions RevertBlock and CustomAppend are consistent.
The index originally stored cumulative values in a CAmount type but this allowed for
potential overflow issues which were observed on Signet. Fix this by
storing the values that are in danger of overflowing in a arith_uint256.
Also turns an unnecessary copy into a reference in RevertBlock and
CustomAppend and gets
rid of the explicit total unspendable tracking which can be calculated
by adding the four categories of unspendables together.
`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.
- don't log function name
- take into account that GetName() always ends with " index"
- replace deprecated LogPrintf with LogInfo
- remove trailing \n
- adjusted log level where needed
Moved CBlockUndo disk read lookups from child index classes to
the base index class.
The goal is for child index classes to synchronize only through
events, without directly accessing the chain database.
This change will enable future parallel synchronization mechanisms,
reduce database access (when batched), and contribute toward the
goal of running indexes in a separate process (with no chain
database access).
Besides that, this commit also documents how NextSyncBlock() behaves.
It is not immediately clear this function could return the first
block after the fork point during a reorg.
Move ReadBlock code from CoinStatsIndex::CustomRewind to BaseIndex::Rewind
Move ReadUndo code from CoinStatsIndex::ReverseBlock to BaseIndex::Rewind
This commit does change behavior slightly. Since the new CustomRemove
methods only take a single block at a time instead of a range of
disconnected blocks, when they call CopyHeightIndexToHashIndex they will
now do an index seek for each removed block instead of only seeking once
to the height of the earliest removed block. Seeking instead of scanning
is a little worse for performance if there is a >1 block reorg, but
probably not noticeable unless the reorg is very deep.
This fixes the log output when -logsourcelocations is used.
Also, instead of 'ERROR:', the log will now say '[error]', like other
errors logged with LogError.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's! error\("([^"]+)"! LogError("\1\\n"!g' $( git grep -l ' error(' ./src/ )
-END VERIFY SCRIPT-
This is needed for the next commit.
-BEGIN VERIFY SCRIPT-
# Separate sed invocations to replace one-line, and two-line error(...) calls
sed -i --regexp-extended 's!( +)return (error\(.*\);)!\1\2\n\1return false;!g' $( git grep -l 'return error(' )
sed -i --null-data --regexp-extended 's!( +)return (error\([^\n]*\n[^\n]*\);)!\1\2\n\1return false;!g' $( git grep -l 'return error(' )
-END VERIFY SCRIPT-
The legacy serialization was vulnerable to maleation and is fixed by
adopting the same serialization procedure as was already in use for
MuHash.
This also includes necessary test fixes where the hash_serialized2 was
hardcoded as well as correction of the regtest chainparams.
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
During a reorg, continuing execution when a block cannot be
reversed leaves the coinstats index in an inconsistent state,
which was surely overlooked when 'CustomRewind' was implemented.
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 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.
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.
Making the checks to identify BIP30 available outside of validation.cpp is needed for reporting and tracking statistics on specific blocks and the UTXO set correctly.
faf98aecf876fae0ec6d4d16b7e66f3a35253180 Remove unused includes in rpc/fees.cpp (MacroFake)
1111ddeedf7ea801507db4e23b4737ec183eb19c Remove unused includes from dbwrapper.h (MacroFake)
fa77fdd0475fa15a1a3641c5d5a2bf7ad095aa84 Add missing includes (MacroFake)
fa869ce2c2b906d8b087c4e7a5f1804a74b1c522 Add missing includes to node/chainstate (MacroFake)
Pull request description:
Unused includes are confusing, but also cause unrelated compile errors when the unused includes were to be removed.
Fix that by adding the missing includes where they are needed and then remove them where they are not needed. This is also checked by iwyu.
ACKs for top commit:
hebasto:
ACK faf98aecf876fae0ec6d4d16b7e66f3a35253180, I have reviewed the code and it looks OK, I agree it can be merged.
jarolrod:
Code Review ACK faf98aecf8
Tree-SHA512: 75f3c6e6f6ecf8a98233e1a1463c75ca4e0eb3ec341150d274141072fe95413a3c2ec6386d1c527899cc63d43f63f5eb5991509847412773362808ddfb1bb435
Replace CommitInternal method with CustomCommit and use interfaces::Chain
instead of CChainState to generate block locator.
This commit does not change behavior in any way, except in the
(m_best_block_index == nullptr) case, which was added recently in
https://github.com/bitcoin/bitcoin/pull/24117 as part of an ongoing attempt to
prevent index corruption if bitcoind is interrupted during startup. New
behavior in that case should be slightly better than the old behavior (skipping
the entire custom+base commit now vs only skipping the base commit previously)
and this might avoid more cases of corruption.
Replace Rewind method with CustomRewind and pass block hashes and
heights instead of CBlockIndex* pointers
This commit does not change behavior in any way.
Replace overriden index Init() methods that use the best block
CBlockIndex* pointer with pure CustomInit() callbacks that are passed
the block hash and height.
This gets rid of more CBlockIndex* pointer uses so indexes can work
outside the bitcoin-node process. It also simplifies the initialization
call sequence so index implementations are not responsible for
initializing the base class.
There is a slight change in behavior here since now the best block
pointer is loaded and checked before the custom index init functions are
called instead of while they are called.
The indexing codepath logic in node/coinstats.cpp is simple enough to be
moved into CoinStatsIndex::LookUpStats, avoiding an additional layer of
function calls. Callers are modified accordingly.
Also, add 2 missed BOOST_CHECKs to the coinstatsindex_initial_sync unit
test.
