7d54a88b44 iwyu: Fix warnings in `src/rpc` and treat them as errors (Hennadii Stepanov)
462ff18d79 iwyu, refactor: Fix includes in some source files (Hennadii Stepanov)
Pull request description:
This PR continues the ongoing effort to enforce IWYU warnings.
See [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu).
ACKs for top commit:
stickies-v:
re-ACK 7d54a88b44
Tree-SHA512: aeeb8e2851e4012c4e8474c524fafae01f6c323537d533c693e9b018ee2752cedf19b35b3ac0433a287b2ad7953ae0a466d20ddd3a00e854985d2404abce2512
b48a27691d fuzz: assert invalid addresses are not routable (Fabian Jahr)
e6c775c6d6 net: treat RFC 9637 new IPv6 documentation range as invalid (Fabian Jahr)
Pull request description:
[RFC 9637](https://www.rfc-editor.org/rfc/rfc9637.html) was newly introduced in 2024 but we don't handle this yet. This pull handles it in the same way as RFC 3849 of which 9637 is an extension.
This came up in #36196 because the new `GetMappedAS()` benchmark from https://github.com/bitcoin/bitcoin/pull/35285 asserts on these addresses being unmapped. New maps using `--fill` couldn't be embedded because the filling assigns these addresses. Making this range invalid in the code fixes the benchmarks.
ACKs for top commit:
0xB10C:
ACK b48a27691d
willcl-ark:
ACK b48a27691d
hodlinator:
re-ACK b48a27691d
sedited:
ACK b48a27691d
Tree-SHA512: d28552c0620f2b4c7de311d9ddf90bffb0f2dfdac4d76e305fc6b8138c8f3f070958a8a50679b45e1875cb5b24f6a7a10297ad327200b3ed5cd5ab5338187bcc
8e4b7ab725 fuzz: use per-level fetch scopes in coinscache_sim (Andrew Toth)
5292386b78 doc: improve CoinsViewOverlay documentation (Andrew Toth)
d552c52b08 coins: log error reason when prevout fetch submission fails (Andrew Toth)
2ffaa6e6a7 coins: delete Sync and SetBackend on CoinsViewOverlay (Andrew Toth)
330022993f coins: filter coinbase txid from parallel input fetching (Andrew Toth)
Pull request description:
This addresses various follow-ups requested in https://github.com/bitcoin/bitcoin/pull/35295.
- add the coinbase txid to the filter so inputs spending the coinbase are not fetched.
- delete Sync and SetBackend from CoinsViewOverlay
- various logging and documentation improvements
- improve coinscache_sim fuzzing so we continue parallel fetching while more caches are added on to the cache stack
ACKs for top commit:
optout21:
reACK 8e4b7ab725
l0rinc:
ACK 8e4b7ab725
sedited:
ACK 8e4b7ab725
Tree-SHA512: 38001f96be6f893e2610bb81f379ecc0c40ffd39da5bfe1f5db47db1ef2f725d80ae3f9b5e25acd64e65013176ba3ba4e3e8585cb55420b2793845c292beda23
fa3971011d ci: Exclude subtrees from iwyu (MarcoFalke)
fa8566152a refactor: Bump old copyright header in univalue (MarcoFalke)
Pull request description:
The iwyu CI may modify subtrees when iwyu thinks a header inside a subtree is "associated" (due to the naming).
This happens to not be a problem on current master, but can become a problem if an iwyu-enforced file is renamed or a file is iwyu-enforced in the future.
Fix this by excluding subtrees.
Can be tested by running the iwyu CI on `src/test/fuzz/minisketch.cpp` and seeing a change in `minisketch.h` before this CI fix.
ACKs for top commit:
hebasto:
re-ACK fa3971011d.
Tree-SHA512: 9a555ab020f0f1a2bc4d70ea72011f8d42ba4bfe4a463947d31b0d208b4671b76b466f92a18b6295bc7a8c5bb67c6f697844f673fc02e18983b062d25bc0dc8c
2777300c68 fuzz: Implement connect_block harness (Robin David)
40add915be test: Add reset to CuckooCache (Eugene Siegel)
Pull request description:
Adds a fuzz target that directly calls `ConnectBlock` with `fJustCheck` set to true, so it hits block/transaction validation without writing undo data or updating the chainstate.
This PR is essentially https://github.com/bitcoin/bitcoin/pull/34651 with some minor tweaks and style cleanups. Additional validation harnesses (e.g. https://github.com/bitcoin/bitcoin/pull/34895) could build on this test's setup.
ACKs for top commit:
Crypt-iQ:
ACK 2777300c68
nervana21:
tACK 2777300c68
Tree-SHA512: e2dc74154a6e29e0f3eaec9caeeec53d64bcc96adb0d1739281da97712dd931c3937eaf71f977bfc9c9330f26e35b3633f72788143b76e24e17c37b0a4258ba4
80eaa6cabf bugfix: give TxDownloadManager its own RNG (Greg Sanders)
Pull request description:
TxDownloadManagerImpl retains a reference to PeerManagerImpl::m_rng,
which is non-thread-safe and guarded by g_msgproc_mutex.
BlockConnected runs on the validation background thread while holding
only m_tx_download_mutex. Reconsidering an orphan with multiple
announcers could therefore use m_rng concurrently with message
processing.
Regression introduced in #35986
Added a regression test on second commit, can remove it from the PR if deemed superfluous.
This is a Project Loupe find.
ACKs for top commit:
maflcko:
review ACK 80eaa6cabf🐓
hodlinator:
ACK 80eaa6cabf
sedited:
ACK 80eaa6cabf
Tree-SHA512: 2dbc4a9298bfa1375dc364ead4b1ec74c2ebe54fb7c311180fa06fc32240406be2979a2dd6ae0e7a23b099ddcd63f5c76c126d84a1405fc7eacd337eb009dd88
5e0d7a286a refactor: Drastically narrow scope of http_bitcoin namespace and rename it to bitcoin_http (Hodlinator)
8f9fd8698a refactor: Make HTTPRemoteClient fields private (Hodlinator)
d72f67fd6c refactor: Expose additional HTTPRemoteClient fields through accessors (Hodlinator)
10bbae302f refactor: Expose HTTPRemoteClient fields to tests through methods (Hodlinator)
5b06d90831 refactor: Replace HTTPServer::MaybeDispatchRequestsFromClient() with HTTPRemoteClient::TryReadRequest() (Hodlinator)
a1183c02aa refactor: Extract Send() and Receive() into HTTPRemoteClient from HTTPServer (Hodlinator)
6d9b61d4f8 refactor: Extract HTTPRemoteClient::MaybeDisconnect() from HTTPServer::DisconnectClients() (Hodlinator)
6fec8d6914 refactor: Make HTTPRequest fields private (Hodlinator)
b8cd77237b refactor: Make HTTPRequest::GetHeader() return saner optional type (Hodlinator)
e5be0dc35e refactor: Make HTTPResponse a struct since all fields are public (Hodlinator)
Pull request description:
The new HTTP server implementation in v32 has `HTTPServer` reaching into and modifying fields of `HTTPRemoteClient` and `HTTPRequest`. This PR encapsulates field data of the latter 2 types which enforces invariants and reduces cognitive load[^1]. Exposing data through accessor methods also implies adding lock annotations.
Commits:
* Makes `HTTPResponse` a struct since it is used that way. (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3336757663) [^2]
* `HTTPRequest`:
* Saner return type for `GetHeader()` (old type was mirroring the now removed libevent-wrapper and made later commits ugly).
* Make fields private.
* Simplifies boolean logic in `HTTPServer::DisconnectClients()`. (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3336757663)
* Extraction of `HTTPServer` functions into `HTTPRemoteClient`:
Refactors `HTTPRemoteClient` to be more self-contained rather than having `HTTPServer` reach into the fields of other objects. (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3339543447, https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3339543447)
* Severely narrows `http_bitcoin` namespace and renames it to `bitcoin_http` (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3264510816)
Follow-up to #35182.
[^1]: Core Guidelines: C.9: Minimize exposure of members - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c9-minimize-exposure-of-members
[^2]: Core Guidelines: C.2: Use class if the class has an invariant; use struct if the data members can vary independently - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c2-use-class-if-the-class-has-an-invariant-use-struct-if-the-data-members-can-vary-independently
ACKs for top commit:
achow101:
ACK 5e0d7a286a
janb84:
ACK 5e0d7a286a
winterrdog:
tACK 5e0d7a286a
Tree-SHA512: e1c5aa067538e31247ca74923e451038c90750ccc941ae16711dd976c8cd750bd1afaee6e4378aeee91440f7727955d9bfb32aa25a0a745613d0d771a674ebc8
TxDownloadManagerImpl retains a reference to PeerManagerImpl::m_rng,
which is non-thread-safe and guarded by g_msgproc_mutex.
BlockConnected runs on the validation background thread while holding
only m_tx_download_mutex. Reconsidering an orphan with multiple
announcers could therefore use m_rng concurrently with message
processing.
Regression introduced in 9cc7dc50bd
http_bitcoin was mostly used during #35182 to distinguish from http_libevent counterpart:
- The http_libevent namespace was introduced around the legacy code in 89c54ae4cb.
- The http_bitcoin namespace was introduced in 68b5d289d1 and extended in subsequent commits.
- The http_libevent namespace together with code it contained was removed in 8c1eea0777.
bitcoin_http is a better name as it is Bitcoin Core's implementation of the HTTP protocol, not HTTP protocol's implementation of bitcoin 402 payment required codes or anything like that.
The namespace only remains for a few constants and a type which don't have HTTP in their names.
fad1e6bf23 util: refactor: Remove deprecated SetMockTime(i64) alias (MarcoFalke)
faf87c3535 test: refactor: Use FakeNodeClock over manual/global SetMockTime (MarcoFalke)
Pull request description:
The deprecated test-only alias is only used in a few places and required in none.
In fact, it is incorrectly used in two unit tests, so first fixup those, and then remove it.
ACKs for top commit:
rustaceanrob:
ACK fad1e6bf23
Tree-SHA512: 1fa49e363bf8ccad07d61a77d3bc55c84724cd4cf034756b534cdb56614f334c0009d62dbdb08a940ddf46e0143ec6ecd6b4608baa2f8f581f56a6eef0f0abb8
7f9c4e2928 doc: add release notes (ismaelsadeeq)
e18d392689 test: add mempool estimator i/o fuzz test (ismaelsadeeq)
970f02096d fees: persist mempool policy estimator data (ismaelsadeeq)
7dcb37989d fees: move fee_estimates.dat into fees directory (ismaelsadeeq)
0db2b69e6d rpc: add verbosity option to estimatesmartfee options (ismaelsadeeq)
06bb65730e fees: gate mempool estimates on recent block coverage (ismaelsadeeq)
cfe585df25 validation: emit block mempool removal signal from ConnectTip (ismaelsadeeq)
0d88558f95 fees: return mempool estimates when it's lower than block policy (ismaelsadeeq)
693b1351af fees: add caching to MemPoolFeeRateEstimator (ismaelsadeeq)
c9bb3df29f fees: add MemPoolFeeRateEstimator class (ismaelsadeeq)
9cacf677a9 rpc: add fee_rate_estimator option to estimatesmartfee (ismaelsadeeq)
ba6c61bbdd fees: add FeeRateEstimatorManager class (ismaelsadeeq)
2cb6b831e0 fees: add EstimateFeeRate and MaximumTarget to CBlockPolicyEstimator (ismaelsadeeq)
5adb2ab084 refactor: test block policy estimator directly (ismaelsadeeq)
9c8309a890 test: rename policy estimator tests to block policy estimator tests (ismaelsadeeq)
e3d5ef1b5f fees: move StringForBlockPolicyEstimateReason to block policy estimator (ismaelsadeeq)
74245c20e0 fees: split wallet and estimator fee reasons (ismaelsadeeq)
Pull request description:
This PR is another attempt to fix#27995 using a better approach.
For background and motivation, see #27995 and the discussion in the Delving Bitcoin post [Mempool Based Fee Estimation on Bitcoin Core](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703).
This PR is currently limited to using the mempool only to lower what is recommended by the Block Policy Estimator.
Accurate and safe fee estimation using the mempool is challenging. There are open questions about how to prevent mempool games that are theoretically possible for miners [(a variant of the Finney attack)](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/6).
This is one reason this PR uses the mempool only to lower the Block Policy Estimator result. The Block Policy Estimator itself is not gameable in this way, so the combined estimate is not susceptible to this attack increasing the returned feerate.
The underlying assumption is that, with the current tools and work done to make RBF and CPFP feasible and reliable (TRUC transaction relay, ephemeral anchors, cluster size 2 package RBF), underestimation is safer than overestimation. We now assume it is relatively easy to fee-bump later if a transaction does not confirm, whereas once a fee is overestimated there is no way to recover from that.
Another open question when using the mempool for fee estimation is how to account for incoming transaction inflow.
[Bitcoin Augur](https://github.com/block/bitcoin-augur) does this by using past inflow plus a constant expected inflow to predict future inflow. I find this unconvincing for fee estimation and potentially prone to more overestimation, as past conditions are not always representative of the future. See my [review of the Augur fee rate estimator and open questions](https://github.com/block/bitcoin-augur/issues/3).
This PR uses a much simpler approach based on current user behavior, similar to the widely used method employed by mempool.space: looking at the top block of the mempool and selecting a percentile feerate depending on whether the user is economical or conservative.
Empirical data from both myself and Clara Shikhelman shows that the 75th percentile feerate for economical users and the 50th percentile feerate for conservative users provide positive confirmation guarantees, hence this is what is used in this PR.
Parallel research by Rene Pickhardt and his student suggests that using the [average fee per byte of the block template performs well](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/12).
All of these are constants that can be adjusted. There is parallel work exploring these constants and running benchmarks across fee estimators to find a sweet spot.
See also work in LND, the [LND Budget Sweeper](https://delvingbitcoin.org/t/lnds-deadline-aware-budget-sweeper/1512), which applies this idea successfully. Their approach is to estimate fees initially with bitcoind, then increment gradually as the confirmation deadline approaches, using a fixed fee budget.
Historical data indicates that this PR's approach can [reduce overestimation quite significantly (~29%)](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/8).
This is particularly useful in scenarios where the Block Policy Estimator recommends a high feerate while the mempool is empty.
<img width="1800" height="1090" alt="56f3ba26c0184521c42bb82ec9d8c9f2224d4f8e" src="https://github.com/user-attachments/assets/c035c40c-8ece-42a7-b290-d29f1ac9bf4d" />
As seen in the image above, there is only one remaining unfixed case: when there is a sudden inflow of transactions and the feerate rises, the Block Policy Estimator takes time to reflect this. In that case, users will continue to see a low feerate estimate until it slowly updates. From the historical data linked above, [this occurs about ~26% of the time](https://delvingbitcoin.org/t/mempool-based-fee-estimation-on-bitcoin-core/703/8).
Overall, we observe a **73% success rate with 0% overestimation, and 26% underestimation** with this approach.
See https://bitcoincorefeerate.com/stats for recent running stats that have almost identical data.
This PR also includes refactors that enable this work. Rather than splitting the PR and implementing changes incrementally, I opted for an end-to-end implementation:
### 1. Refactors
* Split the mixed fee reason enum into separate wallet and block policy concepts. The wallet now has a `FeeReason` enum for why the wallet selected a fee rate (`FEE_RATE_ESTIMATOR`, `MEMPOOL_MIN`, `USER_SPECIFIED`, `FALLBACK`, `REQUIRED`), while the Block Policy Estimator uses `BlockPolicyEstimateReason` for its internal threshold details.
* Move `StringForBlockPolicyEstimateReason` to the Block Policy Estimator code, keeping the estimator-specific strings with the estimator.
* Move detailed Block Policy Estimator logging out of wallet transaction creation and into the estimator path. Wallet transaction creation now logs the selected fee and wallet fee reason instead of leaking estimator internals.
* Keep the wallet RPC `fee_reason` field name for compatibility, but update its meaning to report the wallet fee reason instead of the Block Policy Estimator's internal threshold reason.
* Rename policy estimator tests and files to block-policy-specific names where appropriate.
* Update Block Policy Estimator unit tests to be independent of the mempool and validation interface.
### 2. Introduce Mempool-Based Fee Estimator and Fee Estimator Manager
* Introduce `FeeRateEstimation` and `FeeRateEstimationError` as common estimator result types, avoiding new out-parameters for fee estimation results.
* Add `FeeRateEstimatorType` to identify the estimator that produced a result.
* Add `FeeRateEstimatorManager`, responsible for owning the Block Policy Estimator and Mempool Fee Rate Estimator.
* Update the node context to store a `std::unique_ptr` to `FeeRateEstimatorManager` instead of `CBlockPolicyEstimator`.
* Update `CBlockPolicyEstimator` to no longer subscribe directly to the validation interface; instead, `FeeRateEstimatorManager` subscribes and forwards relevant notifications.
* Add a mempool fee estimator that generates a block template when called, calculates a percentile feerate, and returns the 75th percentile for economical mode or the 50th percentile for conservative mode.
* When the selected estimate is below the node's fee floor, `estimatesmartfee` still returns at least the max of `mempoolminfee` and `minrelaytxfee`.
* Add caching to the mempool estimator so new estimates are generated at most every 7 seconds while the chain tip is unchanged, assuming enough [transactions have propagated](https://bitcoin.stackexchange.com/questions/125776/how-long-does-it-take-for-a-transaction-to-propagate-through-the-network/125777#125777) to make a meaningful difference.
This heuristic will likely be replaced by requesting block templates via the general-purpose block template cache proposed here: https://github.com/bitcoin/bitcoin/issues/33389
* Update `MempoolTransactionsRemovedForBlock` to receive the connected block as well as the transactions removed from the mempool.
* Track the weight of block transactions and mempool transactions removed due to block connection after each block connection.
This data is tracked for the last 6 mined blocks. A mempool feerate estimate is returned only when the ratio of mempool transaction weight removed due to block connection to block transaction weight is greater than 75% across the tracked window. This heuristic provides rough confidence that the node's mempool matches that of the majority of the hashrate. The 75% threshold is arbitrary and can be adjusted.
There is a caveat when transactions in the local mempool are consistently not mined by the network, as described in #27995 (e.g. due to filtering).
Accounting for these transactions during fee estimation is not necessary, as they should be evicted from the mempool itself (see #33510). Handling this again within fee estimation would be redundant.
* Persist statistics for the 6 most recent mined blocks to `fees/mempool_policy_estimator.dat` during periodic flushes and shutdown, so this data is available after restarts.
* Move Block Policy Estimator data from `fee_estimates.dat` to `fees/block_policy_estimates.dat`, migrating the legacy file during startup when needed.
* Add `fee_rate_estimator` to the `estimatesmartfee` options object. Supported values are `"none"` (default combined behavior), `"block_policy"` (use only the Block Policy Estimator), and `"mempool_policy"` (use only the Mempool Fee Rate Estimator). Unknown values are treated as `"none"`.
* Add `verbosity` to the `estimatesmartfee` options object. With `verbosity >= 2`, the RPC returns recent mempool health statistics.
* Expose the selected fee rate estimator in `estimatesmartfee` results when `fee_rate_estimator` is `"none"` and the estimate succeeds.
* Add unit, functional, and fuzz test coverage for the new estimator behavior, persistence, RPC options, and estimator I/O.
<details>
<summary>see example output</summary>
```bash
bitcoin-cli estimatesmartfee 1 economical '{"verbosity": 2, "fee_rate_estimator": "none"}'
```
```json
{
"feerate": 0.00002133,
"estimator": "mempool_policy",
"blocks": 2,
"mempool_health_statistics": [
{
"block_height": 927953,
"block_weight": 3991729,
"mempool_txs_weight": 3942409
}
]
}
```
</details>
ACKs for top commit:
willcl-ark:
reACK 7f9c4e2928
jsarenik:
Approach ACK 7f9c4e2
Tree-SHA512: c35b423eea0eb34524cf5ad07822c0ab8d53e2ab78965b58c8738044c61c77352184822360ed077a51bfbf83d0226d221e988f7156b1948023707c7e1fb31495
Move block policy fee estimates from fee_estimates.dat to
fees/block_policy_estimates.dat.
On startup, migrate the legacy file to the new path when only the legacy
file exists. If both files exist, keep the new file and remove the
legacy file.
Rename the block policy estimator args source files to the generic
estimator_args.{cpp,h} names and rename FeeestPath to
BlockPolicyFeeEstPath while the path helper is moved into the shared fee
estimator argument code.
Return the removed mempool transaction info from
CTxMemPool::removeForBlock instead of dispatching the
MempoolTransactionsRemovedForBlock notification from the mempool.
Emit it from ConnectTip after mempool removal and before BlockConnected,
passing the connected block, the removed mempool transactions, and the
block height to the callback.
Because the signal now originates from ConnectTip, where the IBD state is
known, gate it on !IsInitialBlockDownload(): the notification is no longer
fired for blocks connected during initial block download or reindex, while
the mempool removal in removeForBlock still runs unconditionally. This keeps
fee rate estimators from recording blocks connected before the node is
synced.
Add MemPoolFeeRateEstimator, which calls Bitcoin Core's block
assembler with the mempool and chainstate to build a block template and
use its chunk fee rates for fee rate estimation.
Add CalculateMaxWeightPercentiles to return the 50th and 75th
percentile chunk feerates by cumulative block weight. If sparse,
EstimateFeeRate uses the higher of the minimum relay fee rate and the
current mempool minimum fee rate.
The 50th percentile is returned as the conservative estimate, and the
75th percentile as the economical estimate.
Wire MemPoolFeeRateEstimator into FeeRateEstimatorManager and add
FeeRateEstimatorType::MEMPOOL_POLICY for result attribution.
Add unit tests for the mempool fee rate estimator and fee estimator
string conversions, plus fuzz coverage for the string conversions.
Co-authored-by: willcl-ark <will@256k1.dev>
2c16efbb7b psbt: Remove unused IsNull() methods (nebula-21)
Pull request description:
This PR removes the `IsNull()` methods from `PartiallySignedTransaction`, `PSBTInput`, and `PSBTOutput`, along with their calls from the fuzz target.
This methods have no production callers, their only callers are the fuzz target. As such, keeping these methods seems not useful.
The motivation for this PR came from jeanpablojp's comment on [#35848](https://github.com/bitcoin/bitcoin/pull/35848#issuecomment-5274013825), added him as coauthor.
ACKs for top commit:
maflcko:
review ACK 2c16efbb7b🥑
vicjuma:
ACK 2c16efbb7b
sedited:
ACK 2c16efbb7b
Tree-SHA512: 129933ae9803a2d053e340ee2a85efd1e5d9e5e38833fac0a0a9cbd467fec3157087959c464a8eeedb04ea99b1e5d7eba5a2fafde2e78a4fabcd247fac855477
4e5327bc98 fuzz: refactor: scope fake clocks to target phases (Hao Xu)
e33410d888 fuzz: document arbitrary mocktimes (Hao Xu)
Pull request description:
Follow-up to #35482 (https://github.com/bitcoin/bitcoin/pull/35482#discussion_r3612852792), addressing a remaining issue with the lifetime of the mock node clock.
This replaces the process-wide `FakeNodeClock` accessor with scoped clocks in the affected fuzz target initialization and input-processing phases, following the existing `FakeSteadyClock` pattern. The active clock is passed to `ResetChainmanAndMempool()` by reference.
Tested the affected fuzz targets with `-runs=1`:
- `cmpctblock`
- `process_message`
- `process_messages`
- `utxo_snapshot`
- `utxo_snapshot_invalid`
ACKs for top commit:
maflcko:
review ACK 4e5327bc98🚉
nervana21:
re-ACK 4e5327bc98
Tree-SHA512: 7763bb2a06e3f33bcae3ad7b43f6d30a231e39197e8d274eadd496da1194fc0178b27cf016b451f36309d9277ce414c85daf734604c85a9deb3803b26b968e6a
1156ce6754 test: Tighten `Coin` equality and add debug output (rustaceanrob)
Pull request description:
If the `==` operator on two `Coin` fails, the developer should also see the conditions under which it failed. All that is required is adding a `<<` operator, moving the `==` out of the namespace, and switching `==` sites to `BOOST_TEST`.
Here we also tighten what it means for a coin to be "equal."
This is a pre-requiste for https://github.com/bitcoin/bitcoin/pull/35713 but seems to be a benefit on its own.
ACKs for top commit:
josibake:
reACK 1156ce6754
maflcko:
review ACK 1156ce6754🔋
Tree-SHA512: de5c612998518371ded3d25abdf1c96640e33d9961902dc4765a7e8f5088d8698a66ad63bc0a9822ec2b53e41e72dc95f78196822429af30b2ec29baa31c1ed1
Avoid exposing a process-wide FakeNodeClock accessor from the test
utility module. Initialize separate scoped clocks for target setup and
input processing, and pass the active clock to ResetChainmanAndMempool
by reference.
ResetChainmanAndMempool sets each scoped clock to the selected chain's
genesis time. Avoid hard-coding the mainnet genesis timestamp when
constructing these clocks, because the targets use REGTEST parameters
and the value is overwritten during reset.
Initialize each clock from the fuzz harness's existing mock time until
ResetChainmanAndMempool sets the REGTEST genesis time.
This commit does not change behavior.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: nervana21 <205626986+nervana21@users.noreply.github.com>
If the `==` operator on two `Coin` fails, the developer should also see
the conditions under which it failed. All that is required is adding a
`<<` operator, moving the `==` out of the namespace, and switching `==`
sites to `BOOST_TEST`.
Here we also tighten what it means for a coin to be "equal."
This is a pre-requiste for #35713 but seems to be a benefit on its own.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
fe7d475d45 private broadcast: bound broadcast attempts per tx to 1k (Gregory Sanders)
Pull request description:
Since rebroacasts introduce additional state, bound the state growth by capping the number of rebroadcasts. With ~72 bytes per record, 10k transactions rebroadcasting for ~42 hours will result about 703 MiB allocated with overhead.
ACKs for top commit:
andrewtoth:
ACK fe7d475d45
frankomosh:
ReACK fe7d475d45
sedited:
ACK fe7d475d45
Tree-SHA512: e4ec5156b90ad24d68b561df03ad09bdf0ac7535886ff56891cb698cf64ff0e1e484075b76040bba6194baf874c9237028c82debf7405136447ba5b5faee589c
Rename policyestimator_tests.cpp to blockpolicyestimator_tests.cpp.
Also rename the policy_estimator fuzz target to block_policy_estimator so the
test names match CBlockPolicyEstimator.
This makes the block policy fee rate estimator test files accurate and concise,
which makes adding another fee rate estimator test files straightforward.
Now that the wallet reports its own FeeReason, StringForBlockPolicyEstimateReason
is only used internally by the block policy estimator. Move it from
common/messages into the block policy fee rate estimator.
Also add the detailed FeeCalculation debug log to estimateSmartFee, where
the FeeCalculation data originates, and always populate feeCalc locally so
the log is available even when the caller does not pass a valid
FeeCalculation pointer.
The block policy estimator's FeeReason enum mixed two unrelated
concerns: the threshold that produced an estimateSmartFee result
(NONE, HALF_ESTIMATE, ...) and the reason the wallet selected a fee
rate (FALLBACK, MEMPOOL_MIN, REQUIRED).
Split them so each layer owns the reasons it reports:
- Add a wallet-facing FeeReason enum with the reasons the wallet can
select a fee rate: FEE_RATE_ESTIMATOR, MEMPOOL_MIN, USER_SPECIFIED,
FALLBACK, and REQUIRED.
- Rename the estimator enum to BlockPolicyEstimateReason and narrow it
to estimator reasons: NONE, HALF_ESTIMATE, FULL_ESTIMATE,
DOUBLE_ESTIMATE, and CONSERVATIVE.
- Return wallet fee selection metadata through MinimumFeeRateResult
instead of exposing FeeCalculation to wallet callers. The returned
target is now optional and is only set for fee rate estimator results.
Flatten GetMinimumFeeRate() with early returns while preserving the fee
selection order: user feerate still only applies the required-fee check,
while smart-fee results keep fallback, mempool-min, and required fallbacks.
The returned target is cleared for fallback, mempool-min, and required
results.
Replace the CreateTransactionInternal log with a simpler message that
does not depend on estimateSmartFee internals. Detailed estimator
logging will be added in a follow-up commit.
01dde6b205 fuzz: Fix assertion in txorphan (marcofleon)
Pull request description:
`EraseTx()` calls `LimitOrphans()`, which may evict announcements from a peer that didn't announce the erased transaction, causing that peer's usage to decrease. Relax the assertion in the `EraseTx()` branch that claimed usage of a non-announcer peer should be unchanged. Also, add assertions for the other cases.
ACKs for top commit:
dergoegge:
utACK 01dde6b205
instagibbs:
ACK 01dde6b205
Tree-SHA512: 2e597b85fd41058c2fa79fa55f0d37e12505065b5e27aba7b9680e0c249a5450e6fa97b45394d6ffe1318f42538134ffa9c423b126c455f6f8e6d8ca59eed4b6
c3945bfd2b doc: use derivehdkey in multisig tutorial (Sjors Provoost)
3662e33669 test: use derivehdkey in M-of-N multisig demo (Sjors Provoost)
d9570f0838 rpc: add derivehdkey (Sjors Provoost)
62da9f9614 wallet: add GetExtKey helper (Sjors Provoost)
aaf1548475 wallet: generalize GetActiveHDPubKeys helper (Sjors Provoost)
3821452c4a refactor: add hardened derivation helper (Sjors Provoost)
0ab61caafd rpc: ParsePathBIP32 helper (Sjors Provoost)
e36c4b76e1 util: reject out-of-range BIP32 keypath indices (Sjors Provoost)
ba78c31a00 fuzz: check ParseHDKeypath/WriteHDKeypath round-trip (Sjors Provoost)
8cce969085 Have ParseHDKeypath handle h derivation marker (Sjors Provoost)
fc53077762 test: move parse_hd_keypath test to bip32_tests (Sjors Provoost)
dab525eb77 key: add DeriveExtKey() helper (Sjors Provoost)
Pull request description:
Adds a `derivehdkey` RPC that returns an xpub, or optionally the xprv, at an arbitrary BIP32 path (with at least one hardened step), derived from a wallet HD key.
The main use case is coordinating a multisig setup, where each participant shares an xpub derived at a hardened path (e.g. `m/87h/0h/0h`) distinct from their default single-signature descriptors. See the (updated) `doc/multisig-tutorial.md` and (updated) functional test to see how that workflow improves.
The first commits are some helpful helpers:
- _key: add DeriveExtKey() helper_ - performs the actual derivation
- _test: move parse_hd_keypath test to bip32_tests_ - from `psbt_wallet_tests`
- _Have ParseHDKeypath handle h derivation marker_
- _util: reject out-of-range BIP32 keypath indices_ - `ParseHDKeypath` would previously map overflowing values without `h` to hardened.
- _fuzz: check ParseHDKeypath/WriteHDKeypath round-trip_
- _rpc: ParsePathBIP32 helper_
- _refactor: add hardened derivation helper_ - `HasHardenedDerivation()`, to enforce the "at least one hardened step" rule
- _wallet: generalize GetActiveHDPubKeys helper_ - extracts code from `gethdkeys` which `derivehdkey` needs
- _wallet: add GetExtKey helper_ - reconstruct an xprv from a wallet xpub (analog of `GetKey()`); behavior-preserving prep, also simplifies `gethdkeys`.
Meat and potatoes:
- _rpc: add derivehdkey_ - the RPC itself, plus the `UnusedKey` filter on `GetHDPubKeys` that drives key selection.
- _test: use derivehdkey in M-of-N multisig demo_ - rewrites the functional multisig test to use the RPC and `<0;1>` syntax.
- _doc: use derivehdkey in multisig tutorial_ - same for the prose tutorial.
ACKs for top commit:
pseudoramdom:
code review ACK c3945bfd2b
achow101:
ACK c3945bfd2b
w0xlt:
That being the case, ACK c3945bfd2b
Tree-SHA512: 661f17c9bfe26017eb14c27ba7af37093387100d3baa25f5d29bba9c1aedc40d19afe1bdfc126a18d018857bb02f1fc84386f10b8f4f4b8e9d6f4b0691d9e302
Rather than rebroadcasting forever, bound attempts at
private broadcast, report remaining attempts over RPC
results, and allow exhausted transactions to be
retried when submitted.
EraseTx calls LimitOrphans, which may evict announcements from a peer
that didn't announce the erased transaction, causing that peer's usage
to decrease. Relax the assertion in the EraseTx branch that claimed
usage of a non-announcer peer is unchanged. Also, add assertions for
the other cases.
fab74a0e92 refactor: Use C++14 digit separator for large int literals (MarcoFalke)
fae759be79 scripted-diff: Use inline constexpr over plain constexpr (MarcoFalke)
fa74f58a26 scripted-diff: Use inline const over (static) const (MarcoFalke)
fab1a62c87 refactor: Use inline constexpr for string literals in headers (MarcoFalke)
fa08bbed8d contrib: Adjust generate-seeds.py to write inline constexpr (MarcoFalke)
fad753611b scripted-diff: Use inline constexpr over (static) const (MarcoFalke)
faedb52583 refactor: Make CFeeRate(integral) ctor constexpr (MarcoFalke)
5555d5dcb5 scripted-diff: Use inline constexpr over static constexpr (MarcoFalke)
fa6e1a1e85 refactor: Remove static from constexpr functions in headers (MarcoFalke)
Pull request description:
Both are fine and this refactor doesn't change any behavior.
However, `inline constexpr` from C++17 will ensure each symbol has a single address
across all TU, making the release binary minimally smaller. (For me it is smaller by about 1kB)
ACKs for top commit:
l0rinc:
reACK fab74a0e92
rustaceanrob:
ACK fab74a0e92
hebasto:
ACK fab74a0e92, I have reviewed the code and it looks OK.
Tree-SHA512: 6ec94136c12bcbf696812d0661c9857318a69e367c79fc00b9ca0b4068f269d10e5548d95c9ba2070225308c12d7a54fe8cb8447de7e0979cba99f48892b35f9
77440814bf fuzz: reset SOCKS5 interrupt between inputs (Hao Xu)
Pull request description:
Reset `g_socks5_interrupt` before each `socks5` fuzz input.
`CThreadInterrupt` remains interrupted until explicitly reset. Previously,
inputs executed after the first input setting the interrupt flag inherited its
state. As corpus inputs are shuffled between all-input coverage runs, the
number of affected inputs and the resulting coverage counts could differ.
Tested with the complete 91-input `socks5` corpus. The all-input deterministic
coverage check passes.
ACKs for top commit:
nervana21:
tACK 77440814bf
maflcko:
lgtm ACK 77440814bf
sedited:
ACK 77440814bf
Tree-SHA512: d1b2b33661f9796628fd7eb1f4ddb212b07110ebbfa7516e305f8aa21bde7898b4bf8fc6f6570df22f8cf6380f1287cb9b6135683ad49b2bdbe83ad9a1af23b9
87b080fe2b fuzz: reset the reused mempool in process_message(s) (Hao Xu)
d522fd3196 fuzz: prepare deterministic mempool rebuilds (Hao Xu)
b11456386b fuzz: let the test input toggle IBD in the p2p fuzz targets (Hao Xu)
2a29cee684 test: add helper to reset chainman and mempool (Hao Xu)
2a4ef42d34 fuzz: share a single FakeNodeClock in the chainman-resetting fuzz targets (Hao Xu)
Pull request description:
## Problem
`process_message` and `process_messages` keep the node in IBD (`ResetIbd()`) and
mine their coinbases with the default bare-`OP_TRUE` output script. As a result
`net_processing` returns early at the `IsInitialBlockDownload()` check and never
reaches the transaction-handling path; and even if it did, a tx spending a
bare-`OP_TRUE` coinbase is rejected as `NONSTANDARD` by
`ValidateInputsStandardness`. The reused mempool therefore always stays empty and
that path is never exercised.
## Changes
Both targets now get the same treatment:
1. **Toggle IBD from the test input** — a `bool` decides whether to also
`JumpOutOfIbd()`, exercising both the IBD and non-IBD paths. In
`process_message` it is consumed last, so existing corpus entries read `false`
and are unchanged. In `process_messages` the messages run in a loop, so the
bool must be consumed *first* (see the corpus note below).
2. **Use a spendable `P2WSH_OP_TRUE` coinbase** — both anyone-can-spend (an
`OP_TRUE` witness, no signature) and a standard witness output, so a fuzz-built
tx spending a mature coinbase can actually be accepted into the mempool.
3. **Reset the rng before rebuilding (preparation)** — rebuilding the chainman
(and, in the next commit, the mempool) consumes the global PRNG. Reset it with
`MakeRandDeterministicDANGEROUS()` first so the rebuild is deterministic across
iterations. Mirrors the `cmpctblock` harness.
4. **Reset the reused mempool** — now that the mempool can become non-empty,
rebuild it together with the chainman in `ResetChainmanAndMempool()` when the
block index grew or the mempool changed. A dirty mempool is detected by its
sequence number rather than its size, since a tx can be added and removed
within one iteration (leaving the size unchanged).
## Corpus note
~~In `process_messages` the IBD bool is consumed before the message loop (first
integral read), which shifts the `FuzzedDataProvider` layout. Existing
`process_messages` corpus entries can be migrated by appending a single `0x00`
byte at the end (read as `false`, keeping the IBD path); every other consumed
value stays the same. This is a qa-assets change accompanying this PR.~~
This note no longer applies because the IBD toggle is now consumed inside the
message loop. Appending a single `0x00` byte would not reliably target that bool
or preserve the rest of the input layout.
The accompanying `qa-assets` update should migrate or regenerate the affected
`process_messages` corpus entries for the current layout.
ACKs for top commit:
Crypt-iQ:
crACK 87b080fe2b
maflcko:
review ACK 87b080fe2b🏁
frankomosh:
Review ACK 87b080fe2b
Tree-SHA512: e557b2ca3329767a45fe8315c63df9c3191a3a46a17c5e75ea3e4ad0c25e0e500a687fa650297a386b0a2ebb95503d069089ca5ae3d0a34caab98367aeb28683
7502b9ddba fuzz: check http_request body matches framing (ameen-alam)
Pull request description:
The http_request target asserted that ReadBody() returns an empty string. That held for the libevent-based http_libevent::HTTPRequest, where the harness only parsed the request line and headers and never populated a body. Commit 9c20859b5f (PR #35182) replaced libevent with http_bitcoin::HTTPRequest, and the target was switched over in e427c227fa; its LoadBody() now decodes Content-Length and chunked bodies per RFC 9112, so any fully-parsed request carrying a body trips the stale assertion (e.g. "POST / HTTP/1.1\r\nContent-Length: 3\r\n\r\nabc").
Replace the emptiness check with a framing-consistency check that mirrors LoadBody()'s own branch logic: a chunked body is bounded by MAX_BODY_SIZE, a Content-Length body is exactly that many bytes, and a request with neither framing header has no body. This strengthens the target instead of dropping the assertion.
**Steps to reproduce (old assertion):**
Build the fuzz binary and pass this input as a file to the `http_request` target:
`POST / HTTP/1.1\r\nContent-Length: 3\r\n\r\nabc`
→ `test/fuzz/http_request.cpp:49: Assertion 'body.empty()' failed`
**Testing the fix:**
Ran the updated target ~16 min under libFuzzer with ASAN/UBSAN
(14.2M execs, no crashes), plus targeted inputs for each branch:
Content-Length body, chunked, `Transfer-Encoding: identity` + Content-Length,
no framing headers, and `Content-Length: 0`. Happy to contribute the repro
input to qa-assets as a follow-up.
ACKs for top commit:
pinheadmz:
ACK 7502b9ddba
marcofleon:
tACK 7502b9ddba
Tree-SHA512: 4f2eb6bdb3a4556866a84fe0f1d0d8cf506e2efd1b1c7493a99f67ca452b31a140034d418c4064142b1a66c3a6c34b97df0e2b12c21ea86cd4019ffc7cff3b27
