backport: bitcoin#24836, #25379, #25773, #25957, #25986, #26158, #26294, #26349, #26388, #26404, #26424, #26625, #27350 #7085
+1,127
−905
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
…r descriptor wallets 0582932 test: add test for fast rescan using block filters (top-up detection) (Sebastian Falbesoner) ca48a46 rpc: doc: mention rescan speedup using `blockfilterindex=1` in affected wallet RPCs (Sebastian Falbesoner) 3449880 wallet: fast rescan: show log message for every non-skipped block (Sebastian Falbesoner) 935c6c4 wallet: take use of `FastWalletRescanFilter` (Sebastian Falbesoner) 70b3513 wallet: add `FastWalletRescanFilter` class for speeding up rescans (Sebastian Falbesoner) c051026 wallet: add method for retrieving the end range for a ScriptPubKeyMan (Sebastian Falbesoner) 8452791 wallet: support fetching scriptPubKeys with minimum descriptor range index (Sebastian Falbesoner) 088e38d 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 bitcoin#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 bitcoin#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`): https://github.com/bitcoin/bitcoin/blob/e191fac4f3c37820f0618f72f0a8e8b524531ab8/src/wallet/scriptpubkeyman.cpp#L1703-L1710 One of the unaddressed issues of bitcoin#15845 was that [the filter was only created once outside the loop](bitcoin#15845 (comment)) 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 0582932 Sjors: re-utACK 0582932 aureleoules: ACK 0582932 - minor changes, documentation and updated test since last review w0xlt: re-ACK bitcoin@0582932 Tree-SHA512: 3289ba6e4572726e915d19f3e8b251d12a4cec8c96d041589956c484b5575e3708b14f6e1e121b05fe98aff1c8724de4564a5a9123f876967d33343cbef242e1
…mework BACKPORT NOTE: missing changes are in: - src/bench/addrman.cpp - src/bench/verify_script.cpp - src/bench/chacha_poly_aead.cpp - src/bench/descriptors.cpp - src/bench/chacha20.cpp - src/bench/crypto_hash.cpp But backport is not partial because adding these files without these changes will cause compilation error 3e9d0be build: only run high priority benchmarks in 'make check' (furszy) 466b54b bench: surround main() execution with try/catch (furszy) 3da7cd2 bench: explicitly make all current benchmarks "high" priority (furszy) 05b8c76 bench: add "priority level" to the benchmark framework (furszy) f159378 bench: place benchmark implementation inside benchmark namespace (furszy) Pull request description: This is from today's meeting, a simple "priority level" for the benchmark framework. Will allow us to run certain benchmarks while skip non-prioritized ones in `make check`. By default, `bench_bitcoin` will run all the benchmarks. `make check`will only run the high priority ones, and have marked all the existent benchmarks as "high priority" to retain the current behavior. Could test it by modifying any benchmark priority to something different from "high", and run `bench_bitcoin -priority-level=high` and/or `bench_bitcoin -priority-level=medium,low` (the first command will skip the modified bench while the second one will include it). Note: the second commit could be avoided by having a default arg value for the priority level but.. an explicit set in every `BENCHMARK` macro call makes it less error-prone. ACKs for top commit: kouloumos: re-ACK 3e9d0be achow101: ACK 3e9d0be theStack: re-ACK 3e9d0be stickies-v: re-ACK bitcoin@3e9d0be Tree-SHA512: ece59bf424c5fc1db335f84caa507476fb8ad8c6151880f1f8289562e17023aae5b5e7de03e8cbba6337bf09215f9be331e9ef51c791c43bce43f7446813b054 fixup bench
knst
changed the title
…cOS native" task da16893 ci: Use `macos-ventura-xcode:14.1` image for "macOS native" task (Hennadii Stepanov) 7028365 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](bitcoin#25160 (comment)) 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"](bitcoin#26386 (comment)) on `arm64` in CI. ACKs for top commit: Sjors: utACK da16893 Tree-SHA512: 36785d33b7f11b3cdbc53bcfbf97d88bf821fad248c825982dd9f8e3413809a4ef11190eaf950e60fdf479b62ff66920c35d9ea42d534723f015742eec7e19b6
…tions, sinceblock}` response eb679a7 rpc: make `address` field optional (w0xlt) Pull request description: Close bitcoin#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 eb679a7 aureleoules: ACK eb679a7 Tree-SHA512: b267439626e2ec3134ae790c849949a4c40ef0cebd20092e8187be3db0a61941b2da10bbbba92ca880b8369f46c1aaa806d057eaa5159325f65cbec7cb33c52f
…ompeer.py 8a9f1e4 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 bitcoin#26412 ACKs for top commit: jonatack: ACK 8a9f1e4 Tree-SHA512: f6188ab3cfd863034e44e9806d0d99a8781462bec94141501aefc71589153481ffb144e126326ab81807c2b2c93de7f4aac5d09dbcf26c4512a176e06fc6e5f8
0f38524 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 0f38524 Zero-1729: ACK 0f38524 Tree-SHA512: 3f405f5479a0d39cf150fd80b4d854ffe4eef718a358202c619e34a08d98c1252b82fc70d106cdf2215dc5a50c6f6cd5e26fe7ed87156f6b08f8e97d963affb7
3a0b352 refactor: move url.h/cpp from lib util to lib common (fanquake) 058eb69 build: add missing event cflags to libbitcoin_util (fanquake) Pull request description: Move `util/url` to `common/url`. Also add missing `event_*` flags to `libbitcoin_util`. bitcoin#26293 + the commit dropping boost cppflags from `libbitcoin_util` shows this issue. i.e: ```bash CXX util/libbitcoin_util_a-url.o util/url.cpp:7:10: fatal error: 'event2/http.h' file not found #include <event2/http.h> ^~~~~~~~~~~~~~~ 1 error generated. ``` ACKs for top commit: hebasto: ACK 3a0b352 ryanofsky: Code review ACK 3a0b352 Tree-SHA512: 600a76fd334267a02d332df9b67891a38d3fd7f5baf8a82b2447879b3bc65eab2552d2c081c0a5f1ec927bf80df7fc1f0cbbdda4cb76994b46dadf260b8e1cb3
e866f0d [functional test] submitrawpackage RPC (glozow) fa07651 [rpc] add new submitpackage RPC (glozow) Pull request description: It would be nice for LN/wallet/app devs to test out package policy, package RBF, etc., but the only interface to do so right now is through unit tests. This PR adds a `-regtest` only RPC interface so people can test by submitting raw transaction data. It is regtest-only, as it would be unsafe/confusing to create an actual mainnet interface while package relay doesn't exist. Note that the functional tests are there to ensure the RPC interface is working properly; they aren't for testing policy itself. See src/test/txpackage_tests.cpp. ACKs for top commit: t-bast: Tested ACK against eclair bitcoin@e866f0d ariard: Code Review ACK e866f0d instagibbs: code review ACK e866f0d Tree-SHA512: 824a26b10d2240e0fd85e5dd25bf499ee3dd9ba8ef4f522533998fcf767ddded9f001f7a005fe3ab07ec95e696448484e26599803e6034ed2733125c8c376c84
…_limits.py tests f2f6068 test: MiniWallet: add `send_self_transfer_chain` to create chain of txns (Andreas Kouloumos) 1d6b438 test: use MiniWallet to simplify mempool_package_limits.py tests (Andreas Kouloumos) Pull request description: While `wallet.py` includes the MiniWallet class and some helper methods, it also includes some methods that have been moved there without having any direct relation with the MiniWallet class. Specifically `make_chain`, `create_child_with_parents` and `create_raw_chain` methods that were extracted from `rpc_packages.py` at f8253d6 in order to be used on both `mempool_package_limits.py` and `rpc_packages.py`. Since that change, due to the introduction of additional methods in MiniWallet, the functionality of those methods can now be replicated with the existing MiniWallet methods and simultaneously simplify those tests by using the MiniWallet. This PR's goals are - to simplify the `mempool_package_limits.py` functional tests with usage of the MiniWallet. - to make progress towards the removal of the `make_chain`, `create_child_with_parents` and `create_raw_chain` methods of `wallet.py`. For the purpose of the aforementioned goals, a helper method `MiniWallet.send_self_transfer_chain` is introduced and method `bulk_transaction` has been integrated in `create_self_transfer*` methods using an optional `target_weight` option. ACKs for top commit: MarcoFalke: ACK f2f6068 👜 Tree-SHA512: 3ddfa0046168cbf7904ec6b1ca233b3fdd4f30db6aefae108b6d7fb69f34ef6fb2cf4fa7cef9473ce1434a0cc8149d236441a685352fef35359a2b7ba0d951eb
Otherwise this error appears:
test_framework.authproxy.JSONRPCException: min relay fee not met, 25500 < 30332 (-26)
fa2537c test: Target exact weight in MiniWallet _bulk_tx (MacroFake) Pull request description: Seems better to target the exact weight than a weight that is up to more than 2000 WU larger. Also, replace a broad `-acceptnonstdtxn=1` with `-datacarriersize=100000` to document the test assumptions better. ACKs for top commit: theStack: Code-review ACK fa2537c Tree-SHA512: cf02c3082a13195b8aa730866aeaf2575ce01974ae2b0244739d8cfc12e60c66312729ed703bb3214651744166a3b560bfaa8dc302ef46ed79fc4d1fe7fcc214
…let` 17cad44 test: refactor `RPCPackagesTest` to use `MiniWallet` (w0xlt) Pull request description: This PR refactors `RPCPackagesTest` to use `MiniWallet` and removes `create_child_with_parents`, `make_chain`, and `create_raw_chain` from `test_framework/wallet`, as requested in bitcoin#25965. Close bitcoin#25965. ACKs for top commit: glozow: ACK 17cad44 pablomartin4btc: tested ACK 17cad44; went thru all changes and recommendations from @kouloumos & @glozow; also went up to bitcoin#20833 to get a bit of background of the origin and purpose of these tests. kouloumos: ACK 17cad44 Tree-SHA512: 9228c532afaecedd577019dbc56f8749046d66f904dd69eb23e7ca3d7806e2132d90af29be276c7635fefb37ef348ae781eb3b225cd6741b20300e6f381041c3
fa6b402 test: Run mempool_packages.py with MiniWallet (MarcoFalke) fa448c2 test: Return fee from MiniWallet (MarcoFalke) faec09f test: Return chain of MiniWallet txs from MiniWallet chain method (MarcoFalke) faa12d4 test: Refactor MiniWallet sign_tx (MarcoFalke) fa2d821 test: Return wtxid from create_self_transfer_multi (MarcoFalke) Pull request description: This allows to run the test even when no wallet is compiled in. Also, it is a lot nicer to read now. ACKs for top commit: glozow: reACK fa6b402 Tree-SHA512: de0338068fd51db01d64ce270f94fd2982a63a6de597325cd1e1f11127e9075bd4aeacace0ed76d09a2db8b962b27237cf11edb4c1fe1a01134d397f8a11bd05
…subtests via decorator e669833 test: dedup package limit checks via decorator in mempool_package_limits.py (Sebastian Falbesoner) 72f25e2 test: refactor: use Satoshis for fees in mempool_package_limits.py (Sebastian Falbesoner) Pull request description: The subtests in the functional test mempool_package_limits.py all follow the same pattern: 1. first, check that the mempool is currently empty 2. create and submit certain single txs to the mempool, prepare list of hex transactions 3. check that `testmempoolaccept` on the package hex fails with a "package-mempool-limits" error on each tx result 4. after mining a block, check that submitting the package succeeds Note that steps 1,3,4 are identical for each of the subtests and only step 2 varies, so this might be a nice opportunity to deduplicate code by using a newly introduced decorator which executes the necessary before and after the essential part of the subtest. This also makes it easier to add new subtests without having to copy-paste those parts once again. In addition, the first commit switches the fee unit from BTC to Satoshis, which allows to get rid of some imports (`COIN` and `Decimal`) and a comment for the `test_desc_size_limits` subtest is fixed (s/25KvB/21KvB/). ACKs for top commit: ismaelsadeeq: ACK e669833 glozow: utACK e669833 Tree-SHA512: 84a85e739de7387391c13bd46aeb015a74302ea7c6f0ca3d4e2b1b487d38df390dc118eb5b1c11d3e4206bff316a4dab60ef6b25d8feced672345d4e36ffd205
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was done?
Regular backports from bitcoin core v25
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: