Merge bitcoin/bitcoin#25351: rpc, wallet: Scan mempool after import* - Second attempt

1be7964 test, wallet: Add mempool rescan test for import RPCs (Fabian Jahr)
833ce76 rpc, wallet: Document mempool rescan after importdescriptor, importwallet (Fabian Jahr)
0e396d1 rpc, wallet: Document mempool scan after importmulti (Fabian Jahr)
e6d3ef8 rpc, wallet: Document mempool scan after importpubkey (Fabian Jahr)
6d3db52 rpc, wallet: Document and test mempool scan after importprivkey (João Barbosa)
3abdbbb rpc, wallet: Document and test mempool scan after importaddress (João Barbosa)
236239b wallet: Rescan mempool for transactions as well (Fabian Jahr)

Pull request description:

  This PR picks up the work from #18964 and closes #18954.

  It should incorporate all the unaddressed feedback from the PR:
  - Mempool rescan now expanded to all relevant import* RPCs
  - Added documentation in the help of each RPC
  - More tests

ACKs for top commit:
  Sjors:
    re-utACK 1be7964 (only a test change)
  achow101:
    ACK 1be7964
  w0xlt:
    reACK bitcoin/bitcoin@1be7964

Tree-SHA512: b62fed5f97c6c242b2af417b41c9696a1f18878483d9e1c9429791f9c05257f57a00540a9a84df23c49faf6a61c3109c22972de81540083f38b506217804fcc5

Author: achow101

src/wallet/rpc/backup.cpp

@@ -100,11 +100,13 @@ RPCHelpMan importprivkey()
                 "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, RPCArg::DefaultHint{"current label if address exists, otherwise \"\""}, "An optional label"},
