Merge bitcoin/bitcoin#25957: wallet: fast rescan with BIP157 block filters for descriptor wallets #1226
Conversation
…r descriptor wallets
WalkthroughThis pull request introduces fast wallet rescanning using block filters. It adds block filter query capabilities to the Chain interface, creates a FastWalletRescanFilter class to optimize wallet transaction rescanning, refactors script pubkey manager return types to use unordered sets, and updates RPC help text and logging infrastructure. A new functional test validates the feature. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
src/interfaces/chain.h(2 hunks)src/logging.cpp(2 hunks)src/logging.h(1 hunks)src/node/interfaces.cpp(3 hunks)src/wallet/rpc/backup.cpp(2 hunks)src/wallet/rpc/transactions.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(1 hunks)src/wallet/scriptpubkeyman.h(2 hunks)src/wallet/wallet.cpp(6 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_fast_rescan.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
C++20 codebase should be placed under src/
Files:
src/logging.hsrc/interfaces/chain.hsrc/wallet/rpc/backup.cppsrc/wallet/rpc/transactions.cppsrc/logging.cppsrc/wallet/scriptpubkeyman.cppsrc/node/interfaces.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/wallet.cpp
**
⚙️ CodeRabbit configuration file
**: # CodeRabbit AI Review Instructions for Dash BackportsYour Role
You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.
Critical Validation Rules
1. File Operations Must Match (AUTO-REJECT if violated)
- If Bitcoin modifies an existing file → Dash MUST modify (not create new)
- If Bitcoin creates a new file → Dash creates
- If Bitcoin deletes a file → Dash deletes
- Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys
2. Size Ratio Check (80-150% of Bitcoin)
- Count functional lines changed (exclude comments/whitespace)
- Dash changes should be 80-150% of Bitcoin's size
- Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash
3. No Scope Creep
- Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
- No unrelated refactoring or style changes
- Only Bitcoin's intended changes + minimal Dash adaptations
4. Bitcoin-Specific Code Detection
- Auto-reject witness/segwit code:
msg_wtxidrelay,MSG_WTX, witness imports- Auto-reject RBF (replace-by-fee) functionality
- Note: PSBT is supported in Dash (don't flag)
5. Mandatory Adaptations Only
bitcoin→dashin strings/pathsBitcoin→Dashin user-facing text- Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
- Hardcoded test values specific to Dash
- No other changes unless absolutely required
6. Completeness Check
- All files changed in Bitcoin must be present
- Extra files need clear justification (Dash-specific compatibility)
- Missing files = incomplete backport
Review Process
- First: Check file operations match exactly
- Second: Calculate size ratio
- Third: Scan for scope creep patterns
- Fourth: Detect Bitcoin-specific code
- Fifth: Verify all changes are minimal adaptations
Output Format
VALIDATION: [PASS/FAIL] File Operatio...
Files:
src/logging.hsrc/interfaces/chain.hsrc/wallet/rpc/backup.cppsrc/wallet/rpc/transactions.cpptest/functional/wallet_fast_rescan.pysrc/logging.cpptest/functional/test_runner.pysrc/wallet/scriptpubkeyman.cppsrc/node/interfaces.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/wallet.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be placed in test/functional/ and written in Python
Files:
test/functional/wallet_fast_rescan.pytest/functional/test_runner.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T19:54:21.426Z
Learning: In Dash backports from Bitcoin Core, including necessary compilation fixes (such as API compatibility changes like UniValue get_int() → getInt<int>()) alongside the core backport is standard and expected practice. These compatibility fixes ensure the backported code compiles in Dash's evolved codebase while preserving Bitcoin's original functionality and intent.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: The `add_wallet_options` method is not available in Dash's test framework and should be removed when backporting Bitcoin Core tests that use this method, as demonstrated in wallet_rescan_unconfirmed.py.
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: When backporting Bitcoin Core commits that use Python's textwrap.dedent() function in test files, the textwrap import statement needs to be explicitly added if it's missing in the Dash test file.
📚 Learning: 2025-07-30T14:45:15.700Z
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: The `add_wallet_options` method is not available in Dash's test framework and should be removed when backporting Bitcoin Core tests that use this method, as demonstrated in wallet_rescan_unconfirmed.py.
Applied to files:
test/functional/wallet_fast_rescan.pytest/functional/test_runner.py
📚 Learning: 2025-07-30T14:45:15.700Z
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.
Applied to files:
test/functional/wallet_fast_rescan.pytest/functional/test_runner.py
📚 Learning: 2025-07-28T20:34:29.061Z
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Applied to files:
test/functional/wallet_fast_rescan.py
📚 Learning: 2025-07-29T22:13:54.797Z
Learnt from: DashCoreAutoGuix
Repo: DashCoreAutoGuix/dash PR: 0
File: :0-0
Timestamp: 2025-07-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.
Applied to files:
test/functional/wallet_fast_rescan.py
🧬 Code graph analysis (4)
src/wallet/rpc/transactions.cpp (1)
src/rpc/util.cpp (4)
HelpExampleCli(201-204)HelpExampleCli(201-201)HelpExampleRpc(219-224)HelpExampleRpc(219-219)
test/functional/wallet_fast_rescan.py (1)
src/wallet/rpc/backup.cpp (8)
descriptors(2030-2030)listdescriptors(1981-2086)listdescriptors(1981-1981)backupwallet(2088-2119)backupwallet(2088-2088)spk(2037-2037)restorewallet(2121-2173)restorewallet(2121-2121)
src/node/interfaces.cpp (2)
src/wallet/wallet.cpp (2)
block_hash(319-322)block_hash(319-319)src/index/blockfilterindex.cpp (1)
filter(274-274)
src/wallet/scriptpubkeyman.h (1)
src/wallet/scriptpubkeyman.cpp (8)
GetScriptPubKeys(2490-2493)GetScriptPubKeys(2490-2490)GetScriptPubKeys(2495-2505)GetScriptPubKeys(2495-2495)GetWalletDescriptor(2485-2488)GetWalletDescriptor(2485-2485)GetEndRange(2507-2510)GetEndRange(2507-2507)
🪛 Flake8 (7.3.0)
test/functional/wallet_fast_rescan.py
[error] 52-52: f-string is missing placeholders
(F541)
🪛 Ruff (0.14.7)
test/functional/wallet_fast_rescan.py
8-8: typing.List is deprecated, use list instead
(UP035)
52-52: f-string without any placeholders
Remove extraneous f prefix
(F541)
52-52: Logging statement uses f-string
(G004)
54-54: Logging statement uses f-string
(G004)
60-60: Logging statement uses f-string
(G004)
63-63: Logging statement uses f-string
(G004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (17)
src/wallet/rpc/transactions.cpp (1)
849-867: LGTM - Help text correctly documents fast rescan feature.The expanded help text appropriately documents that rescans are faster when using descriptor wallets with block filters enabled via
-blockfilterindex=1. This aligns with the Bitcoin PR bitcoin#25957 backport for BIP157 block filter support.src/logging.h (1)
70-70: LGTM - SCAN log category flag correctly added.The new
SCANflag at bit position 28 is correctly positioned afterTXRECONCILIATION(bit 27) and before the Dash-specific flags that start at bit 32. This provides a dedicated logging category for rescan-related messages as part of the BIP157 fast rescan feature.src/logging.cpp (2)
182-182: LGTM - SCAN category correctly added to LogCategories array.The
{BCLog::SCAN, "scan"}entry enables the logging system to parse and recognize the "scan" category from command-line arguments and configuration.
300-301: LGTM - LogCategoryToStr correctly handles SCAN flag.The switch case correctly maps
BCLog::LogFlags::SCANto the string"scan", maintaining consistency with the LogCategories array entry.src/interfaces/chain.h (2)
8-8: LGTM - Required include for block filter types.The
<blockfilter.h>include is necessary for theBlockFilterTypeenum andGCSFilter::ElementSettype used in the new interface methods.
151-157: LGTM - Block filter interface methods correctly defined and implemented.The two new pure virtual methods provide the necessary interface for BIP157 fast rescanning:
hasBlockFilterIndex- checks if the block filter index is available (lines 994-997 in src/node/interfaces.cpp)blockFilterMatchesAny- performs filter matching, handling edge cases where the index or filter might not be found (lines 998-1007 in src/node/interfaces.cpp)The ChainImpl implementations properly match the interface signatures, correctly handling nullopt cases and delegating to underlying block filter index operations.
test/functional/test_runner.py (1)
168-168: Test entry correctly added for descriptor wallets only.The test entry for
wallet_fast_rescan.py --descriptorsis properly configured and placed. The--descriptorsflag is correct since the fast rescan feature with BIP157 block filters is specific to descriptor wallets. The placement in BASE_SCRIPTS ensures it runs as part of the default test suite.src/wallet/rpc/backup.cpp (2)
1823-1830: importdescriptors help: blockfilterindex note correctly backportedThe added sentences about rescans being faster when
-blockfilterindex=1is enabled match the upstream Bitcoin CoreimportdescriptorsRPC documentation and simply extend help text without changing behavior. This is an appropriate, faithful backport.
2123-2130: restorewallet help: descriptor + block filter guidance matches upstreamThe new restorewallet help text about faster rescans for descriptor wallets with
-blockfilterindex=1matches Bitcoin Core’s RPC docs and accurately reflects the new fast-rescan path. No Dash-specific quirks introduced.src/node/interfaces.cpp (1)
9-23: ChainImpl block filter integration matches Bitcoin Core designThe added includes and the
hasBlockFilterIndex/blockFilterMatchesAnyimplementations mirror the Bitcoin Core Chain interface for BIP157 filters: they query the globalBlockFilterIndexregistry, look up the block undercs_main, and returnstd::optional<bool>indicating match/unknown. This is the minimal, correct wiring needed for the wallet’sFastWalletRescanFilterand stays within the original PR’s scope.Also applies to: 994-1007
test/functional/wallet_fast_rescan.py (1)
23-101: wallet_fast_rescan test: logic and Dash-specific adaptations look correctThis test closely follows the upstream fast-rescan scenario: descriptor wallet with a backup, funding end-of-range and fixed descriptors over several blocks, then restoring/importing with and without
-blockfilterindexand asserting identical txids plus the expected “fast variant using block filters” vs “slow variant inspecting all blocks” log messages. The Dash-specific tweaks (pkh descriptors only,NUM_DESCRIPTORS = 5, no segwit assumptions) are minimal and clearly commented, so behavior remains aligned with Bitcoin’s PR while fitting Dash’s feature set.Based on learnings, witness- and segwit-only helpers are correctly avoided here.
src/wallet/scriptpubkeyman.cpp (1)
2485-2510: DescriptorScriptPubKeyMan scriptPubKey accessors match fast-rescan usageThe new methods:
GetWalletDescriptor()returningm_wallet_descriptorby value,GetScriptPubKeys()andGetScriptPubKeys(int32_t minimum_index)building anunordered_set<CScript, SaltedSipHasher>undercs_desc_man, andGetEndRange()exposingm_max_cached_index + 1,provide exactly the API needed by
FastWalletRescanFilterto (a) snapshot scriptPubKeys per descriptor at a given starting index and (b) track descriptor range growth over time. This mirrors the upstream fast-rescan implementation and doesn’t introduce any Dash-specific behavior.src/wallet/scriptpubkeyman.h (1)
230-231: ScriptPubKeyMan interface extension and descriptor overrides are consistent with upstreamAdding
GetScriptPubKeys()to the ScriptPubKeyMan base class (defaulting to an empty set) and overriding it inDescriptorScriptPubKeyMan, along with the newGetWalletDescriptor()andGetEndRange()accessors, cleanly exposes the minimal state needed for the fast-rescan filter. LegacyScriptPubKeyMan behavior stays unchanged (returns{}), and the locking annotation onGetWalletDescriptormatches existing call sites that already lockcs_desc_man. This matches the intended API surface from Bitcoin PR bitcoin#25957 without extra scope.Also applies to: 628-631
src/wallet/wallet.cpp (4)
9-9: LGTM: Include added for block filter support.The
blockfilter.hinclude is necessary for theBlockFilterType::BASIC_FILTERenum and block filter functionality used in the fast rescan feature.
285-341: All required DescriptorScriptPubKeyMan methods are present and correctly typed.The
FastWalletRescanFilterclass implementation is valid. Both required methods exist insrc/wallet/scriptpubkeyman:
GetScriptPubKeys(int32_t minimum_index = 0)returnsstd::unordered_set<CScript, SaltedSipHasher>(line 2495)GetEndRange()returnsint32_t(line 2507)The method signatures and return types match the usage in the class at lines 300, 311, and 337.
1828-1874: All required infrastructure exists for the fast rescan integration.The chain interface methods
hasBlockFilterIndex()andblockFilterMatchesAny()are properly declared insrc/interfaces/chain.h(lines 153, 157) and implemented insrc/node/interfaces.cpp(lines 994, 998). TheBCLog::SCANlogging category is defined insrc/logging.h(line 70) within the LogFlags enum.The fast rescan logic correctly integrates these dependencies:
- Line 1829 checks for block filter index availability
- Lines 1865 and 1872 use BCLog::SCAN for filtered block inspection logging
- The conditional fetch_block logic (lines 1859-1874) properly handles cases where filter results are unavailable
4104-4114: Line 4111 correctly adapts to GetScriptPubKeys() returning unordered_set.The change from indexed access to iterator dereferencing (
*script_pub_keys.begin()) is the proper adaptation forGetScriptPubKeys()now returningconst std::unordered_set<CScript, SaltedSipHasher>instead of a vector. Unordered sets do not support operator[], so dereferencing the iterator is the correct approach.
| print(w.importdescriptors([{"desc": descsum_create(f"pkh({fixed_key.privkey})"), "timestamp": "now"}])) | ||
| descriptors = w.listdescriptors()['descriptors'] | ||
| assert_equal(len(descriptors), NUM_DESCRIPTORS) | ||
| w.backupwallet(WALLET_BACKUP_FILENAME) | ||
|
|
||
| self.log.info(f"Create txs sending to end range address of each descriptor, triggering top-ups") | ||
| for i in range(NUM_BLOCKS): | ||
| self.log.info(f"Block {i+1}/{NUM_BLOCKS}") | ||
| for desc_info in w.listdescriptors()['descriptors']: |
There was a problem hiding this comment.
Clean up debug print and trivial f-string lint issue
Two small issues that don’t affect behavior but will trip linters:
- Line 47:
print(w.importdescriptors(...))appears to be leftover debug output; upstream tests typically just call the RPC or assert its result. Dropping theprintkeeps the test quiet and closer to the Bitcoin version. - Line 52:
self.log.info(f"Create txs sending to end range address of each descriptor, triggering top-ups")uses an f-string with no placeholders. Removing thefprefix fixes Flake8 F541/Ruff F541 without changing output.
A minimal diff that stays within the backport’s scope:
- print(w.importdescriptors([{"desc": descsum_create(f"pkh({fixed_key.privkey})"), "timestamp": "now"}]))
+ w.importdescriptors([{"desc": descsum_create(f"pkh({fixed_key.privkey})"), "timestamp": "now"}])
@@
- self.log.info(f"Create txs sending to end range address of each descriptor, triggering top-ups")
+ self.log.info("Create txs sending to end range address of each descriptor, triggering top-ups")🧰 Tools
🪛 Flake8 (7.3.0)
[error] 52-52: f-string is missing placeholders
(F541)
🪛 Ruff (0.14.7)
52-52: f-string without any placeholders
Remove extraneous f prefix
(F541)
52-52: Logging statement uses f-string
(G004)
54-54: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
In test/functional/wallet_fast_rescan.py around lines 47 to 55, remove the
leftover debug print and the unnecessary f-string: replace the debug print call
print(w.importdescriptors([...])) with a plain RPC call or an assertion (e.g.,
call w.importdescriptors([...]) or
assert_equal(w.importdescriptors([...])['success'], True)) so the test stays
quiet, and change self.log.info(f"Create txs sending to end range address of
each descriptor, triggering top-ups") to self.log.info("Create txs sending to
end range address of each descriptor, triggering top-ups") by removing the
redundant f prefix.
Summary
Backport of Bitcoin PR bitcoin#25957 - Fast wallet rescanning using BIP157 block filters for descriptor wallets.
Changes:
hasBlockFilterIndex,blockFilterMatchesAny)FastWalletRescanFilterclass for efficient block filtering during rescanScriptPubKeyMan::GetScriptPubKeys()to returnunordered_setfor block filter matchingGetEndRange()method for tracking descriptor ranges during rescanwallet_fast_rescan.pyDash-specific adaptations:
BlockFilterType::BASICtoBlockFilterType::BASIC_FILTER(avoids macro conflict with relic BLS library)pkhdescriptors instead ofwpkh(Dash doesn't have SegWit)batchparameter toSyncTransactioncalls (Dash-specific signature)NUM_DESCRIPTORSin test from 9 to 5 (no SegWit descriptors)Test plan
wallet_fast_rescan.pypasses (requires test environment setup)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.