Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet2: remove refresh() from scan_tx #9389

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

j-berman
Copy link
Collaborator

Fixes #9354

The problem

In #8566, I introduced a call to refresh() inside scan_tx so that when scan_tx executes to completion, wallet data would reflect the latest chain state (e.g. the number of confirmations on the tx/tx's locked status would be up to date). I also introduced a new variable m_skip_to_height to skip the wallet's refresh height to the tx's height if it hasn't synced to that height yet. The logic for m_skip_to_height mirrors the logic when a user includes a restore height when restoring the wallet.

When refresh "skips" to m_skip_to_height (or a user restores a wallet at a provided restore height), the wallet downloads every block hash from the chain even from before m_skip_to_height (or provided restore height), which is (suspiciously) slow.

Thus, when restoring a wallet and then calling scan_tx with a remote daemon, scan_tx is slow to complete.

The solution

This PR includes a simple fix to remove the call to refresh() inside scan_tx.

The other additional changes are there to ensure when scan_tx is called twice when the wallet is not yet synced to the height of txs scanned, those txs are reprocessed correctly (necessary change for this PR because scan_tx doesn't call refresh() anymore).

Warning: when calling scan_tx when the wallet is not synced, the num confirmation data and locked status on scanned txs will not reflect the latest chain state with this change. A wallet must call refresh after scan_tx to get the latest state. Pinging @woodser on this since this reverts to behavior noted here.

Misc. comment

@woodser suggested here to use the daemon's state to calculate things like num confirmations/locked status on a wallet's outputs, rather than use the wallet's latest sync state.

There are quite a few places where the wallet currently uses wallet state instead of daemon state. So it's a bit involved to decide exactly where daemon state should be used that won't break current wallet UX.

I'm proposing this PR as a simple fix for #9354 to improve current behavior.

@selsta
Copy link
Collaborator

selsta commented Jun 29, 2024

Are there any changes necessary to monero-gui?

@j-berman
Copy link
Collaborator Author

j-berman commented Jun 29, 2024

Nope

EDIT: double checking

@j-berman
Copy link
Collaborator Author

Fixed a one line issue in the test.

Are there any changes necessary to monero-gui?

When you restore a wallet in the GUI with the latest restore height, you have to wait until it downloads block hashes before you can scan a tx anyway (source). scan_tx should be close to instantaneous after that point in the GUI today, and so the issue this PR is improving shouldn't affect GUI users AFAICT.

FWIW downloading block hashes is slow and unnecessary in this case, and is worth a change, but it's not a necessary change for this PR.

I also just tested restore at current height + scan a tx in the GUI using this PR and it behaved the same as without this PR, with no changes to the GUI code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scan_tx stucks on newer versions
3 participants