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

SettingsWallet: Update scan transaction for changes to behavior in monero repo PR 8566 #4051

Merged
merged 4 commits into from
Sep 30, 2023

Conversation

j-berman
Copy link
Contributor

@j-berman j-berman commented Oct 20, 2022

EDIT: also ensures GUI simple mode and bootstrap mode users default to "untrusted" daemon

  1. Update balance after calling scanTransaction in order to reflect correct balance along with wallet2: fix rescanning tx via scan_tx [release] monero#8566
  2. Display clear error when user is connected to an untrusted daemon and requests to scan a tx that is older than their wallet's most recent tx.

Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

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

Patch looks ok, why is this necessary now?

@j-berman
Copy link
Contributor Author

j-berman commented Oct 24, 2022

Assume a user receives tx0 then tx1. Assume the wallet has already scanned both txs.

The user then calls scan_tx(tx0) to re-scan tx0.

First the wallet will remove tx1 and tx0, then re-process tx0 then tx1 in order.

In monero-project/monero#8566, when the wallet re-processes tx1, I chose to ignore the on_money_received callback to maintain a clean UX in the CLI. The callback only fires for the tx provided to scan_tx. So in this case it only fires for tx0.

The GUI updates the displayed balance inside this callback:

monero-gui/main.qml

Lines 770 to 779 in 62878a0

function onWalletMoneyReceived(txId, amount) {
// refresh transaction history here
console.log("Confirmed money found")
// history refresh is handled by walletUpdated
currentWallet.history.refresh(currentWallet.currentSubaddressAccount) // this will refresh model
currentWallet.subaddress.refresh(currentWallet.currentSubaddressAccount)
if(middlePanel.state == "History")
middlePanel.historyView.update();
}

So without this PR here, and with the new PR in the Monero repo, the balance the GUI displays after calling scan_tx(tx0) is the balance after the wallet has processed just tx0, ignoring tx1.

This PR makes sure to update the balance after scan_tx (i.e. after all txs are processed) to reflect the correct balance.

Alternatively I could have re-fired the callback for all txs but that's weird in the CLI, since it will indicate feedback to the user that they received both tx0 and tx1 which is confusing to the user because the user only requested to scan tx0.

This PR wasn't necessary before my Monero repo PR because the balance wouldn't change at all when re-scanning a tx before.

@j-berman j-berman changed the title SettingsWallet: Update balance after scanning transaction SettingsWallet: Update scan transaction for changes to behavior in monero repo PR 8566 Oct 28, 2022
@j-berman
Copy link
Contributor Author

Updated to catch the error when the wallet is connected to an untrusted daemon and the user attempts to scan a tx that is not their most recent tx. The wallet displays

GUI error untrusted daemon
:

console.error("Error: ", currentWallet.errorString);
if (currentWallet.errorString == "The wallet has already seen 1 or more recent transactions than the scanned tx") {
informationPopup.title = qsTr("Error") + translationManager.emptyString;
informationPopup.text = currentWallet.errorString + ".\n\nIn order to rescan the transaction, you can re-sync your wallet by resetting the wallet restore height in the Settings > Info page. Make sure to use a restore height from before your wallet's earliest transaction." + translationManager.emptyString;
Copy link
Collaborator

@selsta selsta Oct 28, 2022

Choose a reason for hiding this comment

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

qsTr() missing

Edit: Or wait, since we don't have translated error strings for the first message it might make sense to keep in in English.

@j-berman
Copy link
Contributor Author

Spotted by @plowsof: the GUI unexpectedly assumes simple mode daemons are trusted. Updated to ensure they do not.

I don't see any issues with this today -- features that would be harmful when connected to an untrusted daemon are already not available in simple mode (rescanning spents, key image sync). However, without this change, and with the change to scan transaction in the monero repo, simple mode users would be left at added risk using the scan transaction feature, which is supposed to error out as in the screenshot above and described here.

@plowsof
Copy link
Contributor

plowsof commented Oct 29, 2022

@selsta advised me on what specific things should be tested. The new changes/fixes work perfectly now,

  • Simple mode users can scan their latest transaction / warning is displayed when scanning older transaction.
  • Advanced mode users can scan an older transaction with a trusted node / see warning if untrusted.

Thank you! my test script

@j-berman
Copy link
Contributor Author

@selsta raised a valid point to me that bootstrap mode wallets don't need to be considered trusted when the user's daemon is fully synced. I don't see a particularly clean way to guarantee this behavior and figure it's safer to default assume the daemon is untrusted for bootstrap mode wallets

@luigi1111 luigi1111 merged commit 588b843 into monero-project:master Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants