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

Improved Wallet State Management #30

Merged
merged 5 commits into from Dec 5, 2019

Conversation

@yeastplume
Copy link
Member

@yeastplume yeastplume commented Nov 4, 2019

Link to Rendered Document

Edit by lehnberg: Correcting link to rendered document

@lehnberg
Copy link
Contributor

@lehnberg lehnberg commented Nov 4, 2019

Nice. Couple of nitpicky points:

  • Most RFCs will be "enhancements", and wallet-update-process sounds quite generic. What would a more descriptive title be? improved-wallet-state-management?

  • For better readability for future readers, write in present form with the assumption that the RFC has already been merged. So rather than proposing changes, describe the already accepted changes. Similarly current wallet procedures become previous wallet procedures, etc.

  • IIRC, largest doesn't lead to all outputs being locked, just the most possible. This to reduce UTXOs, but comes at a privacy cost. It might be worth noting that a smallest strategy might reduce output likability, but may also lead to increased transaction fees.

@yeastplume yeastplume changed the title Wallet Update Process Enhancements Improved Wallet State Managment Nov 4, 2019
@yeastplume
Copy link
Member Author

@yeastplume yeastplume commented Nov 4, 2019

Updated the tense and title as per suggestions.

selection-strategy=all will sweep as many outputs as it can, up to a hardcoded limit of 500. In most cases this will have the effect of locking all user funds.

There was some conversation a while back where someone was arguing that the default selection strategy didn't actually reduce overall UTXO set size effectively. I admit this isn't something I've studied in any particular detail, so would be interested in hearing more opinions here. The only thing I can say with relative certainty is the current default 'sweep' method is terrible for usability regardless of it's effect on UTXO set size.

@lehnberg
Copy link
Contributor

@lehnberg lehnberg commented Nov 5, 2019

Looks much better @yeastplume 👍

selection-strategy=all will sweep as many outputs as it can, up to a hardcoded limit of 500. In most cases this will have the effect of locking all user funds.

I did not know this. Seems I had completely misunderstood this strategy to be something more like largest.

@DavidBurkett
Copy link
Contributor

@DavidBurkett DavidBurkett commented Nov 5, 2019

There was some conversation a while back where someone was arguing that the default selection strategy didn't actually reduce overall UTXO set size effectively.

Yea, I was the one that said that. While it results in a temporary reduction, as long as you're using a selection strategy that routinely spends "dust" (ie. don't use a "largest first" strategy), your current inputs will eventually be spent as you continue to use Grin, so you're mostly just inconveniencing yourself by spending them all everytime, and drawing a big circle around your inputs, reducing Grin to an accounts-based model which is bad for privacy.

@DavidBurkett
Copy link
Contributor

@DavidBurkett DavidBurkett commented Nov 5, 2019

For better readability for future readers, write in present form with the assumption that the RFC has already been merged. So rather than proposing changes, describe the already accepted changes. Similarly current wallet procedures become previous wallet procedures, etc.

I actually find it more confusing now that the tenses have been updated. I like when the RFC clearly states what takes place today, and how things will work after the RFC is implemented. I wouldn't suggest changing this RFC again, but maybe it's something we can talk about elsewhere?

Copy link
Contributor

@DavidBurkett DavidBurkett left a comment

Great work!

@yeastplume
Copy link
Member Author

@yeastplume yeastplume commented Nov 6, 2019

Added more clarity on smallest selection process, fixed typos.

Copy link
Member

@quentinlesceller quentinlesceller left a comment

Looking good 👍 some minors typo fix below before entering FCP.

text/0000-wallet-update-process.md Outdated Show resolved Hide resolved
text/0000-wallet-update-process.md Outdated Show resolved Hide resolved
text/0000-wallet-update-process.md Outdated Show resolved Hide resolved
text/0000-wallet-update-process.md Outdated Show resolved Hide resolved
text/0000-wallet-update-process.md Outdated Show resolved Hide resolved
text/0000-wallet-update-process.md Outdated Show resolved Hide resolved
text/0000-wallet-update-process.md Outdated Show resolved Hide resolved
text/0000-wallet-update-process.md Outdated Show resolved Hide resolved
text/0000-wallet-update-process.md Outdated Show resolved Hide resolved
text/0000-wallet-update-process.md Outdated Show resolved Hide resolved

* For every unspent output in the wallet:
* If the output's status in the user's wallet is 'unconfirmed' and it is present in the UTXO set, change the output's status to 'confirmed', and update the associated transaction status to 'confirmed'.
* If the output's status in the user's wallet is 'confirmed' and it no longer appears in the UTXO set, set its status to 'Spent'. Note these outputs will usually be locked so they cannot be selected for spending again.
Copy link
Contributor

@jaspervdm jaspervdm Nov 15, 2019

Choose a reason for hiding this comment

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

Do you think it would be a good idea to create a new tx log entry when 'confirmed' outputs no longer exist in the UTXO set? This would give a more consistent feel for people who are using their seed on multiple wallets at the same time, as spending on wallet A would also appear as a spend on wallet B. Obviously this information would not be perfect but it would prevent situations where funds suddenly disappear without a trace.

Copy link
Member Author

@yeastplume yeastplume Nov 18, 2019

Choose a reason for hiding this comment

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

I think that could be a good enhancement. Right now the updater simply ignores the case where an output is not locked/doesn't have an associated transaction, i.e. it only updates a transaction if there's one to find. Could end up with a lot of spam in the tx log though since there's no real way to group outputs in this case, you'd just have to create a new TX for each instance of this occurring.

Copy link
Contributor

@jaspervdm jaspervdm Nov 18, 2019

Choose a reason for hiding this comment

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

We could group all outputs (without an associated tx log entry) that are no longer unspent in the same update cycle in 1 tx log entry. There is a chance that we group more than 1 tx but I don't think that is a big deal. It would however prevent a lot of spam.

@DavidBurkett
Copy link
Contributor

@DavidBurkett DavidBurkett commented Nov 16, 2019

If implemented carefully, this should actually solve a lot of the privacy issues we have with using shared nodes. We can probably do this in a way that prevents side channel leakage like Monero and Zcash have been found to be vulnerable to: https://hackerone.com/reports/713321

We just need to not block the thread for any operations that are only performed when our own transactions are received from a node, and never make special requests for our own transactions.

If we also introduce a way to anonymously submit transactions to random nodes via TOR, we could actually make it so shared nodes are just as private as using your own node, with one caveat: Wallets that are only on long enough to receive a transaction and logoff will still be terrible for privacy.

@kargakis
Copy link
Contributor

@kargakis kargakis commented Nov 18, 2019

Wallets that are only on long enough to receive a transaction and logoff will still be terrible for privacy.

@DavidBurkett can you explain why is this the case with a wallet connected via Tor?

@quentinlesceller
Copy link
Member

@quentinlesceller quentinlesceller commented Nov 26, 2019

In accordance with our RFC process, as of November 15 this can be considered in Final Comment Period with a disposition to merge. Previously announced on Keybase.

@quentinlesceller quentinlesceller merged commit 3fc9719 into mimblewimble:master Dec 5, 2019
@quentinlesceller quentinlesceller changed the title Improved Wallet State Managment Improved Wallet State Management Dec 5, 2019
@quentinlesceller
Copy link
Member

@quentinlesceller quentinlesceller commented Dec 5, 2019

🎉 Wohooo! This RFC has now been merged! 🤸‍♀️
Tracking issue: mimblewimble/grin-wallet#244

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

Successfully merging this pull request may close these issues.

None yet

6 participants