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.

Type	Reviewers
Concept ACK	rkrux

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33874 (wallet: refactor ProcessDescriptorImport by naiyoma)
#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 [apostrophe in “dont’” is misplaced; should be “don’t” to be correct and clear]

2026-01-03

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:145 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 commented at 8:12 am on July 16, 2025:

Hi @achow101 kindly give some update on this.

saikiran57 commented at 11:38 am on July 30, 2025:

HI @achow101 can you please approve this PR

saikiran57 force-pushed on Jun 4, 2025

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

saikiran57 requested review from achow101 on Jul 16, 2025

mprenditore commented at 9:03 am on July 18, 2025: none

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

DrahtBot added the label Needs rebase on Jul 28, 2025

DrahtBot added the label CI failed on Jul 28, 2025

DrahtBot removed the label Needs rebase on Jul 28, 2025

DrahtBot commented at 1:36 pm on July 28, 2025: contributor

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

in test/functional/wallet_transactiontime_rescan.py:166 in 49e3793d0f outdated

162+
163+        # check user has 0 balance and no transactions
164+        assert_equal(restorewo_wallet.getbalance(), 0)
165+        assert_equal(len(restorewo_wallet.listtransactions()), 0)
166+
167+        # rescan will continue if any of the descriptors has now as timestamp

DrahtBot commented at 1:36 pm on July 28, 2025:

“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]

saikiran57 force-pushed on Jul 28, 2025

DrahtBot removed the label CI failed on Jul 28, 2025

saikiran57 force-pushed on Jul 30, 2025

mprenditore commented at 9:18 am on August 8, 2025: none

Hello, anything still pending or can now be merged?

achow101 commented at 6:32 pm on August 8, 2025: member

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 commented at 6:55 am on August 12, 2025: contributor

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

saikiran57 commented at 7:13 am on September 2, 2025: contributor

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

in test/functional/wallet_transactiontime_rescan.py:141 in 6e523bda47 outdated

135@@ -136,8 +136,34 @@ def run_test(self):
136         # blocks of the past 2 hours, based on the current MTP timestamp; in order to avoid
137         # importing the last address (wo3), we advance the time further and generate 10 blocks
138         set_node_times(self.nodes, cur_time + ten_days + ten_days + ten_days + ten_days)
139-        self.generatetoaddress(minernode, 10, m1)
140 
141+        # send 2 btc to our third watch-only address
142+        self.log.info('Send 2 btc to user')

achow101 commented at 9:47 pm on November 17, 2025:

It’s unnecessary to log this kind of information. This would be more appropriate as a comment, but it’s really not necessary at all (yes this test does that elsewhere, but it shouldn’t).

in test/functional/wallet_transactiontime_rescan.py:145 in 6e523bda47 outdated

141+        # send 2 btc to our third watch-only address
142+        self.log.info('Send 2 btc to user')
143+        miner_wallet.sendtoaddress(wo3, 2)
144+
145+        # generate more blocks and check blockcount
146+        self.generatetoaddress(minernode, COINBASE_MATURITY, m1)

achow101 commented at 9:48 pm on November 17, 2025:

It’s not necessary to generate COINBASE_MATURITY blocks since we’re not spending those coins. This also makes the previously existing comment incorrect as that states only 10 blocks are being mined.

in test/functional/wallet_transactiontime_rescan.py:148 in 6e523bda47 outdated

144+
145+        # generate more blocks and check blockcount
146+        self.generatetoaddress(minernode, COINBASE_MATURITY, m1)
147+        assert_equal(minernode.getblockcount(), initial_mine + 600)
148+
149+        self.log.info('Check user\'s final balance and transaction count')

achow101 commented at 9:48 pm on November 17, 2025:

This log is unnecessary.

in test/functional/wallet_transactiontime_rescan.py:212 in 6e523bda47 outdated

210+        )
211+        assert_equal(all([r["success"] for r in import_res]), True)
212+
213+        # check user has 5 btc balance and 2 transactions
214+        assert_equal(restorewo_wallet.getbalance(), 5)
215+        assert_equal(len(restorewo_wallet.listtransactions()), 2)

