eventhandler: implement handleOperatorRemoved #1796
Conversation
…n-liquidate shares on OperatorAdded event
nkryuchkov
changed the title
eth/eventhandler/handlers.go
Outdated
| if !share.Liquidated { | ||
| continue | ||
| } |
There was a problem hiding this comment.
If I understand code correctly, !share.Liquidated must never be true here because it would mean we couldn't successfully finish (commit) delete transaction ? If so, would be better to log an error here instead of silently skipping.
There was a problem hiding this comment.
We also have the ClusterLiquidated event which may make a share Liquidated, so it can be both true and false
There was a problem hiding this comment.
Right, perhaps leaving a comment noting that wouldn't hurt :)
But then I wonder what happens to validator in the following scenario (consider all of these events apply sequentially):
- 4 operators are running validator
- at some point validator(all his shares) gets liquidated because he couldn't pay his bills
- then those 4 operators themselves get de-registered from SSV network
- then those 4 operators re-register again - also reviving this validator (that shouldn't ever get revived in that manner) they previously managed shares for ?
well, actually the whole validator won't get revived ^ if I'm reading the code correctly, only the share belonging to the operator referenced in the event will be revived (unliquidated) ... which also means (again, if I got that right) this whole algorithm will only work as expected to handle just 1 operator at a time de-regestering -> re-regestering (won't work in scenario where 2 operators out of N total in cluster de-register, and only then after that try to re-register again in any order they like)
There was a problem hiding this comment.
@iurii-ssv I agree about the comment. But I'm not sure I understand your concern about the re-registering logic. We process events one by one, so if 2 operators exist and 2 are trying to re-register, then after processing the first one there would be 3 active operators, so registering the second operator would lead to 4 active operators and share reactivation. If I didn't catch your thought correctly, please follow-up
There was a problem hiding this comment.
Well yeah, this operator/share management logic is a bit more involved than I expected ... so I decided to write a test or two to verify whether I understand it correctly ... and got a bit stuck.
There seems to be no "combined" tests for interactions between operator and shares written yet - but rather these aspects are tested in isolation. Here is my sketch for how it could look like (essentially turning my comment from above into code, and adding some more) - a branch with 1 commit on top of what you have in this PR's branch - see diff. Note also, there are 2 minor fixes in there too, namely (so I recommend to "take as is and adjust it as you see fit"):
sharePubKey := validatorData2.operatorsShares[0].sec.GetPublicKey().Serialize()seems to have been a typo- and what I mentioned in eventhandler: implement handleOperatorRemoved #1796 (comment)
- plus some minor adjustments to
test ClusterLiquidated + ClusterReactivated events handling
why did I get stuck ... well, first of all I don't really know what behavior we want to have (to define proper expectations in tests), I did it best I could and encountered that we increment operatorId (in smart contract) even for operators who are re-registering (using same pubkey - they end up with different operatorId)
- as the result ^ we can't utilize
operatorIdfor certain things, for example when checking whether share belongs to operator or not - meaning, re-registered operator can't find/recognize his "old" share - hence the tests I've added cannot digest that, but I don't think they are the problem; rather it seems we should either not increment
operatorIdin smart contract or not rely/use it like we do - also, less relevant, but I'm not sure why we only have a part of solidity code but not the rest (the rest I guess is pre-compiled and used in tests ?), and hence can't quite understand how these events relate/compare to what we have in main smart contract
There was a problem hiding this comment.
@iurii-ssv I think that contract is just for simulator, the actual contract is likely this one https://github.com/ssvlabs/ssv-network/blob/583b7b7cb1c1abc5d4c3b13bafca59bf315113b6/contracts/modules/SSVOperators.sol#L26
Can you please create a PR for your changes?
There was a problem hiding this comment.
Can you please create a PR for your changes?
Yup, why not - #1842
| // Now start the OperatorRemoved event handling | ||
| // Call the contract method | ||
| _, err = boundContract.SimcontractTransactor.RemoveOperator(auth, 4) | ||
| require.NoError(t, err) | ||
| sim.Commit() |
There was a problem hiding this comment.
I think we are removing the wrong operator here (wrong ID - not the one this test added) - this is because createOperators starts numerating operators with 1 (not 0) - not a big deal this being the last test ... but still better adjust it to use op[0].id instead
There was a problem hiding this comment.
Actually, don't bother changing it just yet - I'm preparing a change (see #1796 (comment)) you'll probably wanna use (and will include this too).
Closes #1786