rpc, wallet: Scan mempool after import*

fjahr commented at 3:51 PM on June 12, 2022: contributor

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

DrahtBot added the label RPC/REST/ZMQ on Jun 12, 2022

DrahtBot added the label Wallet on Jun 12, 2022

fjahr force-pushed on Jun 12, 2022

achow101 commented at 7:19 PM on June 14, 2022: member

ACK abdaa251610569a346af55a1121c8ab385efb655

in src/wallet/rpc/backup.cpp:1603 in abdaa25161 outdated

1599 | @@ -1592,7 +1600,7 @@ RPCHelpMan importdescriptors()
1600 |          "                                                              Use the string \"now\" to substitute the current synced blockchain time.\n"
1601 |          "                                                              \"now\" can be specified to bypass scanning, for outputs which are known to never have been used, and\n"
1602 |          "                                                              0 can be specified to scan the entire blockchain. Blocks up to 2 hours before the earliest timestamp\n"
1603 | -        "                                                              of all descriptors being imported will be scanned.",
1604 | +        "                                                              of all descriptors being imported will be scanned as well as the mempool.",

MarcoFalke commented at 9:36 AM on June 15, 2022:

        "of all descriptors being imported will be scanned as well as the mempool.",

nit: The whitespace isn't needed and can be removed while touching this line. (If you need to retouch later)

fjahr commented at 5:53 PM on June 26, 2022:

done

achow101 added this to the milestone 24.0 on Jun 17, 2022

DrahtBot commented at 12:39 AM on June 21, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

in src/wallet/test/wallet_tests.cpp:768 in a43ee48e8e outdated

 764 | @@ -765,7 +765,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
 765 |      // being blocked
 766 |      wallet = TestLoadWallet(context);
 767 |      BOOST_CHECK(rescan_completed);
 768 | -    BOOST_CHECK_EQUAL(addtx_count, 2);
 769 | +    BOOST_CHECK_EQUAL(addtx_count, 3);

Sjors commented at 8:12 AM on June 23, 2022:

a43ee48e8e904764fc12f58f40e569c7f2208876: maybe add a comment to explain what those 3 transactions are: 2 m_coinbase_txns entries and mempool_tx?

I'm confused why it's 3 here and 5 below.

w0xlt commented at 2:09 PM on June 23, 2022:

https://github.com/bitcoin/bitcoin/commit/a43ee48e8e904764fc12f58f40e569c7f2208876: maybe add a comment to explain what those 3 transactions are: 2 m_coinbase_txns entries and mempool_tx? I'm confused why it's 3 here and 5 below.

Agreed.

fjahr commented at 5:59 PM on June 26, 2022:

Yeah, I see what you mean. The TestLoadWallet() helper function had a call to postInitProcess() which meant that the mempool was checked twice. Removing that led to the numbers in the CreateWallet test being much more sensible and there seem to be no other negative effects from that change. At least the test isn't failing and I also didn't see any other reason to keep it. I added some comments as well.

luke-jr commented at 9:45 PM on June 26, 2022:

IMO bumping the numbers was better. :/

fjahr commented at 6:47 PM on July 3, 2022:

IMO bumping the numbers was better. :/

Could you say why you like it better? When the numbers are bumped I feel they don't match what the test is doing and I would need to add more comments to explain why this is the case. Hence it seemed like the better choice to me. Also curious to hear your feedback here @w0xlt and @Sjors :)

luke-jr commented at 3:01 AM on July 6, 2022:

Seems like it would be undefined behaviour (between modules) to skip postInitProcess.

Counting AddToWallet calls might be too low-level to be a good test, but that seems out of scope here IMO.

Sjors commented at 10:30 AM on July 7, 2022:

I have no strong feelings about this, as long as it's clear from the code documentation where the transactions are coming from.

Sjors commented at 9:00 AM on June 23, 2022: member

I'm unable to get this to work with descriptor wallets; still seeing the issue I reported in #25453.

Step 1: create a regular descriptor wallet, put some coins in it (I used a Taproot address for this) Step 2: get ~one of its~ the matching receive descriptor ~s~ via listdescriptors Step 3: create a watch-only wallet Step 4: send coins to self, and before it confirms do: Step 5: call importdescriptors on the watch-only wallet, using the above descriptor and some recent timestamp.

It will not add the mempool transaction until you either restart or it confirms.