achow101 commented at 9:49 pm on November 17, 2025:

I don’t think this test case is necessary, it doesn’t seem to me to be testing anything that isn’t already being tested.

in test/functional/wallet_importdescriptors.py:105 in 6e523bda47

101@@ -102,6 +102,20 @@ def run_test(self):
102                      labels=["Descriptor import test"])
103         assert_equal(w1.getwalletinfo()['keypoolsize'], 0)
104 
105+        # Test importing of a P2PKH descriptor with timestamp as never

rkrux commented at 2:34 pm on November 25, 2025:

I think the test cases can be greatly simplified with the following diff:

 0diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
 1index 49c41b0a1f..588d7725dc 100755
 2--- a/test/functional/wallet_importdescriptors.py
 3+++ b/test/functional/wallet_importdescriptors.py
 4@@ -25,6 +25,7 @@ from test_framework.descriptors import descsum_create
 5 from test_framework.util import (
 6     assert_equal,
 7     assert_raises_rpc_error,
 8+    assert_greater_than,
 9 )
10 from test_framework.wallet_util import (
11     get_generate_key,
12@@ -63,6 +64,43 @@ class ImportDescriptorsTest(BitcoinTestFramework):
13             assert_equal(result[0]['error']['code'], error_code)
14             assert_equal(result[0]['error']['message'], error_message)
15 
16+    def test_importdescriptor_never_timestamp(self, node, funding_wallet):
17+        self.log.info("Test importing descriptors with timestamp as never")
18+        assert_greater_than(funding_wallet.getbalance(), 0)
19+
20+        key = get_generate_key()
21+        funding_amount = 1 # BTC
22+        txid = funding_wallet.send(outputs=[{key.p2wpkh_addr: funding_amount}])["txid"]
23+        self.generate(node, 1, sync_fun=self.no_op)
24+
25+        wpkh_desc = descsum_create("wpkh(" + key.pubkey + ")")
26+        spare_pkh_desc = descsum_create("pkh(" + key.pubkey + ")")
27+
28+        node.createwallet(wallet_name='never_wallet', disable_private_keys=True, blank=True)
29+        never_wallet = node.get_wallet_rpc('never_wallet')
30+        assert_equal(never_wallet.getbalance(), 0)
31+        with node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning last"]):
32+            self.test_importdesc({"desc": wpkh_desc, "timestamp": "never"}, success=True, wallet=never_wallet)
33+        assert_equal(never_wallet.getbalance(), 0)
34+        assert_equal(never_wallet.listtransactions(), [])
35+
36+        node.createwallet(wallet_name='now_wallet', disable_private_keys=True, blank=True)
37+        now_wallet = node.get_wallet_rpc('now_wallet')
38+        assert_equal(now_wallet.getbalance(), 0)
39+        with node.assert_debug_log(expected_msgs=["Rescanning last"]):
40+            self.test_importdesc({"desc": wpkh_desc, "timestamp": "now"}, success=True, wallet=now_wallet)
41+        assert_equal(now_wallet.getbalance(), funding_amount)
42+        assert_equal(now_wallet.listtransactions()[0]["txid"], txid)
43+
44+        node.createwallet(wallet_name='never_now_wallet', disable_private_keys=True, blank=True)
45+        never_now_wallet = node.get_wallet_rpc('never_now_wallet')
46+        assert_equal(never_now_wallet.getbalance(), 0)
47+        # passing "now" timestamp for the spare_pkh_desc import causes the rescan that finds the transaction of the wpkh_desc
48+        with node.assert_debug_log(expected_msgs=["Rescanning last"]):
49+            never_now_wallet.importdescriptors([{"desc": wpkh_desc, "timestamp": "never"}, {"desc": spare_pkh_desc, "timestamp": "now"}])
50+        assert_equal(never_now_wallet.getbalance(), funding_amount)
51+        assert_equal(never_now_wallet.listtransactions()[0]["txid"], txid)
52+
53     def run_test(self):
54         self.log.info('Setting up wallets')
55         self.nodes[0].createwallet(wallet_name='w0', disable_private_keys=False)
56@@ -102,20 +140,6 @@ class ImportDescriptorsTest(BitcoinTestFramework):
57                      labels=["Descriptor import test"])
58         assert_equal(w1.getwalletinfo()['keypoolsize'], 0)
59 
60-        # Test importing of a P2PKH descriptor with timestamp as never
61-        key = get_generate_key()
62-        self.log.info("Should import a p2pkh descriptor with timestamp as never")
63-        import_request = {"desc": descsum_create("pkh(" + key.pubkey + ")"),
64-                 "timestamp": "never",
65-                 "label": "Descriptor import test with never as timestamp"}
66-        self.test_importdesc(import_request, success=True)
67-        test_address(w1,
68-                     key.p2pkh_addr,
69-                     solvable=True,
70-                     ismine=True,
71-                     labels=["Descriptor import test with never as timestamp"])
72-        assert_equal(w1.getwalletinfo()['keypoolsize'], 0)
73-
74         self.log.info("Test can import same descriptor with public key twice")
75         self.test_importdesc(import_request, success=True)
76 
77@@ -784,5 +808,7 @@ class ImportDescriptorsTest(BitcoinTestFramework):
78             assert_equal(w_multipath.getrawchangeaddress(address_type="bech32"), w_multisplit.getrawchangeaddress(address_type="bech32"))
79         assert_equal(sorted(w_multipath.listdescriptors()["descriptors"], key=lambda x: x["desc"]), sorted(w_multisplit.listdescriptors()["descriptors"], key=lambda x: x["desc"]))
80 
81+        self.test_importdescriptor_never_timestamp(self.nodes[0], w0)
82+
83 if __name__ == '__main__':
84     ImportDescriptorsTest(__file__).main()

