Skip to content

Commit

Permalink
chore!: backport #1946 (GetAllConsumerChains fix) to `feat/pss-upgrad…
Browse files Browse the repository at this point in the history 
…e-v50` branch (#1950)

fix!: Replace GetAllConsumerChains with lightweight version (#1946)

* add GetAllConsumerChainIDs

* replace GetAllConsumerChains with GetAllRegisteredConsumerChainIDs

* add changelog entry

* move HasToValidate to grpc_query.go as it's used only there

* apply review suggestions

Co-authored-by: Marius Poke <[email protected]>
  • Loading branch information
@sainoe @mpoke
sainoe and mpoke authored Jun 12, 2024
1 parent f346a85 commit 330cfdb
Show file tree
Hide file tree
Showing 14 changed files with 327 additions and 305 deletions.
3 changes: 3 additions & 0 deletions .changelog/unreleased/bug-fixes/provider/1946-get-consumer-chains.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
- Replace `GetAllConsumerChains` with lightweight version
(`GetAllRegisteredConsumerChainIDs`) that doesn't call into the staking module
([\#1946](https://github.com/cosmos/interchain-security/pull/1946))
34 changes: 14 additions & 20 deletions tests/mbt/driver/core.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@ package main
import (
"fmt"
"log"
gomath "math"
"strings"
"testing"
"time"

gomath "math"

"cosmossdk.io/math"

channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
Expand All @@ -33,7 +32,6 @@ import (
consumerkeeper "github.com/cosmos/interchain-security/v5/x/ccv/consumer/keeper"
consumertypes "github.com/cosmos/interchain-security/v5/x/ccv/consumer/types"
providerkeeper "github.com/cosmos/interchain-security/v5/x/ccv/provider/keeper"
providertypes "github.com/cosmos/interchain-security/v5/x/ccv/provider/types"
"github.com/cosmos/interchain-security/v5/x/ccv/types"
)

Expand Down Expand Up @@ -222,11 +220,7 @@ func (s *Driver) getStateString() string {
state.WriteString("\n")

state.WriteString("Consumers Chains:\n")
consumerChains := s.providerKeeper().GetAllConsumerChains(s.providerCtx())
chainIds := make([]string, len(consumerChains))
for i, consumerChain := range consumerChains {
chainIds[i] = consumerChain.ChainId
}
chainIds := s.providerKeeper().GetAllRegisteredConsumerChainIDs(s.providerCtx())
state.WriteString(strings.Join(chainIds, ", "))
state.WriteString("\n\n")

Expand Down Expand Up @@ -264,11 +258,11 @@ func (s *Driver) getChainStateString(chain ChainId) string {
if !s.isProviderChain(chain) {
// Check whether the chain is in the consumer chains on the provider

consumerChains := s.providerKeeper().GetAllConsumerChains(s.providerCtx())
consumerChainIDs := s.providerKeeper().GetAllRegisteredConsumerChainIDs(s.providerCtx())

found := false
for _, consumerChain := range consumerChains {
if consumerChain.ChainId == string(chain) {
for _, consumerChainID := range consumerChainIDs {
if consumerChainID == string(chain) {
found = true
}
}
Expand Down Expand Up @@ -371,16 +365,16 @@ func (s *Driver) endAndBeginBlock(chain ChainId, timeAdvancement time.Duration)
return header
}

func (s *Driver) runningConsumers() []providertypes.Chain {
consumersOnProvider := s.providerKeeper().GetAllConsumerChains(s.providerCtx())
func (s *Driver) runningConsumerChainIDs() []ChainId {
consumerIDsOnProvider := s.providerKeeper().GetAllRegisteredConsumerChainIDs(s.providerCtx())

consumersWithIntactChannel := make([]providertypes.Chain, 0)
for _, consumer := range consumersOnProvider {
if s.path(ChainId(consumer.ChainId)).Path.EndpointA.GetChannel().State == channeltypes.CLOSED ||
s.path(ChainId(consumer.ChainId)).Path.EndpointB.GetChannel().State == channeltypes.CLOSED {
consumersWithIntactChannel := make([]ChainId, 0)
for _, consumerChainID := range consumerIDsOnProvider {
if s.path(ChainId(consumerChainID)).Path.EndpointA.GetChannel().State == channeltypes.CLOSED ||
s.path(ChainId(consumerChainID)).Path.EndpointB.GetChannel().State == channeltypes.CLOSED {
continue
}
consumersWithIntactChannel = append(consumersWithIntactChannel, consumer)
consumersWithIntactChannel = append(consumersWithIntactChannel, ChainId(consumerChainID))
}
return consumersWithIntactChannel
}
Expand Down Expand Up @@ -448,8 +442,8 @@ func (s *Driver) RequestSlash(
// DeliverAcks delivers, for each path,
// all possible acks (up to math.MaxInt many per path).
func (s *Driver) DeliverAcks() {
for _, chain := range s.runningConsumers() {
path := s.path(ChainId(chain.ChainId))
for _, chainID := range s.runningConsumerChainIDs() {
path := s.path(chainID)
path.DeliverAcks(path.Path.EndpointA.Chain.ChainID, gomath.MaxInt)
path.DeliverAcks(path.Path.EndpointB.Chain.ChainID, gomath.MaxInt)
}
Expand Down
72 changes: 36 additions & 36 deletions tests/mbt/driver/mbt_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,21 +306,21 @@ func RunItfTrace(t *testing.T, path string) {
// needs a header of height H+1 to accept the packet
// so, we do two blocks, one with a very small increment,
// and then another to increment the rest of the time
runningConsumersBefore := driver.runningConsumers()
runningConsumerChainIDsBefore := driver.runningConsumerChainIDs()

driver.endAndBeginBlock("provider", 1*time.Nanosecond)
for _, consumer := range driver.runningConsumers() {
UpdateProviderClientOnConsumer(t, driver, consumer.ChainId)
for _, consumerChainID := range driver.runningConsumerChainIDs() {
UpdateProviderClientOnConsumer(t, driver, string(consumerChainID))
}
driver.endAndBeginBlock("provider", time.Duration(timeAdvancement)*time.Second-1*time.Nanosecond)

runningConsumersAfter := driver.runningConsumers()
runningConsumerChainIDsAfter := driver.runningConsumerChainIDs()

// the consumers that were running before but not after must have timed out
for _, consumer := range runningConsumersBefore {
for _, consumerChainID := range runningConsumerChainIDsBefore {
found := false
for _, consumerAfter := range runningConsumersAfter {
if consumerAfter.ChainId == consumer.ChainId {
for _, consumerChainIDAfter := range runningConsumerChainIDsAfter {
if consumerChainIDAfter == consumerChainID {
found = true
break
}
Expand All @@ -334,8 +334,8 @@ func RunItfTrace(t *testing.T, path string) {
// because setting up chains will modify timestamps
// when the coordinator is starting chains
lastTimestamps := make(map[ChainId]time.Time, len(consumers))
for _, consumer := range driver.runningConsumers() {
lastTimestamps[ChainId(consumer.ChainId)] = driver.runningTime(ChainId(consumer.ChainId))
for _, consumerChainID := range driver.runningConsumerChainIDs() {
lastTimestamps[consumerChainID] = driver.runningTime(consumerChainID)
}

driver.coordinator.CurrentTime = driver.runningTime("provider")
Expand Down Expand Up @@ -366,12 +366,12 @@ func RunItfTrace(t *testing.T, path string) {
// for all connected consumers, update the clients...
// unless it was the last consumer to be started, in which case it already has the header
// as we called driver.setupConsumer
for _, consumer := range driver.runningConsumers() {
if len(consumersToStart) > 0 && consumer.ChainId == consumersToStart[len(consumersToStart)-1].Value.(string) {
for _, consumerChainID := range driver.runningConsumerChainIDs() {
if len(consumersToStart) > 0 && string(consumerChainID) == consumersToStart[len(consumersToStart)-1].Value.(string) {
continue
}

UpdateProviderClientOnConsumer(t, driver, consumer.ChainId)
UpdateProviderClientOnConsumer(t, driver, string(consumerChainID))
}

case "EndAndBeginBlockForConsumer":
Expand Down Expand Up @@ -492,33 +492,33 @@ func RunItfTrace(t *testing.T, path string) {
t.Logf("Comparing model state to actual state...")

// compare the running consumers
modelRunningConsumers := RunningConsumers(currentModelState)
modelRunningConsumerChainIDs := RunningConsumers(currentModelState)

systemRunningConsumers := driver.runningConsumers()
actualRunningConsumers := make([]string, len(systemRunningConsumers))
for i, chain := range systemRunningConsumers {
actualRunningConsumers[i] = chain.ChainId
systemRunningConsumerChainIDs := driver.runningConsumerChainIDs()
actualRunningConsumerChainIDs := make([]string, len(systemRunningConsumerChainIDs))
for i, chainID := range systemRunningConsumerChainIDs {
actualRunningConsumerChainIDs[i] = string(chainID)
}

// sort the slices so that we can compare them
sort.Slice(modelRunningConsumers, func(i, j int) bool {
return modelRunningConsumers[i] < modelRunningConsumers[j]
sort.Slice(modelRunningConsumerChainIDs, func(i, j int) bool {
return modelRunningConsumerChainIDs[i] < modelRunningConsumerChainIDs[j]
})
sort.Slice(actualRunningConsumers, func(i, j int) bool {
return actualRunningConsumers[i] < actualRunningConsumers[j]
sort.Slice(actualRunningConsumerChainIDs, func(i, j int) bool {
return actualRunningConsumerChainIDs[i] < actualRunningConsumerChainIDs[j]
})

require.Equal(t, modelRunningConsumers, actualRunningConsumers, "Running consumers do not match")
require.Equal(t, modelRunningConsumerChainIDs, actualRunningConsumerChainIDs, "Running consumers do not match")

// check validator sets - provider current validator set should be the one from the staking keeper
CompareValidatorSets(t, driver, currentModelState, actualRunningConsumers, realAddrsToModelConsAddrs)
CompareValidatorSets(t, driver, currentModelState, actualRunningConsumerChainIDs, realAddrsToModelConsAddrs)

// check times - sanity check that the block times match the ones from the model
CompareTimes(driver, actualRunningConsumers, currentModelState, timeOffset)
CompareTimes(driver, actualRunningConsumerChainIDs, currentModelState, timeOffset)

// check sent packets: we check that the package queues in the model and the system have the same length.
for _, consumer := range actualRunningConsumers {
ComparePacketQueues(t, driver, currentModelState, consumer, timeOffset)
for _, consumerChainID := range actualRunningConsumerChainIDs {
ComparePacketQueues(t, driver, currentModelState, consumerChainID, timeOffset)
}
// compare that the sent packets on the proider match the model
CompareSentPacketsOnProvider(driver, currentModelState, timeOffset)
Expand All @@ -528,8 +528,8 @@ func RunItfTrace(t *testing.T, path string) {
CompareJailedValidators(driver, currentModelState, timeOffset, addressMap)

// for all newly sent vsc packets, figure out which vsc id in the model they correspond to
for _, consumer := range actualRunningConsumers {
actualPackets := driver.packetQueue(PROVIDER, ChainId(consumer))
for _, consumerChainID := range actualRunningConsumerChainIDs {
actualPackets := driver.packetQueue(PROVIDER, ChainId(consumerChainID))
actualNewPackets := make([]types.ValidatorSetChangePacketData, 0)
for _, packet := range actualPackets {

Expand All @@ -545,7 +545,7 @@ func RunItfTrace(t *testing.T, path string) {
actualNewPackets = append(actualNewPackets, packetData)
}

modelPackets := PacketQueue(currentModelState, PROVIDER, consumer)
modelPackets := PacketQueue(currentModelState, PROVIDER, consumerChainID)
newModelVscIds := make([]uint64, 0)
for _, packet := range modelPackets {
modelVscId := uint64(packet.Value.(itf.MapExprType)["value"].Value.(itf.MapExprType)["id"].Value.(int64))
Expand Down Expand Up @@ -785,15 +785,15 @@ func CompareValSet(modelValSet map[string]itf.Expr, systemValSet map[string]int6
}

func CompareSentPacketsOnProvider(driver *Driver, currentModelState map[string]itf.Expr, timeOffset time.Time) {
for _, consumer := range driver.runningConsumers() {
vscSendTimestamps := driver.providerKeeper().GetAllVscSendTimestamps(driver.providerCtx(), consumer.ChainId)
for _, consumerChainID := range driver.runningConsumerChainIDs() {
vscSendTimestamps := driver.providerKeeper().GetAllVscSendTimestamps(driver.providerCtx(), string(consumerChainID))

actualVscSendTimestamps := make([]time.Time, 0)
for _, vscSendTimestamp := range vscSendTimestamps {
actualVscSendTimestamps = append(actualVscSendTimestamps, vscSendTimestamp.Timestamp)
}

modelVscSendTimestamps := VscSendTimestamps(currentModelState, consumer.ChainId)
modelVscSendTimestamps := VscSendTimestamps(currentModelState, string(consumerChainID))

for i, modelVscSendTimestamp := range modelVscSendTimestamps {
actualTimeWithOffset := actualVscSendTimestamps[i].Unix() - timeOffset.Unix()
Expand All @@ -802,7 +802,7 @@ func CompareSentPacketsOnProvider(driver *Driver, currentModelState map[string]i
modelVscSendTimestamp,
actualTimeWithOffset,
"Vsc send timestamps do not match for consumer %v",
consumer.ChainId,
consumerChainID,
)
}
}
Expand Down Expand Up @@ -860,9 +860,9 @@ func (s *Stats) EnterStats(driver *Driver) {

// max number of in-flight packets
inFlightPackets := 0
for _, consumer := range driver.runningConsumers() {
inFlightPackets += len(driver.packetQueue(PROVIDER, ChainId(consumer.ChainId)))
inFlightPackets += len(driver.packetQueue(ChainId(consumer.ChainId), PROVIDER))
for _, consumerChainID := range driver.runningConsumerChainIDs() {
inFlightPackets += len(driver.packetQueue(PROVIDER, consumerChainID))
inFlightPackets += len(driver.packetQueue(consumerChainID, PROVIDER))
}
if inFlightPackets > s.maxNumInFlightPackets {
s.maxNumInFlightPackets = inFlightPackets
Expand Down
16 changes: 8 additions & 8 deletions x/ccv/provider/keeper/distribution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,14 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
}

// Iterate over all registered consumer chains
for _, consumer := range k.GetAllConsumerChains(ctx) {
for _, consumerChainID := range k.GetAllRegisteredConsumerChainIDs(ctx) {
// transfer the consumer rewards to the distribution module account
// note that the rewards transferred are only consumer whitelisted denoms
rewardsCollected, err := k.TransferConsumerRewardsToDistributionModule(ctx, consumer.ChainId)
rewardsCollected, err := k.TransferConsumerRewardsToDistributionModule(ctx, consumerChainID)
if err != nil {
k.Logger(ctx).Error(
"fail to transfer rewards to distribution module for chain %s: %s",
consumer.ChainId,
consumerChainID,
err,
)
continue
Expand All @@ -97,12 +97,12 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {

// temporary workaround to keep CanWithdrawInvariant happy
// general discussions here: https://github.com/cosmos/cosmos-sdk/issues/2906#issuecomment-441867634
if k.ComputeConsumerTotalVotingPower(ctx, consumer.ChainId) == 0 {
if k.ComputeConsumerTotalVotingPower(ctx, consumerChainID) == 0 {
err := k.distributionKeeper.FundCommunityPool(context.Context(ctx), rewardsCollected, k.accountKeeper.GetModuleAccount(ctx, types.ConsumerRewardsPool).GetAddress())
if err != nil {
k.Logger(ctx).Error(
"fail to allocate rewards from consumer chain %s to community pool: %s",
consumer.ChainId,
consumerChainID,
err,
)
}
Expand All @@ -116,7 +116,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
if err != nil {
k.Logger(ctx).Error(
"cannot get community tax while allocating rewards from consumer chain %s: %s",
consumer.ChainId,
consumerChainID,
err,
)
continue
Expand All @@ -127,7 +127,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
// allocate tokens to consumer validators
feeAllocated := k.AllocateTokensToConsumerValidators(
ctx,
consumer.ChainId,
consumerChainID,
feeMultiplier,
)
remaining = remaining.Sub(feeAllocated)
Expand All @@ -138,7 +138,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
if err != nil {
k.Logger(ctx).Error(
"fail to allocate rewards from consumer chain %s to community pool: %s",
consumer.ChainId,
consumerChainID,
err,
)
continue
Expand Down
36 changes: 20 additions & 16 deletions x/ccv/provider/keeper/genesis.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,47 +108,51 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) {
// ExportGenesis returns the CCV provider module's exported genesis
func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
// get a list of all registered consumer chains
registeredChains := k.GetAllConsumerChains(ctx)
registeredChainIDs := k.GetAllRegisteredConsumerChainIDs(ctx)

var exportedVscSendTimestamps []types.ExportedVscSendTimestamp
// export states for each consumer chains
var consumerStates []types.ConsumerState
for _, chain := range registeredChains {
gen, found := k.GetConsumerGenesis(ctx, chain.ChainId)
for _, chainID := range registeredChainIDs {
// no need for the second return value of GetConsumerClientId
// as GetAllRegisteredConsumerChainIDs already iterated through
// the entire prefix range
clientID, _ := k.GetConsumerClientId(ctx, chainID)
gen, found := k.GetConsumerGenesis(ctx, chainID)
if !found {
panic(fmt.Errorf("cannot find genesis for consumer chain %s with client %s", chain.ChainId, chain.ClientId))
panic(fmt.Errorf("cannot find genesis for consumer chain %s with client %s", chainID, clientID))
}

// initial consumer chain states
cs := types.ConsumerState{
ChainId: chain.ChainId,
ClientId: chain.ClientId,
ChainId: chainID,
ClientId: clientID,
ConsumerGenesis: gen,
UnbondingOpsIndex: k.GetAllUnbondingOpIndexes(ctx, chain.ChainId),
UnbondingOpsIndex: k.GetAllUnbondingOpIndexes(ctx, chainID),
}

// try to find channel id for the current consumer chain
channelId, found := k.GetChainToChannel(ctx, chain.ChainId)
channelId, found := k.GetChainToChannel(ctx, chainID)
if found {
cs.ChannelId = channelId
cs.InitialHeight, found = k.GetInitChainHeight(ctx, chain.ChainId)
cs.InitialHeight, found = k.GetInitChainHeight(ctx, chainID)
if !found {
panic(fmt.Errorf("cannot find init height for consumer chain %s", chain.ChainId))
panic(fmt.Errorf("cannot find init height for consumer chain %s", chainID))
}
cs.SlashDowntimeAck = k.GetSlashAcks(ctx, chain.ChainId)
cs.SlashDowntimeAck = k.GetSlashAcks(ctx, chainID)
}

cs.PendingValsetChanges = k.GetPendingVSCPackets(ctx, chain.ChainId)
cs.PendingValsetChanges = k.GetPendingVSCPackets(ctx, chainID)
consumerStates = append(consumerStates, cs)

vscSendTimestamps := k.GetAllVscSendTimestamps(ctx, chain.ChainId)
exportedVscSendTimestamps = append(exportedVscSendTimestamps, types.ExportedVscSendTimestamp{ChainId: chain.ChainId, VscSendTimestamps: vscSendTimestamps})
vscSendTimestamps := k.GetAllVscSendTimestamps(ctx, chainID)
exportedVscSendTimestamps = append(exportedVscSendTimestamps, types.ExportedVscSendTimestamp{ChainId: chainID, VscSendTimestamps: vscSendTimestamps})
}

// ConsumerAddrsToPrune are added only for registered consumer chains
consumerAddrsToPrune := []types.ConsumerAddrsToPrune{}
for _, chain := range registeredChains {
consumerAddrsToPrune = append(consumerAddrsToPrune, k.GetAllConsumerAddrsToPrune(ctx, chain.ChainId)...)
for _, chainID := range registeredChainIDs {
consumerAddrsToPrune = append(consumerAddrsToPrune, k.GetAllConsumerAddrsToPrune(ctx, chainID)...)
}

params := k.GetParams(ctx)
Expand Down
Loading

0 comments on commit 330cfdb

Please sign in to comment.