From d77fbf4f49c99b102b072fdf95d35e00ebbf0825 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Mon, 17 Jul 2023 09:18:24 -0700 Subject: [PATCH 1/4] fix!: proper deletion of pending packets (#1146) * Update relay.go * expectation func * reg test * Update CHANGELOG.md --- CHANGELOG.md | 1 + testutil/keeper/expectations.go | 19 +++++++++++++++ x/ccv/consumer/keeper/relay.go | 10 ++++---- x/ccv/consumer/keeper/relay_test.go | 36 +++++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c017fc5b1..7109f43bbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ Add an entry to the unreleased section whenever merging a PR to main that is not targeted at a specific release. These entries will eventually be included in a release. +* (fix!) proper deletion of pending packets [#1146](https://github.com/cosmos/interchain-security/pull/1146) * (feat!) optimize pending packets storage on consumer, with migration! [#1037](https://github.com/cosmos/interchain-security/pull/1037) ## v3.1.0 diff --git a/testutil/keeper/expectations.go b/testutil/keeper/expectations.go index 1e59085d23..8a231f4760 100644 --- a/testutil/keeper/expectations.go +++ b/testutil/keeper/expectations.go @@ -14,6 +14,7 @@ import ( channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" ibctmtypes "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint" providertypes "github.com/cosmos/interchain-security/v3/x/ccv/provider/types" + "github.com/cosmos/interchain-security/v3/x/ccv/types" host "github.com/cosmos/ibc-go/v7/modules/core/24-host" ccv "github.com/cosmos/interchain-security/v3/x/ccv/types" @@ -139,3 +140,21 @@ func ExpectGetCapabilityMock(ctx sdk.Context, mocks MockedKeepers, times int) *g ctx, host.PortPath(ccv.ConsumerPortID), ).Return(nil, true).Times(times) } + +func GetMocksForSendIBCPacket(ctx sdk.Context, mocks MockedKeepers, channelID string, times int) []*gomock.Call { + return []*gomock.Call{ + mocks.MockChannelKeeper.EXPECT().GetChannel(ctx, types.ConsumerPortID, + "consumerCCVChannelID").Return(channeltypes.Channel{}, true).Times(times), + mocks.MockScopedKeeper.EXPECT().GetCapability(ctx, + host.ChannelCapabilityPath(types.ConsumerPortID, "consumerCCVChannelID")).Return( + capabilitytypes.NewCapability(1), true).Times(times), + mocks.MockChannelKeeper.EXPECT().SendPacket(ctx, + capabilitytypes.NewCapability(1), + types.ConsumerPortID, + "consumerCCVChannelID", + gomock.Any(), + gomock.Any(), + gomock.Any(), + ).Return(uint64(888), nil).Times(times), + } +} diff --git a/x/ccv/consumer/keeper/relay.go b/x/ccv/consumer/keeper/relay.go index 073a1d3996..120c0218c2 100644 --- a/x/ccv/consumer/keeper/relay.go +++ b/x/ccv/consumer/keeper/relay.go @@ -186,6 +186,7 @@ func (k Keeper) SendPackets(ctx sdk.Context) { } pending := k.GetPendingPackets(ctx) + idxsForDeletion := []uint64{} for _, p := range pending { // send packet over IBC @@ -203,18 +204,19 @@ func (k Keeper) SendPackets(ctx sdk.Context) { // IBC client is expired! // leave the packet data stored to be sent once the client is upgraded k.Logger(ctx).Info("IBC client is expired, cannot send IBC packet; leaving packet data stored:", "type", p.Type.String()) - return + break } // Not able to send packet over IBC! // Leave the packet data stored for the sent to be retried in the next block. // Note that if VSCMaturedPackets are not sent for long enough, the provider // will remove the consumer anyway. k.Logger(ctx).Error("cannot send IBC packet; leaving packet data stored:", "type", p.Type.String(), "err", err.Error()) - return + break } + idxsForDeletion = append(idxsForDeletion, p.Idx) } - - k.DeleteAllPendingDataPackets(ctx) + // Delete pending packets that were successfully sent and did not return an error from SendIBCPacket + k.DeletePendingDataPackets(ctx, idxsForDeletion...) } // OnAcknowledgementPacket executes application logic for acknowledgments of sent VSCMatured and Slash packets diff --git a/x/ccv/consumer/keeper/relay_test.go b/x/ccv/consumer/keeper/relay_test.go index 48ad7f5ab9..6dbebe273f 100644 --- a/x/ccv/consumer/keeper/relay_test.go +++ b/x/ccv/consumer/keeper/relay_test.go @@ -307,3 +307,39 @@ func TestSendPacketsFailure(t *testing.T) { consumerKeeper.SendPackets(ctx) require.Equal(t, 3, len(consumerKeeper.GetPendingPackets(ctx))) } + +// Regression test for https://github.com/cosmos/interchain-security/issues/1145 +func TestSendPacketsDeletion(t *testing.T) { + // Keeper setup + consumerKeeper, ctx, ctrl, mocks := testkeeper.GetConsumerKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) + defer ctrl.Finish() + consumerKeeper.SetProviderChannel(ctx, "consumerCCVChannelID") + consumerKeeper.SetParams(ctx, consumertypes.DefaultParams()) + + // Queue two pending packets + consumerKeeper.AppendPendingPacket(ctx, types.SlashPacket, &types.ConsumerPacketData_SlashPacketData{ // Slash appears first + SlashPacketData: &types.SlashPacketData{ + Validator: abci.Validator{}, + ValsetUpdateId: 88, + Infraction: stakingtypes.Infraction_INFRACTION_DOWNTIME, + }, + }) + consumerKeeper.AppendPendingPacket(ctx, types.VscMaturedPacket, &types.ConsumerPacketData_VscMaturedPacketData{ + VscMaturedPacketData: &types.VSCMaturedPacketData{ + ValsetUpdateId: 90, + }, + }) + + // Get mocks for a successful SendPacket call that does NOT return an error + expectations := testkeeper.GetMocksForSendIBCPacket(ctx, mocks, "consumerCCVChannelID", 1) + // Append mocks for a failed SendPacket call, which returns an error + expectations = append(expectations, mocks.MockChannelKeeper.EXPECT().GetChannel(ctx, types.ConsumerPortID, + "consumerCCVChannelID").Return(channeltypes.Channel{}, false).Times(1)) + gomock.InOrder(expectations...) + + consumerKeeper.SendPackets(ctx) + + // Expect the first successfully sent packet to be popped from queue + require.Equal(t, 1, len(consumerKeeper.GetPendingPackets(ctx))) + require.Equal(t, types.VscMaturedPacket, consumerKeeper.GetPendingPackets(ctx)[0].Type) +} From d8373547507384e3479fc12ccd2de77ddc74383e Mon Sep 17 00:00:00 2001 From: omahs <73983677+omahs@users.noreply.github.com> Date: Mon, 17 Jul 2023 20:38:25 +0200 Subject: [PATCH 2/4] docs: fix typos (#1144) * Fix: typo * Fix: typo * Fix: typo * Fix: typos * Fix: typo * Fix: typos * Fix: typos --------- Co-authored-by: MSalopek Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> --- docs/docs/adrs/adr-template.md | 2 +- docs/docs/consumer-development/consumer-chain-governance.md | 2 +- docs/docs/consumer-development/onboarding.md | 6 +++--- docs/docs/features/key-assignment.md | 2 +- docs/docs/features/proposals.md | 4 ++-- docs/docs/features/slashing.md | 4 ++-- docs/docs/introduction/terminology.md | 2 +- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/docs/adrs/adr-template.md b/docs/docs/adrs/adr-template.md index 2e085330df..b9e3af1678 100644 --- a/docs/docs/adrs/adr-template.md +++ b/docs/docs/adrs/adr-template.md @@ -36,6 +36,6 @@ If the proposed change will be large, please also indicate a way to do the chang ## References -> Are there any relevant PR comments, issues that led up to this, or articles referrenced for why we made the given design choice? If so link them here! +> Are there any relevant PR comments, issues that led up to this, or articles referenced for why we made the given design choice? If so link them here! * {reference link} diff --git a/docs/docs/consumer-development/consumer-chain-governance.md b/docs/docs/consumer-development/consumer-chain-governance.md index ca99286d93..c8586efe3a 100644 --- a/docs/docs/consumer-development/consumer-chain-governance.md +++ b/docs/docs/consumer-development/consumer-chain-governance.md @@ -16,7 +16,7 @@ For an example, see the [Democracy Consumer](https://github.com/cosmos/interchai ## CosmWasm -There several great DAO and governance frameworks written as CosmWasm contracts. These can be used as the main governance system for a consumer chain. Actions triggered by the CosmWasm governance contracts are able to affect parameters and trigger actions on the consumer chain. +There are several great DAO and governance frameworks written as CosmWasm contracts. These can be used as the main governance system for a consumer chain. Actions triggered by the CosmWasm governance contracts are able to affect parameters and trigger actions on the consumer chain. For an example, see [Neutron](https://github.com/neutron-org/neutron/). diff --git a/docs/docs/consumer-development/onboarding.md b/docs/docs/consumer-development/onboarding.md index 89c5f32dc5..dc870b76de 100644 --- a/docs/docs/consumer-development/onboarding.md +++ b/docs/docs/consumer-development/onboarding.md @@ -20,7 +20,7 @@ To help validators and other node runners onboard onto your chain, please prepar This should include (at minimum): -- [ ] genesis.json witout CCV data (before the propsal passes) +- [ ] genesis.json without CCV data (before the proposal passes) - [ ] genesis.json with CCV data (after spawn time passes) - [ ] information about relevant seed/peer nodes you are running - [ ] relayer information (compatible versions) @@ -73,7 +73,7 @@ Example of a consumer chain addition proposal. // will be responsible for starting their consumer chain validator node. "spawn_time": "2023-02-28T20:40:00.000000Z", // Unbonding period for the consumer chain. - // It should should be smaller than that of the provider. + // It should be smaller than that of the provider. "unbonding_period": 86400000000000, // Timeout period for CCV related IBC packets. // Packets are considered timed-out after this interval elapses. @@ -96,7 +96,7 @@ Example of a consumer chain addition proposal. // channel is created on top of the same connection as the CCV channel. // Note that transfer_channel_id is the ID of the channel end on the consumer chain. // it is most relevant for chains performing a sovereign to consumer changeover - // in order to maintan the existing ibc transfer channel + // in order to maintain the existing ibc transfer channel "distribution_transmission_channel": "channel-123" } ``` diff --git a/docs/docs/features/key-assignment.md b/docs/docs/features/key-assignment.md index c68343255c..a44ed8a32a 100644 --- a/docs/docs/features/key-assignment.md +++ b/docs/docs/features/key-assignment.md @@ -49,7 +49,7 @@ gaiad tx provider assign-consensus-key '' --from "}` -Check that the key was assigned correcly by querying the provider: +Check that the key was assigned correctly by querying the provider: ```bash gaiad query provider validator-consumer-key cosmosvalcons1e....3xsj3ayzf4uv6 diff --git a/docs/docs/features/proposals.md b/docs/docs/features/proposals.md index b68fd343d6..25664bfc1e 100644 --- a/docs/docs/features/proposals.md +++ b/docs/docs/features/proposals.md @@ -32,7 +32,7 @@ Minimal example: "revision_number": 1, }, // Unbonding period for the consumer chain. - // It should should be smaller than that of the provider. + // It should be smaller than that of the provider. "unbonding_period": 86400000000000, // Timeout period for CCV related IBC packets. // Packets are considered timed-out after this interval elapses. @@ -44,7 +44,7 @@ Minimal example: "genesis_hash": "d86d756e10118e66e6805e9cc476949da2e750098fcc7634fd0cc77f57a0b2b0", "binary_hash": "376cdbd3a222a3d5c730c9637454cd4dd925e2f9e2e0d0f3702fc922928583f1" // relevant for chains performing a sovereign to consumer changeover - // in order to maintan the existing ibc transfer channel + // in order to maintain the existing ibc transfer channel "distribution_transmission_channel": "channel-123" } ``` diff --git a/docs/docs/features/slashing.md b/docs/docs/features/slashing.md index a28b16e8c2..4c74153b15 100644 --- a/docs/docs/features/slashing.md +++ b/docs/docs/features/slashing.md @@ -3,7 +3,7 @@ sidebar_position: 4 --- # Consumer Initiated Slashing -A consumer chain is essentially a regular Cosmos-SDK based chain that uses the interchain security module to achieve economic security by stake deposited on the provider chain, instead of it's own chain. +A consumer chain is essentially a regular Cosmos-SDK based chain that uses the interchain security module to achieve economic security by stake deposited on the provider chain, instead of its own chain. In essence, provider chain and consumer chains are different networks (different infrastructures) that are bound together by the provider's validator set. By being bound to the provider's validator set, a consumer chain inherits the economic security guarantees of the provider chain (in terms of total stake). To maintain the proof of stake model, the consumer chain is able to send evidence of infractions (double signing and downtime) to the provider chain so the offending validators can be penalized. @@ -17,7 +17,7 @@ reported by consumer chains are acted upon on the provider as soon as the provid Instead of slashing, the provider will only jail offending validator for the duration of time established in the chain parameters. :::info -Slash throttling (sometimes called jail throttling) mechanism insures that only a fraction of the validator set can be jailed at any one time to prevent malicious consumer chains from harming the provider. +Slash throttling (sometimes called jail throttling) mechanism ensures that only a fraction of the validator set can be jailed at any one time to prevent malicious consumer chains from harming the provider. ::: ## Double-signing (equivocation) diff --git a/docs/docs/introduction/terminology.md b/docs/docs/introduction/terminology.md index 5f6722fbd9..59994353cf 100644 --- a/docs/docs/introduction/terminology.md +++ b/docs/docs/introduction/terminology.md @@ -8,7 +8,7 @@ You may have heard of one or multiple buzzwords thrown around in the cosmos and ## Shared Security -Shared security is a family of technologies that include optimistic rollups, zk-rollups, sharding and Interchain Security. Ie. any protocol or technology that can allow one blockchain to lend/share it's proof-of-stake security with another blockchain or off-chain process. +Shared security is a family of technologies that include optimistic rollups, zk-rollups, sharding and Interchain Security. Ie. any protocol or technology that can allow one blockchain to lend/share its proof-of-stake security with another blockchain or off-chain process. ## Interchain Security From d67d1bca18440733982bd0680193f31eac2bd59d Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 17 Jul 2023 14:33:45 -0700 Subject: [PATCH 3/4] build(deps): bump bufbuild/buf-setup-action from 1.23.1 to 1.24.0 (#1151) Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.23.1 to 1.24.0. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](https://github.com/bufbuild/buf-setup-action/compare/v1.23.1...v1.24.0) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> --- .github/workflows/proto-registry.yml | 2 +- .github/workflows/proto.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/proto-registry.yml b/.github/workflows/proto-registry.yml index 08748fe079..d79db5af70 100644 --- a/.github/workflows/proto-registry.yml +++ b/.github/workflows/proto-registry.yml @@ -13,7 +13,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 - - uses: bufbuild/buf-setup-action@v1.23.1 + - uses: bufbuild/buf-setup-action@v1.24.0 - uses: bufbuild/buf-push-action@v1 with: input: "proto" diff --git a/.github/workflows/proto.yml b/.github/workflows/proto.yml index 0049c05d20..9791a871c0 100644 --- a/.github/workflows/proto.yml +++ b/.github/workflows/proto.yml @@ -14,7 +14,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 - - uses: bufbuild/buf-setup-action@v1.23.1 + - uses: bufbuild/buf-setup-action@v1.24.0 - uses: bufbuild/buf-breaking-action@v1 with: input: "proto" From 781eefb267882e70574be86d2f7f4d6f5abcf75f Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Mon, 17 Jul 2023 14:57:28 -0700 Subject: [PATCH 4/4] chore: add release/v3.1.x targets for bots (#1140) update Co-authored-by: MSalopek --- .github/dependabot.yml | 10 ++++++++++ .mergify.yml | 8 ++++++++ 2 files changed, 18 insertions(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index ec9119084e..65ff156753 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -37,3 +37,13 @@ updates: open-pull-requests-limit: 0 labels: - dependencies + + - package-ecosystem: gomod + directory: "/" + schedule: + interval: daily + target-branch: "release/v3.1.x" + # Only allow automated security-related dependency updates on release branches. + open-pull-requests-limit: 0 + labels: + - dependencies diff --git a/.mergify.yml b/.mergify.yml index 63b8b61550..e08625bf28 100644 --- a/.mergify.yml +++ b/.mergify.yml @@ -26,3 +26,11 @@ pull_request_rules: backport: branches: - release/v3.0.x + - name: Backport patches to the release/v3.1.x branch + conditions: + - base=main + - label=A:backport/v3.1.x + actions: + backport: + branches: + - release/v3.1.x