The changes in test/functional/wallet_transactiontime_rescan.py can be removed IMHO.

saikiran57 commented at 1:05 pm on December 5, 2025:

its done thanks for updated test cases.

saikiran57 commented at 1:05 pm on December 5, 2025:

Hi @achow101 regrading this what do you think “The changes in test/functional/wallet_transactiontime_rescan.py can be removed IMHO.”

achow101 commented at 0:03 am on December 24, 2025:

Yes the changes to wallet_transactiontime_rescan.py can be removed as the changes to wallet_importdescriptor.py sufficiently cover those cases.

in src/wallet/rpc/backup.cpp:135 in 6e523bda47 outdated

127@@ -128,11 +128,17 @@ 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+            int64_t required_timestamp = timestamp.getInt<int64_t>();
133+            if (required_timestamp < 0) {
134+                throw JSONRPCError(RPC_TYPE_ERROR, "timestamp should be greater than zero");
135+            }

rkrux commented at 2:35 pm on November 25, 2025:

Would be good to add a small test case for this newly added condition as well.

saikiran57 commented at 1:14 pm on December 5, 2025:

I tried add the function test cases but the python was giving an exception raise JSONRPCException(response['error'], status) Then I debug the issue in throw JSONRPCError(RPC_TYPE_ERROR, "timestamp should be greater than zero"); it setting if (jsonrpc_version == JSONRPCVersion::V1_LEGACY) reply.pushKV(“error”, NullUniValue); so in authproxy.py

            if 'error' in response:
 raise JSONRPCException(response['error'], status)

checking if error is there or not then its throwing exception so I think we must address JSONRPCException issues in a separate ticket instead of fixing here.

rkrux commented at 2:54 pm on November 25, 2025: contributor

Concept ACK 6e523bda47854333d267f53cfa5292fd2eb40ec7

Agree with the intent to avoid rescanning if the user specifies so.

saikiran57 force-pushed on Dec 5, 2025

DrahtBot added the label CI failed on Dec 5, 2025

saikiran57 force-pushed on Dec 8, 2025

DrahtBot removed the label CI failed on Dec 8, 2025

in src/wallet/rpc/backup.cpp:135 in cda5cb3ec2 outdated

