Added rescan option for import descriptors by saikiran57 · Pull Request #31668 · bitcoin/bitcoin

saikiran57 · 2025-01-16T08:11:27Z

Fix related to issue: #31263

DrahtBot · 2025-01-16T08:11:30Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31668.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	rkrux

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33392 (wallet, rpc: add UTXO set check and incremental rescan to importdescriptors by musaHaruna)
#32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
#32784 (wallet: derivehdkey RPC to get xpub at arbitrary path by Sjors)
#29136 (wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

dont' -> don't [misspelling of "don't" in comment; current form is invalid and may confuse readers]

^{2026-02-03 14:48:15}

DrahtBot · 2025-01-16T09:00:10Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/35699494417}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

maflcko · 2025-01-16T13:28:38Z

This needs a test and the tests fixed

src/wallet/rpc/backup.cpp

fanquake · 2025-02-20T20:47:11Z

Moved to draft for now. You'll need to squash your commits, and address the various review feedback.

DrahtBot · 2025-03-03T11:15:14Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/38094021491}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

mprenditore · 2025-03-04T16:40:54Z

Moved to draft for now. You'll need to squash your commits, and address the various review feedback.

@fanquake code has been updated as per the discussions and all checks are now passing.
Hope now it's good to be merged.

src/wallet/rpc/backup.cpp

davidrobinsonau · 2025-03-06T10:41:37Z

Cloned fresh bitcoin src, then "gh pr checkout 31668" and built on Ubuntu WSL2.
Confirmed importdescriptors with "timestamp":"now" causes "RescanFromTime: Rescanning last 16 blocks".
Confirmed importdescriptors with "timestamp":"never" returns in less then a second and no messages about rescanning blocks on bitcoind console.
Thank you!

mprenditore · 2025-03-12T14:22:13Z

Hello @maflcko @furszy @fanquake
Can this be merged now?

src/wallet/rpc/backup.cpp

w0xlt

It would be good to add test cases for the "never" option.

src/wallet/rpc/backup.cpp

test/functional/wallet_importdescriptors.py

rkrux

Code review 5fbef21

Suggested few changes.

test/functional/wallet_importdescriptors.py

src/wallet/rpc/backup.cpp

test/functional/wallet_importdescriptors.py

test/functional/wallet_transactiontime_rescan.py

src/wallet/rpc/backup.cpp

rkrux · 2025-12-30T10:39:18Z

Please squash the third commit as it's a lint fix, ref:

bitcoin/CONTRIBUTING.md

Lines 202 to 230 in 2bcb3f6

    
           ### Squashing Commits 
        
           If your pull request contains fixup commits (commits that change the same line of code repeatedly) or too fine-grained 
        
           commits, you may be asked to [squash](https://git-scm.com/docs/git-rebase#_interactive_mode) your commits 
        
           before it will be reviewed. The basic squashing workflow is shown below. 
        
               git checkout your_branch_name 
        
               git rebase -i HEAD~n 
        
               # n is normally the number of commits in the pull request. 
        
               # Set commits (except the one in the first line) from 'pick' to 'squash', save and quit. 
        
               # On the next screen, edit/refine commit messages. 
        
               # Save and quit. 
        
               git push -f # (force push to GitHub) 
        
           Please update the resulting commit message, if needed. It should read as a 
        
           coherent message. In most cases, this means not just listing the interim 
        
           commits. 
        
           If your change contains a merge commit, the above workflow may not work and you 
        
           will need to remove the merge commit first. See the next section for details on 
        
           how to rebase. 
        
           Please refrain from creating several pull requests for the same change. 
        
           Use the pull request that is already open (or was created earlier) to amend 
        
           changes. This preserves the discussion and review that happened earlier for 
        
           the respective change set. 
        
           The length of time required for peer review is unpredictable and will vary from 
        
           pull request to pull request.

Also, please resolve the comments in the PR that have been addressed already to make it easier for others to go through the PR.

saikiran57 · 2026-01-12T09:10:24Z

hi @achow101 I've fixed all the review comments. Can you please approve this pr and merge it if everything is okay.

rkrux

Commits need to be cleaned up, please check the inline comments.

test/functional/wallet_importdescriptors.py

test/functional/wallet_transactiontime_rescan.py

src/wallet/rpc/backup.cpp

rkrux · 2026-01-22T13:36:17Z

Also, please un-resolve the conversations that are still ongoing for other reviewers to see.

saikiran57 · 2026-01-22T14:04:33Z

HI @rkrux the reason I've made a separate commit is https://github.com/bitcoin/bitcoin/pull/31668/changes#r2599095627 again I've made single commit for a clean review. Mostly I've fixed all the review comments and that's why I've resolved those. I hope the reviewers can easily understand the fix and if they still feel some improvement in their I'm happy to fix those issue. Thanks again for active reviewing.

rkrux

the reason I've made a separate commit is https://github.com/bitcoin/bitcoin/pull/31668/changes#r2599095627 again I've made single commit for a clean review

My suggestions have been misunderstood.

To reiterate, the issue in the previous approach in this PR was that code changes were added in the first commit only for them to be removed in the second commit, which is confusing for the reviewers and misleading in the git logs. In #31668 (comment) and #31668 (comment), I suggested to clean up the commits and not merge them.

This comment from furszy that's mentioned in the quoted text looks fine to me and should be implemented. Once this is done, I believe then all the existing suggestions would have been implemented, and the PR would be ready for review again.

Also, the error in the failing job in the CI seem unrelated and it will most likely not occur again on the next run.

Run git clone --depth=1 https://github.com/bitcoin-core/qa-assets "${RUNNER_TEMP}/qa-assets"
Cloning into 'D:\a\_temp/qa-assets'...
remote: Internal Server Error
fatal: unable to access 'https://github.com/bitcoin-core/qa-assets/': The requested URL returned error: 500
Error: Process completed with exit code 128.

saikiran57 · 2026-02-03T15:13:08Z

the reason I've made a separate commit is https://github.com/bitcoin/bitcoin/pull/31668/changes#r2599095627 again I've made single commit for a clean review

My suggestions have been misunderstood.

To reiterate, the issue in the previous approach in this PR was that code changes were added in the first commit only for them to be removed in the second commit, which is confusing for the reviewers and misleading in the git logs. In #31668 (comment) and #31668 (comment), I suggested to clean up the commits and not merge them.

This comment from furszy that's mentioned in the quoted text looks fine to me and should be implemented. Once this is done, I believe then all the existing suggestions would have been implemented, and the PR would be ready for review again.

Also, the error in the failing job in the CI seem unrelated and it will most likely not occur again on the next run.
Run git clone --depth=1 https://github.com/bitcoin-core/qa-assets "${RUNNER_TEMP}/qa-assets"
Cloning into 'D:\a\_temp/qa-assets'...
remote: Internal Server Error
fatal: unable to access 'https://github.com/bitcoin-core/qa-assets/': The requested URL returned error: 500
Error: Process completed with exit code 128.

HI @rkrux I'm really confused now, Can you help what should I do. Current changes are latest and in one single commit.

DrahtBot added the CI failed label Jan 16, 2025

furszy reviewed Jan 17, 2025

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

maflcko reviewed Jan 21, 2025

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

DrahtBot removed the CI failed label Jan 21, 2025

DrahtBot mentioned this pull request Jan 21, 2025

Remove the legacy wallet and BDB dependency #28710

Merged

fanquake marked this pull request as draft February 20, 2025 20:47

DrahtBot added the CI failed label Mar 3, 2025

DrahtBot removed the CI failed label Mar 3, 2025

saikiran57 marked this pull request as ready for review March 3, 2025 12:13

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 54a20e6 to c353883 Compare March 3, 2025 13:56

davidrobinsonau reviewed Mar 5, 2025

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

DrahtBot added CI failed and removed CI failed labels Mar 5, 2025

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 0b6addc to e77515a Compare March 5, 2025 13:29

DrahtBot mentioned this pull request Mar 5, 2025

wallet: Disable creating and loading legacy wallets #31250

Merged

maflcko reviewed Mar 12, 2025

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

saikiran57 force-pushed the set_rescan_option_import_descriptors branch 2 times, most recently from 630210e to 1ac3559 Compare March 19, 2025 15:56

w0xlt reviewed Apr 2, 2025

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 1ac3559 to 8ebba0a Compare April 8, 2025 08:02

DrahtBot added the CI failed label Apr 8, 2025

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 8ebba0a to 7f3bb5c Compare April 8, 2025 11:02

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 1960799 to ee38847 Compare December 10, 2025 07:44

DrahtBot added the CI failed label Dec 10, 2025

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 33f26e4 to 625bbd4 Compare December 10, 2025 07:46

DrahtBot removed the CI failed label Dec 10, 2025

achow101 reviewed Dec 24, 2025

View reviewed changes

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 625bbd4 to 5fbef21 Compare December 26, 2025 09:25

rkrux suggested changes Dec 29, 2025

View reviewed changes

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 5fbef21 to 3458d90 Compare December 30, 2025 10:14

DrahtBot added the CI failed label Dec 30, 2025

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 2c30351 to 0b407dd Compare December 30, 2025 10:25

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 0b407dd to 9bd280c Compare December 30, 2025 11:19

DrahtBot added Needs rebase and removed CI failed labels Dec 30, 2025

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 9bd280c to 1733ddf Compare January 3, 2026 05:07

DrahtBot removed the Needs rebase label Jan 3, 2026

DrahtBot mentioned this pull request Jan 5, 2026

wallet: derivehdkey RPC to get xpub at arbitrary path #32784

Draft

rkrux suggested changes Jan 22, 2026

View reviewed changes

test/functional/wallet_importdescriptors.py Outdated Show resolved Hide resolved

test/functional/wallet_transactiontime_rescan.py Show resolved Hide resolved

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

saikiran57 force-pushed the set_rescan_option_import_descriptors branch 2 times, most recently from 02d374c to 34b09c4 Compare January 22, 2026 13:48

DrahtBot added the CI failed label Jan 22, 2026

maflcko mentioned this pull request Jan 22, 2026

Link past ACKs to a collapsible history section maflcko/DrahtBot#74

Closed

rkrux suggested changes Jan 23, 2026

View reviewed changes

DrahtBot removed the CI failed label Jan 30, 2026

Implement rescan stop with timestamp as never for import descriptors

5164b64

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 34b09c4 to 5164b64 Compare February 3, 2026 14:47

Conversation

saikiran57 commented Jan 16, 2025

Uh oh!

DrahtBot commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

DrahtBot commented Jan 16, 2025

Uh oh!

maflcko commented Jan 16, 2025

Uh oh!

Uh oh!

Uh oh!

fanquake commented Feb 20, 2025

Uh oh!

DrahtBot commented Mar 3, 2025

Uh oh!

mprenditore commented Mar 4, 2025

Uh oh!

Uh oh!

davidrobinsonau commented Mar 6, 2025

Uh oh!

mprenditore commented Mar 12, 2025

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rkrux left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rkrux commented Dec 30, 2025

Uh oh!

saikiran57 commented Jan 12, 2026

Uh oh!

rkrux left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rkrux commented Jan 22, 2026

Uh oh!

saikiran57 commented Jan 22, 2026

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

saikiran57 commented Feb 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

DrahtBot commented Jan 16, 2025 •

edited

Loading

rkrux left a comment •

edited

Loading

rkrux left a comment •

edited

Loading