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

feat: add extra validations when importing existing accounts #640

Open
tomyrd opened this issue Dec 17, 2024 · 4 comments
Open

feat: add extra validations when importing existing accounts #640

tomyrd opened this issue Dec 17, 2024 · 4 comments

Comments

@tomyrd
Copy link
Collaborator

tomyrd commented Dec 17, 2024

when adding existing (i.e., not new) accounts, do we need to do any extra checks? Specifically:

  1. Should we require that the imported state is the latest state on the network? (like we do in case of locked accounts).
  2. How should we handle the situation when the imported state is "in the future" as compared to the current sync point of the client? For example, should we force the user to do a sync first?

From #631 (review)

@tomyrd
Copy link
Collaborator Author

tomyrd commented Dec 17, 2024

For (1), do we mean checking the latest state even when not overwritting? I'm not against the idea, and we already check it when overwritting accounts. It would prevent the user from importing accounts which will automatically be locked in the next sync.
For (2), I did some testing and the client works fine with future accounts, we may want to enforce a sync just for the client state to be consistent with the account state

@bobbinth
Copy link
Contributor

do we mean checking the latest state even when not overwriting? I'm not against the idea, and we already check it when overwriting accounts. It would prevent the user from importing accounts which will automatically be locked in the next sync.

yes, correct.

I did some testing and the client works fine with future accounts, we may want to enforce a sync just for the client state to be consistent with the account state

Yeah - there could be different options to handle this. For example, if we try to import an account which has been updated in block 200, but the client is currently synced to block 100, we could return an error. Then, it would be the responsibility of the CLI to sync to the latest block and only then to import the account.

@igamigo
Copy link
Collaborator

igamigo commented Dec 19, 2024

Should we require that the imported state is the latest state on the network? (like we do in case of locked accounts).

I'm wondering what the benefits of this could be. On the next sync, the client should already handle receiving the new account states correctly. So there could be a case where I'm importing a new account state because it has some assets or code that I don't locally have and which I could use successfully without really needing to have the latest state. But I guess for actually executing transactions you would need to sync anyway, so maybe it's better to enforce having the latest state.

@bobbinth
Copy link
Contributor

I was thinking primarily from the consistency of data standpoint - i.e., we can always say something like "the client has the state current as of block x". It may be OK to let some elements of the state be "in the future", but my fear is that it may become difficult to reason about how these inconsistencies manifest in various places (or if we have all the edge-cases covered for handling such inconsistencies).

One example (and not sure if this is even possible with the way we do state syncs currently) is if we can't sync to the latest state for some reason. Say the client is currently at block 100, and we import an account state which is at block 200. Then, on the next sync, we only sync to block 150 where the state of the account was not the same as at 100 but also not yet at 200. Then, figuring out what to do with the account becomes a bit more difficult.

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

No branches or pull requests

3 participants