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

Problem: migrate function does not reset properly validator nonce #154

Merged
merged 2 commits into from
Oct 27, 2022

Conversation

thomas-nguy
Copy link
Collaborator

Problem: To reset the state of each orchestrator (nonce), the current code loop over all the attestations voters and set their nonce to 0
Before #75 we are keeping ALL the attestations in the state, but with the new improvement, we are keeping only the most recent one’s.

Validator that miss to vote on the most recent attestation will be miss by the migrate function and their state will remains.

The solution is to change the code to get the list of validators and set their nonce to zero for each of them

@thomas-nguy thomas-nguy temporarily deployed to CI October 27, 2022 02:20 Inactive
adu-crypto
adu-crypto previously approved these changes Oct 27, 2022
Copy link

@adu-crypto adu-crypto left a comment

Choose a reason for hiding this comment

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

lgtm

tomtau
tomtau previously approved these changes Oct 27, 2022
Copy link

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

I'm not familiar with that logic in detail -- is it all right as it is with setting nonces to zero (as the previous msgs with the same nonce can't be relayed due to a different height/bridge address), or will this also need some global or per validator revision number?

// Reset all validators ethereum event nonce to zero
delegateKeys := k.getDelegateKeys(ctx)
for _, delegateKey := range delegateKeys {
if len(delegateKey.ValidatorAddress) != 0 {
Copy link

Choose a reason for hiding this comment

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

when would there be 0-len addresses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should technically not happen.

After thinking, I decided to remove the safeguard because it the len is nil, then the function will panic allowing us to detect corrupted state during the upgrade

@thomas-nguy
Copy link
Collaborator Author

I'm not familiar with that logic in detail -- is it all right as it is with setting nonces to zero (as the previous msgs with the same nonce can't be relayed due to a different height/bridge address), or will this also need some global or per validator revision number?

Should be alright setting nonce to 0 because the new ethereum's gravity address will have the nonce set to 1

Validators will have to report nonce from 1 onward as the nonce need to be reported in in a sequential order

@thomas-nguy thomas-nguy dismissed stale reviews from tomtau and adu-crypto via 8ecf91d October 27, 2022 06:03
@thomas-nguy thomas-nguy requested a review from tomtau October 27, 2022 06:04
@thomas-nguy thomas-nguy temporarily deployed to CI October 27, 2022 06:29 Inactive
@thomas-nguy thomas-nguy merged commit 2107c6b into v2.0.0-cronos Oct 27, 2022
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