Skip to content

Commit

Permalink
fix(abci v2): Fix (#161)
Browse files Browse the repository at this point in the history
* `UpdatedValidatorsCacheKey`

* ABCI: `DeleteValidatorByPowerIndex` logic

* validate ABCI events are not stuck

* wait for blocks jail test

* BeforeJailedValidatorsKey

* lint + fix remove val test (check bonded)

* lint

* logs

* touchups
  • Loading branch information
Reecepbcups authored Apr 14, 2024
1 parent ef9ddf2 commit dc4ad45
Show file tree
Hide file tree
Showing 12 changed files with 310 additions and 41 deletions.
11 changes: 11 additions & 0 deletions INTEGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,16 @@ app.ModuleManager = module.NewManager(

...

// Register POA Staking Hooks
app.StakingKeeper.SetHooks(
stakingtypes.NewMultiStakingHooks(
...
app.POAKeeper.Hooks(),
),
)

...

// Add PoA to BeginBlock logic
// NOTE: This must be before the staking module begin blocker
app.ModuleManager.SetOrderBeginBlockers(
Expand All @@ -80,6 +90,7 @@ app.ModuleManager.SetOrderBeginBlockers(
)

// Add PoA to end blocker logic
// NOTE: This must be before the staking module end blocker
app.ModuleManager.SetOrderEndBlockers(
...
poa.ModuleName,
Expand Down
7 changes: 7 additions & 0 deletions e2e/poa_add_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/strangelove-ventures/interchaintest/v8"
"github.com/strangelove-ventures/interchaintest/v8/chain/cosmos"
"github.com/strangelove-ventures/interchaintest/v8/testutil"
"github.com/strangelove-ventures/poa/e2e/helpers"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -95,4 +96,10 @@ func TestPOAAddValidator(t *testing.T) {
assertSignatures(t, ctx, chain, 1) // They are not signing, should be 0
assertConsensus(t, ctx, chain, 2) // ensure they were added to CometBFT

// validate that the ABCI events are not "stuck" (where the event is not cleared by POA or x/staking)
_, err = helpers.POASetPower(t, ctx, chain, admin, pending[0].OperatorAddress, 2_000_000)
require.NoError(t, err)

require.NoError(t, testutil.WaitForBlocks(ctx, 4, chain))

}
11 changes: 9 additions & 2 deletions e2e/poa_jail_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package e2e

import (
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -28,7 +29,7 @@ func TestPOAJailing(t *testing.T) {
updatedSlashingCfg.ModifyGenesis = cosmos.ModifyGenesis(append(defaultGenesis, []cosmos.GenesisKV{
{
Key: "app_state.slashing.params.signed_blocks_window",
Value: "3",
Value: "10",
},
{
Key: "app_state.slashing.params.min_signed_per_window",
Expand Down Expand Up @@ -67,14 +68,16 @@ func TestPOAJailing(t *testing.T) {
}

// Wait for the stopped node to be jailed & persist
require.NoError(t, testutil.WaitForBlocks(ctx, 5, chain.Validators[0]))
t.Log("Waiting for validator to become jailed")
require.NoError(t, testutil.WaitForBlocks(ctx, 15, chain.Validators[0]))

// Validate 1 validator is jailed (status 1)
vals := helpers.GetValidators(t, ctx, chain)
jailedValAddr := ""
require.True(t, func() bool {
for _, v := range vals.Validators {
if v.Status == int(stakingtypes.Unbonded) || v.Status == int(stakingtypes.Unbonding) {
fmt.Println("Validator", v.OperatorAddress, "is jailed", v.Status)
jailedValAddr = v.OperatorAddress
return true
}
Expand All @@ -94,4 +97,8 @@ func TestPOAJailing(t *testing.T) {
require.True(t, unjailTime.After(now))
}
}

// wait to ensure the chain is not halted
t.Log("Waiting for chain to progress")
require.NoError(t, testutil.WaitForBlocks(ctx, 5, chain))
}
18 changes: 17 additions & 1 deletion e2e/poa_remove_test.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
package e2e

import (
"context"
"fmt"
"testing"

sdkmath "cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/stretchr/testify/require"

stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"

"github.com/strangelove-ventures/interchaintest/v8"
"github.com/strangelove-ventures/interchaintest/v8/chain/cosmos"
"github.com/strangelove-ventures/interchaintest/v8/testutil"
Expand Down Expand Up @@ -75,7 +78,7 @@ func TestPOARemoval(t *testing.T) {

assertSignatures(t, ctx, chain, initialValidators-1)
require.Equal(t, initialValidators-1, len(consensus.Validators), "BFT consensus should have one less validator")
require.Equal(t, initialValidators-1, len(vals), "Validators should have one less validator")
require.Equal(t, initialValidators-1, getBondedValidators(t, ctx, chain), "Bonded validators should have one less active validator")
}

func getValToRemove(t *testing.T, vals helpers.Validators, delegationAmt int64) string {
Expand All @@ -90,3 +93,16 @@ func getValToRemove(t *testing.T, vals helpers.Validators, delegationAmt int64)

return valToRemove
}

func getBondedValidators(t *testing.T, ctx context.Context, chain *cosmos.CosmosChain) int {
vals := helpers.GetValidators(t, ctx, chain).Validators

bonded := 0
for _, v := range vals {
if v.Status == int(stakingtypes.Bonded) {
bonded++
}
}

return bonded
}
14 changes: 9 additions & 5 deletions keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ type Keeper struct {
logger log.Logger

// state management
Schema collections.Schema
Params collections.Item[poa.Params]
PendingValidators collections.Item[poa.Validators]
Schema collections.Schema
Params collections.Item[poa.Params]
PendingValidators collections.Item[poa.Validators]
UpdatedValidatorsCache collections.KeySet[string]
BeforeJailedValidators collections.KeySet[string]

CachedBlockPower collections.Item[poa.PowerCache]
AbsoluteChangedInBlockPower collections.Item[poa.PowerCache]
Expand All @@ -59,8 +61,10 @@ func NewKeeper(
logger: logger,

// Stores
Params: collections.NewItem(sb, poa.ParamsKey, "params", codec.CollValue[poa.Params](cdc)),
PendingValidators: collections.NewItem(sb, poa.PendingValidatorsKey, "pending", codec.CollValue[poa.Validators](cdc)),
Params: collections.NewItem(sb, poa.ParamsKey, "params", codec.CollValue[poa.Params](cdc)),
PendingValidators: collections.NewItem(sb, poa.PendingValidatorsKey, "pending", codec.CollValue[poa.Validators](cdc)),
UpdatedValidatorsCache: collections.NewKeySet(sb, poa.UpdatedValidatorsCacheKey, "updated_validators", collections.StringKey),
BeforeJailedValidators: collections.NewKeySet(sb, poa.BeforeJailedValidatorsKey, "before_jailed", collections.StringKey),

CachedBlockPower: collections.NewItem(sb, poa.CachedPreviousBlockPowerKey, "cached_block", codec.CollValue[poa.PowerCache](cdc)),
AbsoluteChangedInBlockPower: collections.NewItem(sb, poa.AbsoluteChangedInBlockPowerKey, "absolute_changed_power", codec.CollValue[poa.PowerCache](cdc)),
Expand Down
47 changes: 39 additions & 8 deletions keeper/poa.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,25 +72,56 @@ func (k Keeper) SetPOAPower(ctx context.Context, valOpBech32 string, newShares i
return stakingtypes.Validator{}, err
}

// no need to process anything, same values
if newBFTConsensusPower == currentPower {
return val, fmt.Errorf("current power (%d) is the same as the new power (%d) for %s", currentPower, newBFTConsensusPower, valOpBech32)
}

// When we SetValidatorByPowerIndex, the Tokens are used to get the shares of power for CometBFT consensus (voting_power).
// We don't `k.stakingKeeper.SetValidator` since we only use this for CometBFT consensus power.
val.Tokens = sdkmath.NewIntFromUint64(uint64(newShares))

// slash all the validator's tokens (100%)
if newShares == 0 && currentPower > 0 {
pk, ok := val.ConsensusPubkey.GetCachedValue().(cryptotypes.PubKey)
if !ok {
return stakingtypes.Validator{}, fmt.Errorf("issue getting consensus pubkey for %s", valOpBech32)
}

consAddr := sdk.GetConsAddress(pk)

height := sdk.UnwrapSDKContext(ctx).BlockHeight()

normalizedToken := k.stakingKeeper.TokensFromConsensusPower(ctx, currentPower)

if _, err := k.stakingKeeper.Slash(ctx, sdk.GetConsAddress(pk), height, normalizedToken.Int64(), sdkmath.LegacyOneDec()); err != nil {
return stakingtypes.Validator{}, err
}
}
// TODO:
if err := k.stakingKeeper.DeleteLastValidatorPower(ctx, valAddr); err != nil {
return stakingtypes.Validator{}, err
}
if err := k.stakingKeeper.DeleteValidatorByPowerIndex(ctx, val); err != nil {
return stakingtypes.Validator{}, err
}
if err := k.slashKeeper.DeleteMissedBlockBitmap(ctx, consAddr); err != nil {
return stakingtypes.Validator{}, err
}
} else {
// Sets the new consensus power for the validator (this is executed in the x/staking ApplyAndReturnValidatorUpdates method)
if err := k.GetStakingKeeper().SetLastValidatorPower(ctx, valAddr, newBFTConsensusPower); err != nil {
return stakingtypes.Validator{}, err
}
if err := k.GetStakingKeeper().SetValidatorByPowerIndex(ctx, val); err != nil {
return stakingtypes.Validator{}, err
}

// Sets the new consensus power for the validator (this is executed in the x/staking ApplyAndReturnValidatorUpdates method)
if err := k.stakingKeeper.SetLastValidatorPower(ctx, valAddr, newBFTConsensusPower); err != nil {
return stakingtypes.Validator{}, err
// A cache to handle updated validators power. Once set, the next begin block will remove it from the cache.
// This allows us to Delete the validator index on the staking side, and ensures power updates do not persist over many blocks.
// This multi block persistence would break ABCI updates via x/staking if the validator's power is updated again, or removed.
if err := k.UpdatedValidatorsCache.Set(ctx, val.OperatorAddress); err != nil {
return stakingtypes.Validator{}, err
}
}

absPowerDiff := uint64(math.Abs(float64(newBFTConsensusPower - currentPower)))
Expand Down Expand Up @@ -129,6 +160,10 @@ func (k Keeper) AcceptNewValidator(ctx context.Context, operatingAddress string,
return err
}

if err := k.stakingKeeper.SetNewValidatorByPowerIndex(ctx, val); err != nil {
return err
}

// since the validator is set, remove it from the pending set
if err := k.RemovePendingValidator(ctx, val.OperatorAddress); err != nil {
return err
Expand Down Expand Up @@ -172,10 +207,6 @@ func (k Keeper) setValidatorInternals(ctx context.Context, val stakingtypes.Vali
return err
}

if err := k.stakingKeeper.SetNewValidatorByPowerIndex(ctx, val); err != nil {
return err
}

return k.stakingKeeper.Hooks().AfterValidatorCreated(ctx, valAddr)
}

Expand Down
84 changes: 84 additions & 0 deletions keeper/staking_hooks.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package keeper

import (
"context"

"github.com/cosmos/cosmos-sdk/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"

"cosmossdk.io/math"
)

// Before a validator is jailed, we must delete it from the power index. else:
// - ERR CONSENSUS FAILURE!!! err="should never retrieve a jailed validator from the power store"

var _ stakingtypes.StakingHooks = Hooks{}

// Hooks wrapper struct for staking keeper
type Hooks struct {
k Keeper
}

// Hooks return the mesh-security hooks
func (k Keeper) Hooks() Hooks {
return Hooks{k}
}

// BeforeValidatorModified implements types.StakingHooks.
func (h Hooks) BeforeValidatorModified(ctx context.Context, valAddr types.ValAddress) error {
h.k.logger.Info("BeforeValidatorModified", "valAddr", valAddr.String())
return h.k.BeforeJailedValidators.Set(ctx, valAddr.String())
}

// BeforeValidatorSlashed implements types.StakingHooks.
func (h Hooks) BeforeValidatorSlashed(ctx context.Context, valAddr types.ValAddress, fraction math.LegacyDec) error {
h.k.logger.Info("BeforeValidatorSlashed", valAddr.String(), fraction.String())
return nil
}

// ----------------------------

// AfterDelegationModified implements types.StakingHooks.
func (h Hooks) AfterDelegationModified(ctx context.Context, delAddr types.AccAddress, valAddr types.ValAddress) error {
return nil
}

// AfterUnbondingInitiated implements types.StakingHooks.
func (h Hooks) AfterUnbondingInitiated(ctx context.Context, id uint64) error {
return nil
}

// AfterValidatorBeginUnbonding implements types.StakingHooks.
func (h Hooks) AfterValidatorBeginUnbonding(ctx context.Context, consAddr types.ConsAddress, valAddr types.ValAddress) error {
return nil
}

// AfterValidatorBonded implements types.StakingHooks.
func (h Hooks) AfterValidatorBonded(ctx context.Context, consAddr types.ConsAddress, valAddr types.ValAddress) error {
return nil
}

// AfterValidatorCreated implements types.StakingHooks.
func (h Hooks) AfterValidatorCreated(ctx context.Context, valAddr types.ValAddress) error {
return nil
}

// AfterValidatorRemoved implements types.StakingHooks.
func (h Hooks) AfterValidatorRemoved(ctx context.Context, consAddr types.ConsAddress, valAddr types.ValAddress) error {
return nil
}

// BeforeDelegationCreated implements types.StakingHooks.
func (h Hooks) BeforeDelegationCreated(ctx context.Context, delAddr types.AccAddress, valAddr types.ValAddress) error {
return nil
}

// BeforeDelegationRemoved implements types.StakingHooks.
func (h Hooks) BeforeDelegationRemoved(ctx context.Context, delAddr types.AccAddress, valAddr types.ValAddress) error {
return nil
}

// BeforeDelegationSharesModified implements types.StakingHooks.
func (h Hooks) BeforeDelegationSharesModified(ctx context.Context, delAddr types.AccAddress, valAddr types.ValAddress) error {
return nil
}
5 changes: 5 additions & 0 deletions keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ var (
// AbsoluteChangedInBlockPowerKey tracks the current blocks total power amount.
// If this becomes >30% of CachedPreviousBlockPowerKey, messages will fail to limit IBC issues.
AbsoluteChangedInBlockPowerKey = collections.NewPrefix(3)

// UpdatedValidatorsCacheKey tracks recently updated validators from SetPower.
UpdatedValidatorsCacheKey = collections.NewPrefix(4)

BeforeJailedValidatorsKey = collections.NewPrefix(5)
)

const (
Expand Down
Loading

0 comments on commit dc4ad45

Please sign in to comment.