Skip to content

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

Merged
achow101 merged 7 commits into
bitcoin:masterfrom
fjahr:202204-import-scan
Jul 18, 2022
Merged

rpc, wallet: Scan mempool after import* - Second attempt#25351
achow101 merged 7 commits into
bitcoin:masterfrom
fjahr:202204-import-scan

Conversation

@fjahr

@fjahr fjahr commented Jun 12, 2022 •
edited
Loading

Copy link
Copy Markdown
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

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

achow101 commented Jun 14, 2022

Copy link
Copy Markdown
Member

ACK abdaa251610569a346af55a1121c8ab385efb655

maflcko
maflcko reviewed Jun 15, 2022
View reviewed changes
Comment thread src/wallet/rpc/backup.cpp Outdated
@achow101 achow101 added this to the 24.0 milestone Jun 17, 2022
@DrahtBot

DrahtBot commented Jun 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Conflicts

No conflicts as of last run.

@achow101 achow101 mentioned this pull request Jun 22, 2022
Closed
Sjors
Sjors reviewed Jun 23, 2022
View reviewed changes

@Sjors Sjors left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/wallet/test/wallet_tests.cpp Outdated

@Sjors Sjors Jun 23, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 w0xlt Jun 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@fjahr fjahr Jun 26, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 luke-jr Jun 26, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO bumping the numbers was better. :/

@fjahr fjahr Jul 3, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 luke-jr Jul 6, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Sjors Jul 7, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sjors
Sjors reviewed Jun 23, 2022
View reviewed changes
Comment thread src/wallet/wallet.cpp Outdated
@w0xlt

w0xlt commented Jun 23, 2022

Copy link
Copy Markdown
Contributor

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

@Sjors

Sjors commented Jun 23, 2022

Copy link
Copy Markdown
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?

w0xlt
w0xlt approved these changes Jun 23, 2022
View reviewed changes

@w0xlt w0xlt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK abdaa25

Some nits:

Comment thread test/functional/wallet_import_rescan.py Outdated
Comment thread test/functional/wallet_importdescriptors.py Outdated
Comment thread src/wallet/test/wallet_tests.cpp Outdated

@w0xlt w0xlt Jun 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

w0xlt commented Jun 23, 2022

Copy link
Copy Markdown
Contributor

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

@Sjors

Sjors commented Jun 24, 2022

Copy link
Copy Markdown
Member

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

@w0xlt

w0xlt commented Jun 24, 2022

Copy link
Copy Markdown
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
fjahr force-pushed the 202204-import-scan branch from abdaa25 to 8f59d58 Compare June 26, 2022 17:51
@fjahr

fjahr commented Jun 26, 2022

Copy link
Copy Markdown
Contributor Author

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

luke-jr
luke-jr reviewed Jun 26, 2022
View reviewed changes
Comment thread test/functional/wallet_importdescriptors.py Outdated
@Sjors

Sjors commented Jun 28, 2022 •
edited
Loading

Copy link
Copy Markdown
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
Sjors approved these changes Jun 28, 2022
View reviewed changes

@Sjors Sjors left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 8f59d58549ab8a9e7ecf8eb3ed2fb6d18b812a6d

(though I only manually tested with a taproot descriptor)

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

fjahr commented Jul 3, 2022

Copy link
Copy Markdown
Contributor Author

Addressed feedback from @luke-jr

w0xlt
w0xlt approved these changes Jul 4, 2022
View reviewed changes

@w0xlt w0xlt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK 1be7964

@Sjors

Sjors commented Jul 7, 2022

Copy link
Copy Markdown
Member

re-utACK 1be7964 (only a test change)

@achow101

achow101 commented Jul 18, 2022

Copy link
Copy Markdown
Member

ACK 1be7964

@achow101
achow101 merged commit 4aaa3b5 into bitcoin:master Jul 18, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers
@Sjors Sjors Sjors approved these changes
@luke-jr luke-jr luke-jr left review comments
@maflcko maflcko maflcko left review comments
@w0xlt w0xlt w0xlt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ Wallet

Projects

None yet

Milestone

24.0

Development

Successfully merging this pull request may close these issues.

import* wallet RPCs don't "rescan" the mempool

8 participants

@fjahr @achow101 @DrahtBot @w0xlt @Sjors @luke-jr @maflcko @promag