Rename the `CCoinsViewDB` async compaction wrapper to `CompactFullAsync()` so it is distinct from the blocking `CDBWrapper::CompactFull()` primitive it calls.
d6359937bf validation: check invariants when inserting into m_blocks_unlinked (stratospher)
0852925bd8 test/doc: remove misleading comment and improve tests (stratospher)
ca4a380281 test: add coverage for UB caused by FindMostWorkChain (stratospher)
c787b3b99b validation: avoid duplicates in m_blocks_unlinked (stratospher)
Pull request description:
This is joint work with @ mzumsande.
note: this requires a pruned node with deep reorgs to trigger. still it breaks assumptions in the codebase and is good to fix. A similar UB was fixed in https://github.com/bitcoin/bitcoin/pull/34521.
This PR prevents duplicate insertions into `m_blocks_unlinked` in `FindMostWorkChain`. There are 3 ways to insert into `m_blocks_unlinked`:
1. `LoadBlockIndex` - not problematic, as each block index is processed only once.
2. `ReceivedBlockTransactions` - not problematic, as this is usually only called once per block when it is first accepted in `AcceptBlock`. in the rare case it’s triggered again after pruning, the block would have been removed from `m_blocks_unlinked` when it was initially pruned, so duplicates still can’t arise.
3. `FindMostWorkChain` - problematic when multiple candidate tips share common chains of ancestors, traversals from each tip to the fork point may insert duplicate (`pprev`, `pindex`) entries for blocks whose parents have been pruned.
When the missing parent is later received and `ReceivedBlockTransactions` processes `m_blocks_unlinked`, the same entry may be processed multiple times. This can result in the block being re-added to `setBlockIndexCandidates` with a modified `nSequenceId`, violating its ordering invariants and leading to undefined behavior. So avoid duplicate insertions into `m_blocks_unlinked` in `FindMostWorkChain`.
### how to test:
use the updated `feature_pruning.py` which adds coverage for this scenario.
- on master: the test (with the below diff) fails since `nSequenceId` is being modified for an entry in `setBlockIndexCandidates`
- on this branch: the test (with the below diff) passes
```diff --git a/src/validation.cpp b/src/validation.cpp
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3814,6 +3814,12 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd
pindex->nHeight, pindex->m_chain_tx_count, prev_tx_sum(*pindex), CLIENT_NAME, FormatFullVersion(), CLIENT_BUGREPORT);
}
pindex->m_chain_tx_count = prev_tx_sum(*pindex);
+ for (const auto& c : m_chainstates) {
+ if (c->setBlockIndexCandidates.contains(pindex)) {
+ LogInfo("### pindex UB = %s", pindex);
+ assert(false);
+ }
+ }
pindex->nSequenceId = nBlockSequenceId++;
for (const auto& c : m_chainstates) {
c->TryAddBlockIndexCandidate(pindex);
```
ACKs for top commit:
sedited:
Re-ACK d6359937bf
marcofleon:
crACK d6359937bf
stringintech:
ACK d6359937bf
mzumsande:
Sure - Code Review ACK [d635993](d6359937bf)
Tree-SHA512: bb21adc2d92fe1865bbbcebf775a850ca3eccac6fe83d7bca10b78eee4c0abf782e44fa0ddfec9d9a70f42fa40bdc49b68a1d1b4905cdc371ba29117d3120619
Full chainstate compaction can take minutes on large databases.
Move `CCoinsViewDB::CompactFull()` to a named `utxocompact` one-shot background thread so validation only schedules the work.
When validation selects compaction after a full flush, the chainstate was just written and another write is less likely to be needed immediately.
The coins view destructor waits for completion, and a mutex prevents compaction from using `m_db` while `ResizeCache()` replaces it.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Full chainstate flushes are convenient maintenance points for long-term LevelDB cleanup because the chainstate was just written.
Randomize the trigger so nodes that flush near the same height do not compact together.
Add blocking chainstate compaction through `CCoinsViewDB::CompactFull()` and give each post-IBD full flush on the normal chainstate a 1/320 chance to start compaction.
With hourly flushes this averages roughly every two weeks and makes a six-month miss about one in a million.
This keeps the schedule stateless and leaves last-compaction height or timestamp bookkeeping out of chainstate metadata.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
For an entry A -> B in m_blocks_unlinked, the entry B was added into
m_blocks_unlinked either because:
- some ancestor of B was never received (or)
- some ancestor of B was pruned away.
Every insert must satisfy two invariants:
1. B has BLOCK_HAVE_DATA set.
2. No duplicate A -> B entries in m_blocks_unlinked (this is UB zone if
this entry gets popped twice in ReceivedBlockTransactions and
happens to be in setBlockIndexCandidates)
2 bugs (#35070 and #35168) discovered recently stemmed from the
m_blocks_unlinked insertion sites not enforcing these invariants.
So add a helper which wraps around insertion sites of m_blocks_unlinked
with these invariants.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
In a pruned node undergoing a deep reorg, FindMostWorkChain can
insert duplicate entries into m_blocks_unlinked.
This can happen when:
- Traversing from one candidate tip to the fork point adds blocks
whose parents have been pruned.
- Traversing from another candidate tip over the same fork inserts
the same pairs again, since the blocks are shared across
both branches.
When we finally download the missing parent from our peer and call
ReceivedBlockTransactions to process m_blocks_unlinked, the same
entry may be processed multiple times.
This can lead to re-insertion into setBlockIndexCandidates with
a modified nSequenceId, violating its ordering invariants and
causing undefined behavior. So avoid duplicate insertions into
m_blocks_unlinked here.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
ec6cf49b91 blockstorage: Remove cs_LastBlockFile recursive mutex (sedited)
Pull request description:
The `cs_LastBlockFile` mutex is redundant: all critical sections are already covered by cs_main. This is demonstrated in this patch by replacing all instances of locking `cs_LastBlockFile` with pairs of `AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)` annotations. No additional `::cs_main` LOCK(...)s are introduced (besides for test-only code).
It is also not clear for which sections `cs_LastBlockFile` is responsible for. It is annotated for `m_blockfile_cursors`, but sporadically and inconsistently also covers `m_blockfile_info` (e.g. in `LoadBlockIndexDB`).
Since it has no semantic meaning, and seems confusing to developers, remove it.
An alternative to this patch would be expanding the scope of what `cs_LastBlockFile` covers and turning it into a non-recursive mutex. I prepared such a patch some time ago, but found it unsatisfactory. It was not clear to me if the lock was now covering too much or too little, and its purpose remained unclear. If this patch is accepted, I would expect the project to eventually implement a separate, narrowly-scoped block storage lock to allow for a more parallelizable block processing routine.
ACKs for top commit:
stickies-v:
re-ACK ec6cf49b91
janb84:
re- ACK ec6cf49b91
pablomartin4btc:
ACK ec6cf49b91
Tree-SHA512: e5942bc87300b0db9a0b91d5fe26dab455049e6cef7c96bb12b28141fa04711d46c6af105c0e1a83a9f261edde2c8b8b43ecf577a27d54b4610d784676a85627
The cs_LastBlockFile mutex is redundant: all critical sections are
already covered by cs_main. This is demonstrated in this patch by
replacing all instances of locking cs_LastBlockFile with pairs of
`AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)`
annotations. No additional `::cs_main` LOCK(...)s are introduced.
It is also not clear for which sections `cs_LastBlockFile` is
responsible for. It is annotated for `m_blockfile_cursors`, but
sporadically and inconsistently also covers `m_blockfile_info`.
Since it has no semantic meaning, and seems confusing to developers,
remove it.
1d66963749 log: clarify VerifyDB summary log (ViniciusCestarii)
Pull request description:
The final `LogInfo` message about "No coin database inconsistencies" was printed unconditionally, even for check levels 0-2 where no coin DB verification runs and `nGoodTransactions` stays 0. Split into an always-on completion line and a coin-DB result line gated on `nCheckLevel >= 3 && !skipped_l3_checks`.
Before (checklevel=1):
`Verification: No coin database inconsistencies in last 6 blocks (0 transactions)`
After (checklevel=1):
`Verification: checked last 6 blocks at level 1`
ACKs for top commit:
sedited:
ACK 1d66963749
Tree-SHA512: a6c6689cff2f1942a44764a914a1ee01c92efa51489d72f456fccaa1f9ee0640449b55600d43e7dc89d622414854d53e8dfc7e8c87fe454b06e44f76075db267
02b2c41103 logging: use util/log.h where possible (Anthony Towns)
57d7495fe5 IWYU fixes (Anthony Towns)
611878b46f scripted-diff: logging: Drop LogAcceptCategory (Anthony Towns)
34332dba2f util/log, logging: Provide ShouldDebugLog and ShouldTraceLog instead of a generic ShouldLog (Anthony Towns)
abea304dd6 logging: Move GetLogCategory into Logger class (Anthony Towns)
58113e5833 util/log: Rename LogPrintLevel_ into detail_ namespace (Anthony Towns)
f69d1ae56d util/log: Provide util::log::NO_RATE_LIMIT to avoid rate limits (Anthony Towns)
72e92d67df logging: Protect ShrinkDebugFile by m_cs (Anthony Towns)
904c0d07bb util/stdmutex: Drop StdLockGuard (Anthony Towns)
Pull request description:
`ShrinkDebugFile` now takes the logging mutex for its entire run; though it's only called in init so shouldn't have any races in the first place.
Adds a `NO_RATE_LIMIT` tag that can be used with info/warning/error logs to avoid rate-limiting. This allows `LogPrintLevel_` to be restricted to being an internal API.
The `GetLogCategory` function is moved out of the global namespace.
`ShouldLog` is split into separate `ShouldDebugLog` and `ShouldTraceLog` so that filtering checks are somewhat more enforced via function signature checks.
Redundant `LogAcceptCategory` function is removed.
More files are pointed at util/log.h instead of logging.h.
ACKs for top commit:
maflcko:
review ACK 02b2c41103📅
sedited:
Re-ACK 02b2c41103
l0rinc:
untested ACK 02b2c41103
ryanofsky:
Code review ACK 02b2c41103435d8dbaa77a526e484066471b2b8c! Overall a lot of nice improvements here.
Tree-SHA512: 3bffdca91afbe5c45a522815fe82e6f4cfa96529a4a243b29aad21234650502d6cac780126b584ee3e7ec129d8fdd50670d8a05036cc5c36e586b8c4c3563970
ac9aa71b7f mempool: remove all subsequent tx in pkg on failure (Greg Sanders)
Pull request description:
This belt-and-suspenders check, if ever hit in production, could result in an inconsistent mempool if somehow the parent failed in the ConsensusScriptChecks but the child did not. Rather than allow the mempool to get in an inconsistent state, remove the following txs in the package.
ACKs for top commit:
ismaelsadeeq:
Code review ACK ac9aa71b7f
marcofleon:
ACK ac9aa71b7f
sedited:
ACK ac9aa71b7f
Tree-SHA512: c25310045fa4dfd40bd38c9d54fff3f9fdb817e617154444d4691576393eee5f9cc86e26c59ddcdeef20eb6dddab0a168e91aec85c8291e6d68abbcb333d24f8
This belt-and-suspenders check, if ever hit in production,
could result in an inconsistent mempool if somehow the
parent failed in the ConsensusScriptChecks but the child did
not. Rather than allow the mempool to get in an inconsistent
state, remove the following txs in the package.
This makes sure `txsdata` always outlives the Script check queue (since local
objects are destructed in reverse order of construction).
This is the root cause for a security vulnerability reported by Cory Fields in
2024 that could be exploited by crafting an invalid block to cause nodes to
read freed memory. The vulnerability was covertly fixed in commit
`492e1f09943fcb6145c21d470299305a19e17d8b`.
See security advisory for CVE-2024-52911 for more details.
7c75244ade Change pindexMostWork parameter of ActivateBestChainStep() to reference (optout)
c5eb283bca Change CChain::FindFork() to take ref (optout)
20b58e281a Change CChain::Next() to take reference (optout)
fe2d6e25e0 Change CChain::Contains() to take reference (optout)
db56bcd692 test: Add CChain::FindFork() tests (optout)
8333abdd91 test: Add CChain basic tests (optout)
Pull request description:
Refactor `CChain` methods (`Contains()`, `Next()`, `FindFork()`) to use references instead of pointers, to minimize the risk of accidental `nullptr` dereference (memory access violation). Also add missing unit tests to the `CChain` class.
The `CChain::Contains()` method (in `src/chain.h`) dereferences its input without checking. The `Next()` method also calls into this with a `nullptr` if invoked with `nullptr`. While most call sites have indirect guarantee that the input is not `nullptr`, it's not easy to establish this to all call sites with high confidence. These methods are publicly available. There is no known high-level use case to trigger this error, but the fix is easy, and makes the code safer.
Changes:
- Add basic unit tests for `CChain` class methods
- Add unit tests for `CChain::FindFork()`
- Change `CChain::Contains()` to take reference
- Change `CChain::Next()` to take reference
- Change `CChain::FindFork()` to take reference
- Change `pindexMostWork` parameter of `ActivateBestChainStep()` to reference
- Rename changed parameters (`* pindex` --> `& index`)
Alternative. A simpler change is to stick with pointers, with extra checks where needed, see #34416 .
This change is remotely related to and indirectly triggered by #32875 .
Further ideas, not considered in this PR:
- Change `InvalidateBlock()` and `PreciousBlock()` to take references.
- Change `CChain` internals to store references instead of pointers
- Change CChain to always have at least one element (genesis), that way there is always genesis and tip.
- Check related methods to return reference (guaranteed non-null) -- `FindFork`, `FindEarliestAtLeast`, `FindForkInGlobalIndex`, `blockman.AddToBlockIndex`, etc.
ACKs for top commit:
l0rinc:
reACK 7c75244ade
maflcko:
re-review ACK 7c75244ade🌅
achow101:
ACK 7c75244ade
hodlinator:
re-ACK 7c75244ade
Tree-SHA512: 122f40120058f7e1f0273b3afed9c54966c05f06b6f2fee45bc48430617f24a5e4320a9bb7bb0ac986f2accfa22fabae5cc941b949758ddca2e9fcd472b46c33
af0ee28eb6 refactor: use _MiB consistently for Mebibyte conversions (Lőrinc)
b3edd30aa2 util: add _GiB for Gibibyte conversions (Lőrinc)
Pull request description:
### Problem
Byte-size conversions in the codebase currently show up in many equivalent formats (multiplication/division chains, shifts, hex/binary literals), which creates a maintenance burden and makes review error-prone - especially considering the architectural differences of `size_t`.
Inspired by https://github.com/bitcoin/bitcoin/pull/34305#discussion_r2734720002, it seemed appropriate to unify `Mebibyte` usage across the codebase and add `Gibibyte` support with 32/64 bit `size_t` validation.
### Fix
This PR refactors those call sites to use `""_MiB` (existing) and `""_GiB` (new), and adds the encountered value/pattern replacements to unit tests to make review straightforward, and to ensure the conversions remain valid.
The literals are overflow-checked when converting to `size_t`, and unit tests cover the 32-bit boundary cases.
Concretely, it replaces patterns such as:
* `1024*1024`, `1<<20`, `0x100000`, `1048576`, `/ 1024 / 1024`, `* (1.0 / 1024 / 1024)` → `1_MiB` or `double(1_MiB)`
* `1024*1024*1024`, `1<<30`, `0x40000000`, `1024_MiB`, `>> 30` → `1_GiB`
(added unit tests for each replacement category to ease review)
Additionally, declarations whose initializer reads a `_MiB`/`_GiB` literal are switched to braced initialization so a future oversized value is rejected at compile time through the narrowing check instead of silently truncating.
### Note
In the few places where arithmetic involves signed values, the result is identical to the previous code assuming those quantities never become negative.
ACKs for top commit:
achow101:
ACK af0ee28eb6
janb84:
ACK af0ee28eb6
maflcko:
review ACK af0ee28eb6 🖍
hodlinator:
re-ACK af0ee28eb6
Tree-SHA512: 55286ce3f833f88335394a74e9e0b95c7d023e5cdc9ded40accbbbcd870101e4dcc05926865d6bef4c1be1ebd648aa3fdf947ef9575633ccfe56691f145d7a2d
5f36e0ff1e rpc: fix getblockstats UTXO overhead accounting (Lőrinc)
76190489e6 coins: pack `Coin` height/coinbase consistently (Lőrinc)
1f309d1aa2 coins: make `Coin::fCoinBase` a bool (Lőrinc)
Pull request description:
The [`getblockstats` RPC](https://github.com/bitcoin/bitcoin/pull/10757) currently overestimates UTXO overhead by treating the `fCoinBase` bitfield as an additional `bool` in `PER_UTXO_OVERHEAD`.
However, `fCoinBase` and `nHeight` are stored as bitfields and effectively packed into a single 32-bit value; counting an extra bool in the overhead calculation is unnecessary.
This PR introduces the following changes across three commits:
* Store `fCoinBase` as a `bool` bitfield to reduce implicit conversions at call sites.
* Use a consistent height/coinbase packing style across `Coin` serialization, undo serialization, and coinstats hashing (casting `nHeight` to `uint32_t` before shifting to avoid signed-promotion UB).
* Adjust UTXO overhead estimation to match the actual `Coin` layout and update the related tests accordingly.
ACKs for top commit:
achow101:
ACK 5f36e0ff1e
sedited:
ACK 5f36e0ff1e
vasild:
ACK 5f36e0ff1e
optout21:
crACK 5f36e0ff1e
Tree-SHA512: f4a44debed358e9e130da9d6fae5f89289daa34f0bdb7155edc3e9b691c219451f4c80b1e16aca5b011f0fa2fa975484ef1af4ca4563b7c6ba4ca9dd133f30be
Replace hard-coded MiB byte conversions (e.g. `1024*1024`, `1<<20`, `1048576`) with the existing `_MiB` literal to improve readability and avoid repeating constants.
In the few spots where arithmetic involves signed values, the result is identical to the previous code assuming those quantities never turn negative.
Also switch to brace init on every declaration assigned from `_MiB`/`_GiB` literals so a future oversized value (e.g. `unsigned int x{4096_MiB}`) becomes a compile error through the C++11 narrowing check instead of silently truncating.
Extend unit tests to cover the 32-bit `size_t` overflow boundary and to assert equivalence for integer and floating-point conversions.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
ActivateBestChainStep() is always called with non-nullptr pindexMostWork parameter,
change the type of the parameter from pointer to reference to enforce this.
Also rename the parameter (prefix p doesn't make sense any more).
To minimize chance of erroneous nullptr dereference, `CChain::Next()`
is changed to take a reference instead of a pointer.
Call sites have been adapted. Notably, NextSyncBlock() now checks
the FindFork() result before calling into Next(), because
the fork lookup may return null.
The `CChain::Contains()` method dereferences its input without checking,
potentially resulting in nullptr-dereference if invoked with `nullptr`.
To avoid this possibility, its input is changed to a reference instead.
Call sites are adapted accoringly, extra nullptr-check is added as
needed.
8783cc8056 refactor: inline `CCoinsViewBacked` implementation (Lőrinc)
86296f276d coins: make `CCoinsView` methods pure virtual (Lőrinc)
b637566c8d coins: add explicit `CoinsViewEmpty` noop backend (Lőrinc)
90c635c01c fuzz: keep backend assertions aligned to active backend (Lőrinc)
a9f92e3497 refactor: normalize CCoinsView whitespace and signatures (Lőrinc)
38a99f3344 scripted-diff: normalize `CCoinsView` naming (Lőrinc)
06172ef0d5 refactor: rename `hashBlock` to `m_block_hash` to avoid shadowing (Lőrinc)
Pull request description:
### Problem
`CCoinsView` is the coins view interface, but historically it also provided built-in no-op behavior:
* It provided default no-op implementations (returning `std::nullopt`, `uint256()`, `false`, or `nullptr`) instead of being pure virtual.
* Callers could instantiate a bare `CCoinsView` and get silent no-op behavior.
* Mixing the interface definition with a built-in dummy implementation blurred the abstraction boundary.
### Context
This is part of the ongoing coins caching cleanup in #34280.
### Fix
This PR separates the interface from no-op behavior and makes `CCoinsView` pure virtual in incremental steps:
* Add `CoinsViewEmpty` as an explicit no-op coins view for tests, benchmarks, and temporary backends.
* Replace direct bare-`CCoinsView` test and dummy instantiations with `CoinsViewEmpty`.
* Make all `CCoinsView` methods pure virtual (`PeekCoin`, `GetCoin`, `HaveCoin`, `GetBestBlock`, `GetHeadBlocks`, `BatchWrite`, `Cursor`, `EstimateSize`).
* Remove the legacy default implementations from `coins.cpp`.
* Update fuzz and dummy backends to use `CoinsViewEmpty` explicitly.
ACKs for top commit:
w0xlt:
reACK 8783cc8056
ryanofsky:
Code review ACK 8783cc8056. Just updated comments and variable name since last review. The fuzz test code is much clearer now IMO
andrewtoth-exo:
ACK 8783cc8056
ajtowns:
ACK 8783cc8056
Tree-SHA512: cfc831578aa309788c1b5dafbfecca3de388cc4215534c3f3df24f90d7770ed37b1fd7aa134df91d611d0a1ca75929accb98d5ed7df7b52851c259e04f08e4a3
ba01b00d45 refactor: use for loops in FindMostWorkChain (stratospher)
aa0eef735b test: add InvalidateBlock/ReconsiderBlock asymmetry test (stratospher)
1b0b3e2c2c validation: remove redundant marking in FindMostWorkChain (stratospher)
Pull request description:
recent PRs like #31405, #30666 mark all `m_block_index` descendants as invalid immediately whenever an invalid block is encountered in `SetBlockFailureFlags`. so by the time we reach `FindMostWorkChain`, the block in `setBlockIndexCandidates` already has `BLOCK_FAILED_VALID` set on it - not just on its ancestor. this means `pindexTest = pindexFailed` whenever `fFailedChain` fires, and the inner `while (pindexTest != pindexFailed)` loop body is never reached!
I think we can remove it but I've just replaced it with `Assume` in this PR for safety + good to document this invariant in case the code changes in future. (noticed by @ stickies-v in https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2815053885)
the second commit is unrelated and adds a unit test for the situation in https://github.com/bitcoin/bitcoin/issues/32173
ACKs for top commit:
fjahr:
re-ACK ba01b00d45
optout21:
crACK ba01b00d45
w0xlt:
ACK ba01b00d45
ryanofsky:
Code review ACK ba01b00d45, just tweaking comment and for loop condition since last review.
Tree-SHA512: a8be3c30b1c41b76690d16d850e87e9e71fa6a1ecaa8b90ec997ffee1aace48b336a7009a480cd016103759d79c964b3d761a13ae936523808b2930beb68dae5
Introduce `CoinsViewEmpty` as an explicit no-op `CCoinsView` implementation, and define its singleton accessor out of line in `coins.cpp` to avoid `-Wunique-object-duplication` in shared-library builds.`
Use it at call sites that intentionally want a no-op backend instead of constructing anonymous placeholder views.
`CCoinsViewTest` and `CoinsViewBottom` now inherit defaults from `CoinsViewEmpty` (e.g. the unused `EstimateSize()`, which now returns 0).
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
since ed764ea, all descendants in m_block_index are required to be
marked BLOCK_FAILED_VALID when an invalid block is encountered,
as enforced by a CheckBlockIndex assert.
remove the now-redundant marking in FindMostWorkChain's inner loop,
so it is only responsible for cleaning up setBlockIndexCandidates,
not for modifying block validity state
69baddc910 validation: do not add the snapshot block to candidates of bg chainstate (Martin Zumsande)
Pull request description:
The snapshot block needs to be added to the candidates set of the assumed-valid chain because it will be the tip of that chainstate right after snapshot activation.
However, adding it also to the background chainstate is not necessary for anything. Before, the index would be in the set without being connectable. It will be eventually added to the set as part of the normal block download - no extra logic is necessary here.
This simplifies a unit test which had a comment that having the block in the set is "not intended".
This was suggested [here](https://github.com/bitcoin/bitcoin/pull/34521#discussion_r2849281299) and [here](https://github.com/bitcoin/bitcoin/pull/34521#discussion_r2883024248) in #34521
Note that adding the snapshot block was harmless, since `FindMostWorkChain()` lazily removes blocks without data from the set, so this does not fix a bug but just simplifies some code.
ACKs for top commit:
achow101:
ACK 69baddc910
Bortlesboat:
Concept ACK 69baddc910. Removing `TargetBlock()` correctly limits the snapshot-block special-case to cs2 where it's actually needed — the test's own "not intended" comment was the tell.
sedited:
ACK 69baddc910
fjahr:
ACK 69baddc910
stratospher:
ACK 69baddc.
Tree-SHA512: 8942fc422f1898369dd486e37da11758f2ebd4a488d092aa1637ef5bfb85766c4be9ad0718797fb2080f5e8d61383b2ee932bf2bc2f7abc2fb07fe3d72e070c3
25f69d970a release note (Pol Espinasa)
af629821cf test: add background validation test for getblockchaininfo (Pol Espinasa)
a3d6f32a39 rpc, log: add backgroundvalidation to getblockchaininfo (Pol Espinasa)
5b2e4c4a88 log: update progress calculations for background validation (Pol Espinasa)
Pull request description:
`getblockchaininfo` returns `verificationprogress=1` and `initialblockdownload=false` even if there's background validation.
This PR adds information about background validation to rpc `getblockchaininfo` in a similar way to `validationprogress` does.
If assume utxo was used the output of a "sync" node performing background validation:
```
$ ./build/bin/bitcoin-cli getblockchaininfo
...
"mediantime": 1756933740,
"verificationprogress": 1,
"initialblockdownload": false,
"backgroundvalidation": {
"snapshotheight": 880000,
"blocks": 527589,
"bestblockhash": "0000000000000000002326308420fa5ccd28a9155217f4d1896ab443d84148fa",
"mediantime": 1529076654,
"chainwork": "0000000000000000000000000000000000000000020c92fab9e5e1d8ed2d8dbc",
"verificationprogress": 0.2815790617966284
},
"chainwork": "0000000000000000000000000000000000000000df97866c410b0302954919d2",
"size_on_disk": 61198817285,
...
```
If assume utxo was not used the progress is hidden:
```
$ ./build/bin/bitcoin-cli getblockchaininfo
...
"mediantime": 1756245700,
"verificationprogress": 1,
"initialblockdownload": false,
"chainwork": "00000000000000000000000000000000000000000000000000000656d6bb052b",
"size_on_disk": 3964972194,
...
```
The PR also updates the way we estimate the verification progress returning a 100% on the snapshot block and not on the tip as we will stop doing background validation when reaching it.
ACKs for top commit:
fjahr:
ACK 25f69d970a
danielabrozzoni:
ACK 25f69d970a
achow101:
ACK 25f69d970a
sedited:
ACK 25f69d970a
Tree-SHA512: 5e5e08fd39af5f764962b862bc6d8257b0d2175fe920d4b79dc5105578fd4ebe08aee2fe9bfa5c9cad5d7610197a435ebaac0de23e7a5efa740dfea031a8a9d4
d8f4e7caf0 doc: add release notes (ismaelsadeeq)
248c175e3d test: ensure `ValidateInputsStandardness` optionally returns debug string (ismaelsadeeq)
d2716e9e5b policy: update `AreInputsStandard` to return error string (ismaelsadeeq)
Pull request description:
This PR is another attempt at #13525.
Transactions that fail `PreChecks` Validation due to non-standard inputs now returns invalid validation state`TxValidationResult::TX_INPUTS_NOT_STANDARD` along with a debug error message.
Previously, the debug error message for non-standard inputs do not specify why the inputs were considered non-standard.
Instead, the same error string, `bad-txns-nonstandard-inputs`, used for all types of non-standard input scriptSigs.
This PR updates the `AreInputsStandard` to include the reason why inputs are non-standard in the debug message.
This improves the `Precheck` debug message to be more descriptive.
Furthermore, I have addressed all remaining comments from #13525 in this PR.
ACKs for top commit:
instagibbs:
ACK d8f4e7caf0
achow101:
ACK d8f4e7caf0
sedited:
Re-ACK d8f4e7caf0
Tree-SHA512: 19b1a73c68584522f863b9ee2c8d3a735348667f3628dc51e36be3ba59158509509fcc1ffc5683555112c09c8b14da3ad140bb879eac629b6f60b8313cfd8b91
This makes existing behaviour of the block's destructor triggering on
the scheduler thread more explicit by moving it to the thread. The
scheduler thread doing so is useful, since it does not block the thread
doing validation while releasing a block's memory.
DisconnectTip already creates and destroys the block itself, so moving
it into the validation signals is well scoped.
This makes existing behaviour of the block's destructor triggering on
the scheduler thread more explicit by moving it to the thread. The
scheduler thread doing so is useful, since it does not block the thread
doing validation while releasing a block's memory.
Previously, both the caller and the queued event lambda held copies of
the shared_ptr. The block would typically be freed on the scheduler
thread - but only because it went out of scope before the queued event
on the scheduler thread ran. If the scheduler ran first, the block would
instead be freed on the validation thread.
Now, ownership is transferred at each step when invoking the
BlockConnected signal: connected_blocks yields via std::move,
BlockConnected takes by value, and the event lambda move-captures the
shared_ptr. Though it is possible that this only decrements the block's
reference count, blocks are also read from disk in `ConnectTip`, which
now explicitly results in their memory being released on the scheduler
thread.
2f8f2e9001 validation: remove ConnectTrace wrapper class (stickies-v)
b83de7f28e validation: remove sentinel block from ConnectTrace (stickies-v)
Pull request description:
The sentinel pattern in `ConnectTrace` has been unnecessary since conflicted transaction tracking was removed in 5613f9842b. Without that tracking `ConnectTrace` is a trivial wrapper around `std::vector`, so it seems better to just replace it with the vector directly.
Also modernize/update naming along the way, renaming `PerBlockConnectTrace` to `ConnectedBlock`
Refactor, no behaviour change.
ACKs for top commit:
HowHsu:
ACK 2f8f2e9001
sedited:
ACK 2f8f2e9001
w0xlt:
reACK 2f8f2e9001
Tree-SHA512: 0045fcdc1178a160e31ef9d44dcd5fddd21c30c53ed06e84beacddb0b73e7b8120fee874256d1b9ceae45da65164a2e5531992bd374f8d57b6a8455a5354fe57
22335474d7 net: format peer+addr logs with `LogPeer` (Lőrinc)
e55ea534f7 test: add pre-`LogPeer` net log assertion (Lőrinc)
736b17c0f0 log: fix minor formatting in debug logs (Lőrinc)
9cf82bed32 log: show placeholders for missing peer fields (Lőrinc)
Pull request description:
This is an alternative to #34293, but aims to address the remaining logging inconsistencies more broadly.
It extends the example fixed there to every instance, restores the original separator behavior, applies it consistently via a single helper, and adds tests for `logips` (covering both current and new behavior).
### Problem
After #28521 centralized peer address logging into `CNode::LogIP()`, the original comma separator before `peeraddr=` was lost, resulting in inconsistent formatting across net (and recent private broadcast) logs.
Some lines also had double spaces, empty fields, or mismatched format specifiers.
### Fix
Introduces `CNode::LogPeer(bool)` which always emits `peer=<id>` and, when `-logips=1`, appends `, peeraddr=<addr>`. This eliminates hand-rolled separators and makes peer identification predictable.
Minor issues (double spaces, empty placeholders, format specifiers) are fixed along the way in separate commits.
### Reproducer
Run with `-debug=net -logips=1` and observe peer log lines now show `peer=<id>, peeraddr=<addr>` (comma-separated). The new assertion in `feature_logging.py` automates this check.
ACKs for top commit:
naiyoma:
ACK 22335474d7
vasild:
ACK 22335474d7
sedited:
ACK 22335474d7
Tree-SHA512: 562262a58c3042f139099ff4c41e3fc6a97505fe9603c2bf700a97fd0aa052954b47c14da0e50c1fc311db1ae6c04e6a92156c9b85e25c777a637b7766c1dafe
The snapshot block needs to be added to the candidates set of the
assumed-valid chain because it will be the tip of that chainstate
right after snapshot activation.
However, adding it also to the background chainstate is not necessary
for anything. Before, the index would linger in the set without being
connectable. It will be eventually added to the set as part of the
normal block download - no extra logic is necessary here.
20ae9b98ea Extend functional test for setBlockIndexCandidates UB (marcofleon)
854a6d5a9a validation: fix UB in LoadChainTip (marcofleon)
9249e6089e validation: remove LoadChainTip call from ActivateSnapshot (marcofleon)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/issues/34503. See this issue for more details as well.
Fixes a bug where, under certain conditions, `setBlockIndexCandidates` had blocks in it that were worse than the tip. The block index candidate set uses `nSequenceId` as a sort key, so modifying this field while blocks are in the set results in undefined behavior. This PR populates `setBlockIndexCandidates` after the `nSequenceId` modifications, avoiding the UB.
ACKs for top commit:
achow101:
ACK 20ae9b98ea
sedited:
Re-ACK 20ae9b98ea
sipa:
Code review ACK 20ae9b98ea
Tree-SHA512: 121c170bb70fb6365089d578db63c811e7926e129d7206e569947f7a1f6c5ddc8d5f4937b80f1ba1b7d7daa42789b143ca5b56f154b7ab968a1cd55f925f378d
The removal of the chain tip from setBlockIndexCandidates was
happening after nSequenceId was modified. Since the set uses
nSequenceId as a sort key, modifying it while the element is in the
set is undefined behavior, which can cause the erase to fail.
With assumeutxo, a second form of UB exists: two chainstates each
have their own candidate set, but share the same CBlockIndex
objects. Calling LoadChainTip on one chainstate mutates nSequenceIds
that are also in the other chainstate's set.
Fix by populating setBlockIndexCandidates after all changes to
nSequenceId.
This call is a no-op. PopulateAndValidateSnapshot already sets both
the chain tip and the coins cache best block to the snapshot block,
so LoadChainTip always hits the early return when it finds that the
two match (tip->GetBlockHash() == coins_cache.GetBestBlock()).
The sentinel pattern was necessary to collect conflicted transactions
before their associated block was recorded, but that tracking was
removed in 5613f9842b.
This commit renames AreInputsStandard to ValidateInputsStandardness.
ValidateInputsStandardness now returns valid TxValidationState if all inputs
(scriptSigs) use only standard transaction forms else returns invalid
TxValidationState which states why an input is not standard.
33fbaed310 policy: don't CheckEphemeralSpends on reorg (Greg Sanders)
Pull request description:
Similar reasoning to https://github.com/bitcoin/bitcoin/pull/33504
During a deeper reorg it's possible that a long sequence of dust-having transactions that are connected in a linear fashion. On reorg, this could cause each subsequent "generation" to be rejected. These rejected transactions may contain a large amount of competitive fees via normal means.
PreCheck based `PreCheckEphemeralSpends` is left in place because we wouldn't have relayed them prior to the reorg.
ACKs for top commit:
darosior:
re-ACK 33fbaed310
ismaelsadeeq:
reACK 33fbaed310
sedited:
ACK 33fbaed310
Tree-SHA512: cf0a6945066e9f5f8f9a847394c2c1225facf475a8aa4bc811b436513eff79c0a720d4ad21ba6b0f1cc4dfdd61cf46acb148333ac592b2ee252953732326ad1d
At startup, if the needs to catch up, connected mining clients will
receive a flood of new templates as new blocks are connected.
Fix this by adding a cooldown argument to createNewBlock(). When set
to true, block template creation is briefly paused while the best
header chain is ahead of the tip.
This wait only happens when the best header extends the current tip,
to ignore competing branches.
Additionally, cooldown waits for isInitialBlockDownload() to latch to
false, which happens when there is less than a day of blocks left to sync.
When cooldown is false createNewBlock() returns immediately. The argument
is optional, because many tests are negatively impacted by this
mechanism, and single miner signets could end up stuck if no block
was mined for a day.
The getblocktemplate RPC also opts out, because it would add a delay
to each call.
Fixes#33994
f700609e8a doc: Release notes for mining IPC interface bump (Ryan Ofsky)
9453c15361 ipc mining: break compatibility with existing clients (version bump) (Sjors Provoost)
70de5cc2d2 ipc mining: pass missing context to BlockTemplate methods (incompatible schema change) (Sjors Provoost)
2278f017af ipc mining: remove deprecated methods (incompatible schema change) (Ryan Ofsky)
c6638fa7c5 ipc mining: provide default option values (incompatible schema change) (Ryan Ofsky)
a4603ac774 ipc mining: declare constants for default field values (Ryan Ofsky)
ff995b50cf ipc test: add workaround to block_reserved_weight exception test (Ryan Ofsky)
b970cdf20f test framework: expand expected_stderr, expected_ret_code options (Ryan Ofsky)
df53a3e5ec rpc refactor: stop using deprecated getCoinbaseCommitment method (Ryan Ofsky)
Pull request description:
This PR increments the field number of the `Init.makeMining` method and makes the old `makeMining` method return an error, so IPC mining clients not using the latest schema file will get an error and not be able to access the Mining interface.
Normally, there shouldn't be a need to break compatibility this way, but the mining interface has evolved a lot since it was first introduced, with old clients using the original methods less stable and performant than newer clients. So now is a good time to introduce a cutoff, drop deprecated methods, and stop supporting old clients which can't function as well.
Bumping the field number is also an opportunity to make other improvements that would be awkward to implement compatibly:
- Making Cap'n Proto default parameter and field values match default values of corresponding C++ methods and structs.
- Adding missing Context parameters to Mining.createNewBlock and checkBlock methods so these methods will be executed on separate execution threads and not block the Cap'n Proto event loop thread.
More details about these changes are in the commit messages.
ACKs for top commit:
Sjors:
ACK f700609e8a
enirox001:
ACK f700609e8a
ismaelsadeeq:
ACK f700609e8a
sedited:
ACK f700609e8a
Tree-SHA512: 0901886af00214c138643b33cec21647de5671dfff2021afe06d78dfd970664a844cde9a1e28f685bb27edccaf6e0c3f2d1e6bb4164bde6b84f42955946e366d
cae6d895f8 fuzz: add target for CoinsViewOverlay (Andrew Toth)
86eda88c8e fuzz: move backend mutating block to end of coins_view (Andrew Toth)
89824fb27b fuzz: pass coins_view_cache to TestCoinsView in coins_view (Andrew Toth)
73e99a5966 coins: don't mutate main cache when connecting block (Andrew Toth)
67c0d1798e coins: introduce CoinsViewOverlay (Andrew Toth)
69b01af0eb coins: add PeekCoin() (Andrew Toth)
Pull request description:
This is a slightly modified version of the first few commits of #31132, which can be merged as an independent change. It has a small benefit on its own, but will help in moving the parent PR forward.
When accessing coins via the `CCoinsViewCache`, methods like `GetCoin` can call `FetchCoin` which actually mutate `cacheCoins` internally to cache entries when they are pulled from the backing db. This is generally a performance improvement for single threaded access patterns, but it precludes us from accessing entries in a `CCoinsViewCache` from multiple threads without a lock.
Another aspect is that when we use the resettable `CCoinsViewCache` view backed by the main cache for use in `ConnectBlock()`, we will insert entries into the main cache even if the block is determined to be invalid. This is not the biggest concern, since an invalid block requires proof-of-work. But, an attacker could craft multiple invalid blocks to fill the main cache. This would make us `Flush` the cache more often than necessary. Obviously this would be very expensive to do on mainnet.
Introduce `CoinsViewOverlay`, a `CCoinsViewCache` subclass that reads coins without mutating the underlying cache via `FetchCoin()`.
Add `PeekCoin()` to look up a Coin through a stack of `CCoinsViewCache` layers without populating parent caches. This prevents the main cache from caching inputs pulled from disk for a block that has not yet been fully validated. Once `Flush()` is called on the view, these inputs will be added as spent to `coinsCache` in the main cache via `BatchWrite()`.
This is the foundation for async input fetching, where worker threads must not mutate shared state.
ACKs for top commit:
l0rinc:
ACK cae6d895f8
sipa:
reACK cae6d895f8
sedited:
Re-ACK cae6d895f8
willcl-ark:
ACK cae6d895f8
vasild:
Cursory ACK cae6d895f8
ryanofsky:
Code review ACK cae6d895f8. PR is basically back to the form I had acked the first time, implementing `PeekCoin()` by calling `GetCoin()`. This is not ideal because `PeekCoin()` is not supposed to modify caches and `GetCoin()` does that, but it at least avoids problems of the subsequent approach tried where `GetCoin()` calls `PeekCoin` and would result in bugs when subclasses implement `GetCoin` forgetting to override `PeekCoin`. Hopefully #34124 can clean all of this by making relevant methods pure virtual.
Tree-SHA512: a81a98e60ca9e47454933ad879840cc226cb3b841bc36a4b746c34b350e07c546cdb5ddc55ec1ff66cf65d1ec503d22201d3dc12d4e82a8f4d386ccc52ba6441
afb1bc120e validation: Use dirty entry count in flush warnings and disk space checks (Pieter Wuille)
b413491a1c coins: Keep track of number of dirty entries in `CCoinsViewCache` (Pieter Wuille)
7e52b1b945 fuzz: call `EmplaceCoinInternalDANGER` as well in `SimulationTest` (Lőrinc)
Pull request description:
### Problem
Now that non-wiping flushes are possible (#28280, #28233), the cache may be mostly clean at flush time.
But the flush warning, disk-space check, and benchmark logging still used total cache size, so a node with a 10 GiB cache that only needs to write a small fraction of dirty entries could still trigger a scary warning via the disk-space checks.
The previous `DynamicMemoryUsage` metric was also fundamentally wrong for estimating disk writes, even before non-wiping flushes. In-memory coin size differs from on-disk write size due to LevelDB overhead, log doubling, and compaction.
The warning also only fired in `FlushStateToDisk`, so `AssumeUTXO` snapshot loads never warned at all.
### Fix
This PR tracks the actual number of dirty entries via `m_dirty_count` in `CCoinsViewCache`, maintained alongside the existing dirty-flag linked list, `SanityCheck` cross-validating both counts.
The warning and benchmark log move from `FlushStateToDisk` down to `CCoinsViewDB::BatchWrite`, where the actual I/O happens. This is the single place all flush paths converge (regular flushes, syncs, and snapshot loads), so the warning now fires correctly for `AssumeUTXO` too.
The threshold changes from 1 GiB of memory to 10 million dirty entries, which is roughly equivalent but avoids the in-memory vs on-disk size confusion.
The disk-space safety check now uses `GetDirtyCount()` with the existing conservative 48-byte-per-entry estimate, preventing unnecessary shutdowns when the cache is large but mostly clean.
---
Note: the first commit adds fuzz coverage for `EmplaceCoinInternalDANGER` in `SimulationTest` to exercise the accounting paths before modifying them.
Note: this is a revival of #31703 with all outstanding review feedback addressed.
ACKs for top commit:
Eunovo:
Concept ACK afb1bc120e
andrewtoth:
re-ACK afb1bc120e
sipa:
Code review ACK afb1bc120e
sedited:
ACK afb1bc120e
Tree-SHA512: 4133c6669fd20836ae2fb62ed804cdf6ebaa61076927b54fc412e42455a2f0d4cadfab0844064f9c32431eacb1f5e47b78de8e5cde1b26ba7239a7becf92f369
