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

Feature: sync in the background with just view key when locked #4050

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

j-berman
Copy link
Contributor

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

Pairs with monero-project/monero#8619

  • In a wallet's settings, a user can check "Sync in the background when locked". This setting is not supported for view only or HW wallets.
  • When checked and the wallet is locked, the password dialog indicates "Syncing in the background...". The wallet wipes the spend key from memory and continues syncing without it.
  • NOTE: without this PR, today's GUI ALREADY syncs in the background when locked. This new setting ensures that the wallet wipes the spend key from memory when syncing when locked. Even if the setting isn't checked, the wallet still syncs in the background. Perhaps there is a cleaner way to portray this feature in the UI than "Sync in the background when locked"
    • When the wallet is locked, the spend key ideally should be wiped from memory by default. The reason I did not make this the default behavior (and the core reason why this setting is introduced as a toggle in this PR) is to minimize impact if something goes wrong with the implementation.
  • NOTE2: even after the spend key is wiped from memory, the wallet password remains cached in memory. The wallet password can be used to decrypt the spend key which is stored on disk.
  • This PR only implements Option 1 described here wallet: background sync with just the view key [master] monero#8619. Option 2 described in that PR is not exposed to the user from the GUI.
  • If a CLI/RPC wallet sets up a background wallet with a custom password (Option 2 described in the monero repo PR), then the GUI can still open the corresponding full wallet for that background wallet, and the GUI will process that background cache on open and will write to that background wallet on lock.
    • This leaves room on the table to implement a desktop syncing gadget (e.g. that lives in the system tray) that uses Option 2 from the core PR. Idea: run a script that auto-opens a CLI background wallet on startup, and when the user opens their GUI, it auto-stops that CLI wallet and will then automatically process the background cache on open.
      • Keep in mind using Option 2 in this way would likely leave a plaintext view key stored on disk.

TL;DR this PR brings 2 core benefits:

  • Marginally safer syncing while locked (the spend key is wiped from memory, but the wallet password remains in memory).
  • A clear path to a background syncing process that uses just the view key to sync (and e.g. lives in the system tray).

@plowsof
Copy link
Contributor

plowsof commented Oct 21, 2022

@j-berman
Copy link
Contributor Author

Apologies this has gotten a bit messy...

Logistically, I think it would make sense to review the PR's in the following order:

  1. wallet2: fix rescanning tx via scan_tx [master] monero#8568 together with SettingsWallet: Update scan transaction for changes to behavior in monero repo PR 8566 #4051.
  2. wallet: background sync with just the view key [master] monero#8619 together with this PR.

@plowsof
Copy link
Contributor

plowsof commented Oct 22, 2022

I restored from seed with this PR using an old wallet that has >30 transactions with a wallet lock time of 1 minute (easier testing). While background sync was enabled - all outputs are detected - and when the wallet is unlocked it successfully re-checks those scanned blocks and finds the false change outputs. One time after unlocking the wallet , i noticed a 'wait / forcequit' message box (i assume because alot of outputs where found during background sync - the wallet did not crash, it recovered quickly - assuming because its not async) . After fully synced, i performed a sweep_all transaction to myself which completed successfully.

When opening this 'background syncd' wallet with release, the cache file needs to be deleted / recreated:

2022-10-22 00:07:33.936	W Failed to open portable binary, trying unportable
2022-10-22 00:07:33.949	E Error opening wallet: basic_string::_M_replace_aux
2022-10-22 00:07:33.964	E Repairing wallet cache with error:  basic_string::_M_replace_aux

@j-berman
Copy link
Contributor Author

Quality testing thank you @plowsof :)

One time after unlocking the wallet , i noticed a 'wait / forcequit' message box

Can you repro this? That shouldn't happen; processing 30 txs should be close to instantaneous

When opening this 'background syncd' wallet with release, the cache file needs to be deleted / recreated

Yep, monero-project/monero#8619 adds a new field to the wallet cache file to capture background sync'd txs:

https://github.com/j-berman/monero/blob/f2f4e6b482a0a877f68190728abadbf6b753b2c9/src/wallet/wallet2.h#L1319-L1321

Older wallet versions wouldn't be able to read a wallet cache file that has updated.

@plowsof
Copy link
Contributor

plowsof commented Oct 24, 2022

seems like if i set the lock time to 1 minute, while syncing, wallet unlocked (aimlessly jiggling my mouse around without clicking so i see the visual effects are working / wallet isnt locked) - when the wallet lock screen is meant to display - instead the wallet freezes, (when transactions where seen when unlocked) (and there after each time i click 'ok' to unlock it appears again) Let me try and reproduce it reliably if you are unable to see the effect.. it doesn't happen on release though.

Confirmed that my ledger can not see the checkbox to enable the feature

@j-berman
Copy link
Contributor Author

Good eye, I noticed some jank on my end with a larger wallet sometimes too. How does the latest look on your end? I updated the Monero PR as well.

@plowsof
Copy link
Contributor

plowsof commented Oct 26, 2022

You have fixed the problem, it seems to be working flawlessly now (no delays after pressing 'ok', or locking the wallet) , Well done! will test further

@j-berman
Copy link
Contributor Author

In line with @tobtoht's comment here, I think the GUI will also need a deeper pass to more effectively protect against exfiltration of the spend key and plaintext password when background sync is enabled.

I think this should be a pre-requisite for this PR:

  • Wallet password caching should be removed from wallet/api. Password verification should use wallet2::verifyPassword instead of comparing against the cached value. This causes a slight delay when opening dialogs that require the password or when unlocking the wallet.

And the GUI should aim for this too:

  • Use something like sodium_memzero to securely wipe memory from password entry dialogs and the Keys and Seed dialog when they are closed.

@plowsof
Copy link
Contributor

plowsof commented Oct 17, 2023

have we lost enableBackgroundSync / disableBackgroundSync? compiling complains about these being gone from the latest #8619

@j-berman
Copy link
Contributor Author

Yep, this PR is WIP and not ready for testing yet since the latest overhaul to 8619

@j-berman j-berman changed the title Feature: sync in the background with just view key when locked [WIP] Feature: sync in the background with just view key when locked Oct 17, 2023
@j-berman j-berman force-pushed the background-sync branch 2 times, most recently from bb6b7f5 to da67232 Compare November 17, 2023 00:55
@j-berman j-berman changed the title [WIP] Feature: sync in the background with just view key when locked Feature: sync in the background with just view key when locked Nov 17, 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.

2 participants