-                    {"rescan", RPCArg::Type::BOOL, RPCArg::Default{true}, "Rescan the wallet for transactions"},
+                    {"rescan", RPCArg::Type::BOOL, RPCArg::Default{true}, "Scan the chain and mempool for wallet transactions."},
                 },
                 RPCResult{RPCResult::Type::NONE, "", ""},
                 RPCExamples{
@@ -201,6 +203,8 @@ RPCHelpMan importaddress()
             "\nAdds an address or script (in hex) that can be watched as if it were in your wallet but cannot be used to spend. Requires a new wallet backup.\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 address 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"
             "If you have the full public key, you should call importpubkey instead of this.\n"
             "Hint: use importmulti to import more than one address.\n"
             "\nNote: If you import a non-standard raw script in hex form, outputs sending to it will be treated\n"
@@ -210,7 +214,7 @@ RPCHelpMan importaddress()
                 {
                     {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The Bitcoin address (or hex-encoded script)"},
                     {"label", RPCArg::Type::STR, RPCArg::Default{""}, "An optional label"},
-                    {"rescan", RPCArg::Type::BOOL, RPCArg::Default{true}, "Rescan the wallet for transactions"},
+                    {"rescan", RPCArg::Type::BOOL, RPCArg::Default{true}, "Scan the chain and mempool for wallet transactions."},
                     {"p2sh", RPCArg::Type::BOOL, RPCArg::Default{false}, "Add the P2SH version of the script as well"},
                 },
                 RPCResult{RPCResult::Type::NONE, "", ""},
@@ -401,11 +405,13 @@ RPCHelpMan importpubkey()
                 "Hint: use importmulti to import more than one public 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 pubkey 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",
                 {
                     {"pubkey", RPCArg::Type::STR, RPCArg::Optional::NO, "The hex-encoded public key"},
                     {"label", RPCArg::Type::STR, RPCArg::Default{""}, "An optional label"},
-                    {"rescan", RPCArg::Type::BOOL, RPCArg::Default{true}, "Rescan the wallet for transactions"},
+                    {"rescan", RPCArg::Type::BOOL, RPCArg::Default{true}, "Scan the chain and mempool for wallet transactions."},
                 },
                 RPCResult{RPCResult::Type::NONE, "", ""},
                 RPCExamples{
@@ -484,7 +490,7 @@ RPCHelpMan importwallet()
 {
     return RPCHelpMan{"importwallet",
                 "\nImports keys from a wallet dump file (see dumpwallet). Requires a new wallet backup to include imported keys.\n"
-                "Note: Use \"getwalletinfo\" to query the scanning progress.\n",
+                "Note: Blockchain and Mempool will be rescanned after a successful import. Use \"getwalletinfo\" to query the scanning progress.\n",
                 {
                     {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The wallet file"},
                 },
@@ -1250,6 +1256,8 @@ RPCHelpMan importmulti()
                 "Conversely, if all the private keys are provided and the address/script is spendable, the watchonly option must be set to false, or a warning will be returned.\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 keys, addresses or scripts exist but related transactions are still missing.\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",
                 {
                     {"requests", RPCArg::Type::ARR, RPCArg::Optional::NO, "Data to be imported",
@@ -1291,7 +1299,7 @@ RPCHelpMan importmulti()
                         "\"requests\""},
                     {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
                         {
-                            {"rescan", RPCArg::Type::BOOL, RPCArg::Default{true}, "Stating if should rescan the blockchain after all imports"},
+                            {"rescan", RPCArg::Type::BOOL, RPCArg::Default{true}, "Scan the chain and mempool for wallet transactions after all imports."},
                         },
                         "\"options\""},
                 },
@@ -1593,7 +1601,7 @@ RPCHelpMan importdescriptors()
         "                                                              Use the string \"now\" to substitute the current synced blockchain time.\n"
         "                                                              \"now\" can be specified to bypass scanning, for outputs which are known to never have been used, and\n"
         "                                                              0 can be specified to scan the entire blockchain. Blocks up to 2 hours before the earliest timestamp\n"
-        "                                                              of all descriptors being imported will be scanned.",
+                                        "of all descriptors being imported will be scanned as well as the mempool.",
                                         /*oneline_description=*/"", {"timestamp | \"now\"", "integer / string"}
                                     },
                                     {"internal", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether matching outputs should be treated as not incoming payments (e.g. change)"},

src/wallet/test/wallet_tests.cpp

@@ -53,9 +53,6 @@ static const std::shared_ptr<CWallet> TestLoadWallet(WalletContext& context)
     auto database = MakeWalletDatabase("", options, status, error);
     auto wallet = CWallet::Create(context, "", std::move(database), options.create_flags, error, warnings);
     NotifyWalletLoaded(context, wallet);
-    if (context.chain) {
-        wallet->postInitProcess();
-    }
     return wallet;
 }
 
@@ -768,6 +765,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
     // being blocked
     wallet = TestLoadWallet(context);
     BOOST_CHECK(rescan_completed);
+    // AddToWallet events for block_tx and mempool_tx
     BOOST_CHECK_EQUAL(addtx_count, 2);
     {
         LOCK(wallet->cs_wallet);
@@ -780,6 +778,8 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
     // transactionAddedToMempool events are processed
     promise.set_value();
     SyncWithValidationInterfaceQueue();
+    // AddToWallet events for block_tx and mempool_tx events are counted a
+    // second time as the notificaiton queue is processed
     BOOST_CHECK_EQUAL(addtx_count, 4);
 
 
@@ -803,7 +803,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
             SyncWithValidationInterfaceQueue();
         });
     wallet = TestLoadWallet(context);
-    BOOST_CHECK_EQUAL(addtx_count, 4);
+    BOOST_CHECK_EQUAL(addtx_count, 2);
     {
         LOCK(wallet->cs_wallet);
         BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_tx.GetHash()), 1U);

src/wallet/wallet.cpp

@@ -1678,7 +1678,8 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
 /**
  * Scan the block chain (starting in start_block) for transactions
  * from or to us. If fUpdate is true, found transactions that already
- * exist in the wallet will be updated.
+ * exist in the wallet will be updated. If max_height is not set, the
+ * mempool will be scanned as well.
  *
  * @param[in] start_block Scan starting block. If block is not on the active
  *                        chain, the scan will return SUCCESS immediately.
@@ -1799,6 +1800,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
             }
         }
     }
+    if (!max_height) {
+        WalletLogPrintf("Scanning current mempool transactions.\n");
+        WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this));
+    }
     ShowProgress(strprintf("%s " + _("Rescanning…").translated, GetDisplayName()), 100); // hide progress dialog in GUI
     if (block_height && fAbortRescan) {
         WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", block_height, progress_current);

test/functional/wallet_balance.py

@@ -273,6 +273,26 @@ def test_balances(*, fee_node_1=0):
         self.generatetoaddress(self.nodes[1], 1, ADDRESS_WATCHONLY)
         assert_equal(self.nodes[0].getbalance(minconf=0), total_amount + 1)  # The reorg recovered our fee of 1 coin
 
+        if not self.options.descriptors:
+            self.log.info('Check if mempool is taken into account after import*')
+            address = self.nodes[0].getnewaddress()
+            privkey = self.nodes[0].dumpprivkey(address)
+            self.nodes[0].sendtoaddress(address, 0.1)
+            self.nodes[0].unloadwallet('')
+            # check importaddress on fresh wallet
+            self.nodes[0].createwallet('w1', False, True)
+            self.nodes[0].importaddress(address)
+            assert_equal(self.nodes[0].getbalances()['mine']['untrusted_pending'], 0)
+            assert_equal(self.nodes[0].getbalances()['watchonly']['untrusted_pending'], Decimal('0.1'))
+            self.nodes[0].importprivkey(privkey)
+            assert_equal(self.nodes[0].getbalances()['mine']['untrusted_pending'], Decimal('0.1'))
+            assert_equal(self.nodes[0].getbalances()['watchonly']['untrusted_pending'], 0)
+            self.nodes[0].unloadwallet('w1')
+            # check importprivkey on fresh wallet
+            self.nodes[0].createwallet('w2', False, True)
+            self.nodes[0].importprivkey(privkey)
+            assert_equal(self.nodes[0].getbalances()['mine']['untrusted_pending'], Decimal('0.1'))
+
 
 if __name__ == '__main__':
     WalletTest().main()

test/functional/wallet_import_rescan.py

@@ -87,6 +87,7 @@ def check(self, txid=None, amount=None, confirmation_height=None):
         assert_equal(len(txs), self.expected_txs)
 
         addresses = self.node.listreceivedbyaddress(minconf=0, include_watchonly=True, address_filter=self.address['address'])
+
         if self.expected_txs:
             assert_equal(len(addresses[0]["txids"]), self.expected_txs)
 
@@ -98,13 +99,18 @@ def check(self, txid=None, amount=None, confirmation_height=None):
             assert_equal(tx["category"], "receive")
             assert_equal(tx["label"], self.label)
             assert_equal(tx["txid"], txid)
-            assert_equal(tx["confirmations"], 1 + current_height - confirmation_height)
-            assert "trusted" not in tx
+
+            # If no confirmation height is given, the tx is still in the
+            # mempool.
+            confirmations = (1 + current_height - confirmation_height) if confirmation_height else 0
+            assert_equal(tx["confirmations"], confirmations)
+            if confirmations:
+                assert "trusted" not in tx
 
             address, = [ad for ad in addresses if txid in ad["txids"]]
             assert_equal(address["address"], self.address["address"])
             assert_equal(address["amount"], self.expected_balance)
-            assert_equal(address["confirmations"], 1 + current_height - confirmation_height)
+            assert_equal(address["confirmations"], confirmations)
             # Verify the transaction is correctly marked watchonly depending on
             # whether the transaction pays to an imported public key or
             # imported private key. The test setup ensures that transaction
@@ -162,11 +168,12 @@ def setup_network(self):
         self.import_deterministic_coinbase_privkeys()
         self.stop_nodes()
 
-        self.start_nodes()
+        self.start_nodes(extra_args=[["[email protected]"]] * self.num_nodes)
         for i in range(1, self.num_nodes):
             self.connect_nodes(i, 0)
 
     def run_test(self):
+
         # Create one transaction on node 0 with a unique amount for
         # each possible type of wallet import RPC.
         for i, variant in enumerate(IMPORT_VARIANTS):
@@ -207,7 +214,7 @@ def run_test(self):
                 variant.check()
 
         # Create new transactions sending to each address.
-        for i, variant in enumerate(IMPORT_VARIANTS):
+        for variant in IMPORT_VARIANTS:
             variant.sent_amount = get_rand_amount()
             variant.sent_txid = self.nodes[0].sendtoaddress(variant.address["address"], variant.sent_amount)
             self.generate(self.nodes[0], 1)  # Generate one block for each send
@@ -223,6 +230,46 @@ def run_test(self):
             variant.expected_txs += 1
             variant.check(variant.sent_txid, variant.sent_amount, variant.confirmation_height)
 
+        self.log.info('Test that the mempool is rescanned as well if the rescan parameter is set to true')
+
+        # The late timestamp and pruned variants are not necessary when testing mempool rescan
+        mempool_variants = [variant for variant in IMPORT_VARIANTS if variant.rescan != Rescan.late_timestamp and not variant.prune]
+        # No further blocks are mined so the timestamp will stay the same
+        timestamp = self.nodes[0].getblockheader(self.nodes[0].getbestblockhash())["time"]
+
+        # Create one transaction on node 0 with a unique amount for
+        # each possible type of wallet import RPC.
+        for i, variant in enumerate(mempool_variants):
+            variant.label = "mempool label {} {}".format(i, variant)
+            variant.address = self.nodes[1].getaddressinfo(self.nodes[1].getnewaddress(
+                label=variant.label,
+                address_type=variant.address_type.value,
+            ))
+            variant.key = self.nodes[1].dumpprivkey(variant.address["address"])
+            variant.initial_amount = get_rand_amount()
+            variant.initial_txid = self.nodes[0].sendtoaddress(variant.address["address"], variant.initial_amount)
+            variant.confirmation_height = 0
+            variant.timestamp = timestamp
+
+        assert_equal(len(self.nodes[0].getrawmempool()), len(mempool_variants))
+        self.sync_mempools()
+
+        # For each variation of wallet key import, invoke the import RPC and
+        # check the results from getbalance and listtransactions.
+        for variant in mempool_variants:
+            self.log.info('Run import for mempool variant {}'.format(variant))
+            expect_rescan = variant.rescan == Rescan.yes
+            variant.node = self.nodes[2 + IMPORT_NODES.index(ImportNode(variant.prune, expect_rescan))]
+            variant.do_import(variant.timestamp)
+            if expect_rescan:
+                variant.expected_balance = variant.initial_amount
+                variant.expected_txs = 1
+                variant.check(variant.initial_txid, variant.initial_amount)
+            else:
+                variant.expected_balance = 0
+                variant.expected_txs = 0
+                variant.check()
+
 
 if __name__ == "__main__":
     ImportRescanTest().main()

test/functional/wallet_importdescriptors.py

@@ -480,7 +480,9 @@ def run_test(self):
         addr = wmulti_pub.getnewaddress('', 'bech32')
         assert_equal(addr, 'bcrt1qp8s25ckjl7gr6x2q3dx3tn2pytwp05upkjztk6ey857tt50r5aeqn6mvr9') # Derived at m/84'/0'/0'/1
         change_addr = wmulti_pub.getrawchangeaddress('bech32')
-        assert_equal(change_addr, 'bcrt1qt9uhe3a9hnq7vajl7a094z4s3crm9ttf8zw3f5v9gr2nyd7e3lnsy44n8e')
+        assert_equal(change_addr, 'bcrt1qzxl0qz2t88kljdnkzg4n4gapr6kte26390gttrg79x66nt4p04fssj53nl')
+        assert(send_txid in self.nodes[0].getrawmempool(True))
+        assert(send_txid in (x['txid'] for x in wmulti_pub.listunspent(0)))
         assert_equal(wmulti_pub.getwalletinfo()['keypoolsize'], 999)
 
         # generate some utxos for next tests