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

Reviewers, this pull request conflicts with the following ones:

#32349 (test: Increase stack size for “Debug” builds with MSVC by hebasto)
#28710 (Remove the legacy wallet and BDB dependency 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.

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.

Implement rescan stop with timestamp as never for import descriptors c11a3c7142

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:115 in c11a3c7142

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.

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 commented at 1:07 pm on April 25, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Added rescan option for import descriptors #31668

Code Coverage & Benchmarks

Reviews

Conflicts