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

removed zero error case in slash query #1025

Merged
merged 32 commits into from
Dec 14, 2023
Merged

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Dec 12, 2023

Context

There's currently an error case that's preventing validator's with 0 delegations from having their slash queries reset. This check was only meant to protect against a division by zero error, but it's repeated downstream here in a more suitable place.

Really, there are 3 scenarios we could run into:

  • Internal delegation is 0, delegation from query response is 0 (this is covered here) and will return successfully
  • Internal delegation is 0, delegation from query response is slightly larger because of a precision error (this is covered here and corrects the precision error)
  • Internal delegation is 0, delegation from query response is much larger - this would indicate an actual bug and the appropriate error is returned here

All that said, we don't need to throw the original error at the top of the function.

Brief Changelog

  • Removed error case of a zero delegation
  • Added unit test
  • Updated existing unit tests to check the slash query in progress flag

riley-stride and others added 30 commits October 11, 2023 04:07
@ethan-stride
Copy link
Contributor

LGTM
So tracing through the paths -- removing this zero check will successfully reset the slashQueryInProgress flag on the hostZone but when it actually checks if a slash happened it will exit because it will get a zero == zero on the delegatedTokens.Equal(validator.Delegation) check inside of the CheckForSlash call, and importantly it will then exit with nil error so SlashValidatorOnHostZone will not get called.

So this also means that for validators with 0 delegation, even if they really were slashed, we can't detect it with our current methods because we are computing the weight from the delegation changes not able to ICQ the weight directly. If we have the wrong weight on a validator, when we actually add delegation what are the implications? Do we really only have to care about the multiplicative change over time as it relates to our delegations and not the absolute value of the weight for that validator?

(aside: in the precision error case why do we only check one sided? Could precision errors also cause a LT to happen but in a very small amount? Right now even a minuscule difference to the downside is interpreted as a slash)

@sampocs
Copy link
Collaborator Author

sampocs commented Dec 14, 2023

(aside: in the precision error case why do we only check one sided? Could precision errors also cause a LT to happen but in a very small amount? Right now even a minuscule difference to the downside is interpreted as a slash)

Discussed this in standup, but just to reiterate - any downward change is registered as a slash and automatically updated (regardless of the magnitude). The precision error check is only for the GT case because if the query response is much greater than our internal accounting - that indicates an actual error

So this also means that for validators with 0 delegation, even if they really were slashed, we can't detect it with our current methods because we are computing the weight from the delegation changes not able to ICQ the weight directly

That's right, but we would expect the query response also has 0 or represents a precision error case and is caught upstream.

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.

Thanks for doing this!

@sampocs
Copy link
Collaborator Author

sampocs commented Dec 14, 2023

@riley-stride are you signed off on this? I thought from axolo that it looked like you reviewed

@sampocs sampocs changed the base branch from cleanup_v15 to main December 14, 2023 18:53
@sampocs sampocs merged commit 8c2dcb9 into main Dec 14, 2023
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.

4 participants