#31668 Added rescan option for import descriptors
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #31668 Added rescan option for import descriptors | Ready | 0 | Strong | 36 + 156 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Adds support for "never" as an accepted value for the `timestamp` parameter in `importdescriptors`. Descriptors imported with `timestamp: "never"` do not trigger a blockchain rescan, allowing callers to bypass rescanning entirely if no other descriptor in the import request requires one.
Problem: Callers importing descriptors known never to have been used, or wishing to defer rescanning until later, had no way to completely bypass the rescan in `importdescriptors`. Specifying "now" still rescans the blockchain from the last two hours (median time past) through the mempool.
Evidence of importance:
- PR description links to issue #31263 which requests an option to skip rescan during descriptor import.
Risk of a mistake: medium (wallet, rpc_api). A flaw in the rescan trigger logic could incorrectly suppress rescans for other descriptors in a batch, leaving wallets oblivious to existing transactions.
Changed since the previous assessment: Expanded functional test coverage to include comprehensive cases for never, now, mixed batches, and subsequent manual rescanning.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: reviewers to re-review after author's rebase and test fixes 2026-10-16
- author: addressing requested commit split for the timestamp validation error 2026-05-25
- author: address commit splitting for timestamp validation check requested by furszy and rkrux 2026-05-25
- reviewer: review latest rebase and address author's review request 2026-10-16
Agreement
Strong
substantive support, no open objection (mprenditore, davidrobinsonau, rkrux)
Thread notes: Re-classified furszy's separate commit request (rc:2599095627) as a suggestion and marked it contested due to repeated change requests from rkrux. Added furszy's safety objection on std::nullopt access (fixed) and furszy's suggestion on using 'new' instead of 'never'. / Converted rc:2599095627 to a suggestion regarding commit scope and marked it contested per rkrux's reviews. Added claims for wallet unloading (rc:2599145016), alternative timestamp naming (r:3552684128), and nullopt access (rc:32
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| furszy (member) | objection, correctness | The rescan flag is overwritten on each descriptor iteration, making rescan behavior depend solely on the last descriptor in the batch. | fixed · fix pushed · author fixed flag handling across batch descriptors | changed | yes | 2025-01-17: 'This overrides the flag for each imported descriptor, meaning the final value depends solely on the last imported element.' Status from: 2025-01-21: saikiran57: 'Hi @furszy I have updated the commit can you please review it once.' |
| maflcko (member) | objection, interface | Adding a separate rescan boolean silently ignores the timestamp parameter, confusing users. | fixed · fix pushed · author switched to using a timestamp value instead of a separate flag | changed | yes | 2025-01-21: 'This seems confusing to users to silently ignore the timestamp. Why not simply have another timestamp value?' Status from: 2025-03-03: saikiran57: 'Updated the code' |
| maflcko (member) | objection, correctness | Negative timestamps previously initiated a full chain rescan; skipping rescan when timestamps are negative silently breaks prior behavior. | fixed · fix pushed · author updated rescan logic to handle negative timestamps | changed | yes | 2025-03-12: 'Seems a bit odd to skip rescan when all timestamps are negative. Previously, this would rescan the full chain.' Status from: 2025-04-11: saikiran57: 'Hi @achow101 I've improved the logic and taken care your point as well.' |
| achow101 (member) | objection, correctness | Failed imports trigger rescans when prior behavior skipped rescanning on failed imports. | fixed · fix pushed · author improved logic to only rescan on successful imports | changed | yes | 2025-04-10: 'The prior behavior would skip rescanning on failed imports. This seems to no longer be the case.' Status from: 2025-04-11: saikiran57: 'Hi @achow101 I've improved the logic and taken care your point as well.' |
| achow101 (member) | objection, correctness | Tests do not distinguish between 'now' and 'never', failing to verify if rescanning actually worked as intended. | fixed · fix pushed · author updated functional tests based on rkrux's test diff | changed | yes | 2025-06-11: 'The outcome of rescanning due to "timestamp": "now" should be different from the outcome of not rescanning due to "timestamp": "never". As it is now, both tests have the same outcome due to the setup,' Status from: 2025-12-05: saikiran57: 'its done thanks for updated test cases.' |
| furszy (member) | suggestion, interface | Use timestamp='new' instead of 'never' | open | answered | no | 2025-12-08: 'Instead of using `timestamp=never`, which is not really descriptive. What about using `new`?' |
| furszy (member) | suggestion, style | Adding a new timestamp validation error is unrelated to the 'never' timestamp change and should be in its own commit. | answered · rkrux requested changes noting furszy's comment on separate commit is still unaddressed | changed | yes | 2025-12-08: 'This change should be in a different commit.' Status from: 2026-05-25: rkrux: 'This is still not addressed, please address it.' |
| furszy (member) | objection, correctness | Wallets remain loaded after the test, affecting other tests in the file. | fixed · fix pushed · author unloaded wallets and updated test loop | changed | yes | 2025-12-08: 'Need to unload the wallets after using them; otherwise they stay loaded for other tests in this file as well.' Status from: 2025-12-09: saikiran57: 'Updated.' |
| achow101 (member) | objection, approach | Immediately converting std::optional to -1 reintroduces magic numbers rather than properly checking optional values. | fixed · fix pushed · author removed -1 magic number and used std::optional directly | changed | yes | 2025-12-23: 'The purpose of using an optional for `GetImportTimestamp` is to actually use the optional and check whether it has a value, not to immediately go back to essentially the original code with `-1` being a magic number.' Status from: 2026-01-04: saikiran57: 'hi @furszy i'm not using -1 anymore can you please check latest commits.' |
| rkrux (member) | objection, correctness | Fallback value of -1 leaks into user-facing error messages for failed rescans. | fixed · fix pushed · author applied suggested diff to skip error check for 'never' timestamps | changed | yes | 2025-12-29: 'This seems problematic because an internal default value is being used here for user facing messages.' Status from: 2025-12-30: saikiran57: 'done and thanks for the suggestion' |
| furszy (member) | objection, safety | Code may access std::nullopt without checking has_value() when 'never' timestamp is provided. | fixed · fix pushed · author refactored code to check has_value() | changed | yes | 2026-05-16: 'isn't this going to access a `std::nullopt`? should be `request_timestamp.has_value()` because `GetImportTimestamp` returns nullopt only when "never" is provided?' Status from: 2026-05-17: saikiran57: 'Hi @furszy I've refactored the code, could you please review that again.' |
| rkrux (member) | objection, correctness | The functional test for never_funded_wallet expects balance 0 despite claiming there is pre-existing history because the key was never funded. | fixed · fix pushed · author updated test funding and comments | changed | yes | 2026-05-25: 'If I use the following patch, the test passes but the expected balance should not be 0 because there's supposed to be a pre-existing history as per the code comment above.' Status from: 2026-05-27: saikiran57: '# Fund key_now, key_never_now, and key_funded_never on-chain.' |
Support:
- mprenditore (none) (2025-02-20) [read: src/wallet/rpc/backup.cpp]: Agreed with adding 'never' to the list of timestamp values to handle this case.
- davidrobinsonau (none) (Tested ACK, 2025-03-06) [tested]: Confirmed importdescriptors with timestamp:now causes rescan while timestamp:never returns immediately without rescanning.
- rkrux (member) (Concept ACK, 2025-11-25) [read]: Agreed with the intent to avoid rescanning if the user specifies so.
Participants:
- furszy (member, objection, 11 statements 2025-01-17 to 2026-05-16): objected to flag overwrite in descriptor iteration and nullopt access; suggested std::optional and splitting commits
- maflcko (member, objection, 8 statements 2025-01-21 to 2025-06-02): objected to an extra rescan parameter silently ignoring timestamp, breaking negative timestamp rescanning, and using -1 magic numbers
- fanquake (member, neutral, 1 statements 2025-02-20 to 2025-02-20): moved PR to draft and noted commits need squashing and feedback addressed
- mprenditore (none, support, 7 statements 2025-02-20 to 2026-09-04): supported adding 'never' timestamp and repeatedly prompted for merge
- davidrobinsonau (none, support, 5 statements 2025-03-02 to 2025-03-14): tested 'never' vs 'now' timestamp rescanning and supported using -1 or never
- w0xlt (contributor, neutral, 2 statements 2025-04-02 to 2025-04-02): suggested adding test cases for 'never' and minor code suggestion
- achow101 (member, objection, 19 statements 2025-04-10 to 2025-12-24): pointed out rescan regressions on failed imports, weak test distinction between 'now' and 'never', and misuse of std::optional
- rkrux (member, objection, 27 statements 2025-11-25 to 2026-05-25): Concept ACK on avoiding rescan, but requested changes on error string values, test logic correctness, and commit structure
- darosior (member, neutral, 1 statements 2026-05-15 to 2026-05-15): asked for status regarding draft state and CI failure
- sedited (member, neutral, 1 statements 2026-08-10 to 2026-08-10): asked author to rebase due to merge conflict
Files
160 lines under test/bench/ci.
- test/functional/wallet_importdescriptors.py +156/-4
- src/wallet/rpc/backup.cpp +21/-14
- src/wallet/imports.cpp +15/-2
Card
The short text the ranking pass compares across PRs.
Adds a "never" option to the timestamp parameter in importdescriptors, allowing callers to completely skip blockchain rescanning during descriptor import. Currently, specifying "now" still forces a scan of the last two hours of blocks plus the mempool, adding latency when importing newly generated descriptors or when the user intends to rescan manually later. Links to issue #31263.
Data
thread read through 2026-10-16 (129 statements), head 4bd573969b
labels: none
Categories
Wallet
P3 · user request
Adds the option `timestamp: "never"` to `importdescriptors` to completely bypass rescan overhead for fresh descriptors, addressing user request #31263.
Membership: Changes `src/wallet/rpc/backup.cpp` and `src/wallet/imports.cpp` to modify `importdescriptors` behavior.
Factors: security/stability 0, bug 0, performance 1, user value 2, leverage 0