744d47fcee0d32a71154292699bfdecf954a6065 clusterlin: adopt trained cost model (feature) (Pieter Wuille)
4eefdfc5b7d0b86a523683de2a90da910b77a106 clusterlin: rescale costs (preparation) (Pieter Wuille)
ecc9a84f854e5b77dfc8876cf7c9b8d0f3de89d0 clusterlin: use 'cost' terminology instead of 'iters' (refactor) (Pieter Wuille)
9e7129df2962f7c52d07c14a56398bb285cac084 clusterlin: introduce CostModel class (preparation) (Pieter Wuille)
Pull request description:
Part of #30289, replaces earlier #34138.
This introduces a more accurate cost model for SFL, to control how much CPU time is spent inside the algorithm for clusters that cannot be linearized perfectly within a reasonable amount of time.
The goal is having a metric for the amount of work performed, so that txmempool can impose limits on that work: a lower bound that is always performed (unless optimality is reached before that point, of course), and an upper bound to limit the latency and total CPU time spent on this. There are conflicting design goals here:
* On the one hand, it seems ideal if this metric is closely correlated to actual CPU time, because otherwise the limits become inaccurate.
* On the other hand, it seems a nightmare to have the metric be platform/system dependent, as it makes network-wide reasoning nearly impossible. It's expected that slower systems take longer to do the same thing; this holds for everything, and we don't need to compensate for this.
There are multiple solutions to this:
* One extreme is just measuring the time. This is very accurate, but extremely platform dependent, and also non-deterministic due to random scheduling/cache effects.
* The other extreme is using a very abstract metric like counting how many times certain loops/function inside the algorithm run. That is what is implemented in master right now, just counting the sum of the numbers of transactions updated across all `UpdateChunks()` calls. It however necessarily fails to account for significant portions of runtime spent elsewhere, resulting in a rather wide range of "ns per cost" values.
* This PR takes a middle ground, counting many function calls / branches / loops, with weights that were determined through benchmarking on an average on a number of systems.
Specifically, the cost model was obtained by:
* For a variety of machines:
* Running a fixed collection of ~385000 clusters found through random generation and fuzzing, optimizing for difficulty of linearization.
* Linearize each 1000-5000 times, with different random seeds. Sometimes without input linearization, sometimes with a bad one.
* Gather cycle counts for each of the operations included in this cost model, broken down by their parameters.
* Correct the data by subtracting the runtime of obtaining the cycle count.
* Drop the 5% top and bottom samples from each cycle count dataset, and compute the average of the remaining samples.
* For each operation, fit a least-squares linear function approximation through the samples.
* Rescale all machine expressions to make their total time match, as we only care about relative cost of each operation.
* Take the per-operation average of operation expressions across all machines, to construct expressions for an average machine.
* Approximate the result with integer coefficients.
The benchmarks were performed by `l0rinc <pap.lorinc@gmail.com>` and myself, on AMD Ryzen 5950X, AMD Ryzen 7995WX, AMD Ryzen 9980X, Apple M4 Max, Intel Core i5-12500H, Intel Core Ultra 7 155H, Intel N150 (Umbrel), Intel Core i7-7700, Intel Core i9-9900K, Intel Haswell (VPS, virtualized), Intel Xeon E5-2637, ARM Cortex-A76 (Raspberry Pi 5), ARM Cortex-A72 (Raspberry Pi 4).
Based on final benchmarking, the "acceptable" iteration count (which is the minimum spent on every cluster) is to 75000 units, which corresponds to roughly 50 μs on Ryzen 5950X and similar modern desktop hardware.
ACKs for top commit:
instagibbs:
ACK 744d47fcee0d32a71154292699bfdecf954a6065
murchandamus:
reACK 744d47fcee0d32a71154292699bfdecf954a6065
Tree-SHA512: 5cb37a6bdd930389937c435f910410c3581e53ce609b9b594a8dc89601e6fca6e6e26216e961acfe9540581f889c14bf289b6a08438a2d7adafd696fc81ff517
Modify fuzzer logic to avoid setting `FRESH` for an outpoint that already exists unspent in the parent view, and ensure `FRESH` implies `DIRTY`.
This keeps cursor invariants realistic and lets `BatchWrite` failures expose real bugs without resetting state.
The coins view fuzzer can call `AddCoin` with `possible_overwrite=false` for an outpoint that already exists unspent in the view, which violates the `AddCoin` caller contract.
Derive `possible_overwrite` from `PeekCoin` so `possible_overwrite=false` is only used when the outpoint is absent.
This matches the approach used by the `coinscache_sim` fuzzer, which derives the overwrite flag from simulated state.
In validation, `AddCoins(check_for_overwrite=false)` is only used after BIP30 has already ensured the transaction does not overwrite any unspent outputs in the UTXO view.
The coins view fuzz target can call `AddCoins` with arbitrary txids, so using the `check_for_overwrite=false` fast path on non-coinbase transactions may violate the `AddCoin` caller contract and trigger logic errors.
Only use `check_for_overwrite=false` when we have first confirmed that none of the outputs are currently unspent.
Otherwise, fall back to `check_for_overwrite=true` so `AddCoins` determines overwrites via the view.
4b53cbd69220c1c786bb23a72c0b26a6f78a38f7 test: Test for musig() in various miniscript expressions (Ava Chow)
ec0f47b15cb3269015523e6fab8ae9241f4181a1 miniscript: Using Func and Expr when parsing keys, hashes, and locktimes (Ava Chow)
6fd780d4fbc497b657025afe48d0dfbf103ee120 descriptors: Increment key_exp_index in ParsePubkey(Inner) (Ava Chow)
b12281bd86e2298ba6cdd79d55c9d6e23e5136a5 miniscript: Use a reference to key_exp_index in KeyParser (Ava Chow)
ce4c66eb7c5e99e3df1c20d5c0ae8278a714b9f8 test: Test that key expression indexes match key count (Ava Chow)
Pull request description:
The miniscript parser currently only looks for the next `)` when parsing key, hash, and locktime expressions. This fails to parse when the expressions contain a nested expression. Currently, this is only possible with `musig()` inside of key expressions. However, this pattern can be generalized to handling hashes and locktimes, so I implemented those too.
Fixes#34076
ACKs for top commit:
rkrux:
ACK 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7
sipa:
ACK 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7
darosior:
Other than that, Approach ACK 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7. That makes sense to me but i have not closely reviewed the code.
Tree-SHA512: 01040c7b07a59d8e3725ff11ab9543b256aea22535fb94059f490a5bb45319e859666af04c2f0a4edcb8cf1e6dfc7bd8a8271b21ad81143bafccd4d0a39cae9c
cae6d895f8a8cf5f57e05519536fda5d62b10841 fuzz: add target for CoinsViewOverlay (Andrew Toth)
86eda88c8e486eb1db724e60948f71349d050e1d fuzz: move backend mutating block to end of coins_view (Andrew Toth)
89824fb27b228a12d5c2f63106c2a4d793e73107 fuzz: pass coins_view_cache to TestCoinsView in coins_view (Andrew Toth)
73e99a59665551243d6dbe03a0e9baa9cab046b9 coins: don't mutate main cache when connecting block (Andrew Toth)
67c0d1798e6147f48d4bafc2c9e5ff30f2a62340 coins: introduce CoinsViewOverlay (Andrew Toth)
69b01af0eb9017a6ae7ca3134c9dcf89e74dbfa8 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 cae6d895f8a8cf5f57e05519536fda5d62b10841
sipa:
reACK cae6d895f8a8cf5f57e05519536fda5d62b10841
sedited:
Re-ACK cae6d895f8a8cf5f57e05519536fda5d62b10841
willcl-ark:
ACK cae6d895f8a8cf5f57e05519536fda5d62b10841
vasild:
Cursory ACK cae6d895f8a8cf5f57e05519536fda5d62b10841
ryanofsky:
Code review ACK cae6d895f8a8cf5f57e05519536fda5d62b10841. 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
2a1d0db7994eb2aa8527944f62161b6b8af682da doc: Mention private broadcast RPCs in release notes (Andrew Toth)
c3378be10b0a90e81b46e53eb85c41eb8caabac5 test: Cover abortprivatebroadcast in p2p_private_broadcast (Andrew Toth)
557260ca14ac5fb4732f4ce0692a2bf364bb5238 rpc: Add abortprivatebroadcast (Andrew Toth)
15dff452eb61ae9e2fd7b48c957e795c4c397443 test: Cover getprivatebroadcastinfo in p2p_private_broadcast (Andrew Toth)
996f20c18af02281034c51af4b2766d8f4d37a2c rpc: Add getprivatebroadcastinfo (Andrew Toth)
5e64982541f301773156a87988c60ca7797a3f06 net: Add PrivateBroadcast::GetBroadcastInfo (Andrew Toth)
573bb542be80b63b1713a0b76bedaa5e37c3783f net: Store recipient node address in private broadcast (Andrew Toth)
Pull request description:
Follow up from #29415
Sending a transaction via private broadcast does not have any way for a user to track the status of the transaction before it gets returned by another peer. The default logs have been removed as well in #34267. Nor is there any way to abort a transaction once it has been added to the private broadcast queue.
This adds two new RPCs:
- `getprivatebroadastinfo` returns information about what transactions are in the private broadcast queue, including all the peers' addresses we have chosen and timestamps.
- `abortprivatebroadcast` stops broadcasting a transaction in the private broadcast queue.
ACKs for top commit:
nervana21:
tACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
achow101:
ACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
l0rinc:
ACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
danielabrozzoni:
tACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
sedited:
ACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
Tree-SHA512: cc8682d0be68a57b42bea6e3d091da2b80995d9e6d3b98644cb120a05c2b48a97c2e211173289b758c4f4e23f1d1a1f9be528a9b8c6644f71d1dd0ae5f673326
afb1bc120ecce2bf663093e15c93f5592c0d4a98 validation: Use dirty entry count in flush warnings and disk space checks (Pieter Wuille)
b413491a1cdd9a51f2aa10b775650f54f6785e3e coins: Keep track of number of dirty entries in `CCoinsViewCache` (Pieter Wuille)
7e52b1b945c4137e0fb05715090635ce82ed04b3 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 afb1bc120ecce2bf663093e15c93f5592c0d4a98
sipa:
Code review ACK afb1bc120ecce2bf663093e15c93f5592c0d4a98
sedited:
ACK afb1bc120ecce2bf663093e15c93f5592c0d4a98
Tree-SHA512: 4133c6669fd20836ae2fb62ed804cdf6ebaa61076927b54fc412e42455a2f0d4cadfab0844064f9c32431eacb1f5e47b78de8e5cde1b26ba7239a7becf92f369
726b3663cc8e2164d4e9452f12f5866f5e8f6f1a http: properly respond to HTTP request during shutdown (furszy)
59d24bd5dd2a4549888cf7c557461e6b4959f82f threadpool: make Submit return Expected instead of throwing (furszy)
Pull request description:
Fixes#34573.
As mentioned in https://github.com/bitcoin/bitcoin/issues/34573#issuecomment-3891596958, the ThreadPool PR (#33689) revealed an existing issue.
Before that PR, we were returning an incorrect error "Request rejected because http work queue depth exceeded" during shutdown for unhandled requests (we were not differentiating between "queue depth exceeded" and "server interrupted" errors). Now, with the ThreadPool inclusion, we return the proper error but we don't handle it properly.
This PR improves exactly that. Handling the missing error and properly returning it to the user.
The race can be reproduced as follows:
1) The server receives an http request.
2) Processing of the request is delayed, and shutdown is triggered in the meantime.
3) During shutdown, the libevent callback is unregistered and the threadpool interrupted.
4) The delayed request (step 2) resumes and tries to submit a task to the now-interrupted server.
Reproduction test can be found https://github.com/bitcoin/bitcoin/pull/34577#issuecomment-3902672521.
Also, to prevent this kind of issue from happening again, this PR changes task submission
to return the error as part of the function's return value using `util::Expected` instead of
throwing the exception. Unlike exceptions, which require extra try-catch blocks and can be
ignored, returning `Expected` forces callers to explicitly handle failures, and attributes
like `[[nodiscard]]` allow us catch unhandled ones at compile time.
ACKs for top commit:
achow101:
ACK 726b3663cc8e2164d4e9452f12f5866f5e8f6f1a
sedited:
ACK 726b3663cc8e2164d4e9452f12f5866f5e8f6f1a
pinheadmz:
re-ACK 726b3663cc8e2164d4e9452f12f5866f5e8f6f1a
andrewtoth:
ACK 726b3663cc8e2164d4e9452f12f5866f5e8f6f1a
hodlinator:
re-ACK 726b3663cc8e2164d4e9452f12f5866f5e8f6f1a
Tree-SHA512: ef026e299adde1148c9fc575e7d937e957bf0ddedfc1cf081941b568736417c2eefcd8bc8c8aea795d7347040ed05da4371bddcdbda7d385e04bf4dc8d875780
24f93c9af7f6627cd7d09a1a5f10667846b048eb release note (Pol Espinasa)
331a5279d2775fb701a0bf4607436ec05e476df3 wallet, rpc:remove settxfee and paytxfee (Pol Espinasa)
Pull request description:
**Summary**
This PR removes the settxfee RPC and paytxfee setting (Bitcoin Core 31.0).
These two features were deprecated in https://github.com/bitcoin/bitcoin/pull/31278.
ACKs for top commit:
achow101:
ACK 24f93c9af7f6627cd7d09a1a5f10667846b048eb
w0xlt:
reACK 24f93c9af7f6627cd7d09a1a5f10667846b048eb
Tree-SHA512: e090f1a72ba2cbeba7c982dd51dfdcf6baf0a164827337cf56fd85f733e143b8d6116b8cd53c59c812cacef193dfa0b101a830fc455e32bf225e8505e7b2a554
fb3e1bf9c9772631571ca46d29c50330ebf54dfd test: check LoadBlockIndex correctly recomputes invalidity flags (stratospher)
29740c06ac53f55f71acf2a1b42b193aac39f579 validation: remove BLOCK_FAILED_MASK (stratospher)
b5b2956bda32b7b4ebc25c83b4d792ecd01f02b4 validation: reset BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID when loading from disk (stratospher)
37bc207852788340dc2a1b33a73748f43226978a validation: stop using BLOCK_FAILED_CHILD (stratospher)
120c631e16893821ea4c73ff70ac60e4fec0429f refactor: use clearer variables in InvalidateBlock() (stratospher)
18f11695c755c379ca67ca0bce8d17492ad9af18 validation: don't update BLOCK_FAILED_VALID to BLOCK_FAILED_CHILD in InvalidateBlock (stratospher)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/32173
even though we have a distinction between `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` in the codebase,
we don't use it for anything. Whenever we check for BlockStatus, we use `BLOCK_FAILED_MASK` which encompasses both of them.
Since there is no functional difference between `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` and it's added
code complexity to correctly categorise them (ex: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r1914366243, https://github.com/bitcoin/bitcoin/pull/16856#issuecomment-565506585), we could just remove it.
Looking for conceptual feedback on whether it's better to improve handling of `BLOCK_FAILED_CHILD` in the codebase or remove `BLOCK_FAILED_CHILD`.
Of less relevance, but it would also fix a `reconsiderblock` crash that could happen in the situation mentioned in https://github.com/bitcoin/bitcoin/issues/32173#issuecomment-2767030982
Similar attempt in the past in https://github.com/bitcoin/bitcoin/pull/16856#issuecomment-568073859
ACKs for top commit:
stickies-v:
re-ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd
alexanderwiederin:
ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd
mzumsande:
re-ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd
Tree-SHA512: e97b739885c40a8c021966438e9767cc02bc183056236d6a8c64f6819347ae70c0fbcd71cc2528917560d9f4fd56aed45faf1b6c75d98de7b08b621693a97fbc
Unlike exceptions, which can be ignored as they require extra try-catch
blocks, returning expected errors forces callers to always handle
submission failures.
Not throwing an exception also fixes an unclean shutdown bug
#34573 since we no longer throw when attempting to Submit()
from the libevent callback http_request_cb().
even though we have a distinction between BLOCK_FAILED_VALID
and BLOCK_FAILED_CHILD in the codebase, we don't use it for
anything. since there's no functional difference between them
and it's unnecessary code complexity to categorise them correctly,
just mark as BLOCK_FAILED_VALID instead.
Since the deterministic ordering change, SpanningForestState holds a
reference to the DepGraph it is linearizing. So this means we do not
need to pass it to SanityCheck() as an argument anymore.
c1355493e2c26b613109bfac3dcd898b3acca75a refactor: fees: split fee rate format from fee estimate mode (ismaelsadeeq)
922ebf96ed6674ae7acc6f0cde4d7b064f759834 refactor: move-only: move `FeeEstimateMode` enum to `util/fees.h` (ismaelsadeeq)
Pull request description:
### Motivation
Part of #34075
- The `FeeEstimateMode` enum was responsible for both selecting the fee estimation algorithm and specifying the fee rate' format.
#### Changes in this PR:
* The `FeeEstimateMode` enum (`UNSET`, `ECONOMICAL`, `CONSERVATIVE`) is moved to a new util/fees.h header.
* A new `FeeRateFormat `enum (`BTC_KVB`, `SAT_VB`) is introduced in `policy/feerate.h` for feerate formatting.
* The `CFeeRate::ToString()` method is updated to use `FeeRateFormat`.
* All relevant function calls have been updated to use the new `FeeRateFormat` enum for formatting and `FeeEstimateMode` for fee estimation mode.
This refactoring separates these unrelated responsibilities to improve code clarity.
ACKs for top commit:
l0rinc:
ACK c1355493e2c26b613109bfac3dcd898b3acca75a
furszy:
utACK c1355493e2c26b613109bfac3dcd898b3acca75a
musaHaruna:
ACK [c135549](c1355493e2) — reviewed in the context of PR [34075](https://github.com/bitcoin/bitcoin/pull/34075)
willcl-ark:
ACK c1355493e2c26b613109bfac3dcd898b3acca75a
Tree-SHA512: 7cbe36350744313d3d688d3fd282a58c441af1818b1e8ad9cddbc911c499a5205f8d4a39c36b21fed60542db1ef763eb69752d141bcef3393bf33c0922018645
2cb7e99deee1017a6edd94d82de556895138361d test: also reset CConnman::m_private_broadcast in tests (Vasil Dimov)
91b7c874e2b1479ed29f067cd1bef7724aabd951 test: add ConnmanTestMsg convenience method Reset() (Vasil Dimov)
Pull request description:
Member variables of `CConnman::m_private_broadcast` (introduced in
https://github.com/bitcoin/bitcoin/pull/29415) could influence the tests
which creates non-determinism if the same instance of `CConnman` is used
for repeated test iterations.
So, reset the state of `CConnman::m_private_broadcast` from
`ConnmanTestMsg::Reset()`. Currently this affects the fuzz tests
`process_message` and `process_messages`.
Reported in https://github.com/bitcoin/bitcoin/issues/34476#issuecomment-3849088794
ACKs for top commit:
maflcko:
review ACK 2cb7e99deee1017a6edd94d82de556895138361d 🚙
Crypt-iQ:
tACK 2cb7e99deee1017a6edd94d82de556895138361d
frankomosh:
Code Review ACK 2cb7e99deee1017a6edd94d82de556895138361d
brunoerg:
code review ACK 2cb7e99deee1017a6edd94d82de556895138361d
Tree-SHA512: 0f4b114542da8dc611689457ce67034c15cbfe409b006b2db72bc74078ee9513f5ce3d0e6e67d37c127cfa0a5170fe72fe3ea45ce2a61d45a358dd11bd1881f8
Refactor TestCoinsView() to move code that directly modifies
backend_coins_view to the end of the function.
This prepares for a CoinsViewOverlay fuzz target that asserts
the backend_coins_view is not mutated by any methods before
BatchWrite is called.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Refactor TestCoinsView() to accept the cache as a parameter instead of
creating it internally. This prepares for adding a CoinsViewOverlay
fuzz target that needs to pass in a different cache type.
This is a non-functional change.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Introduce `CoinsViewOverlay`, a `CCoinsViewCache` subclass that reads
coins without mutating the underlying cache via `FetchCoin()`.
Use `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.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Introduce a helper to look up a Coin through a stack of CCoinsViewCache layers without populating parent caches.
This is useful for ephemeral views (e.g. during ConnectBlock) that want to avoid polluting CoinsTip() when validating invalid blocks.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
6f113cb1847c6890f1fbd052ff7eb8ea41ccafc5 txgraph: use fallback order to sort chunks (feature) (Pieter Wuille)
0a3351947e736c646a6dfffef24b83d003c569e7 txgraph: use fallback order when linearizing (feature) (Pieter Wuille)
fba004a3df02d8d5d47f1ad0bb1ccbfde01bb2af txgraph: pass fallback_order to TxGraph (preparation) (Pieter Wuille)
941c432a4637efd4e5040259f47f2bfed073af7c txgraph test: subclass TxGraph::Ref like mempool does (preparation) (Pieter Wuille)
39d0052cbf478a729ae0288262003bba9c12690b clusterlin: make optimal linearizations deterministic (feature) (Pieter Wuille)
8bfbba32077cb8682208ef31748a10562be027db txgraph: sort distinct-cluster chunks by equal-feerate-prefix size (feature) (Pieter Wuille)
e0bc73ba9270b860d81e479a7bddcff8cfd8bfb6 clusterlin: sort tx in chunk by feerate and size (feature) (Pieter Wuille)
6c1bcb2c7c1a0017562e99195d74c3a05444633b txgraph: clear cluster's chunk index in ~Ref (preparation) (Pieter Wuille)
7427c7d0983050543f1fc7863121d8e2bf4b1511 txgraph: update chunk index on Compact (preparation) (Pieter Wuille)
3ddafceb9afd9d493b927bc91dae324225ed8e32 txgraph: initialize Ref in AddTransaction (preparation) (Pieter Wuille)
Pull request description:
Part of #30289.
TxGraph's fundamental responsibility is deciding the order of transactions in the mempool. It relies on the `cluster_linearize.h` code to optimize it, but there can and often will be many different orderings that are essentially equivalent from a quality perspective, so we have to pick one. At a high level, the solution will involve one or more of:
* Deciding based on **internal identifiers** (`Cluster::m_sequence`, `DepGraphIndex`). This is very simple, but risks leaking information about transaction receive order.
* Deciding **randomly**, which is private, but may interfere with relay expectations, block propagation, and ability to monitor network behavior.
* Deciding **based on txid**, which is private and deterministic, but risks incentivizing grinding to get an edge (though we haven't really seen such behavior).
* Deciding **based on size** (e.g. prefer smaller transactions), which is somewhat related to quality, but not unconditionally (depending on mempool layout, the ideal ordering might call for smaller transactions first, last, or anywhere in between). It's also not a strong ordering as there can be many identically-sized transactions. However, if it were to encourage grinding behavior, incentivizing smaller transactions is probably not a bad thing.
As of #32545, the current behavior is primarily picking randomly, though inconsistently, as some code paths also use internal identifiers and size. #33335 sought to change it to use random (preferring size in a few places), with the downsides listed above.
This PR is an alternative to that, which changes the order to tie-break based on size everywhere possible, and use lowest-txid-first as final fallback. This is fully deterministic: for any given set of mempool transactions, if all linearized optimally, the transaction order exposed by TxGraph is deterministic.
The transactions within a chunk are sorted according to:
1. `PostLinearize` (which improves sub-chunk order), using an initial linearization created using the rules 2-5 below.
2. Topology (parents before children).
3. Individual transaction feerate (high to low)
4. Individual transaction weight (small to large)
5. Txid (low to high txid)
The chunks within a cluster are sorted according to:
1. Topology (chunks after their dependencies)
2. Chunk feerate (high to low)
3. Chunk weight (small to large)
4. Max-txid (chunk with lowest maximum-txid first)
The chunks across clusters are sorted according to:
1. Feerate (high to low)
2. Equal-feerate-chunk-prefix weight (small to large)
3. Max-txid (chunk with lowest maximum-txid first)
The equal-feerate-chunk-prefix weight of a chunk C is defined as the sum of the weights of all chunks in the same cluster as C, with the same feerate as C, up to and including C itself, in linearization order (but excluding such chunks that appear after C). This is a well-defined approximation of sorting chunks from small to large across clusters, while remaining consistent with intra-cluster linearization order.
ACKs for top commit:
ajtowns:
reACK 6f113cb1847c6890f1fbd052ff7eb8ea41ccafc5 it was good before and now it's better
instagibbs:
ACK 6f113cb1847c6890f1fbd052ff7eb8ea41ccafc5
marcofleon:
light crACK 6f113cb1847c6890f1fbd052ff7eb8ea41ccafc5
Tree-SHA512: 16dc43c62b7e83c81db1ee14c01e068ae2f06c1ffaa0898837d87271fa7179dd98baeb74abc9fe79220e01fdba6876defe60022c2b72badc21d770644a0fe0ac
38fd85c676a072ebf256e806beda9d7533790baa http: replace WorkQueue and threads handling for ThreadPool (furszy)
c323f882ed3841401edee90ab5261d68215ab316 fuzz: add test case for threadpool (TheCharlatan)
c528dd5f8ccc3955b00bdba869f0a774efa97fe1 util: introduce general purpose thread pool (furszy)
6354b4fd7fe819eb13274b212e426a7d10ca75d3 tests: log node JSON-RPC errors during test setup (furszy)
45930a79412dc45f9d391cd7689d029fa4f0189e http-server: guard against crashes from unhandled exceptions (furszy)
Pull request description:
This has been a recent discovery; the general thread pool class created for #26966, cleanly
integrates into the HTTP server. It simplifies init, shutdown and requests execution logic.
Replacing code that was never unit tested for code that is properly unit and fuzz tested.
Although our functional test framework extensively uses this RPC interface (that’s how
we’ve been ensuring its correct behavior so far - which is not the best).
This clearly separates the responsibilities:
The HTTP server now focuses solely on receiving and dispatching requests, while ThreadPool handles
concurrency, queuing, and execution.
This will also allows us to experiment with further performance improvements at the task queuing and
execution level, such as a lock-free structure or task prioritization or any other implementation detail
like coroutines in the future, without having to deal with HTTP code that lives on a different layer.
Note:
The rationale behind introducing the ThreadPool first is to be able to easily cherry-pick it across different
working paths. Some of the ones that are benefited from it are #26966 for the parallelization of the indexes
initial sync, #31132 for the parallelization of the inputs fetching procedure, #32061 for the libevent replacement,
the kernel API #30595 (https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2413702370) to avoid blocking validation among others use cases not publicly available.
Note 2:
I could have created a wrapper around the existing code and replaced the `WorkQueue` in a subsequent
commit, but it didn’t seem worth the extra commits and review effort. The `ThreadPool` implements
essentially the same functionality in a more modern and cleaner way.
ACKs for top commit:
Eunovo:
ReACK 38fd85c676
sedited:
Re-ACK 38fd85c676a072ebf256e806beda9d7533790baa
pinheadmz:
ACK 38fd85c676a072ebf256e806beda9d7533790baa
Tree-SHA512: a0330e54ed504330ca874c42d4e318a909f548b2fb9ac46db8badf5935b9eec47dc4ed503d1b6f98574418e3473420ea45f60498be05545c4325cfa89dcca689
- Introduce a `FeeRateFormat` enum and change `CFeeRate::ToString()`
to use it for `BTC/kvB` vs `sat/vB` output formatting.
- Handle all enum values, hence remove default case in `CFeeRate::ToString()`
and `assert(False)` when a `FeeRateFormat` value is not handled.
- Keep `FeeEstimateMode` focused on fee estimation behavior by removing fee rate format
values from `FeeEstimateMode`.
- Update all formatting call sites and tests to pass `FeeRateFormat` explicitly, separating fee rate format
from fee-estimation mode selection.
This makes TxGraph also use the fallback order to decide the order of
chunks from distinct clusters.
The order of chunks across clusters becomes:
1. Feerate (high to low)
2. Equal-feerate-chunk-prefix (small to large)
3. Max-txid (chunk with lowest maximum-txid first)
This makes the full TxGraph ordering fully deterministic as long as all
clusters in it are optimally linearized.
This adds an std::function<strong_ordering(Ref&,Ref&)> argument to the
MakeTxGraph function, which can be used by the caller (e.g., mempool
code) to provide a fallback order to TxGraph.
This is just preparation; TxGraph does not yet use this fallback order
for anything.
This is a small change to the txgraph fuzz test to make it used objects
derived from TxGraph::Ref (SimTxObject) rather than TxGraph::Ref
directly. This matches how the mempool uses CTxMemPoolEntry, which
derives from TxGraph::Ref.
This is preparation for a future commit which will introduce simulated
txids to the transactions in this fuzz test, to be used as fallback
order.
This allows passing in a fallback order comparator to Linearize(), which
is used as final tiebreak when deciding the order of chunks and
transactions within a chunk, rather than a random tiebreak.
The order of transactions within a chunk becomes:
1. Topology (parents before children)
2. Individual transaction feerate (high to low)
3. Weight (small to large)
4. Fallback (low to high fallback order)
The order of chunks within a cluster becomes:
1. Topology (chunks after their dependencies)
2. Feerate (high to low)
3. Weight (small to large)
4. Max-fallback (chunk with lowest maximum-fallback-tx first)
For now, txgraph passes a naive comparator to Linearize(), which makes
the cluster order deterministic when treating the input transactions as
identified by the DepGraphIndex. However, since DepGraphIndexes are the
result of possibly-randomized operations inside txgraph, this doesn't
actually make txgraph's per-cluster ordering deterministic. That will be
changed in a later commit, by using a txid-based fallback instead.
This makes TxGraph track the equal-feerate-prefix size of all chunks in
all clusters in the main graph, and uses it to sort chunks coming from
distinct clusters.
The order of chunks across clusters becomes:
1. Feerate (high to low)
2. Equal-feerate-prefix (small to large)
3. Cluster sequence number (old to new); this will be changed later.
The equal-feerate-prefix size of a chunk C is defined as the sum
of the weights of all chunks in the same cluster as C, with the same
feerate as C, up to and including C itself, in linearization order (but
excluding such chunks that appear after C).
This is an approximation of sorting chunks from small to large across
clusters, while remaining consistent with intra-cluster linearization
order.
This changes the order of transactions within a chunk to be:
1. Topology (parents before children)
2. Individual transaction feerate (high to low)
3. Individual transaction weight (small to large)
4. Random tiebreak (will be changed in a future commit)
To do so, use a heap of topology-ready transactions within
GetLinearization(), sorted by (2), (3), and (4).
This is analogous to the order of chunks within a cluster, which is
unchanged:
1. Topology (chunks after chunks they depend on)
2. Chunk feerate (high to low)
3. Chunk weight (small to large)
4. Random tiebreak (will be changed in a future commit)
Instead of returning a TxGraph::Ref from TxGraph::AddTransaction(),
pass in a TxGraph::Ref& which is updated to refer to the new transaction
in that graph.
This cleans up the usage somewhat, avoiding the need for dummy Refs in
CTxMemPoolEntry constructor calls, but the motivation is that a future
commit will allow a callback to passed to MakeTxGraph to define a
fallback order on the transaction objects. This does not work when a
Ref is created separately from the CTxMemPoolEntry it ends up living in,
as passing the newly-created Ref to the callback would be UB before it's
emplaced in its final CTxMemPoolEntry.
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
The dirty count is needed because after non-wiping flushes (introduced in #28280 and #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>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: optout <13562139+optout21@users.noreply.github.com>
fa0677d131191d7db9868c4c1b3d780cb6991226 refactor: Use SpanReader over DataStream (MarcoFalke)
fad3eb39564569e7b09982bec68ae41e45a04f87 refactor: Use SpanReader over DataStream (MarcoFalke)
fa06e26764bbd00fc225df5f4601dd4f687273e0 refactor: [qt] Use SpanReader to avoid two vector copies (MarcoFalke)
fabd4d2e2e3ce734730c56660a958f9cf9dc7d38 refactor: Avoid UB in SpanReader::ignore (MarcoFalke)
fa20bc2ec27522959cdf1ad35d54f080aafbfc47 refactor: Use empty() over eof() in the streams interface (MarcoFalke)
fa879db735281d2cce123dbd59d20c7339b2b4ee test: Read debug log for self-checking comment (MarcoFalke)
Pull request description:
This changes all places, where possible, to use SpanReader over DataStream. This makes the code easier to read and reason about, because `SpanReader` can never write data. Also, the code should be minimally faster, because it avoids a full redundant copy of the whole vector of bytes.
ACKs for top commit:
stickies-v:
re-ACK fa0677d131191d7db9868c4c1b3d780cb6991226
achow101:
ACK fa0677d131191d7db9868c4c1b3d780cb6991226
janb84:
re ACK fa0677d131191d7db9868c4c1b3d780cb6991226
sipa:
crACK fa0677d131191d7db9868c4c1b3d780cb6991226
Tree-SHA512: 1d9f43fc6e71d481cf7b8f8457f479745ee331734649e9e2c2ab00ce5d317112796c77afc328612ed004e65ac5c16fc92279d760cfb012cfddce9098c4af810f
fa43897c1d14549e7af0d9f912e765875b634c39 doc: Fix LLM nits in net_processing.cpp (MarcoFalke)
bbbba0fd4b87a5441c90d513d2022f4c4d9678cb scripted-diff: Use references when nullptr is not possible (MarcoFalke)
fac54154660438db6a601584fa91f87bc09395b2 refactor: Separate peer/maybe_peer in ProcessMessages and SendMessages (MarcoFalke)
fac529188e0db44875f8728c9e0b6a05d2145e75 refactor: Pass Peer& to ProcessMessage (MarcoFalke)
fa376095a01c421523ec5d012c6aafb006011788 refactor: Pass CNode& to ProcessMessages and SendMessages (MarcoFalke)
fada8380148c1266f2cc1ddb0f65f42651c82a62 refactor: Make ProcessMessage private again (MarcoFalke)
fa80cd3ceed4eb58732c2f6f748277772a8a1c36 test: [refactor] Avoid calling private ProcessMessage() function (MarcoFalke)
Pull request description:
There is a single unit test, which calls the internal `ProcessMessage` function. This is problematic, because it makes future changes harder, since they will need to carry over this public internal interface each time.
Also, there is a mixed use of pointers and references in p2p code, where just based on context, a pointer may sometimes assumed to be null, or non-null. This is confusing when reading the code, or making or reading future changes.
Fix both issues in a series of commits, to:
* refactor the single unit test to call higher-level functions
* Make `ProcessMessage` private again
* Use references instead of implicit non-null pointers, mostly in a scripted-diff
ACKs for top commit:
optout21:
reACK fa43897c1d14549e7af0d9f912e765875b634c39
ajtowns:
ACK fa43897c1d14549e7af0d9f912e765875b634c39
Crypt-iQ:
crACK fa43897c1d14549e7af0d9f912e765875b634c39
achow101:
ACK fa43897c1d14549e7af0d9f912e765875b634c39
Tree-SHA512: d03d8ea35490a995f121be3d2f3e4a22d1aadfeab30bc42c4f8383dab0e6e27046260e792d9e5a94faa6777490ba036e39c71c50611a38f70b90e3a01f002c9e
This refactor does not change behavior. However, it avoids a vector
copy, which can lead to a minimal speed-up of 1%-5%, depending on the
call-site. This is mostly relevant for the fuzz tests and utils that
read large blobs of data (like a full block).
d511adb664edcfb97be44bc0738f49b679240504 [miner] omit dummy extraNonce via IPC (Sjors Provoost)
bf3b5d6d069a0bbb39af0c487fd597257f862f31 test: clarify getCoinbaseRawTx() comparison (Sjors Provoost)
78df9003d63414e4a17b686af7647aeefd706ec5 [doc] Update comments on dummy extraNonces in tests (Anthony Towns)
Pull request description:
This PR changes the Mining IPC interface to stop including a dummy `extraNonce` in the coinbase `scriptSig` by default, exposing only the consensus-required BIP34 height. This simplifies downstream mining software (including Stratum v2), avoids forcing clients to strip or ignore data we generate, and reduces the risk of incompatibilities if future soft forks add required commitments to the `scriptSig`.
Existing behavior is preserved for RPCs, tests, regtest, and internal mining by explicitly opting in to the dummy `extraNonce` where needed (e.g. to satisfy `bad-cb-length` at low heights), so consensus rules and test coverage are unchanged. The remainder of the PR consists of small comment fixes, naming clarifications, and test cleanups to make the intent and behavior clearer.
ACKs for top commit:
achow101:
ACK d511adb664edcfb97be44bc0738f49b679240504
ryanofsky:
Code review ACK d511adb664edcfb97be44bc0738f49b679240504. Just rebased since last review and make suggested tweaks. I'd really like to see this PR merged for the cleanups and sanity it brings to this code. Needs another reviewer though.
sedited:
ACK d511adb664edcfb97be44bc0738f49b679240504
Tree-SHA512: d41fa813eb6b5626f4f475d8abc506b29090f4a2d218f2d6824db58b5ebe2ed7c584a903b44de18ccec142bb79c257b0aba6d6da073f56175aec88df96aaaaba
Since pk(), pk_k(), pkh(), pk_h(), sha256(), ripemd160(), hash256(),
hash160(), after(), and older() all are single argument expressions that
are parsed immediately, we can use the Expr and Func parsing functions
to determine what the arguments of these expressions are, rather than
searching for the next closing parentheses.
This fixes an issue when pk(), pk_k(), pkh(), and pk_h() include a
musig() expression as Expr properly handles nested expressions.
4fec726c4d352daf2fb4a7e5ed463e44c8815ddb refactor: Simplify Interpret asmap function (Fabian Jahr)
79e97d45c16f043d23ba318a661cc39ec53cf760 doc: Add more extensive docs to asmap implementation (Fabian Jahr)
cf4943fdcdd167a56c278ba094cecb0fa241a8f8 refactor: Use span instead of vector for data in util/asmap (Fabian Jahr)
385c34a05261846dac2b42d47f69b317f534dd40 refactor: Unify asmap version calculation and naming (Fabian Jahr)
fa41fc6a1a7d492b894e206f83e0c9786b44a2f0 refactor: Operate on bytes instead of bits in Asmap code (Fabian Jahr)
Pull request description:
This is a second slice carved out of #28792. It contains the following changes that are crucial for the embedding of asmap data which is added the following PR in the series (probably this will remain in #28792).
The changes are:
- Modernizes and simplifies the asmap code by operating on `std::byte` instead of bits
- Unifies asmap version calculation and naming (previously it was called version and checksum interchangeably)
- Operate on a `span` rather than a vector in the asmap internal to prevent holding the asmap data in memory twice
- Add more extensive documentation to the asmap implementation
- Unify asmap casing in implemetation function names
The first three commits were already part of #28792, the others are new.
The documentation commit came out of feedback gathered at the latest CoreDev. The primary input for the documentation was the documentation that already existed in the Python implementation (`contrib/asmap/asmap.py`) but there are several other comments as well. Please note: I have also asked several LLMs to provide suggestions on how to explain pieces of the implementation and better demonstrate how the parts work together. I have copied bits and pieces that I liked but everything has been edited further by me and obviously all mistakes here are my own.
ACKs for top commit:
hodlinator:
re-ACK 4fec726c4d352daf2fb4a7e5ed463e44c8815ddb
sipa:
ACK 4fec726c4d352daf2fb4a7e5ed463e44c8815ddb
sedited:
Re-ACK 4fec726c4d352daf2fb4a7e5ed463e44c8815ddb
Tree-SHA512: 950a591c3fcc9ddb28fcfdc3164ad3fbd325fa5004533c4a8b670fbf8b956060a0daeedd1fc2fced1f761ac49cd992b79cabe12ef46bc60b2559a7a613d0e166
dfb93646093f8d71be455fc95e2e06ff73fb9211 fuzz: pull latest FuzzedDataProvider.h from upstream (b-l-u-e)
Pull request description:
Pulls down the latest version of https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/fuzzer/FuzzedDataProvider.h, after https://github.com/llvm/llvm-project/pull/177794 was merged upstream.
ACKs for top commit:
fanquake:
ACK dfb93646093f8d71be455fc95e2e06ff73fb9211 - updated the PR description.
Tree-SHA512: 36d003a1d92158537811d044bf4b79d63f0ec79e5b0da9e26a73a869c81578f9597ababcb79bb1949689db506c25341255c59979214ee28343fa08f85e2c3466
facb2aab26dffbc1e46809ac776ed43b9eaa9ad4 test: Turn ElapseSteady into SteadyClockContext (MarcoFalke)
Pull request description:
`ElapseSteady` was introduced a while back, but is only used in one place. It makes more sense if this were a context manager, so that mocktime does not leak from one test into the next.
So turn it into a context manager, rename it and allow easy time advancement via e.g. `steady_ctx += 1h`.
ACKs for top commit:
l0rinc:
ACK facb2aab26dffbc1e46809ac776ed43b9eaa9ad4
ismaelsadeeq:
utACK facb2aab26dffbc1e46809ac776ed43b9eaa9ad4
sedited:
ACK facb2aab26dffbc1e46809ac776ed43b9eaa9ad4
Tree-SHA512: 1df9cc9685d9be4d3ab8deafd99ac1a5ff752064ae54b83bacd6f44ba2c198b091558a306d49d8b1e2200ac669e95915cc792d589fb3a63b2bef7891d325a1e0
