feat!: compute partial set #1642

insumity · 2024-02-13T13:08:56Z

Description

Closes: #1606

This PR introduces the logic needed to compute the ValidatorUpdates in ComputeValidatorUpdates.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! the type prefix if the change is state-machine breaking
confirmed this PR does not introduce changes requiring state migrations, OR confirmed migration code has been added to consumer and/or provider modules
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage

x/ccv/provider/keeper/partial_set_security.go

insumity · 2024-02-15T10:48:22Z

x/ccv/provider/keeper/msg_server.go

@@ -139,19 +139,30 @@ func (k msgServer) SubmitConsumerDoubleVoting(goCtx context.Context, msg *types.
 func (k msgServer) OptIn(goCtx context.Context, msg *types.MsgOptIn) (*types.MsgOptInResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)

-	valAddress, err := sdk.ConsAddressFromBech32(msg.ProviderAddr)
+	providerValidatorAddr, err := sdk.ValAddressFromBech32(msg.ProviderAddr)


Fixed this. Similarly to AssignConsumerKey we consider that ProviderAddr in the message is the operator address and use this operator address to get the consensus address that is used later on.
We made a similar change in OptOut.

insumity · 2024-02-15T10:53:16Z

x/ccv/provider/keeper/keeper.go

@@ -1190,14 +1190,17 @@ func (k Keeper) SetOptedIn(
 	chainID string,
 	providerAddr types.ProviderConsAddress,
 	blockHeight uint64,
+	power uint64,


Introduce power in the opted-in state so we can see if we should send a ValidatorUpdate down to CometBFT based on whether the power changed since last time it was sent. Only makes sense in an epoch-based setting because in a non-epoch-based setting, we could use the validator updates from the staking module.

insumity · 2024-02-15T11:04:48Z

x/ccv/provider/keeper/proposal.go

@@ -255,6 +255,12 @@ func (k Keeper) MakeConsumerGenesis(
 		return false
 	})

+	// currentValidators := k.GetOptedIn(ctx, chainID)


Leaving those commented for now to show how ComputeValidatorUpdates, etc. are going to be used.

x/ccv/provider/keeper/proposal.go

x/ccv/provider/keeper/keeper.go

x/ccv/provider/keeper/partial_set_security.go

mpoke · 2024-02-15T17:18:50Z

x/ccv/provider/keeper/partial_set_security.go

+	k.DeleteAllOptedIn(ctx, chainID)
+	for _, val := range nextValidators {
+		k.SetOptedIn(ctx, chainID, val.ProviderAddr, val.BlockHeight, val.Power)
+	}


Seems very inneficient to delete all opten in entries and set the new ones. If in an epoch there will be no changes in the voting powers, this will be a very expensive way to leave the state unchanged.

Is my understanding correct that at this stage, nothing is written to disk? Only if the transactions succeed, at the end all the changes would get written. If that's the case, why's this expensive? Wouldn't this correspond to a simple write in memory?
I agree that most likely the validator set won't change between epochs but probably it's unlikely that powers would remain the same between epochs for all validators, although this depends on how long an epoch is. Even in this case, I would need to loop through all currently opted in validators and compare with the nextValidators to see if a change took place.

might be premature optimization to worry about this? especially once we have epochs this happens rarely and it's at most ~200 entries being deleted and written per consumer

mpoke · 2024-02-15T17:23:45Z

x/ccv/provider/keeper/keeper.go

+	powerBytes := make([]byte, 8)
+	binary.BigEndian.PutUint64(powerBytes, power)
+
+	store.Set(types.OptedInKey(chainID, providerAddr), append(blockHeightBytes, powerBytes...))


Store OptedInValidator as a value (define it in the proto files).

Is the recommendation here to define it in proto to simplify our life later on when we implement queries that return opted-in validators?

x/ccv/provider/keeper/partial_set_security.go

mpoke · 2024-02-15T17:30:54Z

x/ccv/provider/keeper/partial_set_security.go

+
+// ComputeNextValidators computes the next validator set that is responsible for validating on a consumer chain.
+// The returned opted-in validators correspond to the next `currentValidators`.
+func (k Keeper) ComputeNextValidators(ctx sdk.Context,


I feel the logic in this function can be moved to ComputeValidatorUpdates.

Yes, that's true. I believe that splitting this into two distinct methods makes it easier to read because the ValidateUpdates do not have a 1-to-1 correspodence with the next OptedInValidators.
But I don't have strong feelings on this and we can combine them as well.

Co-authored-by: Marius Poke <[email protected]>

p-offtermatt

Added a few comments. Having both ComputeValidatorUpdates and ComputeNextValidators feels weird to me - maybe we could reconsider the idea of taking diffs between new and old validator sets here

p-offtermatt · 2024-02-16T08:42:39Z

x/ccv/provider/keeper/partial_set_security.go

@@ -67,3 +72,152 @@ func (k Keeper) HandleOptOut(ctx sdk.Context, chainID string, providerAddr types

 	return nil
 }
+
+// ComputeValidatorUpdates computes the validator updates needed to be sent to the consumer chain to capture
+// the newly opted-in and opted-out validators, as well as validators that unbonded.


Suggested change

// the newly opted-in and opted-out validators, as well as validators that unbonded.

// the newly opted-in and opted-out validators, as well as validators that started unbonding.

p-offtermatt · 2024-02-16T08:44:07Z

x/ccv/provider/keeper/partial_set_security.go

+			continue
+		}
+
+		// if `val` has unbonded, its `GetLastValidatorPower` power returns 0


Suggested change

// if `val` has unbonded, its `GetLastValidatorPower` power returns 0

// if `val` has started unbonded, its `GetLastValidatorPower` power returns 0

I think there is subtle ambiguity with the current comments, since there are two things related to unbonding,
namely starting unbonding and finishing unbonding.
I think you referred to the start of unbonding here, right?

I wanted to refer to "not bonded" which means neither "unbonding" nor "unbonded." Technically, if a validator was bonded before and now "not bonded" anymore, it is in the "unbonding" phase. Might be easier to just state "not bonded" and that would be consistent with the check !validator.isBonded() I do in other places.

Yeah, that would be clearer I think

p-offtermatt · 2024-02-16T08:46:54Z

x/ccv/provider/keeper/partial_set_security.go

+			continue
+		}
+
+		// if the voting power did not change since the last time the validator opted in, do not create an update


Comment does not seem correct to me - should this read

Suggested change

// if the voting power did not change since the last time the validator opted in, do not create an update

// if the voting power did not change between now and the last validator power, no need to add an update for this validator

?

Also, to better understand, with epochs, GetLastValidatorPower will need to be replaced with something like GetValidatorPowerAtLastEpoch, right?

The comment should probably be rephrased to:

// if the voting power is the same as the one the validator had when it opted in, do not create an update

because the voting power could change multiple times during an epoch, but at the end of an epoch, it could be that the validator has again the same voting power it had when it opted in.

Also, to better understand, with epochs, GetLastValidatorPower will need to be replaced with something like GetValidatorPowerAtLastEpoch, right?

Not sure I understand what you mean here. GetLastValidatorPower returns the most recent validator power and ComputeValidatorUpdates would be called at the end of every epoch.
Think of the following example. A validator V opts in at the end of an epoch e with power p. If at a later epoch e + X (X >= 1), validator V has still the same power p, then we should not send a ValidatorUpdate for V and this is what this checks here.

Right. GetLastValidatorPower will return the validator power in the last block. But what we need to compare against is the validator power at the last epoch. If epochLength==1, (as is the case right now) these coincide, but if epochLength != 1 this is not the case anymore.

p-offtermatt · 2024-02-16T09:15:30Z

x/ccv/provider/keeper/partial_set_security.go

