Added rescan option for import descriptors

saikiran57 commented at 8:11 am on January 16, 2025: contributor

Fix related to issue: #31263

DrahtBot commented at 8:11 am on January 16, 2025: contributor

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 added the label CI failed on Jan 16, 2025

DrahtBot commented at 9:00 am on January 16, 2025: contributor

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

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 commented at 1:28 pm on January 16, 2025: member

This needs a test and the tests fixed

in src/wallet/rpc/backup.cpp:1727 in 1ae32a9eb5 outdated

1722@@ -1712,10 +1723,8 @@ RPCHelpMan importdescriptors()
1723                 lowest_timestamp = timestamp;
1724             }
1725 
1726-            // If we know the chain tip, and at least one request was successful then allow rescan
1727-            if (!rescan && result["success"].get_bool()) {
1728-                rescan = true;
1729-            }
1730+            // check rescan option for each request if any request set rescan
1731+            rescan = GetImportRescan(request);

furszy commented at 3:18 pm on January 17, 2025:

This overrides the flag for each imported descriptor, meaning the final value depends solely on the last imported element.

saikiran57 commented at 11:57 am on January 21, 2025:

Hi @furszy I have updated the commit can you please review it once.

in src/wallet/rpc/backup.cpp:1652 in 36b29640ba outdated

1648@@ -1638,6 +1649,7 @@ RPCHelpMan importdescriptors()
1649                                     },
1650                                     {"internal", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether matching outputs should be treated as not incoming payments (e.g. change)"},
1651                                     {"label", RPCArg::Type::STR, RPCArg::Default{""}, "Label to assign to the address, only allowed with internal=false. Disabled for ranged descriptors"},
1652+                                    {"rescan", RPCArg::Type::BOOL, RPCArg::Default{true}, "Scan the chain and mempool for wallet transactions."},

maflcko commented at 11:06 am on January 21, 2025:

How does this interact with timestamp? As mentioned in the issue thread, I agree it would be better to have another special timestamp value, no?

saikiran57 commented at 12:19 pm on January 21, 2025:

I’ve seen two major issues while we are importing 2M address to bitcoin using this importdescriptor method.

First issue was rescan because we are passing timestamp as ’now’ so when we pass now it is taking median time of last scanned block, whereas if we take currentimestamp or futuretimestamp it is considering last mined block timestamp. So when the request is imported successfully it is setting rescan flag as true.

—- Rescan operation

In rescan operation it is taking either last mined block timestamp or median timestamp of last block + last 2 hours.

— My solution Skip rescan if we provide rescan option as false (default true) so irrespective timestamp it won’t scan block chain. And whatever timestamp we got as a request we are passing this to ProcessDescriptorImport(*pwallet, request, timestamp);

Second Issue: not part of this fix its my observation I sent a batch of 1000 address each time to importdescriptors method even rescan option as false we may get delay i think its because https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/backup.cpp#L1579 std::map<uint256, std::unique_ptr<ScriptPubKeyMan>> m_spk_managers;

maflcko commented at 1:10 pm on January 21, 2025:

Skip rescan if we provide rescan option as false (default true) so irrespective timestamp it won’t scan block chain.

This seems confusing to users to silently ignore the timestamp. Why not simply have another timestamp value?

saikiran57 commented at 1:29 pm on January 22, 2025:

Hi @maflcko something like{"timestamp | \"now\"|\"never\"", "integer / string"} here never means no scanning. Please suggest your expectation.

mprenditore commented at 4:22 pm on February 20, 2025:

I do agree to add never to the list of timestamp values to handle this case.

davidrobinsonau commented at 11:14 pm on March 2, 2025:

What do you suggest as the return value for the function when never is passed? So that this can be easily checked against for rescan? it can’t be 0 as “0 can be specified to scan the entire blockchain.” -1, null?

static int64_t GetImportTimestamp(const UniValue& data, int64_t now)

 0{
 1    if (data.exists("timestamp")) {
 2        const UniValue& timestamp = data["timestamp"];
 3        if (timestamp.isNum()) {
 4            return timestamp.getInt<int64_t>();
 5        } else if (timestamp.isStr() && timestamp.get_str() == "now") {
 6            return now;
 7        } else if (timestamp.isStr() && timestamp.get_str() == "never") {
 8            return ???
 9        }
10        throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Expected number or \"now\" timestamp value for key. got type %s", uvTypeName(timestamp.type())));
11    }
12    throw JSONRPCError(RPC_TYPE_ERROR, "Missing required timestamp field for key");
13}

mprenditore commented at 10:10 am on March 3, 2025:

@davidrobinsonau I think both could be good, but -1 seems the best option here

saikiran57 commented at 12:14 pm on March 3, 2025:

Updated the code

DrahtBot removed the label CI failed on Jan 21, 2025

fanquake commented at 8:47 pm on February 20, 2025: member

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

fanquake marked this as a draft on Feb 20, 2025

DrahtBot added the label CI failed on Mar 3, 2025

DrahtBot commented at 11:15 am on March 3, 2025: contributor

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

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.

DrahtBot removed the label CI failed on Mar 3, 2025

saikiran57 marked this as ready for review on Mar 3, 2025

saikiran57 force-pushed on Mar 3, 2025

mprenditore commented at 4:40 pm on March 4, 2025: none

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.

in src/wallet/rpc/backup.cpp:1252 in c3538839bd outdated

1245@@ -1246,6 +1246,9 @@ static int64_t GetImportTimestamp(const UniValue& data, int64_t now)
1246         } else if (timestamp.isStr() && timestamp.get_str() == "now") {
1247             return now;
1248         }
1249+        else if (timestamp.isStr() && timestamp.get_str() == "never") {
1250+            return -1;
1251+        }
1252         throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Expected number or \"now\" timestamp value for key. got type %s", uvTypeName(timestamp.type())));

davidrobinsonau commented at 0:04 am on March 5, 2025:

Need to update: Expected a number or "now" or "never" timestamp value for key. got type %s

DrahtBot added the label CI failed on Mar 5, 2025

DrahtBot removed the label CI failed on Mar 5, 2025

saikiran57 force-pushed on Mar 5, 2025

davidrobinsonau commented at 10:41 am on March 6, 2025: none

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 commented at 2:22 pm on March 12, 2025: none

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

in src/wallet/rpc/backup.cpp:1713 in e77515ac38 outdated

