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.

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

handle provider and consumer client expiration #448

Merged
merged 28 commits into from
Nov 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
feee18e
handle expired client when sending packets
mpoke Nov 7, 2022
6aba4b5
add e2e test
mpoke Nov 7, 2022
33d2ed7
add upgradeExpiredClient to e2e tests
mpoke Nov 8, 2022
2e56128
improve incrementTime... functions
mpoke Nov 8, 2022
23900ed
fix golangci-lint error
mpoke Nov 8, 2022
bee27d5
add client expired check
mpoke Nov 8, 2022
1535ef5
replace incrementTimeBy w/ incrementTime
mpoke Nov 9, 2022
8baa1d4
replace AppendPendingVSC w/ AppendPendingVSCs
mpoke Nov 9, 2022
df328c6
simplify logic of sendValidatorUpdates
mpoke Nov 9, 2022
c8229b8
separate PrepareIBCPacketSend from SendIBCPacket
mpoke Nov 9, 2022
18368e9
error handling on SendPacket
mpoke Nov 9, 2022
c03153b
export pending VSC packets
mpoke Nov 9, 2022
6ca687f
improve comments
mpoke Nov 9, 2022
69483d9
use k.GetCCVTimeoutPeriod
mpoke Nov 9, 2022
05da138
remove GetUpgradeKeeper
mpoke Nov 9, 2022
ccd516b
AppendPendingVSCs: use variadic function
mpoke Nov 10, 2022
ef28ec7
remove unnecessary if
mpoke Nov 11, 2022
0bfa8ce
refactor pending VSC CRUD methods
MSalopek Nov 15, 2022
886e0ef
refactor sending valset updates to chains
MSalopek Nov 15, 2022
a691517
add tests for VSC queueing
MSalopek Nov 15, 2022
e3e2c00
refactor after reviews
MSalopek Nov 15, 2022
654df12
refactor after reviews
MSalopek Nov 16, 2022
654b466
Merge marius/435-client-expired-consumer into marius/435-client-expired
MSalopek Nov 16, 2022
8ff123c
and packet queueing to consumer keeper
MSalopek Nov 16, 2022
c46c0f0
refactor e2e tests
MSalopek Nov 17, 2022
e2f69ad
refactor after review session
MSalopek Nov 17, 2022
91024ae
additional refactor after reviews
MSalopek Nov 17, 2022
3b5f91a
Merge branch 'main' into marius/435-client-expired
jtremback Nov 17, 2022
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
5 changes: 5 additions & 0 deletions app/consumer-democracy/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,11 @@ func (app *App) GetE2eEvidenceKeeper() e2e.E2eEvidenceKeeper {
return app.EvidenceKeeper
}

// GetUpgradeKeeper implements the ConsumerApp interface.
func (app *App) GetUpgradeKeeper() upgradekeeper.Keeper {
mpoke marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer needed

return app.UpgradeKeeper
}

