With respect to `std::unordered_map` documentation, the `Hash` type
defined in the template is over the `Key` and not `T`, the value. This
hasher is incorrectly named as the `FilterHeader` is the value within this map.
I consider this a bug as opposed to a refactor as the key and value
relationship is implied to be `filter header -> block hash` when it is
the opposite.
Further, the hasher for the key already exists via `BlockHasher`.
ref: https://en.cppreference.com/w/cpp/container/unordered_map.html
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
This should avoid having to include interfaces/chain.h from a kernel
module. interfaces/chain.h in turn includes a bunch of non-kernel
headers, that break the desired library topology and might introduce
entanglement regressions.
Change ChainstateRole parameter passed to wallets and indexes. Wallets and
indexes need to know whether chainstate is historical and whether it is fully
validated. They should not be aware of the assumeutxo snapshot validation
process.
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.
fa6c0bedd33ac7ad27454adaf9522fd27bef6ea3 refactor: Return uint64_t from GetSerializeSize (MarcoFalke)
fad0c8680ea7ef433c2d6e7c0d5799f81fd861b9 refactor: Use uint64_t over size_t for serialized-size values (MarcoFalke)
fa4f388fc99c9ec7c3cf2bac3863c7b3004bb2ae refactor: Use fixed size ints over (un)signed ints for serialized values (MarcoFalke)
fa01f38e53cfda4155d0ea09ca8b1291b7001fe8 move-only: Move CBlockFileInfo to kernel namespace (MarcoFalke)
fa2bbc9e4cfe017436a5167ab5c443f4412efa3c refactor: [rpc] Remove cast when reporting serialized size (MarcoFalke)
fa364af89bd914ea7cd0d4a5470e0a502e0a2075 test: Remove outdated comment (MarcoFalke)
Pull request description:
Consensus code should arrive at the same conclusion, regardless of the architecture it runs on. Using architecture-specific types such as `size_t` can lead to issues, such as the low-severity [CVE-2025-46597](https://bitcoincore.org/en/2025/10/24/disclose-cve-2025-46597/).
The CVE was already worked around, but it may be good to still fix the underlying issue.
Fixes https://github.com/bitcoin/bitcoin/issues/33709 with a few refactors to use explicit fixed-sized integer types in serialization-size related code and concluding with a refactor to return `uint64_t` from `GetSerializeSize`. The refactors should not change any behavior, because the CVE was already worked around.
ACKs for top commit:
Crypt-iQ:
crACK fa6c0bedd33ac7ad27454adaf9522fd27bef6ea3
l0rinc:
ACK fa6c0bedd33ac7ad27454adaf9522fd27bef6ea3
laanwj:
Code review ACK fa6c0bedd33ac7ad27454adaf9522fd27bef6ea3
Tree-SHA512: f45057bd86fb46011e4cb3edf0dc607057d72ed869fd6ad636562111ae80fea233b2fc45c34b02256331028359a9c3f4fa73e9b882b225bdc089d00becd0195e
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 values are small enough to fit in size_t, but to avoid having to
think about it, just use uint64_t consistently for all architectures.
On 64-bit systems, this refactor is a no-op. On 32-bit systems, it could
avoid bugs in the theoretical and unexpected case where a 32-bit size_t
is too small and overflows.
Bitcoin Core already assumes that 'unsigned int' means uint32_t and
'signed int' means int32_t. See src/compat/assumptions.h. Also, any
serialized integral value must be of a fixed size.
So make the fixed size explicit in this documenting refactor, which does
not change the behavior on any platform.
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.
a602f6fb7bf5f9e57299f4d6e246c82379fad8d2 test: index with an unclean restart after a reorg (Martin Zumsande)
01b95ac6f496e24e525b2fc9d69ee8b543da65ff index: don't commit state in BaseIndex::Rewind (Martin Zumsande)
Pull request description:
The committed state of an index should never be ahead of the flushed chainstate.
Otherwise, in the case of an unclean shutdown, the blocks necessary to revert
from the prematurely committed state are not be available, which would corrupt the coinstatsindex in particular.
Instead, the index state will be committed with the next ChainStateFlushed notification.
Fixes#33208
ACKs for top commit:
achow101:
ACK a602f6fb7bf5f9e57299f4d6e246c82379fad8d2
stickies-v:
re-ACK a602f6fb7bf5f9e57299f4d6e246c82379fad8d2
Tree-SHA512: 2559ea3fe066caf746a54ad7daac5031332f3976848e937c3dc8b35fa2ce925674115d8742458bf3703b3916f04f851c26523b6b94aeb1da651ba5a1b167a419
The committed state of an index should never
be ahead of the flushed chainstate. Otherwise, in the case
of an unclean shutdown, the blocks necessary to revert
from the prematurely committed state would not be
available, which would corrupt the coinstatsindex in particular.
Instead, the index state will be committed with the next
ChainStateFlushed notification.
3aef38f44b76dfda77f47dc1a0e1fdc6ff3c7766 test: exercise index reorg assertion failure (furszy)
acf50233cdfbb336c87d95d97db90a149e131052 index: fix wrong assert of current_tip == m_best_block_index (Hao Xu)
Pull request description:
In BaseIndex::Sync(), pindex in `Rewind(pindex, pindex_next->pprev)` isn't always equal to m_best_block_index since m_best_block_index is updated every SYNC_LOCATOR_WRITE_INTERVAL seconds, during which multiple pindex update could happen. Thus the assert here is wrong.
ACKs for top commit:
achow101:
ACK 3aef38f44b76dfda77f47dc1a0e1fdc6ff3c7766
furszy:
ACK 3aef38f
mzumsande:
Code Review ACK 3aef38f44b76dfda77f47dc1a0e1fdc6ff3c7766
Tree-SHA512: 3ef9cc6dfdec10a9f95d7414c6a11aa216e4cf5974440d80ab19fc919abd2a3bd4c875718c9dc94523c33826f8582ec5a016374deb8fb2d35cd2fb7799b5c82e
`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.
Moves the file from `src/util` to `src/primitives`. Now that the
refactor is complete, Txid and Wtxid are fundamental types, so it
makes sense for them to reside in `src/primitives`.
These remaining miscellaneous changes were identified by commenting out
the `operator const uint256&` conversion and the `Compare(const uint256&)`
method from `transaction_identifier.h`.
In BaseIndex::Sync(), pindex in `Rewind(pindex, pindex_next->pprev)` isn't always
equal to m_best_block_index since m_best_block_index is updated every
SYNC_LOCATOR_WRITE_INTERVAL seconds, during which multiple pindex update could
happen. Thus the assert here is wrong.
Signed-off-by: Hao Xu <hao.xu@linux.dev>
- 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
There is no way to report a close error from `AutoFile` destructor.
Such an error could be serious if the file has been written to because
it may mean the file is now corrupted (same as if write fails).
So, change all users of `AutoFile` that use it to write data to
explicitly close the file and handle a possible error.
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.
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs.
They are also wrong, i.e DEFAULT_SCRIPTCHECK_THREADS is not in
validation.h.
This makes code more consistent and makes it easier to add compile-time checking to
enforce that format strings contain the right specifiers, because it stops
using Untranslated() to create the format string, so the Untranslated()
function will not need to get involved in formatting.
-BEGIN VERIFY SCRIPT-
quote='"[^"]+"'
quotes="(?:$quote|\\s)*"
nonparens="[^()]*"
single_level_paren="\($nonparens\)"
double_level_paren="\($nonparens\($nonparens\)$nonparens\)"
exprs="(?:$double_level_paren|$single_level_paren|$nonparens)*"
git grep -l 'Untranslated' | xargs perl -0777 -i -pe "s/strprintf\((\\W*)Untranslated\(($quotes)\)($exprs)(\))/Untranslated(\1strprintf(\2\3))/gs"
-END VERIFY SCRIPT-
Instead of having a single NodeContext::shutdown member that is used both to
request shutdowns and check if they have been requested, use separate members
for each. Benefits of this change:
1. Should make code a little clearer and easier to search because it is easier
to see which parts of code are triggering shutdowns and which parts are just
checking to see if they were triggered.
2. Makes it possible for init.cpp to specify additional code to run when a
shutdown is requested, like signalling the m_tip_block_cv condition variable.
Motivation for this change was to remove hacky NodeContext argument and
m_tip_block_cv access from the StopRPC function, so StopRPC can just be
concerned with RPC functionality, not other node functionality.
a240e150e837b5a95ed19765a2e8b7c5b6013f35 streams: remove AutoFile::Get() entirely (Pieter Wuille)
e624a9bef16b6335fd119c10698352b59bf2930a streams: cache file position within AutoFile (Pieter Wuille)
Pull request description:
Fixes#30833.
Instead of relying on frequent `ftell` calls (which appear to cause a significant slowdown on some systems) in XOR-enabled `AutoFile`s, cache the file position within `AutoFile` itself.
ACKs for top commit:
achow101:
ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35
davidgumberg:
untested reACK a240e150e8
theStack:
Code-review ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35
Tree-SHA512: fd3681edc018afaf955dc7a41a0c953ca80d46c1129e3c5b306c87c95aae93b2fe7b900794eb8b6f10491f9211645e7939918a28838295e6873eb226fca7006f
