fix: atomically add and remove shares from a committee #1760

anatolie-ssv · 2024-09-24T12:57:11Z

Share addition/removal takes place under a single lock.

Close: #1558

operator/validator/controller.go

operator/validators/validators_map.go

y0sher

lets add a test

more comments

y0sher

This solution indeed works at the moment, but there's something I don't like about it. We still have AddShare and RemoveShare methods in committee.go . This means someone can use them without the atomicity you just added. This means if someone add a usage of them in the future, we might have a race condition again.
I can think of 2 options we have here:

Emphasize in name and comments that these methods are practically unsafe, and that any consumer should use the atomic function in ValidatorsMap
Move the atomicity to inside the Committee struct, which will inherently stop itself when it has no shares left. this way you can probably check in validatorsmap whether the committee has stopped, and if so, remove it from the map. and still keep the functions safe.

I'm leaning more towards 2, what do you guys think? @anatolie-ssv , @MatusKysel , @moshe-blox

anatolie-ssv · 2024-10-01T09:43:03Z

This solution indeed works at the moment, but there's something I don't like about it. We still have AddShare and RemoveShare methods in committee.go . This means someone can use them without the atomicity you just added. This means if someone add a usage of them in the future, we might have a race condition again. I can think of 2 options we have here:
1. Emphasize in name and comments that these methods are practically unsafe, and that any consumer should use the atomic function in `ValidatorsMap`

2. Move the atomicity to inside the `Committee` struct, which will inherently stop itself when it has no shares left. this way you can probably check in `validatorsmap` whether the committee has stopped, and if so, remove it from the map. and still keep the functions safe.
I'm leaning more towards 2, what do you guys think? @anatolie-ssv , @MatusKysel , @moshe-blox

I moved the atomicity functionality to the committee object.

anatolie-ssv · 2024-10-01T09:45:51Z

protocol/v2/ssv/validator/committee.go