128@@ -129,11 +129,17 @@ static int64_t GetImportTimestamp(const UniValue& data, int64_t now)
129     if (data.exists("timestamp")) {
130         const UniValue& timestamp = data["timestamp"];
131         if (timestamp.isNum()) {
132-            return timestamp.getInt<int64_t>();
133+            int64_t required_timestamp = timestamp.getInt<int64_t>();
134+            if (required_timestamp < 0) {
135+                throw JSONRPCError(RPC_TYPE_ERROR, "timestamp should be greater than zero");
136+            }

furszy commented at 3:38 pm on December 8, 2025:

In cda5cb3ec27ac9bf15b947eff0c5cc87cc835c48:

This change should be in a different commit.

saikiran57 commented at 2:39 pm on December 9, 2025:

I didn’t get your comment.

furszy commented at 2:53 pm on December 9, 2025:

The new exception you are throwing here is not related to the “never” timestamp change. It should be on its own commit.

saikiran57 commented at 7:50 am on December 10, 2025:

done.

in src/wallet/rpc/backup.cpp:140 in cda5cb3ec2 outdated

136+            }
137+            return required_timestamp;
138         } else if (timestamp.isStr() && timestamp.get_str() == "now") {
139             return now;
140+        } else if (timestamp.isStr() && timestamp.get_str() == "never") {
141+            return -1;

furszy commented at 3:41 pm on December 8, 2025:

In cda5cb3ec27ac9bf15b947eff0c5cc87cc835c48:

Instead of returning -1, could change the return value to optional, and here return std::nullopt.

saikiran57 commented at 2:39 pm on December 9, 2025:

I didn’t see the usage of std::optional here and I prefer minimal change so that it won’t effect in other places.

furszy commented at 2:56 pm on December 9, 2025:

I didn’t see the usage of std::optional here and I prefer minimal change so that it won’t effect in other places.

if this affects other places that were not modified by this PR, we should take care of them. I would try using std::optional, it might clean some code for you.

saikiran57 commented at 7:50 am on December 10, 2025:

Updated can you check once.

in test/functional/wallet_importdescriptors.py:102 in cda5cb3ec2

 97+        assert_equal(never_now_wallet.getbalance(), 0)
 98+        # passing "now" timestamp for the spare_pkh_desc import causes the rescan that finds the transaction of the wpkh_desc
 99+        with node.assert_debug_log(expected_msgs=["Rescanning last"]):
100+            never_now_wallet.importdescriptors([{"desc": wpkh_desc, "timestamp": "never"}, {"desc": spare_pkh_desc, "timestamp": "now"}])
101+        assert_equal(never_now_wallet.getbalance(), funding_amount)
102+        assert_equal(never_now_wallet.listtransactions()[0]["txid"], txid)

furszy commented at 3:49 pm on December 8, 2025:

In https://github.com/bitcoin/bitcoin/commit/cda5cb3ec27ac9bf15b947eff0c5cc87cc835c48:

Need to unload the wallets after using them; otherwise they stay loaded for other tests in this file as well. Also, you can do this in fewer lines of code with a loop. As far as I can see, the only per-round variants are the timestamp, the balance, and the tx list.

saikiran57 commented at 2:37 pm on December 9, 2025:

Updated.

furszy commented at 3:51 pm on December 8, 2025: member

Instead of using timestamp=never, which is not really descriptive. What about using new?

saikiran57 force-pushed on Dec 9, 2025

DrahtBot added the label CI failed on Dec 9, 2025

DrahtBot removed the label CI failed on Dec 9, 2025

in test/functional/wallet_importdescriptors.py:89 in 1960799bf5 outdated

80+            {
81+                "name": "never_wallet",
82+                "timestamp": "never",
83+                "expected_balance": 0,
84+                "expected_msgs": [],
85+                "unexpected_msgs": ["Rescanning last"],

furszy commented at 2:58 pm on December 9, 2025:

this two args don’t change for all wallets. Can hardcode them below.

saikiran57 commented at 7:50 am on December 10, 2025:

Please check once its different.

in test/functional/wallet_importdescriptors.py:134 in 1960799bf5

129+                assert_equal(wallet.listtransactions()[0]["txid"], txid)
130+            else:
131+                assert_equal(wallet.listtransactions(), [])
132+
133+            # 🔥 Unload wallet after processing
134+            node.unloadwallet(config["name"])

furszy commented at 3:00 pm on December 9, 2025:

LLM output? Remove the emoji please.

saikiran57 commented at 7:49 am on December 10, 2025:

done

saikiran57 force-pushed on Dec 10, 2025

DrahtBot added the label CI failed on Dec 10, 2025

saikiran57 force-pushed on Dec 10, 2025

DrahtBot removed the label CI failed on Dec 10, 2025

in src/wallet/rpc/backup.cpp:392 in 625bbd4964 outdated

388@@ -383,16 +389,18 @@ RPCHelpMan importdescriptors()
389         // Get all timestamps and extract the lowest timestamp
390         for (const UniValue& request : requests.getValues()) {
391             // This throws an error if "timestamp" doesn't exist
392-            const int64_t timestamp = std::max(GetImportTimestamp(request, now), minimum_timestamp);
393+            auto request_timestamp = GetImportTimestamp(request, now).value_or(-1);

achow101 commented at 11:56 pm on December 23, 2025:

In 72e22c3e9bb034591222aee8247c15f1eeeb199a “Implement rescan stop with timestamp as never for import descriptors”

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.

rkrux commented at 12:45 pm on December 29, 2025:

Yes, agree with this comment.

 0diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
 1index 617e0e8879..4cbc6cc8d6 100644
 2--- a/src/wallet/rpc/backup.cpp
 3+++ b/src/wallet/rpc/backup.cpp
 4@@ -389,9 +389,9 @@ RPCHelpMan importdescriptors()
 5         // Get all timestamps and extract the lowest timestamp
 6         for (const UniValue& request : requests.getValues()) {
 7             // This throws an error if "timestamp" doesn't exist
 8-            auto request_timestamp = GetImportTimestamp(request, now).value_or(-1);
 9+            auto request_timestamp = GetImportTimestamp(request, now);
10             // If request_timestamp is never then timestamp will be now
11-            const int64_t timestamp = request_timestamp < 0 ? now : request_timestamp;
12+            const int64_t timestamp = request_timestamp ? *request_timestamp : now;
13             const UniValue result = ProcessDescriptorImport(*pwallet, request, timestamp);
14             response.push_back(result);

furszy commented at 5:21 pm on January 3, 2026:

this is still using the -1 as magic number.

saikiran57 commented at 4:52 am on January 4, 2026:

hi @furszy i’m not using -1 anymore can you please check latest commits.

in test/functional/wallet_importdescriptors.py:82 in 625bbd4964 outdated

77+        spare_pkh_desc = descsum_create("pkh(" + key.pubkey + ")")
78+
79+        wallet_configs = [
80+            {
81+                "name": "never_wallet",
82+                "timestamp": "never",

achow101 commented at 0:13 am on December 24, 2025:

In 72e22c3e9bb034591222aee8247c15f1eeeb199a “Implement rescan stop with timestamp as never for import descriptors”

This field is never used.

rkrux commented at 12:33 pm on December 29, 2025:

Yes, it’s never used. Please remove it.

in test/functional/wallet_importdescriptors.py:122 in 625bbd4964 outdated

117+            # Debug log validation and descriptor import
118+            with node.assert_debug_log(expected_msgs=config["expected_msgs"],
119+                                    unexpected_msgs=config["unexpected_msgs"]):
120+                if config["name"] == "never_now_wallet":
121+                    wallet.importdescriptors(config["import_data"])
122+                else:

achow101 commented at 0:14 am on December 24, 2025:

In 72e22c3e9bb034591222aee8247c15f1eeeb199a “Implement rescan stop with timestamp as never for import descriptors”

Instead of special casing for a specific wallet, test_importdesc can be changed to be also accept lists:

 0--- a/test/functional/wallet_importdescriptors.py
 1+++ b/test/functional/wallet_importdescriptors.py
 2@@ -54,7 +54,9 @@ class ImportDescriptorsTest(BitcoinTestFramework):
 3         if wallet is not None:
 4             wrpc = wallet
 5 
 6-        result = wrpc.importdescriptors([req])
 7+        if not isinstance(req, list):
 8+            req = [req]
 9+        result = wrpc.importdescriptors(req)
10         observed_warnings = []
11         if 'warnings' in result[0]:
12             observed_warnings = result[0]['warnings']

rkrux commented at 12:26 pm on December 29, 2025:

Using this^ suggestion:

0@@ -117,16 +119,14 @@ class ImportDescriptorsTest(BitcoinTestFramework):
1             # Debug log validation and descriptor import
2             with node.assert_debug_log(expected_msgs=config["expected_msgs"],
3                                     unexpected_msgs=config["unexpected_msgs"]):
4-                if config["name"] == "never_now_wallet":
5-                    wallet.importdescriptors(config["import_data"])
6-                else:
7-                    self.test_importdesc(config["import_data"][0], success=True, wallet=wallet)
8+                self.test_importdesc(config["request"], success=True, wallet=wallet)

saikiran57 force-pushed on Dec 26, 2025

in src/wallet/rpc/backup.cpp:135 in 5fbef2178a outdated

129@@ -130,6 +130,9 @@ static std::optional<int64_t> GetImportTimestamp(const UniValue& data, int64_t n
130         const UniValue& timestamp = data["timestamp"];
131         if (timestamp.isNum()) {
132             int64_t required_timestamp = timestamp.getInt<int64_t>();
133+            if (required_timestamp < 0) {
134+                throw JSONRPCError(RPC_TYPE_ERROR, "timestamp should be greater than zero");
135+            }

rkrux commented at 12:31 pm on December 29, 2025:

This can be tested with the following diff:

0@@ -761,6 +761,12 @@ class ImportDescriptorsTest(BitcoinTestFramework):
1                               success=True,
2                               warnings=["Unknown output type, cannot set descriptor to active."])
3 
4+        self.log.info("Test importing a descriptor with negative timestamp")
5+        assert_raises_rpc_error(-3, "timestamp should be greater than zero", w1.importdescriptors, [{"desc": descsum_create("pk(tpubDCJtdt5dgJpdhW4MtaVYDhG4T4tF6jcLR1PxL43q9pq1mxvXgMS9Mzw1HnXG15vxUGQJMMSqCQHMTy3F1eW5VkgVroWzchsPD5BUojrcWs8/*)"),
6+                              "active": True,
7+                              "range": 1,
8+                              "timestamp": -2}])
9+

int64_t required_timestamp = timestamp.getInt<int64_t>();

Although, another way could be to directly parse timestamp as an unsigned integer along the lines of the changes done in #34135 so that this check is not needed at all, but I believe that would increase the diff of this PR a bit. Thus, we can just add a test for this if condition for the time being.

saikiran57 commented at 10:16 am on December 30, 2025:

done

in test/functional/wallet_importdescriptors.py:67 in 3351abafb8 outdated

63@@ -63,6 +64,76 @@ def test_importdesc(self, req, success, error_code=None, error_message=None, war
64             assert_equal(result[0]['error']['code'], error_code)
65             assert_equal(result[0]['error']['message'], error_message)
66 
67+    def test_importdescriptor_never_timestamp(self, node, funding_wallet):

rkrux commented at 12:34 pm on December 29, 2025:

Few nits in this function:

 0diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
 1index a19157ced4..ba051afee2 100755
 2--- a/test/functional/wallet_importdescriptors.py
 3+++ b/test/functional/wallet_importdescriptors.py
 4@@ -64,13 +66,13 @@ class ImportDescriptorsTest(BitcoinTestFramework):
 5             assert_equal(result[0]['error']['code'], error_code)
 6             assert_equal(result[0]['error']['message'], error_message)
 7 
 8-    def test_importdescriptor_never_timestamp(self, node, funding_wallet):
 9+    def test_importdescriptor_never_timestamp(self, *, node, funding_wallet):
10         self.log.info("Test importing descriptors with timestamp as never")
11         assert_greater_than(funding_wallet.getbalance(), 0)
12 
13         key = get_generate_key()
14         funding_amount = 1 # BTC
15-        txid = funding_wallet.send(outputs=[{key.p2wpkh_addr: funding_amount}])["txid"]
16+        funding_tx_id = funding_wallet.send(outputs=[{key.p2wpkh_addr: funding_amount}])["txid"]
17         self.generate(node, 1, sync_fun=self.no_op)
18 
19         wpkh_desc = descsum_create("wpkh(" + key.pubkey + ")")
20@@ -79,30 +81,30 @@ class ImportDescriptorsTest(BitcoinTestFramework):
21         wallet_configs = [
22             {
23                 "name": "never_wallet",
24-                "timestamp": "never",
25+                "request": [{"desc": wpkh_desc, "timestamp": "never"}],
26                 "expected_balance": 0,
27                 "expected_msgs": [],
28                 "unexpected_msgs": ["Rescanning last"],
29-                "import_data": [{"desc": wpkh_desc, "timestamp": "never"}]
30+                "expected_txid": None
31             },
32             {
33                 "name": "now_wallet",
34-                "timestamp": "now",
35+                "request": [{"desc": wpkh_desc, "timestamp": "now"}],
36                 "expected_balance": funding_amount,
37                 "expected_msgs": ["Rescanning last"],
38                 "unexpected_msgs": [],
39-                "import_data": [{"desc": wpkh_desc, "timestamp": "now"}]
40+                "expected_txid": funding_tx_id
41             },
42             {
43                 "name": "never_now_wallet",
44-                "timestamp": None,   # custom case
45+                "request": [
46+                    {"desc": wpkh_desc, "timestamp": "never"},
47+                    {"desc": spare_pkh_desc, "timestamp": "now"},
48+                ],
49                 "expected_balance": funding_amount,
50                 "expected_msgs": ["Rescanning last"],
51                 "unexpected_msgs": [],
52-                "import_data": [
53-                    {"desc": wpkh_desc, "timestamp": "never"},
54-                    {"desc": spare_pkh_desc, "timestamp": "now"},
55-                ]
56+                "expected_txid": funding_tx_id
57             }
58         ]
59 
60@@ -117,16 +119,14 @@ class ImportDescriptorsTest(BitcoinTestFramework):
61-            # Validate resulting balance + transactions
62+            # Validate resulting balance
63             assert_equal(wallet.getbalance(), config["expected_balance"])
64 
65-            if config["expected_balance"] > 0:
66-                assert_equal(wallet.listtransactions()[0]["txid"], txid)
67+            if config["expected_txid"]:
68+                for tx in wallet.listtransactions():
69+                    assert_equal(tx["txid"], config["expected_txid"])
70             else:
71                 assert_equal(wallet.listtransactions(), [])
72 
73@@ -854,7 +860,7 @@ class ImportDescriptorsTest(BitcoinTestFramework):
74             assert_equal(w_multipath.getrawchangeaddress(address_type="bech32"), w_multisplit.getrawchangeaddress(address_type="bech32"))
75         assert_equal(sorted(w_multipath.listdescriptors()["descriptors"], key=lambda x: x["desc"]), sorted(w_multisplit.listdescriptors()["descriptors"], key=lambda x: x["desc"]))
76 
77-        self.test_importdescriptor_never_timestamp(self.nodes[0], w0)
78+        self.test_importdescriptor_never_timestamp(node=self.nodes[0], funding_wallet=w0)
79 
80 if __name__ == '__main__':
81     ImportDescriptorsTest(__file__).main()

saikiran57 commented at 10:28 am on December 30, 2025:

done

in test/functional/wallet_transactiontime_rescan.py:1 in 3351abafb8 outdated

rkrux commented at 12:35 pm on December 29, 2025:

As suggested in this comment as well, please remove all the changes in this file: #31668 (review)

Yes the changes to wallet_transactiontime_rescan.py can be removed as the changes to wallet_importdescriptor.py sufficiently cover those cases.

This should help in reducing the diff, making the review easier.

in src/wallet/rpc/backup.cpp:440 in 3351abafb8 outdated

434                     std::string error_msg{strprintf("Rescan failed for descriptor with timestamp %d. There "
435                             "was an error reading a block from time %d, which is after or within %d seconds "
436                             "of key creation, and could contain transactions pertaining to the desc. As a "
437                             "result, transactions and coins using this desc may not appear in the wallet.",
438-                            GetImportTimestamp(request, now), scanned_time - TIMESTAMP_WINDOW - 1, TIMESTAMP_WINDOW)};
439+                            GetImportTimestamp(request, now).value_or(-1), scanned_time - TIMESTAMP_WINDOW - 1, TIMESTAMP_WINDOW)};

rkrux commented at 12:55 pm on December 29, 2025:

.value_or(-1)

This seems problematic because an internal default value is being used here for user facing messages.

Also, I believe since we are adding the ability to allow skipping rescanning for descriptors, such descriptors need not be considered in the rescan specific errors/checks. This will also avoid using default value of -1 here, which is not intuitive for the reader.

Maybe the following diff? I haven’t spent enough time verifying many scenarios but the wallet_importdescriptors tests do pass.

 0@@ -426,18 +426,26 @@ RPCHelpMan importdescriptors()
 1             for (unsigned int i = 0; i < requests.size(); ++i) {
 2                 const UniValue& request = requests.getValues().at(i);
 3 
 4+                auto request_timestamp = GetImportTimestamp(request, now);
 5+                // Pass through the response of "never" timestamp descriptors
 6+                // that dont' need any scanning.
 7+                if (!request_timestamp) {
 8+                    response.push_back(results.at(i));
 9+                    continue;
10+                }
11+
12                 // If the descriptor timestamp is within the successfully scanned
13                 // range, or if the import result already has an error set, let
14                 // the result stand unmodified. Otherwise replace the result
15                 // with an error message.
16-                if (scanned_time <= GetImportTimestamp(request, now).value_or(-1) || results.at(i).exists("error")) {
17+                if (scanned_time <= *request_timestamp || results.at(i).exists("error")) {
18                     response.push_back(results.at(i));
19                 } else {
20                     std::string error_msg{strprintf("Rescan failed for descriptor with timestamp %d. There "
21                             "was an error reading a block from time %d, which is after or within %d seconds "
22                             "of key creation, and could contain transactions pertaining to the desc. As a "
23                             "result, transactions and coins using this desc may not appear in the wallet.",
24-                            GetImportTimestamp(request, now).value_or(-1), scanned_time - TIMESTAMP_WINDOW - 1, TIMESTAMP_WINDOW)};
25+                            *request_timestamp, scanned_time - TIMESTAMP_WINDOW - 1, TIMESTAMP_WINDOW)};
26                     if (pwallet->chain().havePruned()) {
27                         error_msg += strprintf(" This error could be caused by pruning or data corruption "
28                                 "(see bitcoind log for details) and could be dealt with by downloading and "

saikiran57 commented at 10:29 am on December 30, 2025:

done and thanks for the suggestion

rkrux changes_requested

rkrux commented at 1:05 pm on December 29, 2025: contributor

Code review 5fbef21

Suggested few changes.

saikiran57 force-pushed on Dec 30, 2025

DrahtBot added the label CI failed on Dec 30, 2025

saikiran57 force-pushed on Dec 30, 2025

rkrux commented at 10:39 am on December 30, 2025: contributor

Please squash the third commit as it’s a lint fix, ref: https://github.com/bitcoin/bitcoin/blob/2bcb3f64648ad27d4df10013b5bf0441124f1372/CONTRIBUTING.md?plain=1#L202-L230

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 force-pushed on Dec 30, 2025

DrahtBot removed the label CI failed on Dec 30, 2025

DrahtBot added the label Needs rebase on Jan 3, 2026

Implement rescan stop with timestamp as never for import descriptors f2fa67f5e1

added timestamp check 1733ddfa1f

saikiran57 force-pushed on Jan 3, 2026

DrahtBot removed the label Needs rebase on Jan 3, 2026

Added rescan option for import descriptors #31668

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)