// GetE2eStakingKeeper implements the ConsumerApp interface.
func (app *App) GetE2eStakingKeeper() e2e.E2eStakingKeeper {
return app.StakingKeeper
Expand Down
5 changes: 5 additions & 0 deletions app/consumer/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,11 @@ func (app *App) GetE2eEvidenceKeeper() e2e.E2eEvidenceKeeper {
return app.EvidenceKeeper
}

// GetUpgradeKeeper implements the ConsumerApp interface.
func (app *App) GetUpgradeKeeper() upgradekeeper.Keeper {
mpoke marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto: No longer needed

return app.UpgradeKeeper
}

// TestingApp functions

// GetBaseApp implements the TestingApp interface.
Expand Down
6 changes: 3 additions & 3 deletions docs/quality_assurance.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ IBC packets:
| -- | ------- | ----------- | ------------ | ----------- | ------------- | ------- |
| 2.01 | Create IBC clients | `Scheduled` (ibc-go) | `Done` [TestCreateConsumerClient](../x/ccv/provider/keeper/proposal_test.go#117), [TestInitGenesis](../x/ccv/consumer/keeper/genesis_test.go#26) | `Done` [SetupTest](../tests/e2e/setup_test.go#39), [TestConsumerGenesis](../tests/e2e/channel_init_test.go#21) | `Future work` | `Scheduled` |
| 2.02 | Create CCV channel (handshake) | `Scheduled` (ibc-go) | `Done` [provider/ibc_module_test.go](../x/ccv/provider/ibc_module_test.go), [consumer/ibc_module_test.go](../x/ccv/consumer/ibc_module_test.go) | `Done` [SetupCCVChannel](../tests/e2e/setup_test.go#125) | `Future work` | `Scheduled` |
| 2.03 | Sending IBC packets <br> [SendIBCPacket](../x/ccv/utils/utils.go#40) | `Scheduled` (ibc-go) | `NA` | `Done` [TestSendVSCMaturedPackets](../tests/e2e/valset_update_test.go#39), [TestSendSlashPacket](../tests/e2e/slashing_test.go#648) | `Done` | `Scheduled` |
| 2.03 | Sending IBC packets | `Scheduled` (ibc-go) | `NA` | `Done` [TestSendVSCMaturedPackets](../tests/e2e/valset_update_test.go#39), [TestSendSlashPacket](../tests/e2e/slashing_test.go#648) | `Done` | `Scheduled` |
| 2.04 | Handling acknowledgments | `Scheduled` (ibc-go) | [Scheduled](https://github.com/cosmos/interchain-security/issues/362) | `Partial coverage` [TestOnAcknowledgementPacket](../x/ccv/consumer/keeper/relay_test.go#152), [TestSlashPacketAcknowldgement](../tests/e2e/slashing_test.go#258) | `Done` | `Scheduled` |
| 2.05 | Handling timeouts | `Scheduled` (ibc-go) | [Scheduled](https://github.com/cosmos/interchain-security/issues/362) |`NA` | `Future work` | `Scheduled` |
| 2.06 | Handling IBC client expiration <br> - high priority| `Scheduled` (ibc-go) | `NA` | `NA` | `Future work` | `Scheduled` |
| 2.06 | Handling IBC client expiration | `Scheduled` (ibc-go) | `NA` | `Done` [expired_client.go](../tests/e2e/expired_client.go) | `Future work` | `Scheduled` |
| 2.07 | ICS-20 channel creation | `Scheduled` (ibc-go) | `NA` | `Done` [SetupTransferChannel](../tests/e2e/setup_test.go#152) |`Future work` | `Scheduled` |
| 2.08 | ICS-20 transfer | `Scheduled` (ibc-go) | `NA` | `Done` [TestRewardsDistribution](../tests/e2e/distribution_test.go#17) | `NA` | `Scheduled` |
| 2.09 | Changes in IBC-GO testing suite | `Scheduled` (ibc-go) | `NA` | `NA` | `Partial coverage` | `NA` |
Expand All @@ -63,7 +63,7 @@ IBC packets:

| ID | Concern | Code Review | Unit Testing | E2E Testing | Diff. Testing | Testnet |
| -- | ------- | ----------- | ------------ | ----------- | ------------- | ------- |
| 3.01 | Changes to staking module | `Done` | `Done` (Cosmos-SDK side) | `Partial coverage` <br> [unbonding_test.go](../tests/e2e/unbonding_test.go) <br> redelegation could be expanded, validator unbonding missing | `Partial coverage` | `Scheduled` |
| 3.01 | Changes to staking module | `Done` | `Done` [unbonding_test.go](https://github.com/cosmos/cosmos-sdk/blob/interchain-security-rebase.0.45.6/x/staking/keeper/unbonding_test.go) | `Partial coverage` <br> [unbonding_test.go](../tests/e2e/unbonding_test.go) <br> redelegation could be expanded, validator unbonding missing | `Partial coverage` | `Scheduled` |
| 3.02 | Changes to slashing module | `Done` | `NA` | `Done` <br> [TestValidatorDowntime](../tests/e2e/slashing_test.go#L502) <br> | `Partial coverage` | `Scheduled` |
| 3.03 | Changes to evidence module | `Done` | `NA` | `Done` <br> [TestValidatorDoubleSigning](../tests/e2e/slashing_test.go#L584) <br> | `NA` | `Scheduled` |

Expand Down
27 changes: 19 additions & 8 deletions proto/interchain_security/ccv/consumer/v1/consumer.proto
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import "interchain_security/ccv/v1/ccv.proto";
option go_package = "github.com/cosmos/interchain-security/x/ccv/consumer/types";

import "google/protobuf/any.proto";
import "cosmos/staking/v1beta1/staking.proto";
import "gogoproto/gogo.proto";
import "cosmos_proto/cosmos.proto";
import "google/protobuf/duration.proto";
Expand Down Expand Up @@ -69,14 +68,26 @@ message CrossChainValidator {
];
}

// SlashRequest defines a slashing request for CCV consumer module
message SlashRequest {
interchain_security.ccv.v1.SlashPacketData packet = 1;
cosmos.staking.v1beta1.InfractionType infraction = 2;
// ConsumerPacketType indicates interchain security specific packet types.
enum ConsumerPacketType {
option (gogoproto.goproto_enum_prefix) = false;

// UNSPECIFIED packet type
CONSUMER_PACKET_TYPE_UNSPECIFIED = 0 [(gogoproto.enumvalue_customname) = "UnspecifiedPacket"];
// Slash packet
CONSUMER_PACKET_TYPE_SLASH = 1 [(gogoproto.enumvalue_customname) = "SlashPacket"];
// VSCMatured packet
CONSUMER_PACKET_TYPE_VSCM = 2 [(gogoproto.enumvalue_customname) = "VscMaturedPacket"];
}

// ConsumerPacket contains raw packet bytes and packet type.
message ConsumerPacket {
ConsumerPacketType type = 1;
bytes data = 2;
}

// SlashRequests is a list of slash requests for CCV consumer module
message SlashRequests {
repeated SlashRequest requests = 1
// ConsumerPackets is a list of data packets.
message ConsumerPackets {
repeated ConsumerPacket list = 1
[ (gogoproto.nullable) = false ];
}
3 changes: 0 additions & 3 deletions proto/interchain_security/ccv/consumer/v1/genesis.proto
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@ message GenesisState {
// OutstandingDowntimes nil on new chain, filled on restart.
repeated OutstandingDowntime outstanding_downtime_slashing = 10
[ (gogoproto.nullable) = false ];
// PendingSlashRequests filled in on new chain, nil on restart.
interchain_security.ccv.consumer.v1.SlashRequests pending_slash_requests = 11
mpoke marked this conversation as resolved.
Show resolved Hide resolved
[ (gogoproto.nullable) = false ];
}

// MaturingVSCPacket defines the genesis information for the
Expand Down
4 changes: 2 additions & 2 deletions tests/difference/core/driver/core_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,8 @@ func (s *CoreSuite) TestAssumptions() {
s.T().Fatal(FAIL_MSG)
}

// Consumer has no slash requests
s.Require().Empty(s.consumerKeeper().GetPendingSlashRequests(s.ctx(C)))
// Consumer has no pending data packets
s.Require().Empty(s.consumerKeeper().GetPendingPackets(s.ctx(C)))

// Consumer has no maturities
s.consumerKeeper().IteratePacketMaturityTime(s.ctx(C),
Expand Down
2 changes: 1 addition & 1 deletion tests/difference/core/driver/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ func (b *Builder) createConsumerGenesis(tmConfig *ibctesting.TendermintConfig) *
consumertypes.DefaultHistoricalEntries,
consumertypes.DefaultConsumerUnbondingPeriod,
)
return consumertypes.NewInitialGenesisState(providerClient, providerConsState, valUpdates, consumertypes.SlashRequests{}, params)
return consumertypes.NewInitialGenesisState(providerClient, providerConsState, valUpdates, params)
}

func (b *Builder) createLink() {
Expand Down
1 change: 1 addition & 0 deletions tests/e2e/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ E2e tests are categorized into files as follows:
- `distribution.go` - e2e tests for the _Reward Distribution_ sub-protocol
- `stop_consumer.go` - e2e tests for the _Consumer Chain Removal_ sub-protocol
- `normal_operations.go` - e2e tests for _normal operations_ of ICS enabled chains
- `expired_client.go` - e2e tests for testing expired clients
- `instance_test.go` - ties the e2e test structure into golang's standard test mechanism, with appropriate definitions for concrete app types and setup callback

To run the e2e tests defined in this repo on any arbitrary consumer and provider implementation, copy the pattern exemplified in `instance_test.go` and `specific_setup.go`
2 changes: 1 addition & 1 deletion tests/e2e/channel_init.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func (suite *CCVTestSuite) TestInitTimeout() {
suite.providerChain.NextBlock()

// increment time
incrementTimeBy(suite, initTimeout)
incrementTime(suite, initTimeout)

// check whether the chain was removed
_, found = providerKeeper.GetConsumerClientId(suite.providerCtx(), chainID)
Expand Down
96 changes: 66 additions & 30 deletions tests/e2e/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
"github.com/cosmos/interchain-security/testutil/e2e"
consumertypes "github.com/cosmos/interchain-security/x/ccv/consumer/types"
providertypes "github.com/cosmos/interchain-security/x/ccv/provider/types"
ccv "github.com/cosmos/interchain-security/x/ccv/types"
"github.com/stretchr/testify/require"
Expand All @@ -17,6 +16,7 @@ import (
channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
commitmenttypes "github.com/cosmos/ibc-go/v3/modules/core/23-commitment/types"
"github.com/cosmos/ibc-go/v3/modules/core/exported"
ibctm "github.com/cosmos/ibc-go/v3/modules/light-clients/07-tendermint/types"
ibctmtypes "github.com/cosmos/ibc-go/v3/modules/light-clients/07-tendermint/types"
ibctesting "github.com/cosmos/ibc-go/v3/testing"
)
Expand Down Expand Up @@ -212,29 +212,16 @@ func relayAllCommittedPackets(
// Note that it is expected for the provider unbonding period
// to be one day larger than the consumer unbonding period.
func incrementTimeByUnbondingPeriod(s *CCVTestSuite, chainType ChainType) {
// Get unboding period from staking keeper
// Get unboding periods
providerUnbondingPeriod := s.providerApp.GetStakingKeeper().UnbondingTime(s.providerCtx())
consumerUnbondingPeriod := s.consumerApp.GetConsumerKeeper().GetUnbondingPeriod(s.consumerCtx())
// Note: the assertions below are not strictly necessary, and rely on default values
mpoke marked this conversation as resolved.
Show resolved Hide resolved
s.Require().Equal(consumertypes.DefaultConsumerUnbondingPeriod+24*time.Hour, providerUnbondingPeriod, "unexpected provider unbonding period")
s.Require().Equal(consumertypes.DefaultConsumerUnbondingPeriod, consumerUnbondingPeriod, "unexpected consumer unbonding period")
var jumpPeriod time.Duration
if chainType == Provider {
jumpPeriod = providerUnbondingPeriod
} else {
jumpPeriod = consumerUnbondingPeriod
}
// Make sure the clients do not expire
jumpPeriod = jumpPeriod/4 + time.Hour
for i := 0; i < 4; i++ {
s.coordinator.IncrementTimeBy(jumpPeriod)
// Update the provider client on the consumer
err := s.path.EndpointA.UpdateClient()
s.Require().NoError(err)
// Update the consumer client on the provider
err = s.path.EndpointB.UpdateClient()
s.Require().NoError(err)
}
incrementTime(s, jumpPeriod)
}

func checkStakingUnbondingOps(s *CCVTestSuite, id uint64, found bool, onHold bool, msgAndArgs ...interface{}) {
Expand Down Expand Up @@ -350,25 +337,74 @@ func (suite *CCVTestSuite) commitSlashPacket(ctx sdk.Context, packetData ccv.Sla
return channeltypes.CommitPacket(suite.consumerChain.App.AppCodec(), packet)
}

// incrementTimeBy increments the overall time by jumpPeriod
func incrementTimeBy(s *CCVTestSuite, jumpPeriod time.Duration) {
// Get unboding period from staking keeper
consumerUnbondingPeriod := s.consumerApp.GetConsumerKeeper().GetUnbondingPeriod(s.consumerChain.GetContext())
split := 1
trustingPeriodFraction := s.providerApp.GetProviderKeeper().GetTrustingPeriodFraction(s.providerCtx())
if jumpPeriod > consumerUnbondingPeriod/time.Duration(trustingPeriodFraction) {
// Make sure the clients do not expire
split = 4
jumpPeriod = jumpPeriod / 4
// incrementTime increments the overall time by jumpPeriod
// while updating to not expire the clients
func incrementTime(s *CCVTestSuite, jumpPeriod time.Duration) {
// get trusting period of client on provider endpoint
cs, ok := s.providerApp.GetIBCKeeper().ClientKeeper.GetClientState(s.providerCtx(), s.path.EndpointB.ClientID)
s.Require().True(ok)
providerEndpointTP := cs.(*ibctm.ClientState).TrustingPeriod
// get trusting period of client on consumer endpoint
cs, ok = s.consumerApp.GetIBCKeeper().ClientKeeper.GetClientState(s.consumerCtx(), s.path.EndpointA.ClientID)
s.Require().True(ok)
consumerEndpointTP := cs.(*ibctm.ClientState).TrustingPeriod
// find the minimum trusting period
var minTP time.Duration
mpoke marked this conversation as resolved.
Show resolved Hide resolved
if providerEndpointTP < consumerEndpointTP {
minTP = providerEndpointTP
} else {
minTP = consumerEndpointTP
}
for i := 0; i < split; i++ {
s.coordinator.IncrementTimeBy(jumpPeriod)
// Update the provider client on the consumer
// jumpStep is the maximum interval at which both clients are updated
jumpStep := minTP / 2
for jumpPeriod > 0 {
var step time.Duration
if jumpPeriod < jumpStep {
step = jumpPeriod
} else {
step = jumpStep
}
s.coordinator.IncrementTimeBy(step)
// update the provider client on the consumer
err := s.path.EndpointA.UpdateClient()
s.Require().NoError(err)
// Update the consumer client on the provider
// update the consumer client on the provider
err = s.path.EndpointB.UpdateClient()
s.Require().NoError(err)
jumpPeriod -= step
}
}

// incrementTimeWithoutUpdate increments the overall time by jumpPeriod
// without updating the client to the `noUpdate` chain
func incrementTimeWithoutUpdate(s *CCVTestSuite, jumpPeriod time.Duration, noUpdate ChainType) {
var trustingPeriod time.Duration
var endpointToUpdate *ibctesting.Endpoint
if noUpdate == Consumer {
cs, ok := s.consumerApp.GetIBCKeeper().ClientKeeper.GetClientState(s.consumerCtx(), s.path.EndpointA.ClientID)
s.Require().True(ok)
trustingPeriod = cs.(*ibctm.ClientState).TrustingPeriod
endpointToUpdate = s.path.EndpointA
} else {
cs, ok := s.providerApp.GetIBCKeeper().ClientKeeper.GetClientState(s.providerCtx(), s.path.EndpointB.ClientID)
s.Require().True(ok)
trustingPeriod = cs.(*ibctm.ClientState).TrustingPeriod
endpointToUpdate = s.path.EndpointB
}
// jumpStep is the maximum interval at which the client on endpointToUpdate is updated
jumpStep := trustingPeriod / 2
mpoke marked this conversation as resolved.
Show resolved Hide resolved
for jumpPeriod > 0 {
var step time.Duration
if jumpPeriod < jumpStep {
step = jumpPeriod
} else {
step = jumpStep
}
s.coordinator.IncrementTimeBy(step)
// update the client
err := endpointToUpdate.UpdateClient()
s.Require().NoError(err)
jumpPeriod -= step
}
}

Expand Down
Loading