+
+// ComputeValidatorUpdates computes the validator updates needed to be sent to the consumer chain to capture
+// the newly opted-in and opted-out validators, as well as validators that unbonded.
+func (k Keeper) ComputeValidatorUpdates(ctx sdk.Context,


I feel like I am missing context so my judgement may not be accurate, but it feels like one of ComputeValidatorUpdates and ComputeNextValidators is unnecessary?
Wouldn't NextValidators + a diffing method like Jehan suggested a bit ago do the trick?

See my previous comment here.
More specific to the potentiality of a diff method, I guess you could first call ComputeNextValidators and then call diff(currentValidators, nextValidators) but this diff method would be equally complex ComputeValidatorUpdates and would have a different method signature compared to ComputeNextValidators. So, I don't see much the advantage to this.
Does that sound reasonable?

Yeah, I agree. I think I would still find the diff method more elegant, but it probably comes down to personal preference, and at the end of the day both are ok.

p-offtermatt · 2024-02-16T09:31:24Z

x/ccv/provider/keeper/partial_set_security.go

+	k.DeleteAllOptedIn(ctx, chainID)
+	for _, val := range nextValidators {
+		k.SetOptedIn(ctx, chainID, val.ProviderAddr, val.BlockHeight, val.Power)
+	}


might be premature optimization to worry about this? especially once we have epochs this happens rarely and it's at most ~200 entries being deleted and written per consumer

sainoe · 2024-02-19T08:33:00Z

x/ccv/provider/keeper/proposal.go

@@ -279,7 +285,14 @@ func (k Keeper) MakeConsumerGenesis(
 	}

 	// Apply key assignments to the initial valset.
-	initialUpdatesWithConsumerKeys := k.MustApplyKeyAssignmentToValUpdates(ctx, chainID, initialUpdates)
+	initialUpdatesWithConsumerKeys := k.MustApplyKeyAssignmentToValUpdates(ctx, chainID, initialUpdates,
+		func(address types.ProviderConsAddress) bool {


I think MustApplyKeyAssignmentToValUpdates will be reworked anyway to work with PSS.
But is it necessary to do this IsOptin check through a function argument?

insumity added 2 commits February 13, 2024 09:06

more tests

71ac8e3

interface changes

61a4052

github-actions bot added the C:x/provider Assigned automatically by the PR labeler label Feb 13, 2024

insumity changed the title ~~Insumity/compute partial set~~ feat!: compute partial set Feb 13, 2024

insumity added 2 commits February 13, 2024 14:31

small fixes

4bc4887

cleaning up

f5c3af1

github-advanced-security bot found potential problems Feb 14, 2024

View reviewed changes

x/ccv/provider/keeper/partial_set_security.go Fixed Show resolved Hide resolved

more cleaning up

de1ce67

github-advanced-security bot found potential problems Feb 14, 2024

View reviewed changes

x/ccv/provider/keeper/partial_set_security.go Fixed Show fixed Hide fixed

insumity added 3 commits February 14, 2024 16:57

nit changes

8710bc2

one more test

a3e3d41

nit changes

6fa64fa

github-advanced-security bot found potential problems Feb 15, 2024

View reviewed changes

x/ccv/provider/keeper/partial_set_security.go Fixed Show fixed Hide fixed

nit changes

20f9625

insumity commented Feb 15, 2024

View reviewed changes

insumity marked this pull request as ready for review February 15, 2024 10:54

insumity requested a review from a team as a code owner February 15, 2024 10:54

small typo

7a2b2d9

insumity commented Feb 15, 2024

View reviewed changes

insumity linked an issue Feb 15, 2024 that may be closed by this pull request

Partial Set Security: Construct and send the partial sets to the consumers #1606

Closed

mpoke reviewed Feb 15, 2024

View reviewed changes

insumity and others added 4 commits February 16, 2024 08:52

Update x/ccv/provider/keeper/partial_set_security.go

000760a

Co-authored-by: Marius Poke <[email protected]>

Update x/ccv/provider/keeper/partial_set_security.go

272b87b

Co-authored-by: Marius Poke <[email protected]>

rebase

2bff04a

add godocs

5348f83

p-offtermatt reviewed Feb 16, 2024

View reviewed changes

insumity marked this pull request as draft February 19, 2024 08:16

sainoe reviewed Feb 19, 2024

View reviewed changes

revamp

7144504

insumity closed this Mar 13, 2024

insumity removed a link to an issue Mar 21, 2024

Partial Set Security: Construct and send the partial sets to the consumers #1606

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: compute partial set #1642

feat!: compute partial set #1642

insumity commented Feb 13, 2024 •

edited

Loading

insumity Feb 15, 2024

insumity Feb 15, 2024 •

edited

Loading

insumity Feb 15, 2024

mpoke Feb 15, 2024

insumity Feb 16, 2024

p-offtermatt Feb 16, 2024

mpoke Feb 15, 2024

insumity Feb 16, 2024

mpoke Feb 15, 2024

insumity Feb 16, 2024

p-offtermatt left a comment

p-offtermatt Feb 16, 2024

p-offtermatt Feb 16, 2024

insumity Feb 16, 2024

p-offtermatt Feb 16, 2024

p-offtermatt Feb 16, 2024

insumity Feb 16, 2024

p-offtermatt Feb 16, 2024

p-offtermatt Feb 16, 2024

insumity Feb 16, 2024

p-offtermatt Feb 16, 2024

p-offtermatt Feb 16, 2024

sainoe Feb 19, 2024 •

edited

Loading

	// the newly opted-in and opted-out validators, as well as validators that unbonded.
	// the newly opted-in and opted-out validators, as well as validators that started unbonding.

	// if `val` has unbonded, its `GetLastValidatorPower` power returns 0
	// if `val` has started unbonded, its `GetLastValidatorPower` power returns 0

	// if the voting power did not change since the last time the validator opted in, do not create an update
	// if the voting power did not change between now and the last validator power, no need to add an update for this validator

feat!: compute partial set #1642

feat!: compute partial set #1642

Conversation

insumity commented Feb 13, 2024 • edited Loading

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

insumity Feb 15, 2024 • edited Loading

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

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

p-offtermatt 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sainoe Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

insumity commented Feb 13, 2024 •

edited

Loading

insumity Feb 15, 2024 •

edited

Loading

sainoe Feb 19, 2024 •

edited

Loading