@@ -87,6 +90,9 @@ func (c *Committee) RemoveShare(validatorIndex phase0.ValidatorIndex) {
 	if share, exist := c.Shares[validatorIndex]; exist {
 		c.dutyGuard.StopValidator(share.ValidatorPubKey)
 		delete(c.Shares, validatorIndex)
+		if len(c.Shares) == 0 {
+			c.stop()


@y0sher now that we are able to stop the committee should we be checking its state before doing actions like AddShare, ProcessMessage etc?

Perhaps it's better (more straightforward) to just call Committee.cancel once we determined that committee can stop (shares dropped to 0), presumably this should be enough to finish all the work Committee was doing (while it's controller's job to make sure no new tasks will be assigned to such Committee - which it does handle by atomically removing it from controller.validatorsMap),

and, from my reading of this code, unless we do something with stopped Committee (have some business logic for it in stopped state, like restarting it) there is no reason to even store this stopped atomic.Bool (and have corresponding methods stop and Stopped) - you could simply remove these and adjust the way UpdateCommitteeAtomic works (so that instead of checking Stopped() func mutate will return a bool to signal that committee was stopped by this mutation - which will immediately trigger all the cleanup necessary).

anatolie-ssv · 2024-10-03T18:33:10Z

deployed to stage 311-314 epoch 83532 to 83652

operator/validators/validators_map_test.go

MatusKysel · 2024-11-13T12:15:33Z

operator/validators/validators_map.go

+func (vm *ValidatorsMap) cleanup() {
+	var stopped []spectypes.CommitteeID
+	for k, v := range vm.committees {
+		if v.Stopped() {
+			stopped = append(stopped, k)
+		}
+	}
+	for _, k := range stopped {
+		delete(vm.committees, k)
 	}
-	return nil
 }


why not something more simple

func (vm *ValidatorsMap) cleanup() { for k, v := range vm.committees { if v.Stopped() { delete(vm.committees, k) } } }

also do we want to iterate or just clean up one that's been modified in UpdateCommitteeAtomic?

just to cleanup, fixed now!

operator/validators/validators_map_test.go

operator/validator/controller.go

operator/validators/validators_map_test.go

iurii-ssv

The code should work, I left couple of nits, but mostly I don't quite understand why we need the concept of "stopped" committee - if it isn't something we are planning to use I'd rather we simplify code by getting rid of it.

iurii-ssv · 2024-11-13T17:09:18Z

operator/validator/controller.go

@@ -698,10 +698,9 @@ func (c *controller) UpdateValidatorsMetadata(data map[spectypes.ValidatorPK]*be
 			if err != nil {
 				c.logger.Warn("could not start validator", zap.Error(err))
 			}
-			vc, found := c.validatorsMap.GetCommittee(v.Share().CommitteeID())
-			if found {
+			_ = c.validatorsMap.UpdateCommitteeAtomic(v.Share().CommitteeID(), func(vc *validator.Committee) {


Shouldn't we log an error or something when UpdateCommitteeAtomic returns false here (just as a sanity check/thing) ?

iurii-ssv · 2024-11-13T17:38:08Z

operator/validator/controller.go

+	var vc *validator.Committee
+
 	// Start a committee validator.
-	vc, found := c.validatorsMap.GetCommittee(operator.CommitteeID)
+	found = c.validatorsMap.UpdateCommitteeAtomic(operator.CommitteeID, func(fvc *validator.Committee) {
+		fvc.AddShare(&share.Share)
+		vc = fvc
+	})


Would be cleaner to return validator.Committee from UpdateCommitteeAtomic, wdyt ?

We could, but there are 3 call sites to this function and only this one would benefit from the extra return value. @y0sher @MatusKysel your take here?

I would vote for returning the value

iurii-ssv · 2024-11-13T17:51:33Z

operator/validators/validators_map.go

+	if !ok || v.Stopped() {
+		return nil, false
+	}


It doesn't seem like "stopped" committee can be present in vm.committees map because we immediately remove stopped committees in UpdateCommitteeAtomic, so perhaps no need to add this check here ?

Applies to all methods in ValidatorsMap I think, but only because we don't really "stop" committee from anywhere else other than ValidatorsMap (via UpdateCommitteeAtomic -> RemoveShare) - like I mentioned in #1760 (comment). In theory we could "stop" committee from somewhere else, I just don't see why we would (and it makes code harder to understand or see the whole picture).

iurii-ssv · 2024-11-14T07:17:32Z

operator/validator/controller.go

-			deletedCommittee.Stop()
-		}
+	}) {
+		c.logger.Warn("committee not found", fields.PubKey(pubKey[:]))


Similar to https://github.com/ssvlabs/ssv/pull/1760/files#r1840850336, probably want to log this as Error (not Warn) ? Because if committee is not found here - looks like something is wrong with the way we manage it's lifecycle, and we'd need to address it (warnings in logs are pretty easy to miss, compared to errors).

As I remember in past conversations it was decided not to change the log level and keep it as it was previously (line 898)

Co-authored-by: rehs0y <lyosher@gmail.com>

Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com>

anatolie-ssv requested a review from MatusKysel September 24, 2024 12:57

anatolie-ssv self-assigned this Sep 24, 2024

anatolie-ssv added bug Something isn't working and removed bug Something isn't working labels Sep 24, 2024

MatusKysel reviewed Sep 24, 2024

View reviewed changes

operator/validator/controller.go Outdated Show resolved Hide resolved

MatusKysel approved these changes Sep 24, 2024

View reviewed changes

operator/validator/controller.go Outdated Show resolved Hide resolved

MatusKysel requested review from y0sher and moshe-blox September 24, 2024 13:05

anatolie-ssv changed the title ~~fix: atomically add and remove shares from a commitee~~ fix: atomically add and remove shares from a committee Sep 24, 2024

anatolie-ssv linked an issue Sep 24, 2024 that may be closed by this pull request

rethink committee share management #1558

Open

y0sher reviewed Sep 25, 2024

View reviewed changes

operator/validator/controller.go Outdated Show resolved Hide resolved

y0sher reviewed Sep 25, 2024

View reviewed changes

operator/validators/validators_map.go Outdated Show resolved Hide resolved

y0sher requested changes Sep 25, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

y0sher requested changes Sep 25, 2024

View reviewed changes

anatolie-ssv force-pushed the fix/add-share-rc branch 4 times, most recently from 6c95e40 to f0a092c Compare September 26, 2024 09:41

anatolie-ssv requested a review from y0sher September 30, 2024 07:12

anatolie-ssv changed the title ~~fix: atomically add and remove shares from a committee~~ Atomically add and remove shares from a committee Sep 30, 2024

anatolie-ssv changed the title ~~Atomically add and remove shares from a committee~~ fix: atomically add and remove shares from a committee Sep 30, 2024

anatolie-ssv commented Oct 1, 2024

View reviewed changes

anatolie-ssv force-pushed the fix/add-share-rc branch from 7223a0e to bea9f9a Compare October 3, 2024 11:01

nkryuchkov reviewed Oct 7, 2024

View reviewed changes

operator/validators/validators_map_test.go Outdated Show resolved Hide resolved

anatolie-ssv force-pushed the fix/add-share-rc branch 2 times, most recently from 9e64123 to 4d86735 Compare October 10, 2024 10:33

anatolie-ssv force-pushed the fix/add-share-rc branch from 4d86735 to 25f8e3d Compare October 17, 2024 09:58

anatolie-ssv force-pushed the fix/add-share-rc branch from 25f8e3d to 60d5e2e Compare November 4, 2024 08:21

anatolie-ssv force-pushed the fix/add-share-rc branch 2 times, most recently from 0332913 to 9a78188 Compare November 11, 2024 15:38

MatusKysel requested review from iurii-ssv, MatusKysel and nkryuchkov November 13, 2024 11:42

MatusKysel reviewed Nov 13, 2024

View reviewed changes

anatolie-ssv force-pushed the fix/add-share-rc branch from 9a78188 to b008038 Compare November 13, 2024 13:29

nkryuchkov approved these changes Nov 13, 2024

View reviewed changes

operator/validators/validators_map_test.go Outdated Show resolved Hide resolved

operator/validator/controller.go Outdated Show resolved Hide resolved

operator/validators/validators_map_test.go Outdated Show resolved Hide resolved

anatolie-ssv force-pushed the fix/add-share-rc branch from 0c1e89e to 4853472 Compare November 13, 2024 16:05

iurii-ssv approved these changes Nov 13, 2024

View reviewed changes

iurii-ssv reviewed Nov 14, 2024

View reviewed changes

anatolie-ssv force-pushed the fix/add-share-rc branch from 4853472 to e771d25 Compare November 18, 2024 07:40

anatolie-ssv and others added 12 commits November 18, 2024 09:40

fix: atomically add and remove shares from a commitee (#1558)

f5678f1

Update operator/validator/controller.go

06bf401

Co-authored-by: rehs0y <lyosher@gmail.com>

add test

7f8e7f4

move atomicity to committee

3374c87

add committee test

b5a8475

apply formatting

029516b

fix rebase error

45e679c

spec alignment

f347cca

remove unnecessary iter

31fb201

Update operator/validators/validators_map_test.go

8fde437

Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com>

add return value

e771d25

Merge branch 'stage' into fix/add-share-rc

ba67877

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: atomically add and remove shares from a committee #1760

fix: atomically add and remove shares from a committee #1760

anatolie-ssv commented Sep 24, 2024 •

edited

Loading

y0sher left a comment

This comment was marked as outdated.

y0sher left a comment

anatolie-ssv commented Oct 1, 2024

anatolie-ssv Oct 1, 2024

iurii-ssv Oct 10, 2024 •

edited

Loading

anatolie-ssv commented Oct 3, 2024 •

edited

Loading

MatusKysel Nov 13, 2024

MatusKysel Nov 13, 2024

anatolie-ssv Nov 13, 2024

iurii-ssv left a comment

iurii-ssv Nov 13, 2024

iurii-ssv Nov 13, 2024

anatolie-ssv Nov 15, 2024

MatusKysel Nov 15, 2024

iurii-ssv Nov 13, 2024 •

edited

Loading

iurii-ssv Nov 14, 2024

anatolie-ssv Nov 14, 2024

fix: atomically add and remove shares from a committee #1760

Are you sure you want to change the base?

fix: atomically add and remove shares from a committee #1760

Conversation

anatolie-ssv commented Sep 24, 2024 • edited Loading

y0sher left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

y0sher left a comment

Choose a reason for hiding this comment

anatolie-ssv commented Oct 1, 2024

Choose a reason for hiding this comment

iurii-ssv Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

anatolie-ssv commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iurii-ssv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iurii-ssv Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anatolie-ssv commented Sep 24, 2024 •

edited

Loading

iurii-ssv Oct 10, 2024 •

edited

Loading

anatolie-ssv commented Oct 3, 2024 •

edited

Loading

iurii-ssv Nov 13, 2024 •

edited

Loading