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.

Sign up for GitHub

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

eventhandler: implement handleOperatorRemoved #1796

Open
wants to merge 13 commits into
base: stage
Choose a base branch
from
Open

eventhandler: implement handleOperatorRemoved #1796

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
30ee83d
eventhandler: allow creating removed operator
nkryuchkov Oct 15, 2024
a77c3d3
delete operator data and liquidate shares on OperatorRemoved event; u…
nkryuchkov Oct 17, 2024
e8660f2
wrap error when deleting operator data
nkryuchkov Oct 17, 2024
724a2ab
invert condition
nkryuchkov Oct 17, 2024
5620556
fix event handler tests
nkryuchkov Oct 17, 2024
444bbc2
fix double self calculation logic
nkryuchkov Oct 31, 2024
0fc0a0a
Add a comment
nkryuchkov Oct 31, 2024
93b3aa5
Revert "fix double self calculation logic"
nkryuchkov Oct 31, 2024
ba53fe1
fix double self calculation differently
nkryuchkov Oct 31, 2024
08fd9b4
Merge branch 'stage' into allow-creating-removed-operator
nkryuchkov Dec 26, 2024
a2060e2
eth/eventhandler: fix tests
nkryuchkov Dec 28, 2024
6c24b6a
liquidate share on operator removal if there's no quorum
nkryuchkov Dec 28, 2024
8b8abee
message/validation: check if operator exists
nkryuchkov Dec 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions eth/eventhandler/event_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,13 @@ import (
"math/big"
"time"

"github.com/ssvlabs/ssv/ekm"

"github.com/attestantio/go-eth2-client/spec/phase0"
ethcommon "github.com/ethereum/go-ethereum/common"
ethtypes "github.com/ethereum/go-ethereum/core/types"
spectypes "github.com/ssvlabs/ssv-spec/types"
"go.uber.org/zap"

"github.com/ssvlabs/ssv/ekm"
"github.com/ssvlabs/ssv/eth/contract"
"github.com/ssvlabs/ssv/eth/eventparser"
"github.com/ssvlabs/ssv/eth/executionclient"
Expand Down
54 changes: 53 additions & 1 deletion eth/eventhandler/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ func (eh *EventHandler) handleOperatorAdded(txn basedb.Txn, event *contract.Cont
if err != nil {
return fmt.Errorf("could not get operator data by public key: %w", err)
}

if pubkeyExists {
logger.Warn("malformed event: operator public key already exists",
fields.OperatorPubKey(operatorData.PublicKey))
Expand All @@ -79,11 +80,47 @@ func (eh *EventHandler) handleOperatorAdded(txn basedb.Txn, event *contract.Cont
if err != nil {
return fmt.Errorf("save operator data: %w", err)
}

if exists {
logger.Debug("operator data already exists")
return nil
}

var modifiedShares []*ssvtypes.SSVShare
for _, share := range eh.nodeStorage.Shares().List(txn, registrystorage.ByOperatorID(event.OperatorId)) {
if !share.Liquidated {
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also have the ClusterLiquidated event which may make a share Liquidated, so it can be both true and false

Copy link
Contributor

@iurii-ssv iurii-ssv Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@iurii-ssv iurii-ssv Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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"):

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 operatorId for 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 operatorId in 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please create a PR for your changes?

Yup, why not - #1842


var missingOtherOperators bool
for _, shareMember := range share.Committee {
if shareMember.Signer == event.OperatorId {
continue
}

_, ok, err := eh.nodeStorage.GetOperatorData(txn, shareMember.Signer)
if err != nil {
return fmt.Errorf("get operator data: %w", err)
}

if !ok {
missingOtherOperators = true
break
}
}

if !missingOtherOperators {
nkryuchkov marked this conversation as resolved.
Show resolved Hide resolved
share.Liquidated = false
modifiedShares = append(modifiedShares, share)
}
}

if len(modifiedShares) > 0 {
if err := eh.nodeStorage.Shares().Save(txn, modifiedShares...); err != nil {
return fmt.Errorf("save shares: %w", err)
}
}

if bytes.Equal(event.PublicKey, eh.operatorDataStore.GetOperatorData().PublicKey) {
eh.operatorDataStore.SetOperatorData(od)
logger = logger.With(zap.Bool("own_operator", true))
Expand Down Expand Up @@ -117,7 +154,22 @@ func (eh *EventHandler) handleOperatorRemoved(txn basedb.Txn, event *contract.Co
fields.Owner(od.OwnerAddress),
)

// This function is currently no-op and it will do nothing. Operator removed event is not used in the current implementation.
err = eh.nodeStorage.DeleteOperatorData(txn, od.ID)
if err != nil {
return fmt.Errorf("delete operator data: %w", err)
}

var modifiedShares []*ssvtypes.SSVShare
for _, share := range eh.nodeStorage.Shares().List(txn, registrystorage.ByOperatorID(event.OperatorId)) {
share.Liquidated = true
modifiedShares = append(modifiedShares, share)
Copy link
Contributor

Choose a reason for hiding this comment

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

this will liquidate the validator even if only one operator was removed.

}

if len(modifiedShares) > 0 {
if err := eh.nodeStorage.Shares().Save(txn, modifiedShares...); err != nil {
return fmt.Errorf("save shares: %w", err)
}
}

logger.Debug("processed event")
return nil
Expand Down
Loading