1710         // Get all timestamps and extract the lowest timestamp
1711         for (const UniValue& request : requests.getValues()) {
1712             // This throws an error if "timestamp" doesn't exist
1713-            const int64_t timestamp = std::max(GetImportTimestamp(request, now), minimum_timestamp);
1714+            auto requestTimestamp = GetImportTimestamp(request, now);
1715+            all_rescan_value += (requestTimestamp < 0 ? 0 : 1);

maflcko commented at 6:13 pm on March 12, 2025:

Seems a bit odd to skip rescan when all timestamps are negative. Previously, this would rescan the full chain.

davidrobinsonau commented at 5:05 am on March 13, 2025:

Is there a use case for someone to pass a timestamp prior to January 1, 1970? Which would be a negative number, causing bitcoind not to rescan when it was expected to. Alternatively, the code could be updated to only look for “-1”, meaning anything below that could still be a full rescan.

maflcko commented at 8:18 am on March 13, 2025:

It has been permitted previously and this commit is silently breaking it. If you want to use -1, it seems better to properly sanitize the user input first, or use a different approach.

saikiran57 commented at 10:18 am on March 13, 2025:

I’ve updated code block. Could you please check @maflcko and @davidrobinsonau

davidrobinsonau commented at 8:49 am on March 14, 2025:

This code is getting complex for a simple change. @maflcko, you stated above that instead of having another field, “rescan”, we should simply use a value for the timestamp field to advise if a rescan is needed. This has been done, but sadly, the function “static int64_t GetImportTimestamp(const UniValue& data, int64_t now)” only returns an int64_t, and we need to work with that returned value, AFAIK, we can’t use null as its not a pointer. So that leaves a number, which is currently -1.

The code revision checks for anything negative, which we now know will silently break what was assumed to be ok, the following code is far too complex for a simple return value check.

Can we go back to -1 and update the help to state that to stop a rescan use either “-1” or “never”. Anything lower than -1 will cause a rescan as previously supported. The chances of someone passing -1 as an actual datetime (January 1, 1970) number is very very low. And if they do, the help docs will explain why it didn’t work.

I propose we change 1713 from: all_rescan_value += (requestTimestamp < 0 ? 0 : 1); to if (requestTimestamp != -1) all_rescan_value++;

maflcko commented at 9:08 am on March 14, 2025:

This code is getting complex for a simple change. @maflcko, you stated above that instead of having another field, “rescan”, we should simply use a value for the timestamp field to advise if a rescan is needed. This has been done, but sadly, the function “static int64_t GetImportTimestamp(const UniValue& data, int64_t now)” only returns an int64_t, and we need to work with that returned value, AFAIK, we can’t use null as its not a pointer. So that leaves a number, which is currently -1.

In C++, it is possible to use std::optional. An alternative would be to sanitize user-input and reject negative integer values. There may be other alternatives to write clean code, but it is somewhat the burden of the pull request author to write clean and easily revieable code. If reviewers write the code for the pull request author, they could have created the pull themselves.

I don’t have a strong opinion on this, but personally I don’t feel comfortable to ack the current code, or an added footgun/break for users, albeit small.

saikiran57 commented at 1:48 pm on March 14, 2025:

May I know what is the issue with current fix: It will handle negative and positive, “never” cases.

auto requestTimestamp = GetImportTimestamp(request, now); // if any one entity has valid timestamp we can skp this check if (!all_rescan_value) { if (requestTimestamp < 0) { const UniValue& timestamp = request["timestamp"]; // set all_rescan_value true if requested timestamp is negative otherwise it all_rescan_value value never if (timestamp.isNum() && timestamp.getInt<int64_t>() == requestTimestamp) { all_rescan_value += 1; } } else { all_rescan_value += 1; } }

It will check only if any one entity has valid timestamp (positive or negative) then rescan will be enable. @maflcko in your comment you have mentioned sanitize user-input and reject negative integer values . If i reject negative values it won’t do full rescan right.

maflcko commented at 6:07 pm on April 1, 2025:

@maflcko in your comment you have mentioned sanitize user-input and reject negative integer values . If i reject negative values it won’t do full rescan right.

Yes, if you return early before the rescan happens, then the rescan won’t happen.

saikiran57 force-pushed on Mar 13, 2025

saikiran57 force-pushed on Mar 19, 2025

in src/wallet/rpc/backup.cpp:1734 in 1ac35590f0 outdated

1736-            if (!rescan && result["success"].get_bool()) {
1737-                rescan = true;
1738-            }
1739         }
1740+
1741+        rescan = all_rescan_value > 0 ? true:false;

w0xlt commented at 9:58 pm on April 2, 2025:

0        rescan = all_rescan_value > 0 ;

w0xlt commented at 9:59 pm on April 2, 2025: contributor

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

saikiran57 force-pushed on Apr 8, 2025

DrahtBot added the label CI failed on Apr 8, 2025

saikiran57 force-pushed on Apr 8, 2025

DrahtBot removed the label CI failed on Apr 8, 2025

in src/wallet/rpc/backup.cpp:1719 in 36cf993e2a outdated

