Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Throttle bug fixes + req refactors #565
Throttle bug fixes + req refactors #565
Changes from 9 commits
8dcc077
ea9288a
370f656
7713c21
54c84b5
f2a5522
f4b905f
cddd741
74441c2
777a3f6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not update the packet data to include the
providerConsAddr
and avoid callingGetProviderAddrFromConsumerAddr
later on?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By this, I believe you're referring to the
SlashPacketData
type. This is the protobuf generated type that is sent over the wire from consumer to provider. Adding a new field to this type (that's only used on the provider) seems unnecessary to me. However, if you feel this is important, I can refactor the base branch once the merge conflicts are solved. I'm hesitant to do much more refactoring in this PR due to how disconnected from main it is at this time :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking more like updating
SlashPacketData.Validator.Address
to the provider address obtained fromGetProviderAddrFromConsumerAddr
. But it's not that important.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha, that's a reasonable change, done in 1f920b4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something but doesn't this change allow validator to be slashed all the way to 0 in a single block?
If a consumer sends many slash packets for the same validator?
Besides that, I don't think this addressing the problem in this comment
#462 (comment)
To state the problem more clearly:
We want it to be possible to set the slashFraction or other params so that the throttle is 'invisible'. Ie. the behavior of the system is exactly as in the current spec,
This means, we do not want any slash packets to be held up/delayed (still talking about when fraction=1, replenishPeriod = 1 second ect).
Consider the following
Imagine there are n validators total on the provider, with k<n active. Imagine the actives are v1,v2..vk and the inactives are vk+1,..vn
A consumer sends slash packets for every single provider validator v1..,vn and they are all delivered to the provider in the same block.
It is possible, that all of the slashes for the active validators v1..vk are handled first. This will set the meter to 0 so the slashes for vk+1,.. vn will be blocked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this design we can slash up to but not including 100% of the "initial val set" instantaneously when the replenish fraction is 1. If we want to include 100%, its a one line inequality change. I like 100% not being allowed, but I'm not super attached to that idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change we discussed today is included in 777a3f6, thank you for the efforts and feedback. Even with disagreements at times, the dialogue ended up useful and productive! It will be cool to get this feature properly specced and thoroughly tested before prod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and before I forget,
Yes this is possible with or without slash packet throttling. Does #544 address your concern? If not, might be useful to add onto that issue or create a new one