rescanblockchain also doesn't add the transaction, but I suppose that's out of scope here.

Update: Note that you DO see the confirmed deposit transaction, but you don't see the outgoing mempool transaction.

in src/wallet/wallet.cpp:1802 in abdaa25161 outdated

1797 | @@ -1797,6 +1798,10 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1798 |              }
1799 |          }
1800 |      }
1801 | +    if (!max_height) {
1802 | +        WalletLogPrintf("Scanning current mempool transactions.\n");

Sjors commented at 9:01 AM on June 23, 2022:

I don't see this notice in the log either.

fjahr commented at 5:52 PM on June 26, 2022:

Strange, that may be a hint that the build didn't have this change? But I will go through your steps as well as an additional check.

Sjors commented at 7:09 PM on June 28, 2022:

Indeed it turns out I was running the wrong build (again). Let me try again. At least I know for sure the bug exists on master 506d9b25a3f8ef8a4ce63f989083803d8d4ae6df :-)

w0xlt commented at 12:17 PM on June 23, 2022: contributor

@Sjors I was able to retrieve the mempool transactions by following these steps on regtest and signet.

in test/functional/wallet_import_rescan.py:254 in abdaa25161 outdated

 249 | +            variant.initial_amount = get_rand_amount()
 250 | +            variant.initial_txid = self.nodes[0].sendtoaddress(variant.address["address"], variant.initial_amount)
 251 | +            variant.confirmation_height = 0
 252 | +            variant.timestamp = timestamp
 253 | +
 254 | +        assert_equal(len(self.nodes[0].getrawmempool()), 54)

w0xlt commented at 1:03 PM on June 23, 2022:

nit: this makes it explicit that it is one transaction per variant

        assert_equal(len(self.nodes[0].getrawmempool()), len(mempool_variants))

fjahr commented at 5:53 PM on June 26, 2022:

done

Sjors commented at 1:07 PM on June 23, 2022: member

@Sjors I was able to retrieve the mempool transactions by following these steps on regtest and signet.

Mmm, and without this PR you're able to reproduce the original issue?

in test/functional/wallet_importdescriptors.py:483 in abdaa25161 outdated

 479 | @@ -480,7 +480,7 @@ def run_test(self):
 480 |          addr = wmulti_pub.getnewaddress('', 'bech32')
 481 |          assert_equal(addr, 'bcrt1qp8s25ckjl7gr6x2q3dx3tn2pytwp05upkjztk6ey857tt50r5aeqn6mvr9') # Derived at m/84'/0'/0'/1
 482 |          change_addr = wmulti_pub.getrawchangeaddress('bech32')
 483 | -        assert_equal(change_addr, 'bcrt1qt9uhe3a9hnq7vajl7a094z4s3crm9ttf8zw3f5v9gr2nyd7e3lnsy44n8e')
 484 | +        assert_equal(change_addr, 'bcrt1qzxl0qz2t88kljdnkzg4n4gapr6kte26390gttrg79x66nt4p04fssj53nl')

w0xlt commented at 2:08 PM on June 23, 2022:

Perhaps the assertions below can be added. They test that importdescriptors retrieved the mempool transaction.

Not related to this PR, but transaction send_txid is confirmed in line 457, isn't it ? Why is it still in the mempool in line 463 and 470?

        assert_equal(change_addr, 'bcrt1qzxl0qz2t88kljdnkzg4n4gapr6kte26390gttrg79x66nt4p04fssj53nl')
        assert(any([x == send_txid for x in self.nodes[0].getrawmempool(True)]))
        assert(any([x['txid'] == send_txid for x in wmulti_pub.listunspent(0)]))

fjahr commented at 5:52 PM on June 26, 2022:

Added

Not related to this PR, but transaction send_txid is confirmed in line 457, isn't it ? Why is it still in the mempool in line 463 and 470?

Hm, you are right that this shouldn't really work but it still does due to a race between the two nodes involved. I am kind of surprised that this doesn't lead flakiness in the CI but I couldn't find any related issue. I addressed this in #25476.

w0xlt approved

w0xlt commented at 2:09 PM on June 23, 2022: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/25351/commits/abdaa251610569a346af55a1121c8ab385efb655

Some nits:

w0xlt commented at 6:12 PM on June 23, 2022: contributor

