98868633d1 Bugfix: configure: bitcoin-{cli,tx,util} don't need UPnP, NAT-PMP, or ZMQ (Luke Dashjr)
Pull request description:
As with #23345, these other tools likewise don't use various deps.
ACKs for top commit:
achow101:
ACK 98868633d1
Tree-SHA512: 4be056b8e0c9f69834229aa257187457de1bc34214d320b770834e21ecc1f0ca7aa7b9689fba525928947bfabbb461528795f709014fb9618b82f088fe64f271
0f38524c31 doc: correct deriveaddresses RPC name (Bitcoin Hodler)
Pull request description:
There never was a `deriveaddress` RPC, from what I can tell. It was always called `deriveaddresses` (plural).
ACKs for top commit:
theStack:
ACK 0f38524c31
Zero-1729:
ACK 0f38524c31
Tree-SHA512: 3f405f5479a0d39cf150fd80b4d854ffe4eef718a358202c619e34a08d98c1252b82fc70d106cdf2215dc5a50c6f6cd5e26fe7ed87156f6b08f8e97d963affb7
8a9f1e4d18 test: fix intermittent failure in rpc_getblockfrompeer.py (Martin Zumsande)
Pull request description:
Fixes an intermittent failure in `rpc_getblockfrompeer.py` observed in https://cirrus-ci.com/task/6610115527704576 by adding a sync to make sure the node has processed the header we sent it before we query it for the corresponding block.
Fixes#26412
ACKs for top commit:
jonatack:
ACK 8a9f1e4d18
Tree-SHA512: f6188ab3cfd863034e44e9806d0d99a8781462bec94141501aefc71589153481ffb144e126326ab81807c2b2c93de7f4aac5d09dbcf26c4512a176e06fc6e5f8
ef97b89902 Exclude rand from debug log (Jeff Ruane)
Pull request description:
Currently, `debug.log` is spammed with messages like this from `random.cpp` when functional tests are run.
```
2022-10-25T19:24:34.787663Z [scheduler] [random.cpp:519] [SeedPeriodic] [rand] Feeding 36565 bytes of dynamic environment data into RNG
```
These logs are not useful for debugging and decrease the signal-to-noise ratio of the logs, so they should be suppressed by excluding the `rand` category, as the `libevent` and `leveldb` categories currently are.
ACKs for top commit:
LarryRuane:
ACK ef97b89902
kouloumos:
ACK ef97b89902, confirmed that this log level is only used in `random.cpp` and indeed it seems that it doesn't add any value to the debug.log during functional tests.
satsie:
ACK ef97b89902
theStack:
ACK ef97b89902
Tree-SHA512: 5cea384a3197f0ec77efa9efc77822914450ecf5546606568bbd432c3536040c772c57aef58d3bb083a2e5e756f690766fa1fb382ab1973748db238108a58746
fa29ef00ad refactor: Silence GCC Wmissing-field-initializers in ChainstateManagerOpts (MacroFake)
Pull request description:
The `std::optional` fields in the struct that fall back to chain param defaults if not provided should be initialized to `std::nullopt`. This already happens with the current code.
However, for consistency with `check_block_index` and to silence a GCC warning, add the "missing" `{}`.
ACKs for top commit:
achow101:
ACK fa29ef00ad
hebasto:
ACK fa29ef00ad, tested on Ubuntu 22.04 + GCC 11.3.
jonatack:
ACK fa29ef00ad
Tree-SHA512: bdec9c56df5d601a5616e107fed48737b13b0a7242b6526092fb682b5016544a4bc08666b60304c668d44c6f7ac69d3788093d921382c1d6c577c1f9fe31fc50
Currently, debug.log is spammed with messages from random.cpp
when functional tests are run. These logs are not useful for
debugging, and decrease the signal to noise ratio of the logs.
3fcb545ab2 bench: benchmark transaction creation process (furszy)
a8a75346d7 wallet: SelectCoins, return early if target is covered by preset-inputs (furszy)
f41712a734 wallet: simplify preset inputs selection target check (furszy)
5baedc3351 wallet: remove fetch pre-selected-inputs responsibility from SelectCoins (furszy)
295852f619 wallet: encapsulate pre-selected-inputs lookup into its own function (furszy)
37e7887cb4 wallet: skip manually selected coins from 'AvailableCoins' result (furszy)
94c0766b0c wallet: skip available coins fetch if "other inputs" are disallowed (furszy)
Pull request description:
#### # Context (Current Flow on Master)
In the transaction creation process, in order to select which coins the new transaction will spend,
we first obtain all the available coins known by the wallet, which means walking-through the
wallet txes map, gathering the ones that fulfill certain spendability requirements in a vector.
This coins vector is then provided to the Coin Selection process, which first checks if the user
has manually selected any input (which could be internal, aka known by the wallet, or external),
and if it does, it fetches them by searching each of them inside the wallet and/or inside the
Coin Control external tx data.
Then, after finding the pre-selected-inputs and gathering them in a vector, the Coin Selection
process walks-through the entire available coins vector once more just to erase coins that are
in both vectors. So the Coin Selection process doesn’t pick them twice (duplicate inputs inside
the same transaction).
#### # Process Workflow Changes
Now, a new method, `FetchCoins` will be responsible for:
1) Lookup the user pre-selected-inputs (which can be internal or external).
2) And, fetch the available coins in the wallet (excluding the already fetched ones).
Which will occur prior to the Coin Selection process. Which allows us to never include the
pre-selected-inputs inside the available coins vector in the first place, as well as doing other
nice improvements (written below).
So, Coin Selection can perform its main responsibility without mixing it with having to fetch
internal/external coins nor any slow and unneeded duplicate coins verification.
#### # Summarizing the Improvements:
1) If any pre-selected-input lookup fail, the process will return the error right away.
(before, the wallet was fetching all the wallet available coins, walking through the
entire txes map, and then failing for an invalid pre-selected-input inside SelectCoins)
2) The pre-selected-inputs lookup failure causes are properly described on the return error.
(before, we were returning an "Insufficient Funds" error for everything, even if the failure
was due a not solvable external input)
3) **Faster Coin Selection**: no longer need to "remove the pre-set inputs from the available coins
vector so that Coin Selection doesn't pick them" (which meant to loop-over the entire
available coins vector at Coin Selection time, erasing duplicate coins that were pre-selected).
Now, the available coins vector, which is built after the pre-selected-inputs fetching,
doesn’t include the already selected inputs in the first place.
4) **Faster transaction creation** for transactions that only use manually selected inputs.
We now will return early, as soon as we finish fetching the pre-selected-inputs and
not perform the resources expensive calculation of walking-through the entire wallet
txes map to obtain the available coins (coins that we will not use).
---------------------------
Added a new bench (f6d0bb2) measuring the transaction creation process, for a wallet with ~250k UTXO, only using the pre-selected-inputs inside coin control. Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically.
#### Result on this PR (tip f6d0bb2d):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,048,675.00 | 953.58 | 0.3% | 0.06 | `WalletCreateTransaction`
vs
#### Result on master (tip 4a4289e2):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 96,373,458.20 | 10.38 | 0.2% | 5.30 | `WalletCreateTransaction`
The benchmark took to run in master: **96.37 milliseconds**, while in this PR: **1 millisecond** 🚀 .
ACKs for top commit:
S3RK:
Code Review ACK 3fcb545ab2
achow101:
ACK 3fcb545ab2
aureleoules:
reACK 3fcb545ab2
Tree-SHA512: 42f833e92f40c348007ca565a4c98039e6f1ff25d8322bc2b27115824744779baf0b0a38452e4e2cdcba45076473f1028079bbd0f670020481ec5d3db42e4731
eb679a7896 rpc: make `address` field optional (w0xlt)
Pull request description:
Close https://github.com/bitcoin/bitcoin/issues/26338.
This PR makes optional the `address` field in the response of `listtransactions` and `listsinceblock` RPC.
And adds two tests that fail on master, but not on this branch.
ACKs for top commit:
achow101:
ACK eb679a7896
aureleoules:
ACK eb679a7896
Tree-SHA512: b267439626e2ec3134ae790c849949a4c40ef0cebd20092e8187be3db0a61941b2da10bbbba92ca880b8369f46c1aaa806d057eaa5159325f65cbec7cb33c52f
da16893474 ci: Use `macos-ventura-xcode:14.1` image for "macOS native" task (Hennadii Stepanov)
702836530f ci: Make `getopt` path architecture agnostic (Hennadii Stepanov)
Pull request description:
The "macOS native" CI task always uses the recent OS image.
This PR updates it up to the recent macOS release.
Cirrus Labs [stopped](https://github.com/bitcoin/bitcoin/pull/25160#issuecomment-1162829773) updating macOS images for `x86_64`, therefore, an `arm64` image been used.
Also `make test-security-check` has been dropped as it ["isn't even expected to pass"](https://github.com/bitcoin/bitcoin/issues/26386#issuecomment-1290318628) on `arm64` in CI.
ACKs for top commit:
Sjors:
utACK da16893
Tree-SHA512: 36785d33b7f11b3cdbc53bcfbf97d88bf821fad248c825982dd9f8e3413809a4ef11190eaf950e60fdf479b62ff66920c35d9ea42d534723f015742eec7e19b6
b8b59ff9fe gui: update the screen after loading wallet (w0xlt)
Pull request description:
Currently, the user loads a wallet and the screen does not switch to the selected wallet after loading (File -> Open Wallet -> wallet name).
This PR changes that by making the `OpenWalletActivity::opened` signal connection a `Qt::QueuedConnection` type.
ACKs for top commit:
jarolrod:
ACK b8b59ff9fe
hebasto:
ACK b8b59ff9fe, tested on Ubuntu 22.04.
Tree-SHA512: 43cd755638b643f481014a7933a0af25df2d109e859cb5f878bc04e562950d550716fa38465140060e28526b2441688580cbcbe4ec6819566b4f95162ca5e527
0cc23fc603 Fix typo in comment SHA256->SHA512 (Elichai Turkel)
Pull request description:
The comment says it's the SHA-256 state, while it's actually the SHA-512 state
ACKs for top commit:
andrewtoth:
ACK 0cc23fc603
aureleoules:
ACK 0cc23fc603
Tree-SHA512: 4e390ceefb847d3bbe4f5caab390a4fdd14892fe443f58c32b08b3444fccd611cff22938c3dfa611dfd2497736f779fae4165497b4208e48aa8fc9d2236f943b
Goal 1:
Benchmark the transaction creation process for pre-selected-inputs only.
Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically.
Goal 2:
Benchmark the transaction creation process for pre-selected-inputs and coin selection.
-----------------------
Benchmark Setup:
1) Generates a 5k blockchain, loading the wallet with 5k transactions with two outputs each.
2) Fetch 4 random UTXO from the wallet's available coins and pre-select them as inputs inside CoinControl.
Benchmark (Goal 1):
Call `CreateTransaction` providing the coin control, who has set `m_allow_other_inputs=false` and
the manually selected coins.
Benchmark (Goal 2):
Call `CreateTransaction` providing the coin control, who has set `m_allow_other_inputs=true` and
the manually selected coins.
we are already computing the preset inputs total amount inside `PreSelectedInputs::Insert`,
which internally decides whether to use the effective value or the raw output value based on
the 'subtract_fee_outputs' flag.
so if there is an error in any of the pre-set coins, we can fail right away
without computing the wallet available coins set (calling `AvailableCoins`)
which is a slow operation as it goes through the entire wallet's txes map.
----------------------
And to make the Coin Selection flow cleared, have decoupled SelectCoins in two functions:
1) AutomaticCoinSelection.
2) SelectCoins.
1) AutomaticCoinSelection:
Receives a set of coins and selects the best subset of them to
cover the target amount.
2) SelectCoins
In charge of select all the user manually selected coins first ("pre-set inputs"), and
if coin_control 'm_allow_other_inputs=true', call 'AutomaticCoinSelection' to select a
subset of coins owned by the wallet to cover for the target - preset_inputs.total_amount
remaining value.
First step towards decoupling the pre-selected-inputs fetching functionality
from `SelectCoins`. Which, will let us not waste resources calculating the
available coins if one of the pre-set inputs has an error.
(right now, if one of the pre-set inputs is invalid, we first walk through
the entire wallet txes map just to end up failing right after it finish)
No need to walk through the entire wallet's txes map just to get
coins that we could have gotten by just doing a simple map.find(out.hash).
(Which is what we are doing inside `SelectCoins` anyway)
no need to waste resources calculating the wallet available coins if
they are not going to be used.
The 'm_allow_other_inputs=true` default value change is to correct
an ugly misleading behavior:
The tx creation process was having a workaround patch to automatically
fall back to select coins from the wallet if `m_allow_other_inputs=false`
(previous default value) and no manual inputs were selected.
This could be seen in master in flows like `sendtoaddress`, `sendmany`
and even the GUI, where the `m_allow_other_inputs` value isn't customized
and the wallet still selects and adds coins to the tx internally.
f5ff3d773c rpc: add missing lock around chainman.ActiveTip() (Andrew Toth)
Pull request description:
https://github.com/bitcoin/bitcoin/pull/23927 seems to have missed a lock around `chainman.ActiveChain()`.
ACKs for top commit:
aureleoules:
ACK f5ff3d773c
Tree-SHA512: 3f116ca44c1b2bc0c7042698249ea3417dfb7c0bb81158a7ceecd087f1e02baa89948f9bb7924b1757798a1691a7de6e886aa72a0a9e227c13a3f512cc59d6c9
fa54d3011e test: check for false-positives in rpc_scanblocks.py (Sebastian Falbesoner)
3bca6cd61a test: add compact block filter (BIP158) helper routines (Sebastian Falbesoner)
25ee74dd11 test: add SipHash implementation for generic data in Python (Sebastian Falbesoner)
Pull request description:
This PR adds a fixed false-positive element check to the functional test rpc_scanblocks.py by using a pre-calculated scriptPubKey that collides with the regtest genesis block's coinbase output. Note that determining a BIP158 false-positive at runtime would also be possible, but take too long (we'd need to create and check ~800k output scripts on average, which took at least 2 minutes on average on my machine).
The introduced check is related to issue #26322 and more concretely inspired by PR #26325 which introduces an "accurate" mode that filters out these false-positives. The introduced cryptography routines (siphash for generic data) and helpers (BIP158 ranged hash calculation, relevant scriptPubKey per block determination) could potentially also be useful for more tests in the future that involve compact block filters.
ACKs for top commit:
achow101:
ACK fa54d3011e
Tree-SHA512: c6af50864146028228d197fb022ba2ff24d1ef48dc7d171bccfb21e62dd50ac80db5fae0c53f5d205edabd48b3493c7aa2040f628a223e68df086ec2243e5a93
5826bf546e test: Add test for getblockfrompeer on syncing pruned nodes (Fabian Jahr)
7fa851fba8 rpc: Pruned nodes can not fetch unsynced blocks (Fabian Jahr)
Pull request description:
This PR prevents `getblockfrompeer` from getting used on blocks that the node has not synced past yet if the node is in running in prune mode.
### Problem
While a node is still catching up to the tip that it is aware of via the headers, the user can currently use to fetch blocks close to or at the tip. These blocks are stored in the block/rev file that otherwise contains blocks the node is receiving as part of the syncing process.
This creates a problem for pruned nodes: The files containing a fetched block are not pruned during syncing because they contain a block close to the tip. This means the entire file (~130MB) will not be pruned until the tip has moved on far enough from the fetched block. In extreme cases with heavy pruning (like 550) and multiple blocks being fetched this could mean that the disc usage far exceeds what the user expects, potentially running out of space.
### Approach
There would be certainly other approaches that could fix the problem while still allowing the current behavior, but all of the ideas I came up with seemed like overkill for a niche problem on a new RPC where it's still unclear how and how much it will be used.
### Testing
So far I did not see a simple enough way to test this I am still looking into it and if it's complex will potentially add it in a follow-up. What would be needed is a way to have a node fetch headers but not sync the blocks yet, that seems like a pattern that could be generally useful.
To manually reproduce the problematic behavior:
1. Start a node with current `master` with `-prune=550` and an empty/new datadir, Testnet and Mainnet should both work.
2. While the node is syncing run `getblockfrompeer` on the current tip and a few other recent blocks.
3. Go to your datadir and observe the blocks folder: There should be a few full `blk*.dat` and `rev*.dat` files that are not being pruned. When you "pinned" a few of these files the blocks folder should be significantly above the target size of 550MB.
ACKs for top commit:
Sjors:
utACK 5826bf546e
achow101:
ACK 5826bf546e
aureleoules:
tACK 5826bf546e
Tree-SHA512: aa3f477ec755a9df2331c047cb10b3cd08292522bf6ad7a36a7ea36d7eba4894b84de8bd23003c9baea5ac0c53b77142c3c2819ae7528cece9d10a0d06c850d8
0582932260 test: add test for fast rescan using block filters (top-up detection) (Sebastian Falbesoner)
ca48a4694f rpc: doc: mention rescan speedup using `blockfilterindex=1` in affected wallet RPCs (Sebastian Falbesoner)
3449880b49 wallet: fast rescan: show log message for every non-skipped block (Sebastian Falbesoner)
935c6c4b23 wallet: take use of `FastWalletRescanFilter` (Sebastian Falbesoner)
70b3513904 wallet: add `FastWalletRescanFilter` class for speeding up rescans (Sebastian Falbesoner)
c051026586 wallet: add method for retrieving the end range for a ScriptPubKeyMan (Sebastian Falbesoner)
845279132b wallet: support fetching scriptPubKeys with minimum descriptor range index (Sebastian Falbesoner)
088e38d3bb add chain interface methods for using BIP 157 block filters (Sebastian Falbesoner)
Pull request description:
## Description
This PR is another take of using BIP 157 block filters (enabled by `-blockfilterindex=1`) for faster wallet rescans and is a modern revival of #15845. For reviewers new to this topic I can highly recommend to read the corresponding PR review club (https://bitcoincore.reviews/15845).
The basic idea is to skip blocks for deeper inspection (i.e. looking at every single tx for matches) if our block filter doesn't match any of the block's spent or created UTXOs are relevant for our wallet. Note that there can be false-positives (see https://bitcoincore.reviews/15845#l-199 for a PR review club discussion about false-positive rates), but no false-negatives, i.e. it is safe to skip blocks if the filter doesn't match; if the filter *does* match even though there are no wallet-relevant txs in the block, no harm is done, only a little more time is spent extra.
In contrast to #15845, this solution only supports descriptor wallets, which are way more widespread now than back in the time >3 years ago. With that approach, we don't have to ever derive the relevant scriptPubKeys ourselves from keys before populating the filter, and can instead shift the full responsibility to that to the `DescriptorScriptPubKeyMan` which already takes care of that automatically. Compared to legacy wallets, the `IsMine` logic for descriptor wallets is as trivial as checking if a scriptPubKey is included in the ScriptPubKeyMan's set of scriptPubKeys (`m_map_script_pub_keys`): e191fac4f3/src/wallet/scriptpubkeyman.cpp (L1703-L1710)
One of the unaddressed issues of #15845 was that [the filter was only created once outside the loop](https://github.com/bitcoin/bitcoin/pull/15845#discussion_r343265997) and as such didn't take into account possible top-ups that have happened. This is solved here by keeping a state of ranged `DescriptorScriptPubKeyMan`'s descriptor end ranges and check at each iteration whether that range has increased since last time. If yes, we update the filter with all scriptPubKeys that have been added since the last filter update with a range index equal or higher than the last end range. Note that finding new scriptPubKeys could be made more efficient than linearly iterating through the whole `m_script_pub_keys` map (e.g. by introducing a bidirectional map), but this would mean introducing additional complexity and state and it's probably not worth it at this time, considering that the performance gain is already significant.
Output scripts from non-ranged `DescriptorScriptPubKeyMan`s (i.e. ones with a fixed set of output scripts that is never extended) are added only once when the filter is created first.
## Benchmark results
Obviously, the speed-up indirectly correlates with the wallet tx frequency in the scanned range: the more blocks contain wallet-related transactions, the less blocks can be skipped due to block filter detection.
In a [simple benchmark](https://github.com/theStack/bitcoin/blob/fast_rescan_functional_test_benchmark/test/functional/pr25957_benchmark.py), a regtest chain with 1008 blocks (corresponding to 1 week) is mined with 20000 scriptPubKeys contained (25 txs * 800 outputs) each. The blocks each have a weight of ~2500000 WUs and hence are about 62.5% full. A global constant `WALLET_TX_BLOCK_FREQUENCY` defines how often wallet-related txs are included in a block. The created descriptor wallet (default setting of `keypool=1000`, we have 8*1000 = 8000 scriptPubKeys at the start) is backuped via the `backupwallet` RPC before the mining starts and imported via `restorewallet` RPC after. The measured time for taking this import process (which involves a rescan) once with block filters (`-blockfilterindex=1`) and once without block filters (`-blockfilterindex=0`) yield the relevant result numbers for the benchmark.
The following table lists the results, sorted from worst-case (all blocks contain wallte-relevant txs, 0% can be skipped) to best-case (no blocks contain walltet-relevant txs, 100% can be skipped) where the frequencies have been picked arbitrarily:
wallet-related tx frequency; 1 tx per... | ratio of irrelevant blocks | w/o filters | with filters | speed gain
--------------------------------------------|-----------------------------|-------------|--------------|-------------
~ 10 minutes (every block) | 0% | 56.806s | 63.554s | ~0.9x
~ 20 minutes (every 2nd block) | 50% (1/2) | 58.896s | 36.076s | ~1.6x
~ 30 minutes (every 3rd block) | 66.67% (2/3) | 56.781s | 25.430s | ~2.2x
~ 1 hour (every 6th block) | 83.33% (5/6) | 58.193s | 15.786s | ~3.7x
~ 6 hours (every 36th block) | 97.22% (35/36) | 57.500s | 6.935s | ~8.3x
~ 1 day (every 144th block) | 99.31% (143/144) | 68.881s | 6.107s | ~11.3x
(no txs) | 100% | 58.529s | 5.630s | ~10.4x
Since even the (rather unrealistic) worst-case scenario of having wallet-related txs in _every_ block of the rescan range obviously doesn't take significantly longer, I'd argue it's reasonable to always take advantage of block filters if they are available and there's no need to provide an option for the user.
Feedback about the general approach (but also about details like naming, where I struggled a lot) would be greatly appreciated. Thanks fly out to furszy for discussing this subject and patiently answering basic question about descriptor wallets!
ACKs for top commit:
achow101:
ACK 0582932260
Sjors:
re-utACK 0582932260
aureleoules:
ACK 0582932260 - minor changes, documentation and updated test since last review
w0xlt:
re-ACK 0582932260
Tree-SHA512: 3289ba6e4572726e915d19f3e8b251d12a4cec8c96d041589956c484b5575e3708b14f6e1e121b05fe98aff1c8724de4564a5a9123f876967d33343cbef242e1
fa3da8307b test: Check debug log as well in p2p_sendtxrcncl.py (MacroFake)
fae0439486 test: Check correct disconnect reason in p2p_sendtxrcncl.py (MacroFake)
fa590cfaae test: Fix intermittent issue in p2p_sendtxrcncl.py (MacroFake)
Pull request description:
Should fix https://github.com/bitcoin/bitcoin/issues/26364
I can't reproduce this, but my guess would be that `PeerNoVerack::on_version`, which sends the `wtxidrelay` message, is executed in the event loop and thus may run after the main thread sending `msg_verack`.
Also, fix another bug.
Finally, add some `assert_debug_log` to ensure the right code branch is executed (and not some random, unrelated disconnect).
ACKs for top commit:
naumenkogs:
ACK fa3da8307b
Tree-SHA512: ee2988372223ea22e94e9783ef6d37114a3945991b6d60f0157917f3850fb7077c566564c3771d915ee74ab28202a3b73c6d89ddec6e2c918462db9a3c02e6cf
`m_headers_sync` is already reset in IsContinuationOfLowWorkHeadersSync
if there is a failure, so there is no need to also reset in
TryLowWorkHeaderSync.
aaaa7bd0ba iwyu: Add missing includes (MacroFake)
fa9ebec096 Remove g_parallel_script_checks (MacroFake)
fa7c834b9f Move ::fCheckBlockIndex into ChainstateManager (MacroFake)
fa43188d86 Move ::fCheckpointsEnabled into ChainstateManager (MacroFake)
cccca83099 Move ::nMinimumChainWork into ChainstateManager (MacroFake)
fa29d0b57c Move ::hashAssumeValid into ChainstateManager (MacroFake)
faf44876db Move ::nMaxTipAge into ChainstateManager (MacroFake)
Pull request description:
It seems preferable to assign globals to a class (in this case `ChainstateManager`), than to leave them dangling. This should clarify scope for code-readers, as well as clarifying unit test behaviour.
ACKs for top commit:
dergoegge:
Code review ACK aaaa7bd0ba
ryanofsky:
Code review ACK aaaa7bd0ba. No changes since last review, other than rebase
aureleoules:
reACK aaaa7bd0ba
Tree-SHA512: 83ec3ba0fb4f1dad95810d4bd4e578454e0718dc1bdd3a794cc4e48aa819b6f5dad4ac4edab3719bdfd5f89cbe23c2740a50fd56c1ff81c99e521c5f6d4e898d
9153ff3e27 rpc: add non-regression test about deriveaddresses crash when index is 2147483647 (muxator)
addf9d6502 rpc: fix crash in deriveaddresses when derivation index is 2147483647 (muxator)
Pull request description:
This PR is a proposal for fixing #26274 (better described there).
The problem is due to a signed int wrapping when the `index` parameter of the `deriveaddresses` RPC call has the value `2^31-1`.
```C++
for (int i = range_begin; i <= range_end; ++i) {
```
* the first commit adds a "temporary" test case (`test/functional/rpc_deriveaddresses_crash.py`) that shows the crash, and can be used to generate a core dump;
* the second commit fixes the problem giving an explicit size to the `i` variable in a for loop, from `int` to `int64_t`. The same commit also removes the ephemeral test case and adds a passing test to `test/functional/rpc_deriveaddresses.py`, in order to prevent future regressions.
This is my first submission to this project and I do not know its conventions. Please advise if something needs to be changed.
ACKs for top commit:
achow101:
ACK 9153ff3e27
Tree-SHA512: 0477b57b15dc2c682cf539d6002f100d44a8c7e668041aa3340c39dcdbd40e083c75dec6896b6c076b044a01c2e5254272ae6696d8a1467539391926f270940a
796b020c37 wallet: add taproot support to external signer (Sjors Provoost)
Pull request description:
Builds on #22558 (merged on 2022-06-28).
[HWI 2.1.0](https://github.com/bitcoin-core/HWI/releases/tag/2.1.0) or newer is required to import and use taproot descriptors. Older versions will work, but won't import a taproot descriptor.
Tested with HWI 2.1.1:
* Trezor T (firmware v2.5.1) on Signet: signs, change detection works
* Ledger Nano S (firmware 2.1.0, Bitcoin app 2.0.6): signs, change detection works
Only the most basic `tr(key)` descriptor is supported, script path spending is completely untested (if it works at all).
ACKs for top commit:
jb55:
utACK 796b020c37
achow101:
ACK 796b020c37
Tree-SHA512: 6dcb7eeb45421a3bbf2bdabeacd29979867db69077d7bf192bb77faa4bfefe446487b8df07bc40f9457009a88e598bdc09f769e6106fed2833ace7ef205a157a
This extra method will be needed for updating the filter set for
faster wallet rescans; after an internal top-up has happened, we only
want to add the newly created scriptPubKeys.
