-
Notifications
You must be signed in to change notification settings - Fork 9
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
F/slashing #126
F/slashing #126
Conversation
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 can't comment on the business logic/design part of this, but I looked through the code and it looks fine!
4c1c418
to
8803670
Compare
d4c95f5
to
4a7bffb
Compare
OK, that took a while, but I think we now have a correct slashing implementation, at the (vault's) accounting level. Both for slashing and slashing propagation. Just set this as ready for review. Please take a look! #127 is also relevant, for the general overview of the process, and testing scenario definitions. |
5e7e334
to
15ac523
Compare
05f6eaa
to
15e8f44
Compare
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.
LGTM! Looking forward to this coming together. 🥳
k, merging due to popular demand. ;-) I understand this is something that can be used for integration tests, etc. Still, I would very much like a review here, even post-merge. |
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.
LGTM as far as I can tell, but I still don't understand this fully.
end_time: _end_time, | ||
} in to_remove | ||
{ | ||
// Check that the validator is active at height and slash it if that is the case |
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'm not sure I understand this. It looks like virtual-staking
informs external-staking
that the validator has been tombstoned, but this triggers a slashing event for users staking on that validator?
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.
Yes, both tombstoning and jailing always imply slashing.
/// Slashes a validator. | ||
/// | ||
/// In test code, this is called from `test_handle_slashing`. | ||
/// In non-test code, this is being called from `ibc_packet_receive` (in the `ConsumerPacket::RemoveValidators` |
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'm probably missing something, but should user slashing not get triggered when a validator itself is slashed?
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.
There's no specific slashing event. As mentioned, slashing is part of validator tombstone, because of double signing; or of validator jailing, because of being offline.
Closes #11. WIP. Publishing as draft for review / feedback.
TODO:
burn
. Likeunstake
but without releasing funds.slash_adjust
. For smart contracts accounting, but no blockchain calls (funds already slashed at blockchain level). (wip)