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

feat: pause unbondings during equivocation proposal voting period #791

Closed
wants to merge 2 commits into from
Closed

feat: pause unbondings during equivocation proposal voting period #791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51d80be
feat: pause unbondings during equivocation proposal voting period
tbruyelle Mar 10, 2023
6df4b95
replace waitFor with existing incrementTime
tbruyelle Mar 22, 2023
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
14 changes: 12 additions & 2 deletions app/provider/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ func New(
stakingtypes.NewMultiStakingHooks(
app.DistrKeeper.Hooks(),
app.SlashingKeeper.Hooks(),
app.ProviderKeeper.Hooks(),
app.ProviderKeeper.Hooks(app.GovKeeper),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell this is not very conventional, but on the other hand, adding the govKeeper to the providerKeeper is even more ugly, because the govKeeper needs the providerKeeper for the proposal routes. So we would have to write something like (pseudo code):

providerKeeper := providerKeeper.NewKeeper( ...  govKeeper ...) // govKeeper is nil for now
govKeeper := govKeeper.NewKeeper( ... providerKeeper ... )
providerKeeper.SetGovKeeper( govKeeper ) // finally set a non-nil govKeeper

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against the current design per say, but imo adding a govKeeper pointer ref to providerKeeper, while using SetGovKeeper is the easiest to understand. No need to add a parameter for govKeeper in providerKeeper.NewKeeper(). You could just auto set govKeeper to nil, until it is explicitly set after the constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see one benefit of having an additional parameter in Hooks(): it breaks compilation after upgrade. Typically when gaia will upgrade its interchain-security dependency, the fix will be obvious, while on the other hand, having to call SetGovKeeper can be easily forgotten.
Is there any validation we could setup to ensure SetGovKeeper is actually called ? A little bit like keeper.mustValidateField but for setters.

),
)