1716             // This throws an error if "timestamp" doesn't exist
1717-            const int64_t timestamp = std::max(GetImportTimestamp(request, now), minimum_timestamp);
1718+            auto requestTimestamp = GetImportTimestamp(request, now);
1719+            // if any one entity has valid timestamp we can skp this check
1720+            if (!all_rescan_value) {
1721+                if (requestTimestamp >= 0) {

achow101 commented at 7:33 pm on April 10, 2025:

nit: These 2 lines can be condensed into one if.

0            if (!all_rescan_value && requestTimestamp >= 0) {

in src/wallet/rpc/backup.cpp:1720 in 36cf993e2a outdated

1717-            const int64_t timestamp = std::max(GetImportTimestamp(request, now), minimum_timestamp);
1718+            auto requestTimestamp = GetImportTimestamp(request, now);
1719+            // if any one entity has valid timestamp we can skp this check
1720+            if (!all_rescan_value) {
1721+                if (requestTimestamp >= 0) {
1722+                    all_rescan_value += 1;

achow101 commented at 7:34 pm on April 10, 2025:

nit:

0                    all_rescan_value ++;

in src/wallet/rpc/backup.cpp:1704 in 36cf993e2a outdated

1700@@ -1693,30 +1701,38 @@ RPCHelpMan importdescriptors()
1701     const int64_t minimum_timestamp = 1;
1702     int64_t now = 0;
1703     int64_t lowest_timestamp = 0;
1704-    bool rescan = false;
1705+    bool rescan = true;

achow101 commented at 7:35 pm on April 10, 2025:

The prior behavior would skip rescanning on failed imports. This seems to no longer be the case.

saikiran57 commented at 7:06 am on April 11, 2025:

Hi @achow101 I’ve improved the logic and taken care your point as well. And also updated functional test case.

achow101 commented at 7:35 pm on April 10, 2025: member

I also would like to see a functional test for the new behavior.

saikiran57 force-pushed on Apr 11, 2025

saikiran57 commented at 1:03 pm on April 23, 2025: contributor

Hi All @achow101 @furszy @mprenditore @maflcko @w0xlt @davidrobinsonau request to re-review this PR and provide ACK.

in test/functional/wallet_importdescriptors.py:112 in c11a3c7142 outdated

110+        self.log.info("Should import a p2pkh descriptor with timestamp as never")
111+        import_request = {"desc": descsum_create("pkh(" + key.pubkey + ")"),
112+                 "timestamp": "never",
113+                 "label": "Descriptor import test with never as timestamp"}
114+        self.test_importdesc(import_request, success=True)
115+        test_address(w1,

achow101 commented at 8:03 pm on April 23, 2025:

While this verifies that the address was imported, what we care about here is whether a rescan occurred. It would be better to send to the address for the descriptor being imported, then check to see that after importing, the transaction is not detected.

saikiran57 commented at 12:15 pm on May 1, 2025:

Could you please check updated test cases.

saikiran57 commented at 2:24 pm on May 12, 2025:

Hi @achow101 could you please check this. Merge it if everything is okay.

achow101 commented at 8:05 pm on April 23, 2025: member

There should also be a test where multiple things are imported, one with a timestamp, and one with never. There should be a rescan in this case.

DrahtBot added the label Needs rebase on Apr 25, 2025

DrahtBot removed the label Needs rebase on Apr 30, 2025

saikiran57 force-pushed on May 1, 2025

in src/wallet/rpc/backup.cpp:1252 in 367a329c3f outdated

1246@@ -1247,11 +1247,18 @@ static int64_t GetImportTimestamp(const UniValue& data, int64_t now)
1247     if (data.exists("timestamp")) {
1248         const UniValue& timestamp = data["timestamp"];
1249         if (timestamp.isNum()) {
1250-            return timestamp.getInt<int64_t>();
1251+            auto requiredTimestamp = timestamp.getInt<int64_t>();
1252+            if (requiredTimestamp < 0) {
1253+                throw JSONRPCError(RPC_TYPE_ERROR, "timestamp should be greater then zero");

DrahtBot commented at 6:46 pm on May 12, 2025:

LLM Linter (✨ experimental)

Possible typos and grammar issues:

timestamp should be greater then zero → timestamp should be greater than zero ““now” can be specified to scanning from last mediantime… → ““now” can be specified to scan from last mediantime… …value for key. got type %s → …value for key. Got type %s …timestamp as provide value → …timestamp as provided value

saikiran57 force-pushed on May 13, 2025

DrahtBot added the label Needs rebase on May 17, 2025

saikiran57 force-pushed on May 19, 2025

in test/functional/wallet_transactiontime_rescan.py:154 in 0341e021fe outdated

149+        assert_equal(all([r["success"] for r in import_res]), True)
150+        # check user has 0 balance and no transactions
151+        assert_equal(restorewo_wallet.getbalance(), 0)
152+        assert_equal(len(restorewo_wallet.listtransactions()), 0)
153+
154+        # rescan will continue if anyone of the descriptor has now or valid timestamp

DrahtBot commented at 7:59 am on May 19, 2025:

anyone of the descriptor -> any of the descriptors [grammatical error, incorrect pronoun and singular/plural mismatch]

maflcko commented at 7:33 am on June 2, 2025:

not sure why this was marked resolved?

DrahtBot removed the label Needs rebase on May 19, 2025

saikiran57 commented at 6:26 am on May 28, 2025: contributor

Hi @achow101 @maflcko @davidrobinsonau can you please check this once and merge it everything is fine.

in src/wallet/rpc/backup.cpp:139 in 0341e021fe outdated

136+            return requiredTimestamp;
137         } else if (timestamp.isStr() && timestamp.get_str() == "now") {
138             return now;
139         }
140-        throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Expected number or \"now\" timestamp value for key. got type %s", uvTypeName(timestamp.type())));
141+        else if (timestamp.isStr() && timestamp.get_str() == "never") {

maflcko commented at 7:51 am on May 28, 2025:

nit: new code should put the } before the else if

in src/wallet/rpc/backup.cpp:131 in 0341e021fe outdated

127@@ -128,11 +128,18 @@ static int64_t GetImportTimestamp(const UniValue& data, int64_t now)
128     if (data.exists("timestamp")) {
129         const UniValue& timestamp = data["timestamp"];
130         if (timestamp.isNum()) {
131-            return timestamp.getInt<int64_t>();
132+            auto requiredTimestamp = timestamp.getInt<int64_t>();

achow101 commented at 9:53 pm on May 28, 2025:

In 0341e021fe6edfa21252f4bd93b9e1a209383891 “Implement rescan stop with timestamp as never for import descriptors”

Please use snake_case for new variables. Additionally, when the type is not annoying to type out, please give the full type rather than use auto.

Same for requestTimestamp below.

in src/wallet/rpc/backup.cpp:401 in 0341e021fe outdated

395@@ -389,16 +396,18 @@ RPCHelpMan importdescriptors()
396         // Get all timestamps and extract the lowest timestamp
397         for (const UniValue& request : requests.getValues()) {
398             // This throws an error if "timestamp" doesn't exist
399-            const int64_t timestamp = std::max(GetImportTimestamp(request, now), minimum_timestamp);
400+            auto requestTimestamp = GetImportTimestamp(request, now);
401+            // If requestTimestamp is never then timestamp will be now
402+            const int64_t timestamp = requestTimestamp < 0 ? now: requestTimestamp;

achow101 commented at 9:54 pm on May 28, 2025:

In 0341e021fe6edfa21252f4bd93b9e1a209383891 “Implement rescan stop with timestamp as never for import descriptors”

nit: space between now and :

0            const int64_t timestamp = requestTimestamp < 0 ? now : requestTimestamp;

saikiran57 force-pushed on Jun 2, 2025

saikiran57 force-pushed on Jun 3, 2025

saikiran57 commented at 8:01 am on June 3, 2025: contributor

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

in test/functional/wallet_transactiontime_rescan.py:159 in 5c213595fe outdated

155         import_res = restorewo_wallet.importdescriptors(
156             [
157                 {"desc": wo1_desc, "timestamp": "now"},
158-                {"desc": wo2_desc, "timestamp": "now"},
159+                {"desc": wo2_desc, "timestamp": "never"},
160                 {"desc": wo3_desc, "timestamp": "now"},

achow101 commented at 9:55 pm on June 3, 2025:

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.

saikiran57 commented at 8:45 am on June 4, 2025:

hi @achow101 I’ve added more test cases.

saikiran57 commented at 10:39 am on June 9, 2025:

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

achow101 commented at 11:20 pm on June 11, 2025:

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.

saikiran57 commented at 5:35 am on June 13, 2025:

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

saikiran57 commented at 10:27 am on June 19, 2025:

Hi @achow101 any update on the fix I provided.

saikiran57 commented at 8:05 am on July 3, 2025:

hi @achow101 can you please review it again.

saikiran57 force-pushed on Jun 4, 2025

saikiran57 force-pushed on Jun 12, 2025

Implement rescan stop with timestamp as never for import descriptors 8a6786847c

saikiran57 force-pushed on Jun 12, 2025

DrahtBot added the label CI failed on Jun 12, 2025

DrahtBot commented at 2:43 pm on June 12, 2025: contributor

🚧 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.

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.

DrahtBot removed the label CI failed on Jun 12, 2025

Added rescan option for import descriptors #31668

Code Coverage & Benchmarks

Reviews

Conflicts