Use number of dirty cache entries in flush warnings/logs#41
Closed
Use number of dirty cache entries in flush warnings/logs#41
Conversation
`FuzzedSock::Accept()` properly returns a new socket, but it forgot to set the output argument `addr`, like `accept(2)` is expected to. This could lead to reading uninitialized data during testing when we read it, e.g. from `CService::SetSockAddr()` which reads the `sa_family` member. Set `addr` to a fuzzed IPv4 or IPv6 address.
Now that all network calls done by `CConnman::OpenNetworkConnection()` are done via `Sock` they can be redirected (mocked) to `FuzzedSocket` for testing.
* Make the methods of `CThreadInterrupt` virtual and store a pointer to it in `CConnman`, thus making it possible to override with a mocked instance. * Initialize `CConnman::m_interrupt_net` from the constructor, making it possible for callers to supply mocked version. * Introduce `FuzzedThreadInterrupt` and `ConsumeThreadInterrupt()` and use them in `src/test/fuzz/connman.cpp` and `src/test/fuzz/i2p.cpp`. This improves the CPU utilization of the `connman` fuzz test. As a nice side effect, the `std::shared_ptr` used for `CConnman::m_interrupt_net` resolves the possible lifetime issues with it (see the removed comment for that variable).
0802398 fuzz: make it possible to mock (fuzz) CThreadInterrupt (Vasil Dimov) 6d9e5d1 fuzz: add CConnman::SocketHandler() to the tests (Vasil Dimov) 3265df6 fuzz: add CConnman::InitBinds() to the tests (Vasil Dimov) 91cbf4d fuzz: add CConnman::CreateNodeFromAcceptedSocket() to the tests (Vasil Dimov) 50da743 fuzz: add CConnman::OpenNetworkConnection() to the tests (Vasil Dimov) e6a917c fuzz: add Fuzzed NetEventsInterface and use it in connman tests (Vasil Dimov) e883b37 fuzz: set the output argument of FuzzedSock::Accept() (Vasil Dimov) Pull request description: Extend `CConnman` fuzz tests to also exercise the methods `OpenNetworkConnection()`, `CreateNodeFromAcceptedSocket()`, `InitBinds()` and `SocketHandler()`. Previously fuzzing those methods would have resulted in real socket functions being called in the operating system which is undesirable during fuzzing. Now that bitcoin#21878 is complete all those are mocked to a fuzzed socket and a fuzzed DNS resolver (see how `CreateSock` and `g_dns_lookup` are replaced in the first commit). ACKs for top commit: achow101: ACK 0802398 jonatack: Review re-ACK 0802398 dergoegge: Code review ACK 0802398 Tree-SHA512: a717d4e79f42bacf2b029c821fdc265e10e4e5c41af77cd4cb452cc5720ec83c62789d5b3dfafd39a22cc8c0500b18169aa7864d497dded729a32ab863dd6c4d
l0rinc
force-pushed
the
l0rinc/warn-dirty-coin-coint
branch
from
2eccef6 to
142d2af
Compare
…aceCoinInternalDANGER` The `EmplaceCoinInternalDANGER` method was unconditionally adding to `cachedCoinsUsage`, but should only do so when `try_emplace` actually inserts a new entry. If the entry already exists, no memory is allocated and usage should not change. Also moves `cachedCoinsUsage = 0` inside the `if (fOk)` block in `Flush()` for consistency - the reset should only happen when the flush succeeds. Adds test coverage by randomly calling `EmplaceCoinInternalDANGER` in `SimulationTest` to verify the accounting remains correct. Co-authored-by: Pieter Wuille <pieter@wuille.net> Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Refactors test code to use an explicit `AddUsage(size_t)` method instead of exposing the raw `cachedCoinsUsage` reference via `usage()`.
Adds `m_dirty_count` member to track the running count of dirty cache entries as follows: * Incremented when entries are marked dirty via `CCoinsCacheEntry::SetDirty` * Decremented when dirty entries are removed or cleaned * Passed through `CoinsViewCacheCursor` and updated during iteration * Validated in `SanityCheck()` by recomputing from scratch The dirty count is needed because after non-wiping flushes (introduced in bitcoin#28280 and bitcoin#28233), the percentage of dirty entries in the cache may be far below 100%. Using total cache size for flush warnings and disk space checks is therefore misleading. Updates all test code to properly initialize and maintain the dirty count. Co-authored-by: l0rinc <pap.lorinc@gmail.com>
…ecks Changes flush warnings to use the actual number of dirty entries being written rather than total cache size or memory usage: * Moves warning from `FlushStateToDisk` to `CCoinsViewDB::BatchWrite` so it applies to both regular flushes and `AssumeUTXO` snapshot writes * Changes threshold from `WARN_FLUSH_COINS_SIZE` (1 GiB) to `WARN_FLUSH_COINS_COUNT` (10M entries), approximately equivalent - this also helps with the confusion caused by UTXO size difference on-disk vs in-memory * Moves benchmark logging to `BatchWrite` where the actual disk I/O occurs to make sure AssumeUTXO also warns * Uses dirty count for disk space check (48 bytes per entry estimate) * Removes redundant `changed` counter since `dirty_count` is now tracked This ensures users are warned appropriately even when only a fraction of the cache is dirty, and provides accurate warnings during `AssumeUTXO` loads. Co-authored-by: l0rinc <pap.lorinc@gmail.com>
l0rinc
force-pushed
the
l0rinc/warn-dirty-coin-coint
branch
from
142d2af to
4b31781
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Revival of bitcoin#31703
Since bitcoin#28280 and bitcoin#28233, the percentage of dirty entries in the cache when a flush happen may in practice be far from 100%. The log output, the decision to warn about a large flush (bitcoin#31534), and the disk-full detection, however always use the entire size of the cache.
Fix this by keeping track of the number of dirty entries in CCoinsViewCache, and using that number. I've dropped the usage of DynamicMemoryUsage as it's the wrong metric, even before the non-wiping flushes were introduced (if everything is dirty, memory usage is correlated with, but not the same as, the amount of disk space written or used). They could be improved by keeping track of proper write size estimates in a follow-up, but here I'm just using a fixed 48 bytes per entry estimate. Also covered
EmplaceCoinInternalDANGERinSimulationTest