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

Jump to bottom

fix(poa-abci-patch): get working #159

Closed
wants to merge 3 commits into from
Closed

fix(poa-abci-patch): get working #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e31eb07
initial
Reecepbcups Apr 14, 2024
4e9a2e3
simplify
Reecepbcups Apr 14, 2024
3000cc7
add e2e for duplicate entry issue
Reecepbcups Apr 14, 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
11 changes: 10 additions & 1 deletion e2e/poa_add_test.go
View file
Open in desktop
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 @@ -81,8 +82,10 @@ func TestPOAAddValidator(t *testing.T) {
require.Equal(t, "0", pending[0].Tokens)
require.Equal(t, "1", pending[0].MinSelfDelegation)

opAddr := pending[0].OperatorAddress

// set power from Gov
txRes, err = helpers.POASetPower(t, ctx, chain, admin, pending[0].OperatorAddress, 1_000_000)
txRes, err = helpers.POASetPower(t, ctx, chain, admin, opAddr, 1_000_000)
require.NoError(t, err)
log.Debug().Msgf("Set pending's power: %v", txRes)

Expand All @@ -95,4 +98,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, opAddr, 2_000_000)
require.NoError(t, err)

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

}
1 change: 0 additions & 1 deletion keeper/poa.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,6 @@ func (k Keeper) AcceptNewValidator(ctx context.Context, operatingAddress string,
return err
}

// TODO: keep or remove since this is done in the endblocker now
if err := k.stakingKeeper.SetNewValidatorByPowerIndex(ctx, val); err != nil {
return err
}
Expand Down
173 changes: 94 additions & 79 deletions module/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (

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

"github.com/strangelove-ventures/poa"
)
Expand All @@ -21,44 +20,64 @@ func (am AppModule) BeginBlocker(ctx context.Context) error {
return nil
}

valUpdates, err := am.keeper.GetStakingKeeper().GetValidatorUpdates(ctx)
// valUpdates, err := am.keeper.GetStakingKeeper().GetValidatorUpdates(ctx)
// if err != nil {
// return err
// }

// if len(valUpdates) != 0 {
vals, err := am.keeper.GetStakingKeeper().GetAllValidators(ctx)
if err != nil {
return err
}