Expand Down Expand Up @@ -426,7 +426,7 @@ func New(
AddRoute(providertypes.RouterKey, ibcprovider.NewProviderProposalHandler(app.ProviderKeeper)).
AddRoute(ibcclienttypes.RouterKey, ibcclient.NewClientProposalHandler(app.IBCKeeper.ClientKeeper))

app.GovKeeper = govkeeper.NewKeeper(
govKeeper := govkeeper.NewKeeper(
appCodec,
keys[govtypes.StoreKey],
app.GetSubspace(govtypes.ModuleName),
Expand All @@ -435,6 +435,11 @@ func New(
&stakingKeeper,
govRouter,
)
app.GovKeeper = *govKeeper.SetHooks(
govtypes.NewMultiGovHooks(
app.ProviderKeeper.Hooks(govKeeper),
),
)

app.TransferKeeper = ibctransferkeeper.NewKeeper(
appCodec,
Expand Down Expand Up @@ -763,6 +768,11 @@ func (app *App) GetE2eStakingKeeper() e2e.E2eStakingKeeper {
return app.StakingKeeper
}

// GetE2eGovKeeper implements the ProviderApp interface.
func (app *App) GetE2eGovKeeper() e2e.E2eGovKeeper {
return app.GovKeeper
}

// GetE2eBankKeeper implements the ProviderApp interface.
func (app *App) GetE2eBankKeeper() e2e.E2eBankKeeper {
return app.BankKeeper
Expand Down
50 changes: 50 additions & 0 deletions tests/e2e/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,56 @@ func checkStakingUnbondingOps(s *CCVTestSuite, id uint64, found bool, onHold boo
}
}

// unbondingsOnHold is a handy struct to hold the different kinds of unbonding
// refcount.
type unbondingsOnHold struct {
unbondingDelegationRefCount int64
validatorUnbondingRefCount int64
redelegationRefCount int64
}

// checkStakingUnbondingOnHoldRefCount checks that valAddr's unbonding refcounts
// match expected.
func checkStakingUnbondingOnHoldRefCount(s *CCVTestSuite, valAddr sdk.ValAddress, expected unbondingsOnHold) {

// check unbondingDelegation
ubds := s.providerApp.GetE2eStakingKeeper().GetUnbondingDelegationsFromValidator(s.providerCtx(), valAddr)
if expected.unbondingDelegationRefCount == 0 {
s.Assert().Empty(ubds, "expected no unbonding delegation")
} else {
s.Assert().NotEmpty(ubds, "no unbonding delegation found")
for _, ubd := range ubds {
s.Assert().NotEmpty(ubd.Entries, "no unbonding delegation entries found")
for _, entry := range ubd.Entries {
s.Assert().Equal(expected.unbondingDelegationRefCount, entry.UnbondingOnHoldRefCount,
"wrong unbonding delegation ref count")
}
}
}

// check redelegation
reds := s.providerApp.GetE2eStakingKeeper().GetRedelegationsFromSrcValidator(s.providerCtx(), valAddr)
if expected.redelegationRefCount == 0 {
s.Assert().Empty(reds, "expected no redelegation")
} else {
s.Assert().NotEmpty(reds, "no redelegation found")
for _, ubd := range reds {
s.Assert().NotEmpty(ubd.Entries, "no redelegation entries found")
for _, entry := range ubd.Entries {
s.Assert().Equal(expected.redelegationRefCount, entry.UnbondingOnHoldRefCount,
"wrong redelegation ref count")
}
}
}

// check validator unbonding
val, found := s.providerApp.GetE2eStakingKeeper().GetValidator(s.providerCtx(), valAddr)
if s.Assert().True(found, "validator not found") {
s.Assert().Equal(expected.validatorUnbondingRefCount, val.UnbondingOnHoldRefCount,
"wrong validator unbonding ref count")
}
}

func checkCCVUnbondingOp(s *CCVTestSuite, providerCtx sdk.Context, chainID string, valUpdateID uint64, found bool, msgAndArgs ...interface{}) {
entries := s.providerApp.GetProviderKeeper().GetUnbondingOpsFromIndex(providerCtx, chainID, valUpdateID)
if found {
Expand Down
236 changes: 236 additions & 0 deletions tests/e2e/proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,236 @@
package e2e

import (
"time"

sdk "github.com/cosmos/cosmos-sdk/types"
evidencetypes "github.com/cosmos/cosmos-sdk/x/evidence/types"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
providertypes "github.com/cosmos/interchain-security/x/ccv/provider/types"
)

func (s *CCVTestSuite) TestPauseUnbondingOnEquivocationProposal() {
tests := []struct {
name string
setup func() govtypes.Content
expectedWithoutProp unbondingsOnHold
expectedDuringProp unbondingsOnHold
}{
{
// Assert a text proposal doesn't pause any existing unbondings
name: "text proposal and unbonding delegation",
setup: func() govtypes.Content {
var (
bondAmt = sdk.NewInt(10000000)
delAddr = s.providerChain.SenderAccount.GetAddress()
)
// delegate bondAmt and undelegate it
delegateAndUndelegate(s, delAddr, bondAmt, 1)

return govtypes.NewTextProposal("title", "desc")
},
expectedWithoutProp: unbondingsOnHold{
unbondingDelegationRefCount: 1,
},
expectedDuringProp: unbondingsOnHold{
unbondingDelegationRefCount: 1,
},
},
{
// Assert an equivocation proposal pauses nothing if there's no existing
// unbondings.
name: "equivocation proposal and no unbonding",
setup: func() govtypes.Content {
var (
val, _ = s.getValByIdx(0)
consAddr, _ = val.GetConsAddr()
)
return providertypes.NewEquivocationProposal("title", "desc",
[]*evidencetypes.Equivocation{{
Height: 1,
Power: 1,
Time: time.Now(),
ConsensusAddress: consAddr.String(),
}},
)
},
expectedWithoutProp: unbondingsOnHold{},
expectedDuringProp: unbondingsOnHold{},
},
{
// Assert an equivocation proposal pauses unbonding delegations
name: "equivocation proposal and unbonding delegation",
setup: func() govtypes.Content {
// create an unbonding entry of type UnbondingDelegation
var (
bondAmt = sdk.NewInt(10000000)
delAddr = s.providerChain.SenderAccount.GetAddress()
val, _ = s.getValByIdx(0)
consAddr, _ = val.GetConsAddr()
)
// delegate bondAmt and undelegate it
delegateAndUndelegate(s, delAddr, bondAmt, 1)

return providertypes.NewEquivocationProposal("title", "desc",
[]*evidencetypes.Equivocation{{
Height: 1,
Power: 1,
Time: time.Now(),
ConsensusAddress: consAddr.String(),
}},
)
},
expectedWithoutProp: unbondingsOnHold{
unbondingDelegationRefCount: 1,
},
expectedDuringProp: unbondingsOnHold{
unbondingDelegationRefCount: 2,
},
},
{
// Assert an equivocation proposal pauses redelegations
name: "equivocation proposal and redelegate",
setup: func() govtypes.Content {
// create an unbonding entry of type UnbondingDelegation
var (
bondAmt = sdk.NewInt(10000000)
delAddr = s.providerChain.SenderAccount.GetAddress()
val, valSrcAddr = s.getValByIdx(0)
_, valDstAddr = s.getValByIdx(1)
consAddr, _ = val.GetConsAddr()
)
// delegate bondAmt and redelegate it
delegateAndRedelegate(s, delAddr, valSrcAddr, valDstAddr, bondAmt)

return providertypes.NewEquivocationProposal("title", "desc",
[]*evidencetypes.Equivocation{{
Height: 1,
Power: 1,
Time: time.Now(),
ConsensusAddress: consAddr.String(),
}},
)
},
expectedWithoutProp: unbondingsOnHold{
redelegationRefCount: 1,
},
expectedDuringProp: unbondingsOnHold{
redelegationRefCount: 2,
},
},
{
// Assert an equivocation proposal pauses validator unbonding
name: "equivocation proposal and validator unbonding",
setup: func() govtypes.Content {
var (
delAddr = s.providerChain.SenderAccount.GetAddress()
val, valAddr = s.getValByIdx(0)
consAddr, _ = val.GetConsAddr()
)
// unbond validator by undelegate all his delegation (test setup only
// delegates from delAddr, there's no validator self delegation)
undelegate(s, delAddr, valAddr, sdk.NewDec(1))

return providertypes.NewEquivocationProposal("title", "desc",
[]*evidencetypes.Equivocation{{
Height: 1,
Power: 1,
Time: time.Now(),
ConsensusAddress: consAddr.String(),
}},
)
},
expectedWithoutProp: unbondingsOnHold{
unbondingDelegationRefCount: 1,
validatorUnbondingRefCount: 1,
},
expectedDuringProp: unbondingsOnHold{
unbondingDelegationRefCount: 2,
validatorUnbondingRefCount: 2,
},
},
}
submitProp := func(s *CCVTestSuite, content govtypes.Content) uint64 {
proposal, err := s.providerApp.GetE2eGovKeeper().SubmitProposal(s.providerCtx(), content)
s.Require().NoError(err)
return proposal.ProposalId
}
addDepositProp := func(s *CCVTestSuite, proposalID uint64, depositAmt sdk.Coins) {
depositorAddr := s.providerChain.SenderAccount.GetAddress()
_, err := s.providerApp.GetE2eGovKeeper().AddDeposit(
s.providerCtx(), proposalID, depositorAddr, depositAmt,
)
s.Require().NoError(err)
}
for i, tt := range tests {
s.Run(tt.name, func() {
if i+1 < len(tests) {
// reset suite to reset provider client
defer s.SetupTest()
}

//-----------------------------------------
// Setup

var (
govContent = tt.setup()
val, valAddr = s.getValByIdx(0)
consAddr, _ = val.GetConsAddr()
govKeeper = s.providerApp.GetE2eGovKeeper()
dustAmt = sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(1)))
minDepositAmt = govKeeper.GetDepositParams(s.providerCtx()).MinDeposit
)
// Equivocation proposal requires validator signing info and a slash log
s.setDefaultValSigningInfo(*s.providerChain.Vals.Validators[0])
s.providerApp.GetProviderKeeper().SetSlashLog(s.providerCtx(),
providertypes.NewProviderConsAddress(consAddr))
// Reduce voting period
votingParams := govKeeper.GetVotingParams(s.providerCtx())
votingParams.VotingPeriod = 3 * time.Second
govKeeper.SetVotingParams(s.providerCtx(), votingParams)
// Reduce deposit period
depositParams := govKeeper.GetDepositParams(s.providerCtx())
depositParams.MaxDepositPeriod = 3 * time.Second
govKeeper.SetDepositParams(s.providerCtx(), depositParams)
// must move one block forward because unbonding validators are detected
// during EndBlock.
s.providerChain.NextBlock()
checkStakingUnbondingOnHoldRefCount(s, valAddr, tt.expectedWithoutProp)

//-----------------------------------------
// #1 Create a proposal, activate it and wait for voting period

proposalID := submitProp(s, govContent)
checkStakingUnbondingOnHoldRefCount(s, valAddr, tt.expectedWithoutProp)
// assert that until voting period starts, there's no pause triggered
addDepositProp(s, proposalID, dustAmt)
checkStakingUnbondingOnHoldRefCount(s, valAddr, tt.expectedWithoutProp)
// activate prop
addDepositProp(s, proposalID, minDepositAmt)
checkStakingUnbondingOnHoldRefCount(s, valAddr, tt.expectedDuringProp)
// assert that an additionnal deposit done after the activation doesn't
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth a quick check here to validate the voting period has not been reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can that happen ? I see no message in the gov module that could reset the voting period.

// trigger additionnal pauses
addDepositProp(s, proposalID, dustAmt)
checkStakingUnbondingOnHoldRefCount(s, valAddr, tt.expectedDuringProp)
// ends the voting period
incrementTime(s, votingParams.VotingPeriod)
checkStakingUnbondingOnHoldRefCount(s, valAddr, tt.expectedWithoutProp)
s.Assert().False(
s.providerApp.GetProviderKeeper().HasEquivocationProposal(s.providerCtx(), proposalID),
"proposalID should be removed from store after voting period",
)

//-----------------------------------------
// #2 Create an other proposal but let it expire (not enough deposit)

proposalID = submitProp(s, govContent)
checkStakingUnbondingOnHoldRefCount(s, valAddr, tt.expectedWithoutProp)
incrementTime(s, depositParams.MaxDepositPeriod)
checkStakingUnbondingOnHoldRefCount(s, valAddr, tt.expectedWithoutProp)
s.Assert().False(
s.providerApp.GetProviderKeeper().HasEquivocationProposal(s.providerCtx(), proposalID),
"proposalID shouldn't be registered if proposal didn't reach the voting period",
)
})
}
}
3 changes: 3 additions & 0 deletions testutil/e2e/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ type ProviderApp interface {
GetE2eSlashingKeeper() E2eSlashingKeeper
// Returns a distribution keeper interface with more capabilities than the expected_keepers interface
GetE2eDistributionKeeper() E2eDistributionKeeper
// Returns a gov keeper interface with more capabilities than the expected_keepers interface
GetE2eGovKeeper() E2eGovKeeper
}

// The interface that any consumer app must implement to be compatible with e2e tests
Expand Down Expand Up @@ -140,6 +142,7 @@ type E2eMintKeeper interface {

type E2eGovKeeper interface {
GetDepositParams(ctx sdk.Context) govtypes.DepositParams
SetDepositParams(sdk.Context, govtypes.DepositParams)
GetVotingParams(ctx sdk.Context) govtypes.VotingParams
SetVotingParams(ctx sdk.Context, votingParams govtypes.VotingParams)
SubmitProposal(ctx sdk.Context, content govtypes.Content) (govtypes.Proposal, error)
Expand Down
Loading