652424ad16 test: additional test coverage for script_verify_flags (Anthony Towns)
417437eb01 script/verify_flags: extend script_verify_flags to 64 bits (Anthony Towns)
3cbbcb66ef script/interpreter: make script_verify_flag_name an ordinary enum (Anthony Towns)
bddcadee82 script/verify_flags: make script_verify_flags type safe (Anthony Towns)
a5ead122fe script/interpreter: introduce script_verify_flags typename (Anthony Towns)
4577fb2b1e rpc: have getdeploymentinfo report script verify flags (Anthony Towns)
a3986935f0 validation: export GetBlockScriptFlags() (Anthony Towns)
5db8cd2d37 Move mapFlagNames and FormatScriptFlags logic to script/interpreter.h (Anthony Towns)
Pull request description:
We currently use 21 of 32 possible bits for `SCRIPT_VERIFY_*` flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is [already reusing bits here](d4a86277ed/src/script/interpreter.h (L175-L195)). Therefore, bump this to 64 bits.
In order to make it easier to update this logic in future, this PR also introduces a dedicated type for the script flags, and disables implicit conversion between that type and the underlying integer type. To make verifying that this change doesn't cause flags to disappear, this PR also resurrects the changes from #28806 so that the script flags that are consensus enforced on each block can be queried via getdeploymentinfo.
ACKs for top commit:
instagibbs:
reACK 652424ad16
achow101:
ACK 652424ad16
darosior:
ACK 652424ad16
theStack:
Code-review ACK 652424ad16🎏
Tree-SHA512: 7b30152196cdfdef8b9700b571b7d7d4e94d28fbc5c26ea7532788037efc02e4b1d8de392b0b20507badfdc26f5c125f8356a479604a9149b8aae23a7cf5549f
Since the only remaining isminetypes are ISMINE_NO and ISMINE_SPENDABLE,
this enum is now just a bool and can be removed. IsMine is changed to
return a bool and any usage of isminetypes and isminefilters are changed
to be the remaining ISMINE_SPENDABLE case.
In descriptor wallets, we consider all outputs to be spendable as we no
longer have mixed mine and watchonly in a wallet. As such,
COutput::spendable is meaningless and can be removed.
Furthermore, CoinFilterParams::only_spendable can be removed as that was
essentially checking for COutput::spendable.
Lastly, AvailableCoinsListUnspent can also be removed as the wrapper is
now only setting the feerate to std::nullopt which is trivial enough that
a dedicated wrapper is not needed.
Previously the SCRIPT_VERIFY_* flags were specified as either uint32_t,
unsigned int, or unsigned. This converts them to a common type alias in
preparation for changing the underlying type.
f49840dd90 doc: Fix typo in files.md (Ryan Ofsky)
f5cf0b1ccc bitcoin wrapper: improve help output (Ryan Ofsky)
c810b168b8 doc: Add description of installed files to files.md (Ryan Ofsky)
94ffd01a02 doc: Add release notes describing libexec/ binaries (Ryan Ofsky)
cd97905ebc cmake: Move internal binaries from bin/ to libexec/ (Ryan Ofsky)
Pull request description:
This change moves binaries that are not typically invoked directly by users from the `bin/` directory to the `libexec/` directory in CMake installs and binary releases. The goal of the PR is to introduce a distinction between internal and external binaries so starting with #31802, we can use IPC to implement features in new binaries without adding those binaries to the CLI. The change also helps reduce clutter in `bin/`, making it easier for users to identify useful tools to run. Summary of changes:
- For **source builds** (i.e. developer builds) — There are no changes.
- For **source installs** (i.e. `cmake --install` result) — `test_bitcoin`, `test_bitcoin-qt`, and `bench_bitcoin` are installed in `${CMAKE_PREFIX_PATH}/libexec` instead of `${CMAKE_PREFIX_PATH}/bin`, so they are no longer on the system `PATH`. However, they can still be invoked from the `libexec/` directory, or from the CLI as `bitcoin test`, `bitcoin test-gui`, and `bitcoin bench`, respectively.
- For **binary releases** — Since `test_bitcoin` is the only test binary enabled in releases, the only change is moving `test_bitcoin` from `bin/` to `libexec/`.
<details><summary>Details</summary>
<p>
The table below shows the install location of each binary after this change, and the availability of each binary.
| Binary | Location | Availability | Change |
|----------------------|--------------|----------------------|-------------------------------|
| `bitcoin` | `bin/` | 📦 Binary release (since #31375) | Unchanged |
| `bitcoin-cli` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoind` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoin-qt` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoin-tx` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoin-util` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoin-wallet` | `bin/` | 📦 Binary release | Unchanged |
| `bench_bitcoin` | `libexec/` | 🛠 Source build only | Moved from `bin/` |
| `bitcoin-chainstate` | `libexec/` | 🛠 Source build only | Newly installed (was built) |
| `bitcoin-gui` | `libexec/` | 🛠 Source build only (until #31802) | Moved from `bin/` |
| `bitcoin-node` | `libexec/` | 🛠 Source build only (until #31802) | Moved from `bin/` |
| `test_bitcoin` | `libexec/` | 📦 Binary release | Moved from `bin/` |
| `test_bitcoin-qt` | `libexec/` | 🛠 Source build only | Moved from `bin/` |
</p>
</details>
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
l0rinc:
re-ACK f49840dd90
Sjors:
re-ACK f49840dd90
achow101:
ACK f49840dd90
janb84:
re ACK f49840dd90
BrandonOdiwuor:
Tested ACK f49840dd90
hodlinator:
re-ACK f49840dd90
willcl-ark:
utACK f49840dd90
Tree-SHA512: 858a2e1a53db11ee3c5c759bfdeea566f242b9ce5e8a898fa435222e41662b8184577c0dc2c4c058294b4de41d8cb3ba3e5d24c748c280efa4a3f84e3ec4344d
c0642e558a [fuzz] fix latency score check in txorphan_protected (glozow)
3d4d4f0d92 scripted-diff: rename "ann" variables to "latency_score" (monlovesmango)
3b92448923 [doc] comment fixups for orphanage changes (glozow)
1384dbaf6d [config] emit warning for -maxorphantx, but allow it to be set (glozow)
b10c55b298 fix up TxOrphanage lower_bound sanity checks (glozow)
cfd71c6704 scripted-diff: rename TxOrphanage outpoints index (glozow)
edb97bb3f1 [logging] add logs for inner loop of LimitOrphans (glozow)
8a58d0e87d scripted-diff: rename OrphanTxBase to OrphanInfo (glozow)
cc50f2f0df [cleanup] replace TxOrphanage::Size() with CountUniqueOrphans (glozow)
ed24e01696 [optimization] Maintain at most 1 reconsiderable announcement per wtxid (Pieter Wuille)
af7402ccfa [refactor] make TxOrphanage keep itself trimmed (glozow)
d1fac25ff3 [doc] 31829 release note (glozow)
Pull request description:
Followup to #31829:
- Release notes
- Have the orphanage auto-trim itself whenever necessary (and test changes) https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169508690
- Reduce duplicate reconsiderations by keeping track of which txns are already reconsiderable so we only mark it for reconsideration for 1 peer at a time https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3001627814
- Rename `OrphanTxBase` to `OrphanInfo`
- Get rid of `Size()` method by replacing all calls with `CountUniqueOrphans`
- Rename outpoints index since they point to wtxids, not iterators https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205557613
- Add more logging in the `LimitOrphans` inner loop to make it easy to see which peers are being trimmed https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3074385460
ACKs for top commit:
sipa:
utACK c0642e558a
marcofleon:
Nice, ACK c0642e558a
Tree-SHA512: f298eae92cf906ed5e4f15a24eeffa7b9e620bcff457772cd77522dd9f0b3b183ffc976871b1b0e6fe93009e64877d518e53d4b9e186e0df58fc16d17f6de90a
-BEGIN VERIFY SCRIPT-
sed -i 's/max_global_ann/max_global_latency_score/g' src/node/txorphanage.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/node/txorphanage.h
sed -i 's/max_global_ann/max_global_latency_score/g' src/test/orphanage_tests.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/test/fuzz/txorphan.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/bench/txorphanage.cpp
sed -i 's/max_ann/max_lat/g' src/node/txorphanage.cpp
-END VERIFY SCRIPT-
62ed1f92ef txgraph: check that DoWork finds optimal if given high budget (tests) (Pieter Wuille)
f3c2fc867f txgraph: add work limit to DoWork(), try optimal (feature) (Pieter Wuille)
e96b00d99e txgraph: make number of acceptable iterations configurable (feature) (Pieter Wuille)
cfe9958852 txgraph: track amount of work done in linearization (preparation) (Pieter Wuille)
6ba316eaa0 txgraph: 1-or-2-tx split-off clusters are optimal (optimization) (Pieter Wuille)
fad0eb091e txgraph: reset quality when merging clusters (bugfix) (Pieter Wuille)
Pull request description:
Part of #30289. Builds on top of #31553.
So far, the `TxGraph::DoWork()` function took no parameters, and just made all clusters reach the "acceptable" internal quality level by performing a minimum number of improvement iterations on it, but:
* Did not attempt to go beyond that.
* Was broken, as the QualityLevel of optimal clusters that merge together was not being reset.
Fix this by adding an argument to `DoWork()` to control how much work it is allowed to do right now, which will first be used to get all clusters to the acceptable level, and if more budget remains, use it to try to get some or all clusters optimal. The function will now return `true` if all clusters are known to be optimal (and thus no further work remains). This is verified in the tests, by remembering whether the graph is optimal, and if it is at the end of the simulation run, verify that the overall linearization cannot be improved further.
ACKs for top commit:
instagibbs:
ACK 62ed1f92ef
ismaelsadeeq:
Code review ACK 62ed1f92ef
glozow:
ACK 62ed1f92ef
Tree-SHA512: 5f57d4052e369f3444e72e724f04c02004e0f66e365faa59c9f145323e606508380fc97bb038b68783a62ae9c10757f1b628b3b00b2ce9a46161fca2d4336d73
This is meant to focus the usages to narrow the scope of the obfuscation optimization.
`Obfuscation::Xor` is mostly a move.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Since 31 byte xor-keys are not used in the codebase, using the common size (8 bytes) makes the benchmarks more realistic.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
150b5c99ca wallet: replace `reload_wallet` with inline functionality (rkrux)
0f86da382d wallet: remove dead code in legacy wallet migration (rkrux)
Pull request description:
A discussion on a previous [PR 32481](https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2145152084) related to legacy wallet dead
code removal made me realize that checking if the legacy
wallet was loaded prior to the start of the migration is not
required ever since legacy wallets can't be loaded in the first
place. I also verified that the `load_on_start` persistent
setting can also not cause the legacy wallets to be loaded, which
further makes the case for removal of the above mentioned checks
during migration.
The current test coverage also shows these lines uncovered.
ACKs for top commit:
achow101:
ACK 150b5c99ca
furszy:
ACK 150b5c99ca
Tree-SHA512: 9bc7043cac1f4051228557208895e43648de3c7ffae6860c0676d1aa2db3a8ed3a09d1f9defacd96ca50bbb9699ba86652ccb0c5e55cc88be248a1fe727c13d9
fa2fbaa4a2 bench: Avoid tmp files in pwd (MarcoFalke)
Pull request description:
It is a bit confusing that one bench run, when aborted, could leave behind temp files in the current working directory. It is similarly confusing to delete those files in the next run of bench.
Fix all issues by using `BasicTestingSetup`, which provides a proper temp folder to use and also cleans up after itself.
Can be tested via:
```
( echo 'my file content' > streams_tmp ) && ls streams_tmp && ./bld-cmake/bin/bench_bitcoin --filter=FindByte && ls streams_tmp
```
Previously the file would be deleted, now it is kept.
ACKs for top commit:
stickies-v:
ACK fa2fbaa4a2
Tree-SHA512: 33798030f990d1b4c95be4682d8dbfad95e8716d5fc0b99d65937196f2ced1ba649193c2adba4155f4eec9fd06e16be6667f3c3705af1880f47b2ff57a76243b
1632fc104b txgraph: Track multiple potential would-be clusters in Trim (improvement) (Pieter Wuille)
4608df37e0 txgraph: add Trim benchmark (benchmark) (Pieter Wuille)
9c436ff01c txgraph: add fuzz test scenario that avoids cycles inside Trim() (tests) (Pieter Wuille)
938e86f8fe txgraph: add unit test for TxGraph::Trim (tests) (glozow)
a04e205ab0 txgraph: Add ability to trim oversized clusters (feature) (Pieter Wuille)
eabcd0eb6f txgraph: remove unnecessary m_group_oversized (simplification) (Greg Sanders)
19b14e61ea txgraph: Permit transactions that exceed cluster size limit (feature) (Pieter Wuille)
c4287b9b71 txgraph: Add ability to configure maximum cluster size/weight (feature) (Pieter Wuille)
Pull request description:
Part of cluster mempool (#30289).
During reorganisations, it is possible that dependencies get added which would result in clusters that violate policy limits (cluster count, cluster weight), when linking the new from-block transactions to the old from-mempool transactions. Unlike RBF scenarios, we cannot simply reject the changes when they are due to received blocks. To accommodate this, add a `TxGraph::Trim()`, which removes some subset of transactions (including descendants) in order to make all resulting clusters satisfy the limits.
Conceptually, the way this is done is by defining a rudimentary linearization for the entire would-be too-large cluster, iterating it from beginning to end, and reasoning about the counts and weights of the clusters that would be reached using transactions up to that point. If a transaction is encountered whose addition would violate the limit, it is removed, together with all its descendants.
This rudimentary linearization is like a merge sort of the chunks of the clusters being combined, but respecting topology. More specifically, it is continuously picking the highest-chunk-feerate remaining transaction among those which have no unmet dependencies left. For efficiency, this rudimentary linearization is computed lazily, by putting all viable transactions in a heap, sorted by chunk feerate, and adding new transactions to it as they become viable.
The `Trim()` function is rather unusual compared to the `TxGraph` functionality added in previous PRs, in that `Trim()` makes it own decisions about what the resulting graph contents will be, without good specification of how it makes that decision - it is just a best-effort attempt (which is improved in the last commit). All other `TxGraph` mutators are simply to inform the graph about changes the calling mempool code decided on; this one lets the decision be made by txgraph.
As part of this, the "oversized" property is expanded to also encompass a configurable cluster weight limit (in addition to cluster count limit).
ACKs for top commit:
instagibbs:
reACK 1632fc104b
glozow:
reACK 1632fc104b via range-diff
ismaelsadeeq:
reACK 1632fc104b🛰️
Tree-SHA512: ccacb54be8ad622bd2717905fc9b7e42aea4b07f824de1924da9237027a97a9a2f1b862bc6a791cbd2e1a01897ad2c7c73c398a2d5ccbce90bfbeac0bcebc9ce
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
A discussion on a previous PR 32481 related to legacy wallet dead
code removal made me realize that checking if the legacy
wallet was loaded prior to the start of the migration is not
required ever since legacy wallets can't be loaded in the first
place. I also verified that the `load_on_start` persistent
setting can also not cause the legacy wallets to be loaded, which
further makes the case for removal of the above mentioned checks
during migration.
The current test coverage also shows these lines uncovered.
In the existing Trim function, as soon as the set of accepted transactions
would exceed the max cluster size or count limit, the acceptance loop is
stopped, removing all later transactions. However, it is possible that by
excluding some of those transactions the would-be cluster splits apart into
multiple would-clusters. And those clusters may well permit far more
transactions before their limits are reached.
Take this into account by using a union-find structure inside TrimTxData to
keep track of the count/size of all would-be clusters that would be formed
at any point, and only reject transactions which would cause these resulting
partitions to exceed their limits.
This is not an optimization in terms of CPU usage or memory; it just
improves the quality of the transactions removed by Trim().
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
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
One of the main issues with AvailableCoins is its performance when txs
have unrelated outputs, so update the benchmark to check the performance
of that.
There is no way to report a close error from `AutoFile` destructor.
Such an error could be serious if the file has been written to because
it may mean the file is now corrupted (same as if write fails).
So, change all users of `AutoFile` that use it to write data to
explicitly close the file and handle a possible error.
Switch to the index-aware `ReadBlock()` overload in `ComputeFilter` so that filter creation will abort if the stored block header hash doesn't match the expected one.
In the `readwriteblock` benchmark, pass the expected hash to `ReadBlock()` to match the new signature without affecting benchmark performance.
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
This can be reproduced according to the developer notes with something
like
( cd ./src/ && ../contrib/devtools/run-clang-tidy.py -p ../bld-cmake -fix -j $(nproc) )
Also, the header related changes were done manually.
This change moves binaries that are not typically invoked directly by users
from the `bin/` directory to the `libexec/` directory in CMake installs and
binary releases. The goal is to simplify the contents of `bin/` for end users
while still making all binaries available when needed. After this change, the
binaries remaining in `bin/` are:
- bitcoin
- bitcoin-cli
- bitcoind
- bitcoin-qt
- bitcoin-tx
- bitcoin-util
- bitcoin-wallet
And the binaries that are moved to `libexec/` are:
- bench_bitcoin
- bitcoin-chainstate(*)
- bitcoin-gui(***)
- bitcoin-node(***)
- test_bitcoin(**)
- test_bitcoin-qt
(*) bitcoin-chainstate was previously missing an install rule and was actually
not installed even when it was enabled.
(**) test_bitcoin is the only libexec/ binary that is currently included in
bitcoin binary releases. The others are only installed when building from
source with relevant cmake options enabled.
(***) bitcoin-node and bitcoin-gui are not currently built by default or
included in binary releases but both of these changes are planned and
implemented in #31802
fd290730f5 validation: clean up and clarify CheckInputScripts logic (Cory Fields)
1a37507895 validation: use a lock for CCheckQueueControl (Cory Fields)
c3b0e6c7f4 validation: make CCheckQueueControl's CCheckQueue non-optional (Cory Fields)
4c8c90b556 validation: only create a CCheckQueueControl if it's actually going to be used (Cory Fields)
11fed833b3 threading: add LOCK_ARGS macro (Cory Fields)
Pull request description:
As part of an effort to cleanup our threading primitives and add safe `SharedMutex`/`SharedLock` impls, I'd like to get rid of the last of our legacy `ENTER_CRITICAL_SECTION`/`LEAVE_CRITICAL_SECTION` usage. This, along with a follow-up [after fixing REVERSE_LOCK](https://github.com/bitcoin/bitcoin/pull/32465) will allow us to do that.
This replaces the old macros with an RAII lock, while simplifying `CCheckQueueControl`. It now requires a `CCheckQueue`, and optionality is handled externally. In the case of validation, it is wrapped in a `std::optional`.
It also adds an `LOCK_ARGS` macro for `UniqueLock` initialization which may be helpful elsewhere.
ACKs for top commit:
fjahr:
re-ACK fd290730f5
ryanofsky:
Code review ACK fd290730f5, just removing assert since last review. Thanks for considering all the comments and feedback!
TheCharlatan:
Re-ACK fd290730f5
Tree-SHA512: 54b9db604ee1bda7d11bce1653a88d3dcbc4f525eed6a85abdd4d6409138674af4bb8b00afa4e0d3d29dadd045a3a39de253a45f0ef9c05f56cba1aac5b59303
785e1407b0 wallet: Use util::Error throughout AddWalletDescriptor (Ava Chow)
Pull request description:
#32023 changed `AddWalletDescriptor` to return `util::Error`, but did not change all of the failure cases to do so. This may result in some callers continuing when there was actually an error. Unify all of the failure cases to use `util::Error` so that all callers handle `AddWalletDescriptor` errors in the same way.
The encapsulated return type is changed from `ScriptPubKeyMan*` to `std::reference_wrapper<DescriptorScriptPubKeyMan>`. This avoids having a value that can be interpreted as a bool, and also removes the need to constantly dynamic_cast the returned value. The only kind of `ScriptPubKeyMan` that can come out of `AddWalletDescriptor` is a `DescriptorScriptPubKeyMan` anyways.
ACKs for top commit:
Sjors:
utACK 785e1407b0
ryanofsky:
Code review ACK 785e1407b0
furszy:
Code review ACK 785e1407b0
Tree-SHA512: 52a48263c8d4161a8c0419b7289c25b0986f8e3bcd10b639eeeb0b6862d08b6c5e70998d20070ab26b39ecd90ab83dc8b71c65d85f70626282cf8cc6abff50e7
32023 changed AddWalletDescriptor to return util::Error, but did not
change all of the failure cases to do so. This may result in some
callers continuing when there was actually an error. Unify all of the
failure cases to use util::Error so that all callers handle
AddWalletDescriptor errors in the same way.
The encapsulated return type is changed from ScriptPubKeyMan* to
std::reference_wrapper<DescriptorScriptPubKeyMan>. This avoids having a
value that can be interpreted as a bool, and also removes the need to
constantly dynamic_cast the returned value. The only kind of
ScriptPubKeyMan that can come out of AddWalletDescriptor is a
DescriptorScriptPubKeyMan anyways.
30a94b1ab9 test, wallet: Remove concurrent writes test (Ava Chow)
b44b7c03fe wallet: Write best block record on unload (Ava Chow)
876a2585a8 wallet: Remove unnecessary database Close step on shutdown (Ava Chow)
98a1a5275c wallet: Remove chainStateFlushed (Ava Chow)
7fd3e1cf0c wallet, bench: Write a bestblock record in WalletMigration (Ava Chow)
6d3a8b195a wallet: Replace chainStateFlushed in loading with SetLastBlockProcessed (Ava Chow)
7bacabb204 wallet: Update best block record after block dis/connect (Ava Chow)
Pull request description:
Implements the idea discussed in https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-2010579484
Currently, `m_last_block_processed` and `m_last_block_processed_height` are not guaranteed to match the block locator stored in the wallet, nor do either of those fields actually represent the last block that the wallet is synced up to. This is confusing and unintuitive.
This PR changes those last block fields to be updated whenever the wallet makes a change to the db for new transaction state found in new blocks. Whenever a block is received that contains a transaction relevant to the wallet, the last block locator will now be written to disk. Furthermore, every block disconnection will now write an updated locator.
To ensure that the locator is relatively recent and loading rescans are fairly quick in the event of unplanned shutdown, it is also now written every 144 blocks (~1 day). Additionally it is now written when the wallet is unloaded so that it is accurate when the wallet is loaded again.
Lastly, the `chainstateFlushed` notification in the wallet is changed to be a no-op. The best block locator record is no longer written when `chainstateFlushed` is received from the node since it should already be mostly up to date.
ACKs for top commit:
rkrux:
ACK 30a94b1ab9
mzumsande:
Code Review ACK 30a94b1ab9
ryanofsky:
Code review ACK 30a94b1ab9. Only changes since last review are using WriteBestBlock method more places and updating comments.
Tree-SHA512: 46117541f8aaf13dde57430e813b4bbbd5e146e2632769675803c8e65a82f149a7cc6026489a127d32684b90124bd2b7c28216dbcfa6a47447300e8f3814e029
It is only used in test. There it is problematic, because it sometimes
relies on m_default_address_type. If the default were changed to
BECH32M, those tests would fail the assert(false).
So just use PKHash{} in all tests and remove GetDestinationForKey.
chainStateFlushed is no longer needed since the best block is updated
after a block is scanned. Since the chainstate being flushed does not
necessarily coincide with the wallet having processed said block, it
does not entirely make sense for the wallet to be recording that block
as its best block, and this can cause race conditions where some blocks
are not processed. Thus, remove this notification.
