c6ca2b85a3e6e73674e210aee4ed69c4af2848e4 validation: do not wipe utxo cache for stats/scans/snapshots (Pieter Wuille)
7099e93d0a80c65a547131d7bab977b09573310c refactor: rename `FlushStateMode::ALWAYS` to `FORCE_FLUSH` (Lőrinc)
Pull request description:
Revival of https://github.com/bitcoin/bitcoin/pull/30610#issuecomment-3432564955 with the remaining comments applied on top
> Since #28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful.
>
> Split the `FlushStateMode::ALWAYS` mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in `scantxoutset`, `gettxoutsetinfo`, snapshot creation.
(slightly updated after #30214)
ACKs for top commit:
optout21:
reACK c6ca2b85a3e6e73674e210aee4ed69c4af2848e4
cedwies:
reACK c6ca2b8 (trivial)
achow101:
ACK c6ca2b85a3e6e73674e210aee4ed69c4af2848e4
sedited:
ACK c6ca2b85a3e6e73674e210aee4ed69c4af2848e4
Tree-SHA512: f3525a85dc512db4a0a9c749ad47c0d3fa44085a121aa54cd77646260a719c71f754ec6570ae77779c0ed68a24799116f79c686e7a17ce57a26f6a598f7bf926
de4242f47476769d0a7f3e79e8297ed2dd60d9a4 refactor: Use reference for chain_start in HeadersSyncState (Daniela Brozzoni)
e37555e5401f9fca39ada0bd153e46b2c7ebd095 refactor: Use initializer list in CompressedHeader (Daniela Brozzoni)
0488bdfefe92b2c9a924be9244c91fe472462aab refactor: Remove unused parameter in ReportHeadersPresync (Daniela Brozzoni)
256246a9fa5b05141c93aeeb359394b9c7a80e49 refactor: Remove redundant parameter from CheckHeadersPoW (Daniela Brozzoni)
ca0243e3a6d77d2b218749f1ba113b81444e3f4a refactor: Remove useless CBlock::GetBlockHeader (Pieter Wuille)
45686522224598bed9923e60daad109094d7bc29 refactor: Use std::span in HasValidProofOfWork (Daniela Brozzoni)
4066bfe561a45f61a3c9bf24bec7f600ddcc7467 refactor: Compute work from headers without CBlockIndex (Daniela Brozzoni)
0bf6139e194f355d121bb2aea74715d1c4099598 p2p: Avoid an IsAncestorOfBestHeaderOrTip call (Pieter Wuille)
Pull request description:
This is a partial* revival of #25968
It contains a list of most-unrelated simplifications and optimizations to the code merged in #25717:
- Avoid an IsAncestorOfBestHeaderOrTip call: Just don't call this function when it won't have any effect.
- Compute work from headers without CBlockIndex: Avoid the need to construct a CBlockIndex object just to compute work for a header, when its nBits value suffices for that. Also use some Spans where possible.
- Remove useless CBlock::GetBlockHeader: There is no need for a function to convert a CBlock to a CBlockHeader, as it's a child class of it.
It also contains the following code cleanups, which were suggested by reviewers in #25968:
- Remove redundant parameter from CheckHeadersPoW: No need to pass consensusParams, as CheckHeadersPow already has access to m_chainparams.GetConsensus()
- Remove unused parameter in ReportHeadersPresync
- Use initializer list in CompressedHeader, also make GetFullHeader const
- Use reference for chain_start in HeadersSyncState: chain_start can never be null, so it's better to pass it as a reference rather than a raw pointer
*I decided to leave out three commits that were in #25968 (4e7ac7b94d04e056e9994ed1c8273c52b7b23931, ab52fb4e95aa2732d1a1391331ea01362e035984, 7f1cf440ca1a9c86085716745ca64d3ac26957c0), since they're a bit more involved, and I'm a new contributor. If this PR gets merged, I'll comment under #25968 to note that these three commits are still up for grabs :)
ACKs for top commit:
l0rinc:
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
polespinasa:
re-ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
sipa:
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
achow101:
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
hodlinator:
re-ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
Tree-SHA512: 1de4f3ce0854a196712505f2b52ccb985856f5133769552bf37375225ea8664a3a7a6a9578c4fd461e935cd94a7cbbb08f15751a1da7651f8962c866146d9d4b
Since #28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful.
Split the FlushStateMode::ALWAYS mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in scantxoutset, gettxoutsetinfo, snapshot creation.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: cedwies <141683552+cedwies@users.noreply.github.com>
SnapshotBlockhash() is only called two places outside of tests, and is used
redundantly in some tests, checking the same field as other checks. Simplify by
dropping the method and using the m_from_snapshot_blockhash field directly.
Remove hardcoded references to m_ibd_chainstate and m_snapshot_chainstate so
MaybeCompleteSnapshotValidation function can be simpler and focus on validating
the snapshot without dealing with internal ChainstateManager states.
This is a step towards being able to validate the snapshot outside of
ActivateBestChain loop so cs_main is not locked for minutes when the snapshot
block is connected.
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.
63b534f97e591d4e107fd5148909852eb2965d27 fuzz: sanity check hardcoded snapshot in utxo_snapshot target (Antoine Poinsot)
3b85eba83abe561078c91f5a5c49cf26c682c19b test util: split up ConnectBlock from MineBlock (Antoine Poinsot)
d1527f6b88656ff4aab3c671c6d9780ea2ae986e qa: correct off-by-one in utxo snapshot fuzz target (Antoine Poinsot)
Pull request description:
The assumeutxo data for the fuzz target could change and invalidate the hash silently, preventing the fuzz target from reaching some code paths. Fix this by introducing a unit test which would break if the snapshot data the fuzz target relies on were to change.
In implementing this i noticed the height used for coins in the fuzz target is actually off-by-one (as if the first block in the created chain was the genesis but it's block `1`), so fix that too.
ACKs for top commit:
mzumsande:
Code Review ACK 63b534f97e591d4e107fd5148909852eb2965d27
fjahr:
tACK 63b534f97e591d4e107fd5148909852eb2965d27
Tree-SHA512: 2399b6e74db9b78aab8efba67c57a405d2d7d880ae3b7d8518a1c96cc6266f61f5e77722cd999adeac5d3e03e73d84cf9ae7bdbcc0afae198cc87049dde4012f
The assumeutxo data for the fuzz target could change and invalidate the hash silently, preventing
the fuzz target from reaching some code paths.
Fix this by sanity checking the snapshot values during initialization.
Makes it friendlier for potential future users of the kernel library if
they do not store the headers in a std::vector, but can guarantee
contiguous memory.
fa899fb7aa8a14acecadd8936ad5824fa0f697ff fuzz: Speed up utxo_snapshot fuzz target (MarcoFalke)
fa386642b4dfd88f74488c288c7886494d69f4ed fuzz: Speed up utxo_snapshot by lazy re-init (MarcoFalke)
fa645c7a861ffa83a53a459263b6a620defe31f9 fuzz: Remove unused DataStream object (MarcoFalke)
fae8c73d9e4eba4603447bb52b6e3e760fbf15f8 test: Disallow fee_estimator construction in ChainTestingSetup (MarcoFalke)
Pull request description:
Two commits to speed up unit and fuzz tests.
Can be tested by running the fuzz target and looking at the time it took, or by looking at the flamegraph. For example:
```
FUZZ=utxo_snapshot perf record -g --call-graph dwarf ./src/test/fuzz/fuzz -runs=100
hotspot ./perf.data
ACKs for top commit:
TheCharlatan:
Re-ACK fa899fb7aa8a14acecadd8936ad5824fa0f697ff
marcofleon:
Re ACK fa899fb7aa8a14acecadd8936ad5824fa0f697ff
brunoerg:
ACK fa899fb7aa8a14acecadd8936ad5824fa0f697ff
Tree-SHA512: d3a771bb12d7ef491eee61ca47325dd1cea5c20b6ad42554babf13ec98d03bef8e7786159d077e59cc7ab8112495037b0f6e55edae65b871c7cf1708687cf717
The re-init is expensive, so skip it if there is no need.
Also, add an even faster fuzz target utxo_snapshot_invalid, which does
not need any re-init at all.
Adds more testing options for creating an environment without networking
and a validation interface. This is useful for improving the performance
of the utxo snapshot fuzz test, which constructs a new TestingSetup on
each iteration.
Add the possibility of giving more guidance to the creation of the
metadata and/or coins, so that the fuzzer gets the chance
to reach more error conditions in ActivateSnapshot and sometimes
successfully creates a valid snapshot.
This also changes the asserts for the success case that were outdated,
and only didn't result in a crash because the fuzzer wasn't able
to reach this code before.
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.
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.
In order to prevent memory DoS, we must ensure that we don't accept a new
header into memory until we've performed anti-DoS checks, such as verifying
that the header is part of a sufficiently high work chain. This commit adds a
new argument to AcceptBlockHeader() so that we can ensure that all call-sites
which might cause a new header to be accepted into memory have to grapple with
the question of whether the header is safe to accept, or needs further
validation.
This patch also fixes two places where low-difficulty-headers could have been
processed without such validation (processing an unrequested block from the
network, and processing a compact block).
Credit to Niklas Gögge for noticing this issue, and thanks to Sjors Provoost
for test code.
Warning: Replacing fs::system_complete calls with fs::absolute calls
in this commit may cause minor changes in behaviour because fs::absolute
no longer strips trailing slashes; however these changes are believed to
be safe.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
11daf6ceb1d9ea1f8d638b123eecfe39d162a7c3 More Span simplifications (Pieter Wuille)
568dd2f83900a11a4dbba1250722791a135bf0a9 Replace MakeSpan helper with Span deduction guide (Pieter Wuille)
Pull request description:
C++17 supports [user-defined deduction guides](https://en.cppreference.com/w/cpp/language/class_template_argument_deduction), allowing class constructors to be invoked without specifying class template arguments. Instead, the code can contain rules to infer the template arguments from the constructor argument types.
This alleviates the need for the `MakeSpan` helper. Convert the existing MakeSpan rules into deduction rules for `Span` itself, and replace all invocations of `MakeSpan` with just `Span` ones.
ACKs for top commit:
MarcoFalke:
re-ACK 11daf6ceb1d9ea1f8d638b123eecfe39d162a7c3 Only change is removing a hunk in the tests 🌕
Tree-SHA512: 10f3e82e4338f39d9b7b407cd11aac7ebe1e9191b58e3d7f4e5e338a4636c0e126b4a1d912127c7446f57ba356c8d6544482e47f97901efea6a54fffbfd7895f
