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.
Migrating a wallet requires having a bestblock record. This is always
the case in normal operation as such a record is always written on
wallet loading if it didn't already exist. However, within the unit
tests and benchmarks, this is not guaranteed. Since migration requires
the record, WalletMigration needs to also add this record before the
benchmark.
55b931934a removed duplicate calling of GetDescriptorScriptPubKeyMan (Saikiran)
Pull request description:
Removed duplicate call to GetDescriptorScriptPubKeyMan and
Instead of checking linearly I have used find method so time complexity reduced significantly for GetDescriptorScriptPubKeyMan
after this fix improved performance of importdescriptor part refs https://github.com/bitcoin/bitcoin/issues/32013.
**Steps to reproduce in testnet environment**
**Input size:** 2 million address in the wallet
**Step1:** call importaddresdescriptor rpc method
observe the time it has taken.
**With the provided fix:**
Do the same steps again
observe the time it has taken.
There is a huge improvement in the performance. (previously it may take 5 to 6 seconds now it will take 1 seconds or less)
main changes i've made during this pr:
1. remove duplicate call to GetDescriptorScriptPubKeyMan method
2. And inside GetDescriptorScriptPubKeyMan method previously we checking **each address linearly** so each time it is calling HasWallet method which has aquired lock.
3. Now i've modified this logic call **find method on the map (O(logn)**) time it is taking, so only once we calling HasWallet method.
**Note:** Smaller inputs in the wallet you may not see the issue but huge wallet size it will definitely impact the performance.
ACKs for top commit:
achow101:
ACK 55b931934a
w0xlt:
ACK 55b931934a
Tree-SHA512: 4a7fdbcbb4e55bd034e9cf28ab4e7ee3fb1745fc8847adb388c98a19c952a1fb66d7b54f0f28b4c2a75a42473923742b4a99fb26771577183a98e0bcbf87a8ca
The migration benchmark crashes if run more than once, because of `std::move(wallet)` and leaves subsequent iterations in an undefined state - avoiding `UndefinedBehaviorSanitizer` null‑dereference error.
`MigrateLegacyToDescriptor` returns both a spendable descriptor wallet and a watch‑only wallet.
If these remain attached, their files stay open and on Windows this can hang CI when removing the test directory.
By constructing them via `MakeWalletLoader` (which owns the `WalletContext`), both wallets are automatically unloaded when the loader is destroyed at the end.
This ensures no lingering handles or resource leaks when running the benchmark on CI with `-sanity-check`.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
7912cd4125 bench: Fix WalletMigration benchmark (pablomartin4btc)
Pull request description:
The keys and scripts created for the Legacy Wallet needed to be persisted in order for the migration to work properly.
Fixes#32277.
ACKs for top commit:
achow101:
ACK 7912cd4125
davidgumberg:
Tested ACK 7912cd4125
furszy:
utACK 7912cd4125
Tree-SHA512: fe7b8e0a80d4d030ad3fd6446717ee09a260ab2bd6140bc817bdca52d233e3af8a8fed2d754743ca2ba022f7d2c8615a36b5070991d12942c13835e8f72e359f
There turned out to be a mismatch in the tx output counts which caused
'ConnectBlockMixedEcdsaSchnorr' benchmark to run slower than
'ConnectBlockAllEcdsa' and 'ConnectBlockAllSchnorr'. This commit makes
the tx output counts uniform across all benchmarks.
This commit also renames the 'taproot_tx' variable to 'tx' to reflect
that this variable represents a general tx and not just a taproot tx.
Since cluster_linearize.h does not actually have a Cluster type anymore, it is more
appropriate to rename the index type to DepGraphIndex.
-BEGIN VERIFY SCRIPT-
sed -i 's/Data type to represent transaction indices in clusters./Data type to represent transaction indices in DepGraphs and the clusters they represent./' $(git grep -l 'using ClusterIndex')
sed -i 's|\<ClusterIndex\>|DepGraphIndex|g' $(git grep -l 'ClusterIndex')
-END VERIFY SCRIPT-
Removed duplicate call to GetDescriptorScriptPubKeyMan and
Instead of checking linearly I have used find method so time complexity reduced significantly for GetDescriptorScriptPubKeyMan
after this fix improved performance of importdescriptor part refs #32013.
7edaf8b64c Benchmark Chainstate::ConnectBlock duration (Eunovo)
Pull request description:
Introduce benchmarks to evaluate ConnectBlock performance for:
- Blocks containing only Schnorr signatures
- Blocks containing both Schnorr and ECDSA signatures
- Blocks containing only ECDSA signatures
The benchmarks in this PR, focus on signature validation. Additional benchmarks may be added in the future to assess other aspects of ConnectBlock.
This is the first step toward implementing Batch Verification of Schnorr Signatures in Core. It provides a way to test and measure the performance improvements of batch verification on Core.
For more details on batch validation, refer to the [batch-verify module on secp](https://github.com/bitcoin-core/secp256k1/pull/1134) and [batch-verify on core](https://github.com/bitcoin/bitcoin/pull/29491).
ACKs for top commit:
josibake:
reACK 7edaf8b64c
fjahr:
utACK 7edaf8b64c
l0rinc:
ACK 7edaf8b64c
Tree-SHA512: 883c8a5e4e4de401ffb9ac9b6789b7fe0737afefbdaf02c6d7e1645392efc4f0d2d28b423ba7e34366a33608e0835793f5e7a1312b5c8063de14446319529cc7
ffff4a293a bench: Update span-serialize comment (MarcoFalke)
fa4d6ec97b refactor: Avoid false-positive gcc warning (MarcoFalke)
fa942332b4 scripted-diff: Bump copyright headers after std::span changes (MarcoFalke)
fa0c6b7179 refactor: Remove unused Span alias (MarcoFalke)
fade0b5e5e scripted-diff: Use std::span over Span (MarcoFalke)
fadccc26c0 refactor: Make Span an alias of std::span (MarcoFalke)
fa27e36717 test: Fix broken span_tests (MarcoFalke)
fadf02ef8b refactor: Return std::span from MakeUCharSpan (MarcoFalke)
fa720b94be refactor: Return std::span from MakeByteSpan (MarcoFalke)
Pull request description:
`Span` has some issues:
* It does not support fixed-size spans, which are available through `std::span`.
* It is confusing to have it available and in use at the same time with `std::span`.
* It does not obey the standard library iterator build hardening flags. See https://github.com/bitcoin/bitcoin/issues/31272 for a discussion. For example, this allows to catch issues like the one fixed in commit fabeca3458.
Both types are type-safe and can even implicitly convert into each other in most contexts.
However, exclusively using `std::span` seems less confusing, so do it here with a scripted-diff.
ACKs for top commit:
l0rinc:
reACK ffff4a293a
theuni:
ACK ffff4a293a.
Tree-SHA512: 9cc2f1f43551e2c07cc09f38b1f27d11e57e9e9bc0c6138c8fddd0cef54b91acd8b14711205ff949be874294a121910d0aceffe0e8914c4cff07f1e0e87ad5b8
4cd95a2921 refactor: modernize remaining outdated trait patterns (Lőrinc)
ab2b67fce2 scripted-diff: modernize outdated trait patterns - values (Lőrinc)
8327889f35 scripted-diff: modernize outdated trait patterns - types (Lőrinc)
Pull request description:
The use of [`std::underlying_type_t<T>`](https://en.cppreference.com/w/cpp/types/underlying_type) or [`std::is_enum_v<T>`](https://en.cppreference.com/w/cpp/types/is_enum) (and similar ones, introduced in C++14) replace the `typename std::underlying_type<T>::type` and `std::is_enum<T>::value` constructs (available in C++11).
The `_t` and `_v` helper alias templates offer a more concise way to extract the type and value directly.
I've modified the instances I found in the codebase one-by-one (noticed them while investigating https://github.com/bitcoin/bitcoin/pull/31868), and afterwards extracted scripted diff commits to do the trivial ones automatically.
The last commit contains the values that were easier done manually.
I've excluded changes from `src/bench/nanobench.h`, `src/leveldb`, `src/minisketch`, `src/span.h` and `src/sync.h` - let me know if you think they should be included instead.
A few of the code changes can also be reproduced by clang-tidy (but not all of them):
```bash
cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON && cmake --build build -j$(nproc)
run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,modernize-type-traits' -fix $(git grep -lE '::(value|type)' ./src ':(exclude)src/bench/nanobench.h' ':(exclude)src/leveldb' ':(exclude)src/minisketch' ':(exclude)src/span.h' ':(exclude)src/sync.h')
```
ACKs for top commit:
laanwj:
Concept and code review ACK 4cd95a2921
Tree-SHA512: a4bcf0f267c0f4e02983b4d548ed6f58d464ec379ac5cd1f998b9ec0cf698b53a9f2557a05a342b661f1d94adefc9a0ce2dc8f764d49453aaea95451e2c4c581
