feat: pause unbondings during equivocation proposal voting period #791
Conversation
Fix cosmos#747 The change registers 2 gov module hooks in the provider module: - `AfterProposalDeposit`: if the proposal is an equivocation proposal in voting period, then call `PutUnbondingOnHold` for each unbondings of each validators found in the proposal. - `AfterProposalVotingPeriodEnded`: if the proposal is an equivocation proposal, then call `UnbondingCanComplete` for each unbondings of each validators found in the proposal. A new key is also added, to store the equivocation proposal ID for which unbondings have been put on hold. This covers 2 specific cases: - The gov module allows additional deposits even if the proposal is already in voting period. So when `AfterProposalDeposit` is invoked, we have to make sure the proposal is in voting period for the first time before puting the unbondings on hold. This is simply handled by checking if the proposal ID exists in the store at the beginning of the hook, and then storing it if not. - If the provider chain is upgraded with this change and there's already an equivocation proposal in voting period, `AfterProposalVotingPeriodEnded` could be invoked without the initial `AfterProposalDeposit`, so some unbondings could be un-paused while they hadn't been paused (conflicts with `AfterUnbondingInitiated` hook). To prevent that, we check the proposal ID exists in the store, which means `AfterProposalDeposit` has been called prevouisly. Co-authored-by: Albert Le Batteux <[email protected]>
tbruyelle
requested review from
jtremback,
mpoke,
sainoe,
shaspitz,
MSalopek,
mmulji-ic,
glnro and
yaruwangway
as code owners
| @@ -376,7 +376,7 @@ func New( | |||
| stakingtypes.NewMultiStakingHooks( | |||
| app.DistrKeeper.Hooks(), | |||
| app.SlashingKeeper.Hooks(), | |||
| app.ProviderKeeper.Hooks(), | |||
| app.ProviderKeeper.Hooks(app.GovKeeper), | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Thanks! Will review this soon. |
| @@ -376,7 +376,7 @@ func New( | |||
| stakingtypes.NewMultiStakingHooks( | |||
| app.DistrKeeper.Hooks(), | |||
| app.SlashingKeeper.Hooks(), | |||
| app.ProviderKeeper.Hooks(), | |||
| app.ProviderKeeper.Hooks(app.GovKeeper), | |||
There was a problem hiding this comment.
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
| // activate prop | ||
| addDepositProp(s, proposalID, minDepositAmt) | ||
| checkStakingUnbondingOnHoldRefCount(s, valAddr, tt.expectedDuringProp) | ||
| // assert that an additionnal deposit done after the activation doesn't |
There was a problem hiding this comment.
Is it worth a quick check here to validate the voting period has not been reset?
There was a problem hiding this comment.
How can that happen ? I see no message in the gov module that could reset the voting period.
|
Do you guys figure that this one is complete? I think it might solve one of the test cases in the 47 work that we have been doing |
|
Converting to draft as this feature requires an analysis on the potential risks. @tbruyelle @jtremback it would be useful to create an ADR to be able to discuss on it. |
OK, we'll work on an ADR with @albttx and @giunatale. |
ADR ready for review #964 |
|
@tbruyelle Closing this as the equivocation proposal was removed by the cryptographic verification of equivocation feature #1340. |
Description
The change registers 2 gov module hooks in the provider module:
AfterProposalDeposit: if the proposal is an equivocation proposal in voting period, then callPutUnbondingOnHoldfor each unbondings of each validators found in the proposal.AfterProposalVotingPeriodEnded: if the proposal is an equivocation proposal, then callUnbondingCanCompletefor each unbondings of each validators found in the proposal.A new key is also added, to store the equivocation proposal ID for which unbondings have been put on hold. This covers 2 specific cases:
The gov module allows additional deposits even if the proposal is already in voting period. So when
AfterProposalDepositis invoked, we have to make sure the proposal is in voting period for the first time before puting the unbondings on hold. This is simply handled by checking if the proposal ID exists in the store at the beginning of the hook, and then storing it if not.If the provider chain is upgraded with this change and there's already an equivocation proposal in voting period,
AfterProposalVotingPeriodEndedcould be invoked without the initialAfterProposalDeposit, so some unbondings could be un-paused while they hadn't been paused (conflicts withAfterUnbondingInitiatedhook). To prevent that, we check the proposal ID exists in the store, which meansAfterProposalDeposithas been called prevouisly.Linked issues
Closes: #747
Type of change
If you've checked more than one of the first three boxes, consider splitting this PR into multiple PRs!
Feature: Changes and/or adds code behavior, irrelevant to bug fixesFix: Changes and/or adds code behavior, specifically to fix a bugRefactor: Changes existing code style, naming, structure, etc.Testing: Adds testingDocs: Adds documentationRegression tests
If
Refactor, describe the new or existing tests that verify no behavior was changed or added where refactors were introduced.New behavior tests
If
FeatureorFix, describe the new or existing tests that verify the new behavior is correct and expected.A new e2e test have been added in
tests/e2e/proposal.go.Versioning Implications
If the above box is checked, which version should be bumped?
MAJOR: Consensus breaking changes to both the provider and consumers(s), including updates/breaking changes to IBC communication between provider and consumer(s)MINOR: Consensus breaking changes which affect either only the provider or only the consumer(s)PATCH: Non consensus breaking changesTargeting
Please select one of the following: