rpc, wallet: Scan mempool after import*

[promag]

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]

Q1 - should this be optional? 🤔
Q2 - should include all import calls here? (draft because of this)

@ryanofsky FYI currently using requestMempoolTransactions.

[maflcko]

Concept ACK.

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

[maflcko]

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]

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]

So in practice ScanForWalletTransactions where stop_block.IsNull would scan mempool too?

@MarcoFalke is this your suggestion?

[promag]

Concept ACK.

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

Why?

[maflcko]

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]

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]

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

[maflcko]

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]

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

[maflcko]

Suggested doc fixup:

diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index 7bf3d169c3..4d1ba7ebf5 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -103,11 +103,13 @@ UniValue importprivkey(const JSONRPCRequest& request)
                 "Hint: use importmulti to import more than one private key.\n"
             "\nNote: This call can take over an hour to complete if rescan is true, during that time, other rpc calls\n"
             "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"
+            "The rescan parameter can be set to false if the key was never used to create transactions. If it is set to false,\n"
+            "but the key was used to create transactions, rescanwallet needs to be called with the appropriate block range.\n"
             "Note: Use \"getwalletinfo\" to query the scanning progress.\n",
                 {
                     {"privkey", RPCArg::Type::STR, RPCArg::Optional::NO, "The private key (see dumpprivkey)"},
                     {"label", RPCArg::Type::STR, /* default */ "current label if address exists, otherwise \"\"", "An optional label"},
-                    {"rescan", RPCArg::Type::BOOL, /* default */ "true", "Rescan the wallet for transactions"},
+                    {"rescan", RPCArg::Type::BOOL, /* default */ "true", "Scan the chain and mempool for wallet transactions."},
                 },
                 RPCResult{RPCResult::Type::NONE, "", ""},
                 RPCExamples{

[fjahr]

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.

[DrahtBot]

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

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

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

[fanquake]

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

[fjahr]

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

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