if len(valUpdates) != 0 {
vals, err := am.keeper.GetStakingKeeper().GetAllValidators(ctx)
if err != nil {
return err
}

for _, val := range vals {
valBz, err := sdk.ValAddressFromBech32(val.OperatorAddress)
if err != nil {
// TODO: Do we want to run this all the time, or with only val updates of != 0?
for _, val := range vals {
// valBz, err := sdk.ValAddressFromBech32(val.OperatorAddress)
// if err != nil {
// return err
// }

if val.IsJailed() {
// This fixes: "failed to apply block; error commit failed for application: changing validator set: duplicate entry"
// This is why we require poa to be before staking in the SetOrderBeginBlocker array
if err := am.keeper.GetStakingKeeper().DeleteValidatorByPowerIndex(ctx, val); err != nil {
return err
}
// TODO: empty set issues on jail
// } else if val.Status == stakingtypes.Bonded && len(valUpdates) > 0 {
// if err := am.keeper.GetStakingKeeper().DeleteValidatorByPowerIndex(ctx, val); err != nil {
// return err
// }

if val.Status == stakingtypes.Bonded {
lastPower, err := am.keeper.GetStakingKeeper().GetLastValidatorPower(ctx, valBz)
if err != nil {
return err
}

// This fixes: "failed to apply block; error commit failed for application: changing validator set: duplicate entry"
// This is why we require poa to be before staking in the SetOrderBeginBlocker array
if err := am.keeper.GetStakingKeeper().DeleteValidatorByPowerIndex(ctx, val); err != nil {
return err
}

if err := am.keeper.GetStakingKeeper().SetLastValidatorPower(ctx, valBz, lastPower); err != nil {
return err
}

if err := am.keeper.GetStakingKeeper().SetValidatorByPowerIndex(ctx, val); err != nil {
return err
}
}
// set it back to bonded
}
// }

// if val.Status == stakingtypes.Bonded {
// lastPower, err := am.keeper.GetStakingKeeper().GetLastValidatorPower(ctx, valBz)
// if err != nil {
// return err
// }

// // This fixes: "failed to apply block; error commit failed for application: changing validator set: duplicate entry"
// // This is why we require poa to be before staking in the SetOrderBeginBlocker array
// if err := am.keeper.GetStakingKeeper().DeleteValidatorByPowerIndex(ctx, val); err != nil {
// return err
// }

// if err := am.keeper.GetStakingKeeper().SetLastValidatorPower(ctx, valBz, lastPower); err != nil {
// return err
// }

// // `SetValidatorByPowerIndex` would forever persist if you do not DeleteValidatorByPowerIndex first.
// // This is used as reference for any future code written as a reminder.
// // Instead, staking handles it for us :)
// // if err := am.keeper.GetStakingKeeper().SetValidatorByPowerIndex(ctx, val); err != nil {
// // return err
// // }
// }
// }
}

return nil
Expand All @@ -70,59 +89,59 @@ func (am AppModule) EndBlocker(ctx context.Context) error {
sdkCtx := sdk.UnwrapSDKContext(ctx)
defer telemetry.ModuleMeasureSince(poa.ModuleName, sdkCtx.BlockTime(), telemetry.MetricKeyEndBlocker)

vals, err := am.keeper.GetStakingKeeper().GetAllValidators(ctx)
if err != nil {
return err
}
// vals, err := am.keeper.GetStakingKeeper().GetAllValidators(ctx)
// if err != nil {
// return err
// }

valUpdates, err := am.keeper.GetStakingKeeper().GetValidatorUpdates(ctx)
if err != nil {
return err
}

for _, valUpdate := range valUpdates {
am.keeper.Logger().Info("ValUpdate Before", "pubkey", valUpdate.PubKey.String(), "power", valUpdate.Power)
am.keeper.Logger().Info("POA EndBlocker ValUpdate", "pubkey", valUpdate.PubKey.String(), "power", valUpdate.Power)
}

for _, v := range vals {
valAddr, err := sdk.ValAddressFromBech32(v.OperatorAddress)
if err != nil {
return err
}

switch v.GetStatus() {
case stakingtypes.Unbonding:
continue
case stakingtypes.Unbonded:
// if the validator is unbonded (above case), delete the last validator power. (H+2)
if err := am.keeper.GetStakingKeeper().DeleteLastValidatorPower(ctx, valAddr); err != nil {
return err
}

case stakingtypes.Unspecified, stakingtypes.Bonded:
if !v.DelegatorShares.IsZero() {
// TODO: new val cache still needed?
// if the validator is freshly created, then perform the validator update.
isNewVal, err := am.keeper.NewValidatorsCache.Has(ctx, v.GetOperator())
if err != nil {
return err
}

// power, err := am.keeper.GetStakingKeeper().GetLastValidatorPower(ctx, valAddr)
// if err != nil {
// return err
// }

if isNewVal {
am.keeper.Logger().Info("New Validator", "operator", v.GetOperator(), "bonded_tokens", v.GetBondedTokens())
if err := am.keeper.NewValidatorsCache.Remove(ctx, v.GetOperator()); err != nil {
return err
}
}
}
continue
}
}
// for _, v := range vals {
// valAddr, err := sdk.ValAddressFromBech32(v.OperatorAddress)
// if err != nil {
// return err
// }

// switch v.GetStatus() {
// case stakingtypes.Unbonding:
// continue
// case stakingtypes.Unbonded:
// // if the validator is unbonded (above case), delete the last validator power. (H+2)
// if err := am.keeper.GetStakingKeeper().DeleteLastValidatorPower(ctx, valAddr); err != nil {
// return err
// }

// case stakingtypes.Unspecified, stakingtypes.Bonded:
// if !v.DelegatorShares.IsZero() {
// // TODO: new val cache still needed?
// // if the validator is freshly created, then perform the validator update.
// isNewVal, err := am.keeper.NewValidatorsCache.Has(ctx, v.GetOperator())
// if err != nil {
// return err
// }

// // power, err := am.keeper.GetStakingKeeper().GetLastValidatorPower(ctx, valAddr)
// // if err != nil {
// // return err
// // }

// if isNewVal {
// am.keeper.Logger().Info("New Validator", "operator", v.GetOperator(), "bonded_tokens", v.GetBondedTokens())
// if err := am.keeper.NewValidatorsCache.Remove(ctx, v.GetOperator()); err != nil {
// return err
// }
// }
// }
// continue
// }
// }

if sdkCtx.BlockHeight() > 1 {
// non gentx messages reset the cached block powers for IBC validations.
Expand All @@ -135,10 +154,6 @@ func (am AppModule) EndBlocker(ctx context.Context) error {
}
}

for _, valUpdate := range valUpdates {
am.keeper.Logger().Info("ValUpdate After", "pubkey", valUpdate.PubKey.String(), "power", valUpdate.Power)
}

return err
}

Expand Down
Loading