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

fjahr · 2022-06-12T15:51:56Z

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

achow101 · 2022-06-14T19:19:51Z

ACK abdaa25

src/wallet/rpc/backup.cpp

DrahtBot · 2022-06-21T00:39:47Z

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

Conflicts

No conflicts as of last run.

Sjors

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.

Sjors · 2022-06-23T08:12:11Z

src/wallet/test/wallet_tests.cpp

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


a43ee48: 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.

a43ee48: 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.

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.

IMO bumping the numbers was better. :/

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 :)

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.

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

src/wallet/wallet.cpp

w0xlt · 2022-06-23T12:17:31Z

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

Sjors · 2022-06-23T13:07:22Z

@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?

w0xlt

Tested ACK abdaa25

Some nits:

test/functional/wallet_import_rescan.py

test/functional/wallet_importdescriptors.py

w0xlt · 2022-06-23T14:09:02Z

src/wallet/test/wallet_tests.cpp

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


a43ee48: 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.

w0xlt · 2022-06-23T18:12:44Z

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

Sjors · 2022-06-24T13:10:02Z

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

w0xlt · 2022-06-24T21:40:53Z

@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 · 2022-06-26T18:00:05Z

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

test/functional/wallet_importdescriptors.py

Sjors · 2022-06-28T19:02:55Z

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

tACK 8f59d58

(though I only manually tested with a taproot descriptor)

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

…llet

fjahr · 2022-07-03T19:08:45Z

Addressed feedback from @luke-jr

w0xlt

reACK 1be7964

Sjors · 2022-07-07T10:29:31Z

re-utACK 1be7964 (only a test change)

achow101 · 2022-07-18T18:14:45Z

ACK 1be7964

… 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 bitcoin#18964 and closes bitcoin#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@1be7964 Tree-SHA512: b62fed5f97c6c242b2af417b41c9696a1f18878483d9e1c9429791f9c05257f57a00540a9a84df23c49faf6a61c3109c22972de81540083f38b506217804fcc5

DrahtBot added RPC/REST/ZMQ Wallet labels Jun 12, 2022

fjahr force-pushed the 202204-import-scan branch 2 times, most recently from 5e4f269 to abdaa25 Compare June 12, 2022 18:41

maflcko reviewed Jun 15, 2022

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

achow101 added this to the 24.0 milestone Jun 17, 2022

achow101 mentioned this pull request Jun 22, 2022

importdescriptors does not scan the mempool #25453

Closed

Sjors reviewed Jun 23, 2022

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

w0xlt approved these changes Jun 23, 2022

View reviewed changes

fjahr force-pushed the 202204-import-scan branch from abdaa25 to 8f59d58 Compare June 26, 2022 17:51

luke-jr reviewed Jun 26, 2022

View reviewed changes

test/functional/wallet_importdescriptors.py Outdated Show resolved Hide resolved

Sjors approved these changes Jun 28, 2022

View reviewed changes

fjahr and others added 7 commits July 3, 2022 21:06

wallet: Rescan mempool for transactions as well

236239b

rpc, wallet: Document and test mempool scan after importaddress

3abdbbb

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

rpc, wallet: Document and test mempool scan after importprivkey

6d3db52

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

rpc, wallet: Document mempool scan after importpubkey

e6d3ef8

rpc, wallet: Document mempool scan after importmulti

0e396d1

rpc, wallet: Document mempool rescan after importdescriptor, importwa…

833ce76

…llet

test, wallet: Add mempool rescan test for import RPCs

1be7964

fjahr force-pushed the 202204-import-scan branch from 8f59d58 to 1be7964 Compare July 3, 2022 19:07

w0xlt approved these changes Jul 4, 2022

View reviewed changes

achow101 merged commit 4aaa3b5 into bitcoin:master Jul 18, 2022

bitcoin locked and limited conversation to collaborators Jul 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fjahr commented Jun 12, 2022 •

edited

Loading

achow101 commented Jun 14, 2022

DrahtBot commented Jun 21, 2022 •

edited

Loading

Sjors left a comment •

edited

Loading

Sjors Jun 23, 2022

w0xlt Jun 23, 2022 •

edited

Loading

fjahr Jun 26, 2022

luke-jr Jun 26, 2022

fjahr Jul 3, 2022

luke-jr Jul 6, 2022

Sjors Jul 7, 2022

w0xlt commented Jun 23, 2022

Sjors commented Jun 23, 2022

w0xlt left a comment

w0xlt Jun 23, 2022 •

edited

Loading

w0xlt commented Jun 23, 2022

Sjors commented Jun 24, 2022

w0xlt commented Jun 24, 2022

fjahr commented Jun 26, 2022

Sjors commented Jun 28, 2022 •

edited

Loading

Sjors left a comment

fjahr commented Jul 3, 2022

w0xlt left a comment

Sjors commented Jul 7, 2022

achow101 commented Jul 18, 2022

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

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

Conversation

fjahr commented Jun 12, 2022 • edited Loading

achow101 commented Jun 14, 2022

DrahtBot commented Jun 21, 2022 • edited Loading

Conflicts

Sjors left a comment • edited Loading

Choose a reason for hiding this comment

Sjors Jun 23, 2022

Choose a reason for hiding this comment

w0xlt Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

fjahr Jun 26, 2022

Choose a reason for hiding this comment

luke-jr Jun 26, 2022

Choose a reason for hiding this comment

fjahr Jul 3, 2022

Choose a reason for hiding this comment

luke-jr Jul 6, 2022

Choose a reason for hiding this comment

Sjors Jul 7, 2022

Choose a reason for hiding this comment

w0xlt commented Jun 23, 2022

Sjors commented Jun 23, 2022

w0xlt left a comment

Choose a reason for hiding this comment

w0xlt Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

w0xlt commented Jun 23, 2022

Sjors commented Jun 24, 2022

w0xlt commented Jun 24, 2022

fjahr commented Jun 26, 2022

Sjors commented Jun 28, 2022 • edited Loading

Sjors left a comment

Choose a reason for hiding this comment

fjahr commented Jul 3, 2022

w0xlt left a comment

Choose a reason for hiding this comment

Sjors commented Jul 7, 2022

achow101 commented Jul 18, 2022

fjahr commented Jun 12, 2022 •

edited

Loading

DrahtBot commented Jun 21, 2022 •

edited

Loading

Sjors left a comment •

edited

Loading

w0xlt Jun 23, 2022 •

edited

Loading

w0xlt Jun 23, 2022 •

edited

Loading

Sjors commented Jun 28, 2022 •

edited

Loading