From 60b38efd85a902c93228daa0f35ea055395af98f Mon Sep 17 00:00:00 2001 From: Mateusz Kaczanowski Date: Sat, 9 Nov 2024 03:50:07 +0100 Subject: [PATCH 1/2] handle multiple upgrades for the same height in chain provider --- internal/pkg/provider/chain/chain.go | 72 +++++++-- internal/pkg/provider/chain/chain_test.go | 186 ++++++++++++++++++++++ 2 files changed, 243 insertions(+), 15 deletions(-) create mode 100644 internal/pkg/provider/chain/chain_test.go diff --git a/internal/pkg/provider/chain/chain.go b/internal/pkg/provider/chain/chain.go index 69e0e2a..881212b 100644 --- a/internal/pkg/provider/chain/chain.go +++ b/internal/pkg/provider/chain/chain.go @@ -2,23 +2,28 @@ package chain import ( "context" - "slices" + "sort" - "blazar/internal/pkg/cosmos" "blazar/internal/pkg/errors" urproto "blazar/internal/pkg/proto/upgrades_registry" "blazar/internal/pkg/provider" v1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" + "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1" ) +type CosmosProposalsProvider interface { + GetProposalsV1(ctx context.Context) (v1.Proposals, error) + GetProposalsV1beta1(ctx context.Context) (v1beta1.Proposals, error) +} + type Provider struct { - cosmosClient *cosmos.Client + cosmosClient CosmosProposalsProvider chain string priority int32 } -func NewProvider(cosmosClient *cosmos.Client, chain string, priority int32) *Provider { +func NewProvider(cosmosClient CosmosProposalsProvider, chain string, priority int32) *Provider { return &Provider{ cosmosClient: cosmosClient, chain: chain, @@ -32,23 +37,60 @@ func (p *Provider) GetUpgrades(ctx context.Context) ([]*urproto.Upgrade, error) return []*urproto.Upgrade{}, err } - // cosmos-sdk allows changing parameters of a previously passed upgrade - // by creating a new upgrade proposal with the same name in upgrade plan - // https://github.com/cosmos/cosmos-sdk/blob/41f92723399ef0affa90c6b3d8e7b47b82361280/x/upgrade/keeper/keeper.go#L185 - // since, upgrades is sorted by proposal ID and we'll only keep last instance for a name - // if a passed upgrade already exists for that name - passedNames := make(map[string]struct{}, len(upgrades)) - filtered := make([]chainUpgrade, 0, len(upgrades)) - slices.Reverse(upgrades) + // Blazar expects one upgrade per height, but the governance allows to create multiple proposals for the same height + // In the end only one upgrade will be expecuted at given height, no matter how many software upgrades proposals are registered onchain + // The most common case fror having more than one proposal is when someone create a new proposal and asks everyone to vote-no on the previous one + // due to invalid data etc. + // + // To handle this case we pick the last proposal for each height with some conditions: + // 1. if there is a proposal in PASSED state, we pick it + // 2. if there are two equal proposals say in VOTING_PERIOD state, we pick the one with the highest proposal id + + // sort upgrades in descending order by proposal id + sort.Slice(upgrades, func(i, j int) bool { + return upgrades[i].ProposalID > upgrades[j].ProposalID + }) + + upgradesByHeight := make(map[int64][]chainUpgrade) for _, upgrade := range upgrades { - if _, ok := passedNames[upgrade.Name]; !ok { + if _, ok := upgradesByHeight[upgrade.Height]; !ok { + upgradesByHeight[upgrade.Height] = make([]chainUpgrade, 0) + } + upgradesByHeight[upgrade.Height] = append(upgradesByHeight[upgrade.Height], upgrade) + } + + filtered := make([]chainUpgrade, 0, len(upgrades)) + for _, upgradesForHeight := range upgradesByHeight { + // if there is only one upgrade for the height, we don't need to do anything + if len(upgradesForHeight) == 1 { + filtered = append(filtered, upgradesForHeight[0]) + continue + } + + // if there is a passed upgrade, we pick it + foundPassed := false + for _, upgrade := range upgradesForHeight { + // the upgrades are sorted by proposal id in descending order + // so the first upgrade in the list is the one with the highest + // proposal id (in case there are two PASSED proposals for the same height) if upgrade.Status == PASSED { - passedNames[upgrade.Name] = struct{}{} + foundPassed = true + filtered = append(filtered, upgrade) + break } - filtered = append(filtered, upgrade) + } + + // if there is no passed upgrade, we pick the one with the highest proposal id + if !foundPassed { + filtered = append(filtered, upgradesForHeight[0]) } } + // sort upgrades in descending order by proposal id because iterating over map doesn't guarantee order + sort.Slice(filtered, func(i, j int) bool { + return filtered[i].ProposalID > filtered[j].ProposalID + }) + return toProto(filtered, p.priority), nil } diff --git a/internal/pkg/provider/chain/chain_test.go b/internal/pkg/provider/chain/chain_test.go new file mode 100644 index 0000000..5b13ee0 --- /dev/null +++ b/internal/pkg/provider/chain/chain_test.go @@ -0,0 +1,186 @@ +package chain + +import ( + "context" + "fmt" + "testing" + "time" + + urproto "blazar/internal/pkg/proto/upgrades_registry" + + sdk "github.com/cosmos/cosmos-sdk/types" + v1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" + "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1" + upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type mockCosmosClient struct { + v1Proposals v1.Proposals + v1beta1Proposals v1beta1.Proposals +} + +func (m *mockCosmosClient) GetProposalsV1(_ context.Context) (v1.Proposals, error) { + return m.v1Proposals, nil +} + +func (m *mockCosmosClient) GetProposalsV1beta1(_ context.Context) (v1beta1.Proposals, error) { + return m.v1beta1Proposals, nil +} + +func TestGetUpgrades(t *testing.T) { + tests := []struct { + name string + proposals v1.Proposals + expected []*urproto.Upgrade + }{ + { + name: "EmptyProposals", + proposals: v1.Proposals{}, + expected: []*urproto.Upgrade{}, + }, + { + name: "Simple", + proposals: v1.Proposals{ + newProposal(t, 1, 100, v1.StatusPassed), + newProposal(t, 2, 200, v1.StatusVotingPeriod), + }, + expected: []*urproto.Upgrade{ + { + Height: 200, + Type: urproto.UpgradeType_GOVERNANCE, + Status: urproto.UpgradeStatus_SCHEDULED, + Source: urproto.ProviderType_CHAIN, + }, + { + Height: 100, + Type: urproto.UpgradeType_GOVERNANCE, + Status: urproto.UpgradeStatus_ACTIVE, + Source: urproto.ProviderType_CHAIN, + }, + }, + }, + { + name: "DuplicateProposalsWithPassedStatus", + proposals: v1.Proposals{ + newProposal(t, 1, 100, v1.StatusPassed), + newProposal(t, 2, 100, v1.StatusPassed), + newProposal(t, 3, 200, v1.StatusPassed), + }, + expected: []*urproto.Upgrade{ + { + Height: 200, + Type: urproto.UpgradeType_GOVERNANCE, + Status: urproto.UpgradeStatus_ACTIVE, + Source: urproto.ProviderType_CHAIN, + }, + { + Height: 100, + Type: urproto.UpgradeType_GOVERNANCE, + Status: urproto.UpgradeStatus_ACTIVE, + Source: urproto.ProviderType_CHAIN, + // the latest proposal in passed state should be returned + ProposalId: int64ptr(2), + }, + }, + }, + { + name: "DuplicateProposalsInVotingPeriod", + proposals: v1.Proposals{ + newProposal(t, 1, 100, v1.StatusVotingPeriod), + newProposal(t, 2, 100, v1.StatusVotingPeriod), + newProposal(t, 3, 200, v1.StatusDepositPeriod), + }, + expected: []*urproto.Upgrade{ + { + Height: 200, + Type: urproto.UpgradeType_GOVERNANCE, + Status: urproto.UpgradeStatus_SCHEDULED, + Source: urproto.ProviderType_CHAIN, + }, + { + Height: 100, + Type: urproto.UpgradeType_GOVERNANCE, + Status: urproto.UpgradeStatus_SCHEDULED, + Source: urproto.ProviderType_CHAIN, + // in case of two equal proposals in non-active state we expect the one with the highest proposal id + ProposalId: int64ptr(2), + }, + }, + }, + { + name: "DuplicateProposalsInActiveAndVotingPeriod", + proposals: v1.Proposals{ + newProposal(t, 1, 100, v1.StatusPassed), + newProposal(t, 2, 100, v1.StatusVotingPeriod), + newProposal(t, 3, 200, v1.StatusDepositPeriod), + }, + expected: []*urproto.Upgrade{ + { + Height: 200, + Type: urproto.UpgradeType_GOVERNANCE, + Status: urproto.UpgradeStatus_SCHEDULED, + Source: urproto.ProviderType_CHAIN, + }, + { + Height: 100, + Type: urproto.UpgradeType_GOVERNANCE, + Status: urproto.UpgradeStatus_ACTIVE, + Source: urproto.ProviderType_CHAIN, + // in case of two proposals where one is in active state and the other in non-active state + // we expect the one in active state + ProposalId: int64ptr(1), + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cosmosClient := &mockCosmosClient{ + v1Proposals: tt.proposals, + } + provider := NewProvider(cosmosClient, "test-chain", 1) + + upgrades, err := provider.GetUpgrades(context.Background()) + require.NoError(t, err) + + assert.Equal(t, len(upgrades), len(tt.expected)) + + for i, upgrade := range upgrades { + assert.Equal(t, tt.expected[i].Height, upgrade.Height) + assert.Equal(t, tt.expected[i].Type, upgrade.Type) + assert.Equal(t, tt.expected[i].Status, upgrade.Status) + assert.Equal(t, tt.expected[i].Source, upgrade.Source) + + if tt.expected[i].ProposalId != nil { + assert.Equal(t, *tt.expected[i].ProposalId, *upgrade.ProposalId) + } + } + }) + } +} + +func newProposal(t *testing.T, id uint64, height int64, status v1.ProposalStatus) *v1.Proposal { + sup := &upgradetypes.MsgSoftwareUpgrade{ + Authority: "x/gov", + Plan: upgradetypes.Plan{ + Name: fmt.Sprintf("test upgrade: %d", height), + Time: time.Now().Add(30 * time.Minute), + Info: "test upgrade info", + Height: height, + }, + } + + proposal, err := v1.NewProposal([]sdk.Msg{sup}, id, time.Now(), time.Now(), "", "title", "summary", sdk.AccAddress{}) + require.NoError(t, err) + + proposal.Status = status + + return &proposal +} + +func int64ptr(i int64) *int64 { + return &i +} From 5f972680ee0f6e69b19fedc08ea1532c3679fb19 Mon Sep 17 00:00:00 2001 From: Mateusz Kaczanowski Date: Sat, 9 Nov 2024 04:08:21 +0100 Subject: [PATCH 2/2] fix typo fror -> for --- internal/pkg/provider/chain/chain.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/provider/chain/chain.go b/internal/pkg/provider/chain/chain.go index 881212b..2b2197c 100644 --- a/internal/pkg/provider/chain/chain.go +++ b/internal/pkg/provider/chain/chain.go @@ -39,7 +39,7 @@ func (p *Provider) GetUpgrades(ctx context.Context) ([]*urproto.Upgrade, error) // Blazar expects one upgrade per height, but the governance allows to create multiple proposals for the same height // In the end only one upgrade will be expecuted at given height, no matter how many software upgrades proposals are registered onchain - // The most common case fror having more than one proposal is when someone create a new proposal and asks everyone to vote-no on the previous one + // The most common case for having more than one proposal is when someone create a new proposal and asks everyone to vote-no on the previous one // due to invalid data etc. // // To handle this case we pick the last proposal for each height with some conditions: