Added rescan option for import descriptors #31668

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.
A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

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_transactiontime_rescan.py

saikiran57 · 2025-06-03T08:01:22Z

Hi @achow101 and @maflcko I've fixed your review comments.

achow101 · 2025-06-03T21:55:01Z

test/functional/wallet_transactiontime_rescan.py

        import_res = restorewo_wallet.importdescriptors(
            [
                {"desc": wo1_desc, "timestamp": "now"},
-                {"desc": wo2_desc, "timestamp": "now"},
+                {"desc": wo2_desc, "timestamp": "never"},
                {"desc": wo3_desc, "timestamp": "now"},


This test with now is basically the same as never. Since the distinction is that now does actually rescan a little bit, it would be good to add in the setup for these tests a transaction that would be scanned by now but not never.

hi @achow101 I've added more test cases.

Hi @achow101 could you please share your thoughts with the latest changes.

Sorry, but your latest test changes do not really implement what I was suggesting.

My point was that you need to distinguish between now and never, and currently the tests do not do that. 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, so we don't know whether there is a bug in the handling of now or never.

My suggestion is not to make yet another copy of the same import and balance check as you have done. Rather, my suggestion is that you should change the setup such that rescanning with now produces a different result (i.e. balance) than with never.

One way you might do this is to change the above set_node_times so that the time used for now overlaps with some of the transactions that are made earlier in the test. An alternative could be to create a transaction after set_node_times but before any imports as such a transaction would definitely fall within the now rescan range. However, it may require other changes to the test as balances may change.

You may have a different idea on how to do that test, it's up to you to decide how to approach writing that. Either way, the difference in behavior between now and never should be tested here that demonstrates how now and never can have different outcomes.

Hi @achow101 Thanks for your suggestion, I hope i understand it correctly and I updated the test cases. Kindly review it again.

Hi @achow101 any update on the fix I provided.

hi @achow101 can you please review it again.

Hi @achow101 kindly give some update on this.

HI @achow101 can you please approve this PR

DrahtBot · 2025-06-12T14:43:17Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/43971724813}
_{LLM reason (✨ experimental): The CI failure is caused by lint errors due to missing newline at the end of a Python test file.}

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-07-18T09:03:07Z

Hello, is possible to review it again? It should be all good now.
Thanks a lot for the support :)

DrahtBot · 2025-07-28T13:36:21Z

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

DrahtBot · 2025-07-28T13:36:54Z

test/functional/wallet_transactiontime_rescan.py

+        assert_equal(restorewo_wallet.getbalance(), 0)
+        assert_equal(len(restorewo_wallet.listtransactions()), 0)
+
+        # rescan will continue if any of the descriptors has now as timestamp


“if any of the descriptors has now as timestamp” → “have” [plural agreement] “if any of the descriptors has now or valid timestamp” → “have” [plural agreement]

mprenditore · 2025-08-08T09:18:08Z

Hello, anything still pending or can now be merged?

achow101 · 2025-08-08T18:32:22Z

Hello, anything still pending or can now be merged?

All PRs must be reviewed prior to being merged. This PR does not have sufficient review, so it cannot be merged.

saikiran57 · 2025-08-12T06:55:57Z

Hi all, could you please complete the review I've resolved all the comments of the previous review comments.

saikiran57 · 2025-09-02T07:13:22Z

Hi @furszy kindly acknowledge this review its been waiting for very long time.

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

achow101 reviewed May 28, 2025

View reviewed changes

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

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

maflcko reviewed Jun 2, 2025

View reviewed changes

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

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

saikiran57 force-pushed the set_rescan_option_import_descriptors branch 2 times, most recently from ec8e5df to 5c21359 Compare June 3, 2025 06:32

achow101 reviewed Jun 3, 2025

View reviewed changes

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 5c21359 to 2fd0fd5 Compare June 4, 2025 08:43

saikiran57 force-pushed the set_rescan_option_import_descriptors branch 2 times, most recently from e7ee990 to 8a67868 Compare June 12, 2025 14:43

DrahtBot added the CI failed label Jun 12, 2025

DrahtBot removed the CI failed label Jun 12, 2025

saikiran57 requested a review from achow101 July 16, 2025 08:14

DrahtBot mentioned this pull request Jul 25, 2025

test: fix RPC coverage check #33064

Merged

DrahtBot added Needs rebase CI failed and removed Needs rebase labels Jul 28, 2025

DrahtBot reviewed Jul 28, 2025

View reviewed changes

saikiran57 force-pushed the set_rescan_option_import_descriptors branch 2 times, most recently from 8a67868 to 8d625b8 Compare July 28, 2025 14:26

DrahtBot removed the CI failed label Jul 28, 2025

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 8d625b8 to 43ec169 Compare July 30, 2025 11:53

Implement rescan stop with timestamp as never for import descriptors

6e523bd

saikiran57 force-pushed the set_rescan_option_import_descriptors branch from 43ec169 to 6e523bd Compare July 30, 2025 11:56

Added rescan option for import descriptors #31668

Are you sure you want to change the base?

Added rescan option for import descriptors #31668

Uh oh!

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

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!

saikiran57 commented Jun 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 12, 2025

Uh oh!

mprenditore commented Jul 18, 2025

Uh oh!

DrahtBot commented Jul 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprenditore commented Aug 8, 2025

Uh oh!

achow101 commented Aug 8, 2025

Uh oh!

saikiran57 commented Aug 12, 2025

Uh oh!

saikiran57 commented Sep 2, 2025

Uh oh!

Uh oh!

DrahtBot commented Jan 16, 2025 •

edited

Loading