rpc, wallet: Scan mempool after import*

promag commented at 10:43 pm on May 12, 2020: member

Mempool transactions are ignored on import* RPC. This results in unexpected results if the mempool contains transactions relevant for the imported material.

Fixes #18954.

promag commented at 10:46 pm on May 12, 2020: member

Q1 - should this be optional? 🤔 Q2 - should include all import calls here? (draft because of this) @ryanofsky FYI currently using requestMempoolTransactions.

fanquake added the label Wallet on May 12, 2020

rpc, wallet: Scan mempool after importaddress f327ae7ee5

rpc, wallet: Scan mempool after importprivkey cb652c5b7e

promag force-pushed on May 12, 2020

in src/wallet/rpcdump.cpp:189 in cb652c5b7e

184@@ -185,6 +185,8 @@ UniValue importprivkey(const JSONRPCRequest& request)
185                 pwallet->ImportScripts({GetScriptForDestination(WitnessV0KeyHash(vchAddress))}, 0 /* timestamp */);
186             }
187         }
188+        // Scan mempool for transactions
189+        pwallet->chain().requestMempoolTransactions(*pwallet);

MarcoFalke commented at 12:38 pm on May 13, 2020:

Why is this done even when rescan is turned off? I think this should only be run when rescan is on.

If the user turns off rescan, they know that

either no transaction has happened in the past, so rescan is not needed
or they do a full rescan later

This also means that this should be added to the rescan RPC.

promag commented at 2:26 pm on May 13, 2020:

This also means that this should be added to the rescan RPC.

Might be the best approach, it would cover any import that rescans and also manual rescan. So in practice ScanForWalletTransactions where stop_block.IsNull would scan mempool too?

promag commented at 5:12 pm on September 6, 2020:

So in practice ScanForWalletTransactions where stop_block.IsNull would scan mempool too? @MarcoFalke is this your suggestion?

MarcoFalke commented at 12:39 pm on May 13, 2020: member

Concept ACK.

From reading the code, the first commit can never pass tests? Maybe squash?

promag commented at 2:00 pm on May 13, 2020: member

Concept ACK.

From reading the code, the first commit can never pass tests?

Why?

MarcoFalke commented at 2:26 pm on May 13, 2020: member

I assumed that the check after importprivkey might fail because of the missing rescan, but that might not be the case, as the wallet already knows the tx is in the mempool.

promag commented at 2:27 pm on May 13, 2020: member

I assumed that the check after importprivkey might fail because of the missing rescan, but that might not be the case, as the wallet already knows the tx is in the mempool.

Correct, that’s why the 2nd commit tests importprivkey with a fresh wallet.

ryanofsky commented at 2:50 pm on May 13, 2020: member

This looks very good, and should be extended to work with other import rpcs (also maybe in the future sethdseed, found I needed a rescan recently there too).

I would probably not make this conditioned on the rescan option, just because as a user I can’t think of any real world case where I’d want to import a key and not have the wallet detect mempool transactions using that key. This is different from the very real rescan=false use-case, which is to avoid rescanning blocks because rescanning blocks is ridiculously slow. If in the future someone thinks of a use-case for importing keys without scanning the mempool, we could always extend the rescan option

MarcoFalke commented at 2:56 pm on May 13, 2020: member

because rescanning blocks is ridiculously slow

The mempool can have 100s of MBs of transactions. I haven’t crunched the numbers, but we should make sure that import RPCs don’t get slow either.

promag commented at 3:00 pm on May 13, 2020: member

Right, and it would be even worse with big wallets.

MarcoFalke added this to the milestone 0.21.0 on May 13, 2020

MarcoFalke commented at 11:47 am on May 14, 2020: member

Suggested doc fixup:

 0diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
 1index 7bf3d169c3..4d1ba7ebf5 100644
 2--- a/src/wallet/rpcdump.cpp
 3+++ b/src/wallet/rpcdump.cpp
 4@@ -103,11 +103,13 @@ UniValue importprivkey(const JSONRPCRequest& request)
 5                 "Hint: use importmulti to import more than one private key.\n"
 6             "\nNote: This call can take over an hour to complete if rescan is true, during that time, other rpc calls\n"
 7             "may report that the imported key exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.\n"
 8+            "The rescan parameter can be set to false if the key was never used to create transactions. If it is set to false,\n"
 9+            "but the key was used to create transactions, rescanwallet needs to be called with the appropriate block range.\n"
10             "Note: Use \"getwalletinfo\" to query the scanning progress.\n",
11                 {
12                     {"privkey", RPCArg::Type::STR, RPCArg::Optional::NO, "The private key (see dumpprivkey)"},
13                     {"label", RPCArg::Type::STR, /* default */ "current label if address exists, otherwise \"\"", "An optional label"},
14-                    {"rescan", RPCArg::Type::BOOL, /* default */ "true", "Rescan the wallet for transactions"},
15+                    {"rescan", RPCArg::Type::BOOL, /* default */ "true", "Scan the chain and mempool for wallet transactions."},
16                 },
17                 RPCResult{RPCResult::Type::NONE, "", ""},
18                 RPCExamples{

fjahr commented at 11:32 am on May 25, 2020: member

Concept ACK

I don’t have a good feeling for the performance of a large mempool rescan but I do believe it will have a considerable impact for nodes with large, full mempools. So I agree it should be added to the rescan RPC and it should not be run for the import* RPCs in case of rescan=false.

laanwj removed this from the milestone 0.21.0 on Oct 8, 2020

laanwj added this to the milestone 0.22.0 on Oct 8, 2020

laanwj removed this from the milestone 22.0 on Jun 10, 2021

DrahtBot added the label Needs rebase on Dec 13, 2021

DrahtBot commented at 11:19 pm on December 13, 2021: member

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

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot commented at 1:07 pm on March 21, 2022: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

MarcoFalke added the label Up for grabs on Mar 21, 2022

fanquake commented at 12:29 pm on April 26, 2022: member

@fjahr or @ryanofsky you might be interested in picking this up?

fanquake closed this on Apr 26, 2022

fjahr commented at 1:27 pm on May 1, 2022: member

@fjahr or @ryanofsky you might be interested in picking this up?

I am working on this now, Up for grabs label can be removed

fanquake removed the label Up for grabs on May 1, 2022

fanquake removed the label Needs rebase on May 1, 2022

achow101 referenced this in commit 4aaa3b5200 on Jul 18, 2022

sidhujag referenced this in commit 51d87829d7 on Jul 18, 2022

DrahtBot locked this on May 1, 2023

rpc, wallet: Scan mempool after import* #18964