fix(abci v2): Fix

INTEGRATION.md

@@ -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(
@@ -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,

e2e/poa_add_test.go

@@ -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"
 )
@@ -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))
+
 }

e2e/poa_jail_test.go

@@ -1,6 +1,7 @@
 package e2e
 
 import (
+	"fmt"
 	"testing"
 	"time"
 
@@ -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",
@@ -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
 			}
@@ -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))
 }

e2e/poa_remove_test.go

@@ -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"
@@ -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 {
@@ -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
+}

keeper/keeper.go

@@ -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]
@@ -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)),

keeper/poa.go

@@ -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)))
@@ -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
@@ -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)
 }
 

keeper/staking_hooks.go

@@ -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
+}

keys.go

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