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

Fix validator weight check #1176

Merged
merged 3 commits into from
Apr 12, 2024
Merged

Fix validator weight check #1176

merged 3 commits into from
Apr 12, 2024

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Apr 12, 2024

Context

The validator weight check was be run after each validator was added. It should instead be done once after all validators were added - otherwise, it can error prematurely.

Brief Changelog

  • Pulled out CheckValidatorWeightsBelowCap into relevant msg_server functions (with and without gov)
  • Updated CheckValidatorWeightsBelowCap to take a chainId as an arg and look up the host zone + validators (instead of taking validators as an arg)
  • Fixed the unit test

Testing

  • Created the following add-validator-propoal-file. This has the same weights that were used for stSAGA
  • Started dockernet on main branch, placing an early exit in register_host.sh before the validators were added normally
  • Added validators using the attached file, confirmed it errored
>>> strided tx stakeibc add-validators GAIA add-vals.json --from admin -y --gas 1000000

raw_log: 'failed to execute message; message index: 0: validator cosmosvaloper14kn0kk33szpwus9nh8n87fjel8djx0y070ymmj
  exceeds weight cap, has 11.39913057478667% of the total weight when the cap is 10%:
  • Restarted the process on this PR's branch, added validators with the same file, and confirmed it succeeded
  • Changed the first validators weight to 1200 and repeated the process - confirming that registration failed

Copy link
Contributor

@shellvish shellvish left a comment

Choose a reason for hiding this comment

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

This looks great to me, thank you for doing it!

Only question, should we also enforce the ValidatorWeightCap is respected when validators are deleted? My bias is yes, but could reasonably see it going either way

@sampocs
Copy link
Collaborator Author

sampocs commented Apr 12, 2024

hmm very good question! I'd lean yes as well. Seems like it's a tradeoff with having some flexibility - but that flexibility doesn't seem that valuable

@sampocs
Copy link
Collaborator Author

sampocs commented Apr 12, 2024

@shellvish I actually just realized that we can only remove validators if they have 0 weight - so the weight distribution shouldn't change when one is removed. I don't think we need the check

@sampocs sampocs merged commit 0394eeb into main Apr 12, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants