Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Fix semantics of ExistenceRequirement::KeepAlive.#3796

Merged
gavofyork merged 2 commits intomasterfrom
gav-fix-keep-alive
Oct 11, 2019
Merged

Fix semantics of ExistenceRequirement::KeepAlive.#3796
gavofyork merged 2 commits intomasterfrom
gav-fix-keep-alive

Conversation

@gavofyork
Copy link
Member

No description provided.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

I am not totally convinced that the solution is to change KeepAlive's semantics. I would personally leave withdraw() to be restrictive as it was before, and make the caller (takeFees) aware of the context and call this function with more care.

Nonetheless, does solve the issue. Would be great if you bring in the test here
https://github.com/paritytech/substrate/pull/3794/files#diff-6232fdc1d60a550161c54229175b6b58R787

and close the other PR as this is an alternative.

@gavofyork
Copy link
Member Author

gavofyork commented Oct 11, 2019

Well, if you look at the documentation, this is strictly a fix: no described semantics have changed. I'm ok with the idea of adding an additional ExistenceRequirement::MustContinueExistence which requires that the account continues to exist after the operation, though I don't know if there's any need for it.

This "fix" will now allow the possibility of storage-leaks: an account may now be created (i.e. nonce is written) without an ED balance and therefore that may never be cleaned up. I would be tempted to make the CheckNonce logic aware of this and avoid writing a new nonce value if the account is dead.

UPDATE:
That won't work because there's no way for the system to determine if an account is alive apart from the existence of an AccountNonce, which legally won't exist if it's the first transaction. In short, whether an account should exist is not something that we can handle automatically at a low-level. Runtime practitioners will need to conscious not to allow free transactions from arbitrary accounts that do not aware at least the ED, since each such transaction will result in storage leakage.

CC @gautamdhameja @shawntabrizi and anyone else. Please ensure that this is well documented for version 2.

@gavofyork gavofyork added the A0-please_review Pull request needs code review. label Oct 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants