9b75cfda4d test: retain the intended behavior of `feature_fee_estimation.py` nodes (ismaelsadeeq)
5c1236f04a test: fix incorrect subtest in `feature_fee_estimation.py` (ismaelsadeeq)
Pull request description:
Attempt to fix#32461
In the `estimatesmartfee` RPC, we return the maximum of the following: the feerate estimate for the target, `minrelaytxfee`, and `mempoolminfee`.
9a05b45da6/src/rpc/fees.cpp (L85)
The test `test_feerate_mempoolminfee`, originally introduced in ea31caf6b4, is incorrect.
It should calculate the fee rate ceiling by taking the maximum of the custom `minrelaytxfee`, `mempoolminfee`, and the highest fee rate observed during the test (`check_smart_estimates`). This is necessary because:
* There is no guarantee that the generated fee rates will exceed both `minrelaytxfee` and `mempoolminfee`.
* Users can start a node with custom fee settings.
Due to the non-deterministic nature of the `feature_fee_estimation.py` test, it often passes by chance. The randomly generated fees typically include a value higher than the custom `minrelaytxfee`, inadvertently hiding the issue.
Issue #32461 identified a random seeds that consistently fails the test because the generated fees never exceed the custom `minrelaytxfee`:
e.g
```
build/test/functional/feature_fee_estimation.py --random=3450808900320758527
```
This PR has two commits which :
* Correctly fixes the test by calculating the fee rate ceiling as the maximum of the node `minrelaytxfee`, `mempoolminfee`, and the highest seen fee rate, when verifying smart fee estimates.
* Improves the subtest name and comment for clarity.
* Restores the original test behavior by appending 4000 WU to the custom `blockmaxweight`.
ACKs for top commit:
achow101:
ACK 9b75cfda4d
glozow:
ACK 9b75cfda4d
theStack:
Light ACK 9b75cfda4d
Tree-SHA512: 0f7fb0496b50a399b58f6fb1afd95414fad454795fbc0046e22dfc54a2062ae0c519a12ebfeb6ad7ef547438868d99eca2351c0d19d0346adaadb500eff6f15f
c10e382d2a flatfile: check whether the file has been closed successfully (Vasil Dimov)
4bb5dd78ea util: check that a file has been closed before ~AutoFile() is called (Vasil Dimov)
8bb34f07df Explicitly close all AutoFiles that have been written (Vasil Dimov)
a69c4098b2 rpc: take ownership of the file by WriteUTXOSnapshot() (Hodlinator)
Pull request description:
`fclose(3)` may fail to flush the previously written data to disk, thus a failing `fclose(3)` is as serious as a failing `fwrite(3)`.
Previously the code ignored `fclose(3)` failures. This PR improves that by changing all users of `AutoFile` that use it to write data to explicitly close the file and handle a possible error.
---
Other alternatives are:
1. `fflush(3)` after each write to the file (and throw if it fails from the `AutoFile::write()` method) and hope that `fclose(3)` will then always succeed. Assert that it succeeds from the destructor 🙄. Will hurt performance.
2. Throw nevertheless from the destructor. Exception within the exception in C++ I think results in terminating the program without a useful message.
3. (this is implemented in the latest incarnation of this PR) Redesign `AutoFile` so that its destructor cannot fail. Adjust _all_ its users 😭. For example, if the file has been written to, then require the callers to explicitly call the `AutoFile::fclose()` method before the object goes out of scope. In the destructor, as a sanity check, assume/assert that this is indeed the case. Defeats the purpose of a RAII wrapper for `FILE*` which automatically closes the file when it goes out of scope and there are a lot of users of `AutoFile`.
4. Pass a new callback function to the `AutoFile` constructor which will be called from the destructor to handle `fclose()` errors, as described in https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2243842400. My thinking is that if that callback is going to only log a message, then we can log the message directly from the destructor without needing a callback. If the callback is going to do more complicated error handling then it is easier to do that at the call site by directly calling `AutoFile::fclose()` instead of getting the `AutoFile` object out of scope (so that its destructor is called) and inspecting for side effects done by the callback (e.g. set a variable to indicate a failed `fclose()`).
ACKs for top commit:
l0rinc:
ACK c10e382d2a
achow101:
ACK c10e382d2a
hodlinator:
re-ACK c10e382d2a
Tree-SHA512: 3994ca57e5b2b649fc84f24dad144173b7500fc0e914e06291d5c32fbbf8d2b1f8eae0040abd7a5f16095ddf4e11fe1636c6092f49058cda34f3eb2ee536d7ba
4f10a57671 depends: Override host compilers for FreeBSD and OpenBSD (Hennadii Stepanov)
Pull request description:
When building depends on FreeBSD/OpenBSD `aarch64`, the host compilers default to `default_host_{CC,CXX}`, which resolves to `gcc`/`g++`. This is incorrect on these systems, where Clang is the default system compiler.
To ensure proper compiler selection, this PR adopts the same approach used for `darwin`:c1d4253d31/depends/builders/darwin.mk (L12-L14)
Fixes https://github.com/bitcoin/bitcoin/issues/32691.
ACKs for top commit:
Sjors:
ACK 4f10a57671
Tree-SHA512: 66ace60b63671f2ab7facb84776298e1489aa0a984559c9a22323adaaedcc86f67158078b18c333cb7bdc2b17a4573e321ee7361596bc941db060c5659014069
67dc7523f3 cmake, test: Disable tests instead of ignoring them (Hennadii Stepanov)
bb9157db5d cmake, refactor: Switch to `Python3::Interpreter` imported target (Hennadii Stepanov)
Pull request description:
This PR:
1. Switches to a modern CMake approach by using the `Python3::Interpreter` imported target, which is more robust than using variables.
2. Disables the `util_rpcauth_test` test explicitly instead of silently ignoring it.
A build and test log for the case when Python is unavailable is provided below:
```
$ cmake -B build
$ cmake --build build -j 16
$ ctest --test-dir build -j $(nproc) -R "^util"
Internal ctest changing into directory: /bitcoin/build
Test project /bitcoin/build
Start 115: util_tests
Start 117: util_trace_tests
Start 114: util_string_tests
Start 116: util_threadnames_tests
Start 1: util_rpcauth_test
1/5 Test #1: util_rpcauth_test ................***Not Run (Disabled) 0.00 sec
2/5 Test #114: util_string_tests ................ Passed 0.11 sec
3/5 Test #117: util_trace_tests ................. Passed 0.11 sec
4/5 Test #116: util_threadnames_tests ........... Passed 0.11 sec
5/5 Test #115: util_tests ....................... Passed 0.13 sec
100% tests passed, 0 tests failed out of 4
Total Test time (real) = 0.13 sec
The following tests did not run:
1 - util_rpcauth_test (Disabled)
```
ACKs for top commit:
purpleKarrot:
ACK 67dc7523f3
janb84:
tACK 67dc7523f3
Tree-SHA512: 5fc7ebe31ac03f4b8a53ecfcfc1cace0f647a1d2c989651988edae96bdfbbe2dee171714e57cb028e65ead1bb40806a82d9821746451dbf005538601fd33ea88
666016e56b ci: use --usecli in one of the CI jobs (Martin Zumsande)
7ea248a020 test: Disable several (sub)tests with cli (Martin Zumsande)
f420b6356b test: skip subtests that check for wrong types with cli (Martin Zumsande)
6530d0015b test: add function to convert to json for height_or_hash params (Martin Zumsande)
54d28722ba test: Don't send empty named args with cli (Martin Zumsande)
cca422060e test: convert tuple to json for cli (Martin Zumsande)
af34e98086 test: make rpc_psbt.py usable with --usecli (Martin Zumsande)
8f8ce9e174 test: rename .rpc to ._rpc and remove unnecessary uses (Martin Zumsande)
5b08885986 test: enable functional tests with large rpc args for cli (Martin Zumsande)
7d5352ac73 test: use -stdin for large rpc commands (Martin Zumsande)
6c364e0c10 test: Enable various tests for usage with cli (Martin Zumsande)
Pull request description:
Fixes#32264
I looked into all current failures listed in the issue, as well all tests that are already disabled for the cli with `self.supports_cli = False`. There are several reasons why existing tests fail with `--usecli` on many systems, the most important ones are:
- Most common reason is that the test executes a RPC call with a large arg that exceeds `MAX_ARG_STRLEN` of the OS, which is usually 128kb on linux: This is fixed by using `-stdin` for these large calls (idea by 0xB10C)
- they test specifically the rpc interface - nothing to do there except disabling.
- Some functional test submit wrong types to params on purpose to test the error message (which is different when using the cli) - deactivated these specific subtests locally for the cli when there is just one or two of them, deactivated the entire tests when there are more spots
- When python sets `None` for an arg, the cli converts this to 'null' in `arg_to_cli`. This is fine e.g. for boolean args, but doesn't work for strings where it's interpreted as the string 'null'. Bypass this for named args by not including args in case the value is `None` for the cli is used (it's effectively the same as leaving the optional arg out).
- the `height_or_hash` param used in some RPC needs to be converted to a JSON (effectively adding full quotes).
- Some tests were marked with `self.supports_cli = False` in the past but run fine on master today - enabled those.
In total, this PR fixes all tests that fail on master and reduces the number of tests that are deactivated (`self.supports_cli = False`) from 40 to 21.
It also adds `--usecli` to one CI job (multiprocess, i686, DEBUG) to detect regressions.
ACKs for top commit:
maflcko:
re-ACK 666016e56b🔀
pinheadmz:
re-ACK 666016e56b
Tree-SHA512: 7a1efd212649ca100b236a1239294d40ecd36e2720e3b173a230b14545bb40b135111db7fed8a0d1448120f5387da146a03f1912e2028c8d03a0b6a3ca8761b0
d6aaffcb11 test: check P2SH sigop count for coinbase tx (brunoerg)
Pull request description:
We currently do not test that `GetP2SHSigOpCount` returns 0 for coinbase transactions (see line L129 at https://corecheck.dev/mutation/src/consensus/tx_verify.cpp). This PR addresses it.
ACKs for top commit:
darosior:
That said, i guess unit-tested dead consensus code is better than not-unit-tested dead consensus code. utACK d6aaffcb11
theStack:
ACK d6aaffcb11
w0xlt:
ACK d6aaffcb11
ishaanam:
ACK d6aaffcb11
pablomartin4btc:
ACK d6aaffcb11
Tree-SHA512: a7d7306f064bb2ec7e93e92625848ae38e150ebb67bde37cd15be1038816b154e867ad21ecd2685d8de5341b67e3b768d30b7654e27b541f33e8f9d63e52261d
ec004cdb86 test: Use rehash() in outbound eviction block-relay (pablomartin4btc)
26598ed21e test: Clarify roles in outbound eviction comments (pablomartin4btc)
Pull request description:
This change avoids relying on `tip_header.hash`, which is `None` when the header is deserialized from hex during `CBlockHeader()` construction.
Instead, `tip_header.rehash()` explicitly computes the hash, making the test behavior more robust.
Using the explicit `rehash()` avoids depending on `wait_for_getheaders()` falling back to any received message, thus making the test more deterministic.
This is a follow-up to #32742.
Also, as noted in a previous review [comment](https://github.com/bitcoin/bitcoin/pull/32742#pullrequestreview-2923802386), "_the hash field is wrong either way, simply due to being the wrong type (it is an optional hex string), as opposed to an optional int_".
---
The first commit intention is to improve clarity around the tests purpose, helping reviewers follow what's being verified and why. What started as a small comment during review of #32742 led me reviewing and try to improve most relevant tests comments for consistency and correctness.
ACKs for top commit:
achow101:
ACK ec004cdb86
theStack:
lgtm ACK ec004cdb86#️⃣
yuvicc:
ACK ec004cdb86
danielabrozzoni:
ACK ec004cdb86
Tree-SHA512: 6a14dedfdc425cd806f63443b3b9f79df69a7717452739f5d7fef1b2bdba23402670d63cf1d6b66c9f1a6b460d4d4a6f185426d0a4982fa95115a234cd6baef7
b789907346 wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet (furszy)
e86d71b749 wallet: refactor, dedup wallet re-loading code (furszy)
1de423e0a0 wallet: introduce method to return all db created files (furszy)
d04f6a97ba refactor: remove sqlite dir path back-and-forth conversion (furszy)
Pull request description:
Currently, the migration process creates a brand-new descriptor wallet with no
connection to the user's legacy wallet when the legacy wallet lacks key material
and contains only watch-only scripts. This behavior is not aligned with user
expectations. If the legacy wallet contains only watch-only scripts, the migration
process should only generate a watch-only wallet instead.
TODO List:
* Explain that `migratewallet` renames the watch-only after migration, and
also that the wallet will not have keys enabled.
ACKs for top commit:
achow101:
ACK b789907346
pablomartin4btc:
tACK b789907346
rkrux:
LGTM ACK b789907346
Tree-SHA512: 1d583ac4b206fb477e9727daf4b5ad9c3e18b12d40e1ab4a61e8565da44c3d0327c892b51cf47b4894405d122e414cefb6b6366c357e02a74a7ca96e06762d83
4be81e9746 feature_taproot: sample tx version border values more (Greg Sanders)
Pull request description:
Currently if the version 3 is selected for an otherwise standard spender, the test will fail. It's unlikely but possible, so change the test to update expectations and sample more aggressively on border values to instigate failures much quicker in the future if another version is made standard.
ACKs for top commit:
maflcko:
lgtm ACK 4be81e9746
darosior:
ACK 4be81e9746
Tree-SHA512: 53267a201aaa495bea9d624930a19e40af3633427b6715965f43b9e1a060b2c9f19c8b10c8168778349fa50715e44cb8e5e9d2ce477d5f324ca8ed28ff7996cd
6efbd1e1dc refactor: CTransaction equality should consider witness data (Cory Fields)
cbf9b2dab1 mempool: codify existing assumption about duplicate txids during removal (Cory Fields)
e9331cd6ab wallet: IsEquivalentTo should strip witness data in addition to scriptsigs (Cory Fields)
Pull request description:
I stumbled upon the `CTransaction` comparison operators while refactoring some nearby code. I found it surprising and not at all obvious that two transactions would test equal even if their witness data differed. It seems like an unnecessary potential footgun. Fix that by comparing against wtxid rather than txid.
Outside of tests, there were only 3 users of these functions in the code-base:
- Its use in the mempool has been replaced with an explicit txid comparison, as that's a tighter constraint and matches the old behavior. glozow suggested also upgrading this to an `Assume()`.
- Its use in the wallet was accidentally doing the correct thing by ignoring witness data. I've changed that to an explicit witness removal so that `IsEquivalentTo` continues to work as-intended.
- Its use in `getrawtransaction` is indifferent to the change.
ACKs for top commit:
maflcko:
review ACK 6efbd1e1dc🦋
achow101:
ACK 6efbd1e1dc
glozow:
ACK 6efbd1e1dc
Tree-SHA512: 89be424889f49e7e26dd2bdab7fbc8b2def59bf002ae8b94989b349ce97245f007d6c96e409a626cbf0de9df83ae2485b4815b40a70f7aa5b6c720eb34a6c017
941b8f54c0 ci: run get_previous_releases as part of test cross win job (Max Edwards)
5e2182140b test: increment mocked time for migrating wallet backups (Max Edwards)
5174565802 ci: disable feature_unsupported_utxo_db functional test (Max Edwards)
3dc90d69a6 test: remove mempool.dat before copying (Max Edwards)
67a6b20d50 test: add windows support to get previous releases script (Max Edwards)
1a1b478ca3 scripted-diff: rename tarball to archive (Max Edwards)
4f06dc8484 test: remove building from source from get prev releases script (Max Edwards)
Pull request description:
This PR updates the `test/get_previous_releases.py` script to also work on Windows by changing to be pure python rather than using unix tools such as `curl` and `tar`.
This enables additional functional tests to run such as `wallet_migration.py`, `mempool_compatability.py` and `wallet_backwards_compatibility.py`.
Unfortunately `feature_unsupported_utxo_db.py` _could_ run but this test requires Bitcoin `v0.14.3` which will not run under windows with emojis in the data directory (as the functional test runner has by default) . This test could be run as it's own step in the ci workflow file and would pass but as it's quite an old version / feature I have assumed it's not worth worrying about and best just to exclude.
Two tests needed to be slightly modified to run under windows. Both were issues with trying to overwrite a file that already exists which windows seems to be more strict on than the unix based systems.
Finally, building from source has been dropped from the `get_previous_releases.py` script. This had not been updated after the move to cmake and so it was assumed that nobody could have been using that feature.
ACKs for top commit:
maflcko:
re-ACK 941b8f54c0🍪
achow101:
ACK 941b8f54c0
hodlinator:
re-ACK 941b8f54c0
Tree-SHA512: 22933d0ec278b9b0ffcd2a8e90026e1a3631b00186e7f78bd65be925049021e319367d488c36a82ab526a07b264bac18c2777f87ca1174b231ed49fed56d11cb
8cc9845b8d wallet, rpc: Use `OUTPUT_TYPES` to describe the output types instead of hardcoding them (w0xlt)
Pull request description:
Follow-up to https://github.com/bitcoin/bitcoin/pull/32429, built on top of it.
This PR addresses the https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2076251627 that the RPC documentation does not use `OUTPUT_TYPES`, but rather hardcodes them, as is already the case for the `getnewaddress` command.
So here the output types are changed from `std::string` to `std::string_view` so that the values are known at compile time or during the early stages of program startup, before main() execution.
It also updates `wallet/rpc/addresses.cpp` to write the RPC docs according to `OUTPUT_TYPES` instead of using hardcoded version.
It also updates the documentation in outputtypes.h, adding Doxygen comments,
ACKs for top commit:
maflcko:
lgtm ACK 8cc9845b8d
achow101:
ACK 8cc9845b8d
Tree-SHA512: e86d813d6d158dd2f6c62519a7ecaa878f2e4f686b5bae82028a106bd6671a13b10fb366f9bb7b94974777217e1852f38e8aa05bba00cd27f94f4412167a3562
0e9f409db3 doc: clarify that the "-j N" goes after the "--build build" part (Salvatore Ingala)
Pull request description:
I was surprised that something like `cmake -j 4 --build build` doesn't work, so this might help others to not make the same mistake.
ACKs for top commit:
achow101:
ACK 0e9f409db3
hebasto:
ACK 0e9f409db3.
stickies-v:
ACK 0e9f409db3
pablomartin4btc:
ACK 0e9f409db3
theStack:
ACK 0e9f409db3
brunoerg:
ACK 0e9f409db3
w0xlt:
ACK 0e9f409db3
Tree-SHA512: 5283f87a68abae4905b773e73aa4529f52d2302d804165b560b22f300a449d8ce1ae3194d2e1fe69be6f7e4d279dd32b8ef0daa18af4ac6a76a51f5abc39ec63
215e5999e2 wallet: Remove unused CachedTxGet{Available,Immature}Credit (Ava Chow)
49675de035 wallet: Have GetDebit use the wallet's TXO set (Ava Chow)
17d453cb3a wallet: Recompute wallet TXOs after descriptor migration (Ava Chow)
764016eb22 wallet: Retrieve TXO directly in FetchSelectedInputs (Ava Chow)
c1801b78f1 wallet: Use wallet's TXO set in AvailableCoins (Ava Chow)
dde7cbe105 wallet: Change balance calculation to use m_txos (Ava Chow)
96e7a89c5e wallet: Recalculate the wallet's txos after any imports (Ava Chow)
ae888c38d0 wallet: Exit IsTrustedTx early if wtx is already in trusted_parents (Ava Chow)
ae0876ec42 wallet: Keep track of transaction outputs owned by the wallet (Ava Chow)
0f269bc48c walletdb: Load Txs last (Ava Chow)
5cc32ee2a7 test: Test for balance update due to untracked output becoming spendable (Ava Chow)
8222341d4f wallet: MarkDirty after AddWalletDescriptor (Ava Chow)
e02f2d331c bench: Have AvailableCoins benchmark include a lot of unrelated utxos (Ava Chow)
Pull request description:
Currently, the wallet is not actually aware about its own transaction outputs. Instead, it will iterate all of the transactions stored in `mapWallet`, and then all of the outputs of those transactions, in order to figure out what belongs to it for the purposes of coin selection and balance calculation. For balance calculation, there is caching that results in it only iterating all of the transactions, but not all of the outputs. However when the cache is dirty, everything is iterated. This is especially problematic for wallets that have a lot of transactions, or transactions that have a lot of unrelated outputs (as may occur with coinjoins or batched payments).
This PR helps to resolve this issue by making the wallet track all of the outputs that belong to it in a new member `m_txos`. Note that this includes outputs that may have already been spent. Both balance calculation (`GetBalance`) and coin selection (`AvailableCoins`) are updated to iterate `m_txos`. This is generally faster since it ignores all of the unrelated outputs, and it is not slower as in the worst case of wallets containing only single output transactions, it's exactly the same number of outputs iterated.
`m_txos` is memory only, and it is populated during wallet loading. When each transaction is loaded, all of its outputs are checked to see if it is `IsMine`, and if so, an entry added to `m_txos`. When new transactions are received, the same procedure is done.
Since imports can change the `IsMine` status of a transaction (although they can only be "promoted" from watchonly to spendable), all of the import RPCs will be a bit slower as they re-iterate all transactions and all outputs to update `m_txos`.
Each output in `m_txos` is stored in a new `WalletTXO` class. This class contains references to the parent `CWalletTx` and the `CTxOut` itself. It also caches the `IsMine` value of the txout. This should be safe as `IsMine` should not change unless there are imports. This allows us to have additional performance improvements in places that use these `WalletTXO`s as they can use the cached `IsMine` rather than repeatedly calling `IsMine` which can be expensive.
The existing `WalletBalance` benchmark demonstrates the performance improvement that this PR makes. The existing `WalletAvailableCoins` benchmark doesn't as all of the outputs used in that benchmark belong to the test wallet. I've updated that benchmark to have a bunch of unrelated outputs in each transaction so that the difference is demonstrated.
This is part of a larger project to have the wallet actually track and store a set of its UTXOs.
Built on #24914 as it requires loading the txs last in order for `m_txos` to be built correctly.
***
## Benchmarks:
Master:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 34,590,013.00 | 28.91 | 0.0% | 812,669,269.00 | 148,360,642.50 | 5.478 | 18,356,853.00 | 0.2% | 0.76 | `WalletAvailableCoins`
| 3,193.46 | 313,139.91 | 0.4% | 96,868.06 | 13,731.82 | 7.054 | 26,238.01 | 0.1% | 0.01 | `WalletBalanceClean`
| 26,871.18 | 37,214.59 | 3.3% | 768,179.50 | 115,544.39 | 6.648 | 154,171.09 | 0.1% | 0.01 | `WalletBalanceDirty`
| 3,177.30 | 314,732.47 | 0.2% | 96,868.06 | 13,646.20 | 7.099 | 26,238.01 | 0.1% | 0.01 | `WalletBalanceMine`
| 10.73 | 93,186,952.53 | 0.1% | 157.00 | 46.14 | 3.403 | 36.00 | 0.0% | 0.01 | `WalletBalanceWatch`
| 590,497,920.00 | 1.69 | 0.1% |12,761,692,005.00 |2,536,899,595.00 | 5.030 | 129,124,399.00 | 0.7% | 6.50 | `WalletCreateEncrypted`
| 182,929,529.00 | 5.47 | 0.0% |4,199,271,397.00 | 785,477,302.00 | 5.346 | 75,363,377.00 | 1.1% | 2.01 | `WalletCreatePlain`
| 699,337.20 | 1,429.93 | 0.7% | 18,054,294.00 | 3,005,072.20 | 6.008 | 387,756.60 | 0.3% | 0.04 | `WalletCreateTxUseOnlyPresetInputs`
| 32,068,583.80 | 31.18 | 0.5% | 562,026,110.00 | 137,457,635.60 | 4.089 | 90,667,459.40 | 0.3% | 1.78 | `WalletCreateTxUsePresetInputsAndCoinSelection`
| 36.62 | 27,306,578.40 | 0.5% | 951.00 | 157.05 | 6.056 | 133.00 | 0.0% | 0.01 | `WalletIsMineDescriptors`
| 35.00 | 28,569,989.42 | 0.7% | 937.00 | 150.33 | 6.233 | 129.00 | 0.0% | 0.01 | `WalletIsMineMigratedDescriptors`
| 203,284,889.00 | 4.92 | 0.0% |4,622,691,895.00 | 872,875,275.00 | 5.296 | 90,345,002.00 | 1.2% | 1.02 | `WalletLoadingDescriptors`
| 1,165,766,084.00 | 0.86 | 0.0% |24,139,316,211.00 |5,005,218,705.00 | 4.823 |2,664,455,775.00 | 0.1% | 1.17 | `WalletMigration`
PR:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 33,975,750.50 | 29.43 | 0.1% | 794,719,150.50 | 145,763,550.00 | 5.452 | 16,036,630.50 | 0.2% | 0.75 | `WalletAvailableCoins`
| 2,442.01 | 409,498.46 | 0.2% | 60,782.04 | 10,500.60 | 5.788 | 9,492.01 | 0.3% | 0.01 | `WalletBalanceClean`
| 2,763.12 | 361,909.21 | 0.2% | 61,493.05 | 11,859.48 | 5.185 | 9,625.01 | 0.2% | 0.01 | `WalletBalanceDirty`
| 2,347.98 | 425,898.72 | 0.3% | 60,782.04 | 10,082.73 | 6.028 | 9,492.01 | 0.2% | 0.01 | `WalletBalanceMine`
| 11.67 | 85,654,630.36 | 0.2% | 176.00 | 50.18 | 3.508 | 40.00 | 0.0% | 0.01 | `WalletBalanceWatch`
| 590,119,519.00 | 1.69 | 0.1% |12,754,398,258.00 |2,534,998,522.00 | 5.031 | 129,078,027.00 | 0.7% | 6.50 | `WalletCreateEncrypted`
| 183,124,790.00 | 5.46 | 0.1% |4,199,212,926.00 | 786,323,886.00 | 5.340 | 75,354,437.00 | 1.1% | 2.02 | `WalletCreatePlain`
| 669,643.00 | 1,493.33 | 0.1% | 17,213,904.20 | 2,877,336.40 | 5.983 | 394,292.80 | 0.3% | 0.04 | `WalletCreateTxUseOnlyPresetInputs`
| 26,205,987.80 | 38.16 | 0.8% | 365,551,340.80 | 112,376,905.20 | 3.253 | 65,684,276.20 | 0.4% | 1.44 | `WalletCreateTxUsePresetInputsAndCoinSelection`
| 34.75 | 28,778,846.38 | 0.1% | 937.00 | 149.41 | 6.271 | 129.00 | 0.0% | 0.01 | `WalletIsMineDescriptors`
| 29.91 | 33,428,072.85 | 0.2% | 920.00 | 128.63 | 7.152 | 126.00 | 0.0% | 0.01 | `WalletIsMineMigratedDescriptors`
| 202,437,985.00 | 4.94 | 0.1% |4,626,686,256.00 | 869,439,274.00 | 5.321 | 90,961,305.00 | 1.1% | 1.02 | `WalletLoadingDescriptors`
| 1,158,394,152.00 | 0.86 | 0.0% |24,143,589,972.00 |4,971,946,380.00 | 4.856 |2,665,355,654.00 | 0.1% | 1.16 | `WalletMigration`
ACKs for top commit:
davidgumberg:
untested reACK 215e599
murchandamus:
reACK 215e5999e2
ishaanam:
reACK 215e5999e2
w0xlt:
reACK 215e5999e2
Tree-SHA512: d6b929de56f67930678db654e46f15fb71008390189c701a026b2d76af8f14a7c9769e49835ce7e2b6515d2934a77aad8de0b1a82231a2e1de5337de25db9629
6bb38bf37f Update p2p_monitor.py (leopardracer)
Pull request description:
Fix typo in variable name in p2p_monitor.py.
ACKs for top commit:
maflcko:
lgtm ACK 6bb38bf37f
Tree-SHA512: 3d18b56766acd35f86e002fda7460f57ef8d09dd8948cc5aa172f720789624f52e1a2e4f7b095fcee7e524bdc63228903056ab2339d206efd5fbbb4189eeb969
dd99cedc0b doc: add `/spenttxouts` to REST-interface.md (Sebastian Falbesoner)
Pull request description:
Seems like adding the `spenttxouts` endpoint to the REST interface description was forgotten in #32540.
ACKs for top commit:
maflcko:
lgtm ACK dd99cedc0b
pablomartin4btc:
ACK dd99cedc0b
Tree-SHA512: 9b1da9cbab914664217cc7f8792092e672518ec7f79c7670eb1c54ef94e6cd52b139e1051035ce33ad62b7b74a169e3abc793d1804760787a11a0dc269d26402
45b1d39757 doc: Add fetching single PRs from upstream (will)
Pull request description:
Current recommendation is to add a new remote fetching all PRs, but this is resource-intensive.
Document a better way to fetch a single PR, and to update a PR which has been force-pushed.
Follows up on a [comment from 32774](https://github.com/bitcoin/bitcoin/pull/32774#discussion_r2156728913)
ACKs for top commit:
pablomartin4btc:
re-ACK 45b1d39
achow101:
ACK 45b1d39757
janb84:
re ACK 45b1d39757
theStack:
ACK 45b1d39757
Tree-SHA512: 3af02aa1335fd941538fabaa527bcfa92907dc6c272e72bc37ca38211b8aeebf32dd1837f976308058360ed1364fec749b49213f2b8bc4e35542da55a7bd30e1
Currently if the version 3 is selected for an otherwise
standard spender, the test will fail. It's unlikely but
possible, so change the test to update expectations and
sample more aggressively on border values to instigate
failures much quicker in the future if another version is
made standard.
fa21631595 test: Use self.log (MarcoFalke)
fa346f7797 test: Move error string into exception (MarcoFalke)
fa1986181f test: Remove useless catch-throw (MarcoFalke)
fa2f1c55b7 move-only util data to test/functional/data/util (MarcoFalke)
faa18bf287 test: Turn util/test_runner into functional test (MarcoFalke)
fa955154c7 test: Add missing skip_if_no_bitcoin_tx (MarcoFalke)
fac9db6eb0 test: Add missing tx util to Binaries (MarcoFalke)
fa91835ec6 test: Use lowercase env var as attribute name (MarcoFalke)
fac49094cd test: Remove duplicate ConfigParser (MarcoFalke)
Pull request description:
The `test/util/test_runner.py` has many issues:
* The boilerplate for the test runner is duplicate or inconsistent with the other (functional) tests. For example, logging options, `ConfigParser` handling, `Binaries` handling ...
* The cmake/ci behavior is brittle and can silently fail, as explained in https://github.com/bitcoin/bitcoin/issues/31476
* corecheck (and likely other places that manually run the tests) completely forget to run it
* If the test is manually called, it runs single threaded, when it could just run in parallel with the other functional tests
Fix all issues by removing the util test_runner and moving the test logic into a new functional test file.
ACKs for top commit:
janb84:
re ACK fa21631595
brunoerg:
re-ACK fa21631595
hebasto:
re-ACK fa21631595, additional feedback has been addressed since my previous [review](https://github.com/bitcoin/bitcoin/pull/32697#pullrequestreview-2940350432).
Tree-SHA512: 694e647887801f002843a74011035d5ed3dfed091d3f0ae18e812a16a4680e04e60e50de0a92af7e047e8ddd6ff5a7834c690f16fd42b74ebc1674bf9989406f
6967e8e8ab add more bad p2p ports (Jameson Lopp)
Pull request description:
Add a few more ports used by extremely well adopted services that require authentication and really ought not be used by bitcoin nodes for p2p traffic.
ACKs for top commit:
Sjors:
utACK 6967e8e8ab
l0rinc:
ACK 6967e8e8ab
glozow:
ACK 6967e8e8ab
Tree-SHA512: bbe86aef2be9727338712ded8f90227f5d12f633ab5d324c8907c01173945d1c4d9899e05565f78688842bbf5ebb010d22173969e4168ea08d4e33f01fe9569d
28299ce776 p2p: remove vestigial READ_STATUS_CHECKBLOCK_FAILED (Greg Sanders)
bac9ee4830 p2p: Add witness mutation check inside FillBlock (Greg Sanders)
Pull request description:
Since #29412, we have not allowed mutated blocks to continue being processed immediately the block is received, but this is only done for the legacy BLOCK message.
Extend these checks as belt-and-suspenders to not allow similar mutation strategies to affect relay by honest peers by applying the check inside `PartiallyDownloadedBlock::FillBlock`, immediately before returning `READ_STATUS_OK`.
ACKs for top commit:
Crypt-iQ:
ACK 28299ce776
achow101:
ACK 28299ce776
stratospher:
ACK 28299ce7.
dergoegge:
Code review ACK 28299ce776
Tree-SHA512: 883d7c12ca096234b425e6fe12e46b0611607600916e6ac8d1c8112224aa76924b7b074754910163ac2ec15379075d618a9ece3642649ac7629cddb0d4e432ea
fa3f100010 test: Use msg_generic in p2p_ping.py (MarcoFalke)
Pull request description:
It seems odd to derive `msg_pong_corrupt` from `msg_pong`, but then overwrite the serialize method, when one can just directly use `msg_generic` to pass the raw bytes to send over the wire.
Fix that by using `msg_generic`. This also serves as a regression test against the fix in commit 33480573cb.
(Can be tested by reverting that commit to observe a failure)
ACKs for top commit:
dergoegge:
utACK fa3f100010
theStack:
ACK fa3f100010
Tree-SHA512: 53d7d2289f27646fdf7d3b86c53e8e707fa4ca4b006d232850f9dc27409d79b7abe1dece95ccef429d4b52c6a89579c0cc5c0ee37046375c3c0310a2d6f9ddd5
7dc43ea503 test: Add msgtype to msg_generic slots (dergoegge)
Pull request description:
`msg_generic` can't be used unless `msgtype` is listed in `__slots__`
Example from a [custom test](6329ce979f/test/functional/p2p_bug28676.py):
```
2025-06-30T10:14:55.418000Z TestFramework (INFO): PRNG seed is: 3137163719543762151
2025-06-30T10:14:55.418000Z TestFramework (INFO): Initializing test directory /tmp/nix-shell-110135-0/bitcoin_func_test_7lmiemmp
2025-06-30T10:14:55.675000Z TestFramework (INFO): Setting up connections & mining some blocks...
2025-06-30T10:14:56.511000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/home/dergoegge/workspace/bitcoin/worktrees/master/test/functional/test_framework/test_framework.py", line 189, in main
self.run_test()
File "/home/dergoegge/workspace/bitcoin/worktrees/master/./build/test/functional/p2p_bug28676.py", line 46, in run_test
self.connections[0].send_without_ping(msg_generic(b"block", bytes.fromhex("0500000006226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f06b816712ffab8c59299a6abb58ccefa1995a1368ca4348782ef268cbc9bacacdbe5494dffff7f200200000001010000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff075105ffffffff00ffffffff0200f90295000000002200204ae81572f06e1b88fd5ced7a1a000945432e83e1551e6f721ee9c00b8cc332600000000000000000266a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf90120000000000000000000000000000000000000000000000000000000000000000000000000")))
File "/home/dergoegge/workspace/bitcoin/worktrees/master/test/functional/test_framework/messages.py", line 1386, in __init__
self.msgtype = msgtype
AttributeError: 'msg_generic' object has no attribute 'msgtype'
```
ACKs for top commit:
maflcko:
lgtm ACK 7dc43ea503
theStack:
ACK 7dc43ea503
Tree-SHA512: 8c634d50a884b063117e8ae29510ffd013e73dda9f8b0f73d098e80038b610ef8d80bd2e576c37f0cedfb4b6baa3d4ebeceb0902f29f90d59e1525f418f712fe
Due to a bug in `boost_headers-config.cmake`, the `Boost::headers`
target's `INTERFACE_INCLUDE_DIRECTORIES` property is set to "/usr",
which is incorrect.
9341b5333a blockstorage: make block read hash checks explicit (Lőrinc)
2371b9f4ee test/bench: verify hash in `ComputeFilter` reads (Lőrinc)
5d235d50d6 net: assert block hash in `ProcessGetBlockData` and `ProcessMessage` (Lőrinc)
Pull request description:
A follow-up to https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2094072165, after which validating the hash of a read block from disk doesn't incur the cost of calculating its hash anymore.
### Summary
This PR adds explicit checks that the read block header's hash matches the one we were expecting.
### Context
After the previous PR, validating a block's hash during read operations became essentially free. This PR leverages that by requiring callers to provide a block's expected hash (or `std::nullopt`), preventing silent failures caused by corrupted or mismatched data. Most `ReadBlock` usages were updated with expected hashes and now fail on mismatch.
### Changes
* added hash assertions in `ProcessGetBlockData` and `ProcessMessage` to validate that the block read from disk matches the expected hash;
* updated tests and benchmark to pass the correct block hash to `ReadBlock()`, ensuring the hash validation is tested - or none if we already expect PoW failure;
* removed the default value for `expected_hash`, requiring an explicit hash for all block reads.
### Why is the hash still optional (but no longer has a default value)
* for header-error tests, where the goal is to trigger failures early in the parsing process;
* for out-of-order orphan blocks, where the child hash isn't available before the initial disk read.
ACKs for top commit:
maflcko:
review ACK 9341b5333a🕙
achow101:
ACK 9341b5333a
hodlinator:
ACK 9341b5333a
janb84:
re ACK 9341b5333a
Tree-SHA512: cf1d4fff4c15e3f8898ec284929cb83d7e747125d4ee759e77d369f1716728e843ef98030be32c8d608956a96ae2fbefa0e801200c333b9eefd6c086ec032e1f
Ensure that tip_header.rehash() is used instead of tip_header.hash, which is None when the header is deserialized from hex.
This avoids depending on wait_for_getheaders() falling back to any received message, making the test more explicit and robust.
Some ambiguous uses of "we" referring to either the node or the peer are replaced with clearer phrasing.
Also rephrase some comments for consistency and readability.
Applies to all relevant outbound eviction tests in p2p_eviction_logic.py.
14653b869b build: Find Boost in config mode (Hennadii Stepanov)
Pull request description:
The `FindBoost` module has been removed by policy [CMP0167](https://cmake.org/cmake/help/latest/policy/CMP0167.html).
ACKs for top commit:
purpleKarrot:
ACK 14653b869b
Tree-SHA512: 5ec88647af83158f9bc04b41a3b72d4da7d84a7c81af351b8dac61cdf7f2f3b34bedd6ff164f21c229f2fd442918aaf21ba7c2c81c346b64de9032aae27b10ce