@Sjors Yes, without this PR (using the master branch), I am able to reproduce the issue.

Sjors commented at 1:10 PM on June 24, 2022: member

@Sjors and your second wallet was a watch-only wallet? Guess I'll have to retry to check my own sanity :-)

w0xlt commented at 9:40 PM on June 24, 2022: contributor

@Sjors the watch-only was created with disable_private_keys=true blank=true and the tr(xpub...) descriptors (internal and external) were imported from the first wallet.

fjahr force-pushed on Jun 26, 2022

fjahr commented at 6:00 PM on June 26, 2022: contributor

Addressed feedback but have yet to go through @Sjors steps to see if I can reproduce his issue.

in test/functional/wallet_importdescriptors.py:485 in 8f59d58549 outdated

 479 | @@ -480,7 +480,9 @@ def run_test(self):
 480 |          addr = wmulti_pub.getnewaddress('', 'bech32')
 481 |          assert_equal(addr, 'bcrt1qp8s25ckjl7gr6x2q3dx3tn2pytwp05upkjztk6ey857tt50r5aeqn6mvr9') # Derived at m/84'/0'/0'/1
 482 |          change_addr = wmulti_pub.getrawchangeaddress('bech32')
 483 | -        assert_equal(change_addr, 'bcrt1qt9uhe3a9hnq7vajl7a094z4s3crm9ttf8zw3f5v9gr2nyd7e3lnsy44n8e')
 484 | +        assert_equal(change_addr, 'bcrt1qzxl0qz2t88kljdnkzg4n4gapr6kte26390gttrg79x66nt4p04fssj53nl')
 485 | +        assert(any([x == send_txid for x in self.nodes[0].getrawmempool(True)]))
 486 | +        assert(any([x['txid'] == send_txid for x in wmulti_pub.listunspent(0)]))

luke-jr commented at 9:51 PM on June 26, 2022:

        assert(any(x == send_txid for x in self.nodes[0].getrawmempool(True)))
        assert(any(x['txid'] == send_txid for x in wmulti_pub.listunspent(0)))

No benefit to making it a list - just causes every element to be compared (using the generator directly stops comparing as soon as it finds a match)

Or even better and clearer...

        assert(send_txid in self.nodes[0].getrawmempool(True))
        assert(send_txid in (x['txid'] for x in wmulti_pub.listunspent(0)))

fjahr commented at 7:07 PM on July 3, 2022:

done

Sjors commented at 7:02 PM on June 28, 2022: member

I redid the experiment and clarified the procedure a bit. ~It might be taproot-specific.~

Oops, I managed to run an earlier build of QT, because I reconfigred using --without-gui at some point. Will retry. Update: works!

Sjors approved

Sjors commented at 7:40 PM on June 28, 2022: member

tACK 8f59d58549ab8a9e7ecf8eb3ed2fb6d18b812a6d

(though I only manually tested with a taproot descriptor)

wallet: Rescan mempool for transactions as well 236239bd40

rpc, wallet: Document and test mempool scan after importaddress

co-authored-by: Fabian Jahr <fjahr@protonmail.com>

3abdbbb90a

rpc, wallet: Document and test mempool scan after importprivkey

co-authored-by: Fabian Jahr <fjahr@protonmail.com>

6d3db52e66

rpc, wallet: Document mempool scan after importpubkey e6d3ef8586

rpc, wallet: Document mempool scan after importmulti 0e396d1ba7

rpc, wallet: Document mempool rescan after importdescriptor, importwallet 833ce76df7

test, wallet: Add mempool rescan test for import RPCs 1be7964189

fjahr force-pushed on Jul 3, 2022

fjahr commented at 7:08 PM on July 3, 2022: contributor

Addressed feedback from @luke-jr

w0xlt approved

w0xlt commented at 2:41 AM on July 4, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/pull/25351/commits/1be796418934ae7370cb0ed501877db59e738106

Sjors commented at 10:29 AM on July 7, 2022: member

re-utACK 1be796418934ae7370cb0ed501877db59e738106 (only a test change)

achow101 commented at 6:14 PM on July 18, 2022: member

ACK 1be796418934ae7370cb0ed501877db59e738106

achow101 merged this on Jul 18, 2022

achow101 closed this on Jul 18, 2022

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

bitcoin locked this on Jul 26, 2023

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

Conflicts