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.

Sign up for GitHub

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

fix: all: avoid map ranging whenever possible to fix non-determinism security warnings #13554

Closed
wants to merge 1 commit into from
Closed

fix: all: avoid map ranging whenever possible to fix non-determinism security warnings #13554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 3 additions & 2 deletions baseapp/baseapp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ func (app *BaseApp) MountStores(keys ...storetypes.StoreKey) {
// MountKVStores mounts all IAVL or DB stores to the provided keys in the
// BaseApp multistore.
func (app *BaseApp) MountKVStores(keys map[string]*storetypes.KVStoreKey) {
for _, key := range keys {
for _, key := range maps.Values(keys) {
Copy link

Choose a reason for hiding this comment

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

Values returns the values of the map m. The values will be in an indeterminate order.

Does this change solve a problem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some code here such as this one, the values can be returned non-deterministically. The critical ones, I invoke sort.* on them :-)

Copy link
Member

Choose a reason for hiding this comment

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

naive question, why is map.Values and map.Keys needed here? does it add anything the previous code didnt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure: for code that's okay with being non-deterministic, they mask the fact that we are ranging over maps given that the static analyzer rule is rigid. We could add a comment that it is okay to range over them but that'll pollute lots of code so just easier to use maps.Values and maps.Key

Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand sorry. it seems like this adds an extra loop of the map to grab the keys or values. It seems this fixes the linter but doesn't actually fix any logic for us. this doesn't seem to sort like the title suggests.

Id prefer not to add things like this if its not fixing an issue we are facing.

Copy link

Choose a reason for hiding this comment

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

FWIW, that static analyzer is bogus and should be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@peterbourgon that's quite strong language most definitely from lack of context/ignorance of the history, but the reaasoning is there was a serious chain halt that happened late last year due to non-deterministic map ranging https://twitter.com/thorchain/status/1459288646142492673 and https://twitter.com/thorchain/status/1459288646142492673 and the cosmos-sdk and other projects have been plagued by such bugs due to non-deterministic map ranging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tac0turtle yeah it requires a balance but sadly would require annotations or perhaps just scoped to specific packages, but we need to figure things out. Also this change is a Draft btw folks, not yet ready for review.

Copy link
Member

Choose a reason for hiding this comment

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

The bugs on chains aren't from map ranging but from non-deterministic ordering across nodes in the network, which i dont see how this fixes that issue. These functions are loops with appends of keys or values to an array, not sorting. We can stop commenting but this feels unnecessary

Copy link

@peterbourgon peterbourgon Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@peterbourgon that's quite strong language most definitely from lack of context/ignorance of the history, but the reaasoning is there was a serious chain halt that happened late last year due to non-deterministic map ranging https://twitter.com/thorchain/status/1459288646142492673 and https://twitter.com/thorchain/status/1459288646142492673 and the cosmos-sdk and other projects have been plagued by such bugs due to non-deterministic map ranging.

I'm aware of the chain halt issues you're referring to. I was directly affected by them! 😅 The problem was not that range over a map is non-deterministic. The problem was the assumption, made by the programmer who wrote the code, that ranging over a map would be — or could be! — deterministic. Any code that needs deterministic iteration order over a map-like collection can't use the builtin map type directly. It either has to extract and sort the keys manually, or use a different type that provides this property.

And it's totally fine to extract the keys from a map, sort them, and iterate the map based on that slice, on an as-needed basis. Perfect, no problem. But this is a special case, it's not the baseline. That ranging over a map is nondeterministic is simply a property of the language. If you write Go, you need to understand that property, same as anything else, like that that switch statements don't fallthrough by default, or that you can read from a zero-value map but not write to it, or etc. etc. These things aren't problems that need to be — or even can be! — corrected by linters. They're the rules of the game.

if !app.fauxMerkleMode {
app.MountStore(key, storetypes.StoreTypeIAVL)
} else {
Expand All @@ -244,7 +244,8 @@ func (app *BaseApp) MountKVStores(keys map[string]*storetypes.KVStoreKey) {
// MountTransientStores mounts all transient stores to the provided keys in
// the BaseApp multistore.
func (app *BaseApp) MountTransientStores(keys map[string]*storetypes.TransientStoreKey) {
for _, key := range keys {
keyL := maps.Values(keys)
Copy link

Choose a reason for hiding this comment

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

Values returns the values of the map m. The values will be in an indeterminate order.

Does this change solve a problem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does.

Copy link

Choose a reason for hiding this comment

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

What problem does it solve? Is it to silence the linter warnings about nondeterinistic iteration order? This change does seem to silence the linter, but it doesn't change the behavior, right? The order is still nondeterministic.

for _, key := range keyL {
app.MountStore(key, storetypes.StoreTypeTransient)
}
}
Expand Down
3 changes: 2 additions & 1 deletion client/v2/autocli/flag/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ func (b *Builder) addMessageFlags(ctx context.Context, flagSet *pflag.FlagSet, m
}

// validate flag options
for name := range commandOptions.FlagOptions {
names := maps.Keys(commandOptions.FlagOptions)
for _, name := range names {
Comment on lines +71 to +72
Copy link

Choose a reason for hiding this comment

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

Keys returns the keys of the map m. The keys will be in an indeterminate order.

Does this change solve a problem?

if fields.ByName(protoreflect.Name(name)) == nil {
return nil, fmt.Errorf("can't find field %s on %s specified as a flag", name, messageType.Descriptor().FullName())
}
Expand Down
9 changes: 8 additions & 1 deletion client/v2/autocli/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ package autocli

import (
"fmt"
"sort"

autocliv1 "cosmossdk.io/api/cosmos/autocli/v1"
"github.com/iancoleman/strcase"
"github.com/spf13/cobra"
"golang.org/x/exp/maps"
"google.golang.org/protobuf/encoding/protojson"
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/reflect/protoregistry"
Expand All @@ -19,12 +21,17 @@ import (
func (b *Builder) BuildQueryCommand(moduleOptions map[string]*autocliv1.ModuleOptions, customCmds map[string]*cobra.Command) (*cobra.Command, error) {
queryCmd := topLevelCmd("query", "Querying subcommands")
queryCmd.Aliases = []string{"q"}
for moduleName, modOpts := range moduleOptions {

// Ensure that building module query commands is in a deterministic pattern.
moduleNames := maps.Keys(moduleOptions)
sort.Strings(moduleNames)
for _, moduleName := range moduleNames {
Comment on lines +25 to +28
Copy link

Choose a reason for hiding this comment

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

Is it documented somewhere that BuildQueryCommand should iterate over module names deterministically? If not, is it necessary to do so? If so, is there a test which enforces this invariant? And isn't manual sorting of keys somewhat error prone? Should there rather be a package/function that ensures deterministic order without requiring explicit sorting at the callsite?

if customCmds[moduleName] != nil {
// custom commands get added lower down
continue
}

modOpts := moduleOptions[moduleName]
queryCmdDesc := modOpts.Query
if queryCmdDesc != nil {
cmd, err := b.BuildModuleQueryCommand(moduleName, queryCmdDesc)
Expand Down
3 changes: 2 additions & 1 deletion core/internal/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"reflect"

"golang.org/x/exp/maps"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"

Expand All @@ -29,7 +30,7 @@ type ModuleInitializer struct {
func ModulesByProtoMessageName() (map[protoreflect.FullName]*ModuleInitializer, error) {
res := map[protoreflect.FullName]*ModuleInitializer{}

for _, initializer := range ModuleRegistry {
for _, initializer := range maps.Values(ModuleRegistry) {
descriptor := initializer.ConfigProtoMessage.ProtoReflect().Descriptor()
fullName := descriptor.FullName()
if _, ok := res[fullName]; ok {
Comment on lines -32 to 36
Copy link

Choose a reason for hiding this comment

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

This code isn't sorting the values, so AFAICT doesn't produce behavior any different than ranging over the map directly?

Expand Down
5 changes: 3 additions & 2 deletions orm/model/ormdb/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"encoding/binary"
"math"

"golang.org/x/exp/maps"
"google.golang.org/protobuf/reflect/protoregistry"

ormv1alpha1 "cosmossdk.io/api/cosmos/orm/v1alpha1"
Expand Down Expand Up @@ -119,12 +120,12 @@ func NewModuleDB(schema *ormv1alpha1.ModuleSchemaDescriptor, options ModuleDBOpt
}

db.filesById[id] = fdSchema
for name, table := range fdSchema.tablesByName {
for _, name := range maps.Keys(fdSchema.tablesByName) {
if _, ok := db.tablesByName[name]; ok {
return nil, ormerrors.UnexpectedError.Wrapf("duplicate table %s", name)
}

db.tablesByName[name] = table
db.tablesByName[name] = fdSchema.tablesByName[name]
}
Comment on lines -122 to 129
Copy link

Choose a reason for hiding this comment

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

This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?

}

Expand Down
3 changes: 2 additions & 1 deletion orm/model/ormtable/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"google.golang.org/protobuf/reflect/protoregistry"

"golang.org/x/exp/maps"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"

Expand Down Expand Up @@ -259,7 +260,7 @@ func Build(options Options) (Table, error) {
}
}

for name := range altNames {
for _, name := range maps.Keys(altNames) {
Comment on lines -262 to +263
Copy link

Choose a reason for hiding this comment

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

This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?

if _, ok := table.indexesByFields[name]; ok {
return nil, fmt.Errorf("duplicate index for fields %s", name)
}
Expand Down
5 changes: 4 additions & 1 deletion runtime/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
runtimev1alpha1 "cosmossdk.io/api/cosmos/app/runtime/v1alpha1"
"cosmossdk.io/core/appmodule"
"cosmossdk.io/depinject"
"golang.org/x/exp/maps"

"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/codec"
Expand Down Expand Up @@ -50,7 +51,9 @@ func ProvideCodecs(moduleBasics map[string]AppModuleBasicWrapper) (

// build codecs
basicManager := module.BasicManager{}
for name, wrapper := range moduleBasics {
names := maps.Keys(moduleBasics)
for _, name := range names {
wrapper := moduleBasics[name]
Comment on lines -53 to +56
Copy link

Choose a reason for hiding this comment

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

This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?

basicManager[name] = wrapper
wrapper.RegisterInterfaces(interfaceRegistry)
wrapper.RegisterLegacyAminoCodec(amino)
Expand Down
6 changes: 4 additions & 2 deletions store/internal/maps/maps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/tendermint/tendermint/crypto/merkle"
"github.com/tendermint/tendermint/crypto/tmhash"
tmcrypto "github.com/tendermint/tendermint/proto/tendermint/crypto"
"golang.org/x/exp/maps"

"github.com/cosmos/cosmos-sdk/types/kv"
)
Expand Down Expand Up @@ -185,8 +186,9 @@ func HashFromMap(m map[string][]byte) []byte {
// The keys are sorted before the proofs are computed.
func ProofsFromMap(m map[string][]byte) ([]byte, map[string]*tmcrypto.Proof, []string) {
sm := newSimpleMap()
for k, v := range m {
sm.Set(k, v)
kL := maps.Keys(m)
for _, k := range kL {
sm.Set(k, m[k])
}
Comment on lines +189 to 192
Copy link

Choose a reason for hiding this comment

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

This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?


sm.Sort()
Expand Down
4 changes: 3 additions & 1 deletion store/internal/proofs/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"sort"

ics23 "github.com/confio/ics23/go"
"golang.org/x/exp/maps"

sdkmaps "github.com/cosmos/cosmos-sdk/store/internal/maps"
)
Expand Down Expand Up @@ -84,7 +85,8 @@ func CreateNonMembershipProof(data map[string][]byte, key []byte) (*ics23.Commit
}

func createExistenceProof(data map[string][]byte, key []byte) (*ics23.ExistenceProof, error) {
for k := range data {
keys := maps.Keys(data)
for _, k := range keys {
Comment on lines +88 to +89
Copy link

Choose a reason for hiding this comment

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

This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?

if k == "" {
return nil, ErrEmptyKeyInData
}
Expand Down
24 changes: 21 additions & 3 deletions store/rootmulti/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,11 @@ func (rs *Store) Commit() types.CommitID {
defer rs.flushMetadata(rs.db, version, rs.lastCommitInfo)

// remove remnants of removed stores
for sk := range rs.removalMap {
keys := make([]types.StoreKey, 0, len(rs.removalMap))
for key := range rs.removalMap {
keys = append(keys, key)
}
Comment on lines +443 to +445

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
for _, sk := range keys {
Comment on lines +442 to +446
Copy link

Choose a reason for hiding this comment

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

This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly. Am I missing something?

if _, ok := rs.stores[sk]; ok {
delete(rs.stores, sk)
delete(rs.storesParams, sk)
Expand Down Expand Up @@ -586,7 +590,12 @@ func (rs *Store) PruneStores(clearPruningManager bool, pruningHeights []int64) (

rs.logger.Debug("pruning heights", "heights", pruningHeights)

for key, store := range rs.stores {
keys := make([]types.StoreKey, 0, len(rs.stores))
for key := range rs.stores {
keys = append(keys, key)
}
Comment on lines +594 to +596

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
for _, key := range keys {
store := rs.stores[key]
Comment on lines -589 to +598
Copy link

Choose a reason for hiding this comment

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

This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?

// If the store is wrapped with an inter-block cache, we must first unwrap
// it to get the underlying IAVL store.
if store.GetStoreType() != types.StoreTypeIAVL {
Expand Down Expand Up @@ -1034,7 +1043,16 @@ func GetLatestVersion(db dbm.DB) int64 {
func commitStores(version int64, storeMap map[types.StoreKey]types.CommitKVStore, removalMap map[types.StoreKey]bool) *types.CommitInfo {
storeInfos := make([]types.StoreInfo, 0, len(storeMap))

for key, store := range storeMap {
keys := make([]types.StoreKey, 0, len(storeMap))
for key := range storeMap {
keys = append(keys, key)
}
sort.Slice(keys, func(i, j int) bool {
ki, kj := keys[i], keys[j]
return ki.Name() < kj.Name()
})
for _, key := range keys {
store := storeMap[key]
commitID := store.Commit()

if store.GetStoreType() == types.StoreTypeTransient {
Expand Down
5 changes: 3 additions & 2 deletions tests/e2e/evidence/suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"strings"

"github.com/stretchr/testify/suite"
"golang.org/x/exp/maps"

clitestutil "github.com/cosmos/cosmos-sdk/testutil/cli"
"github.com/cosmos/cosmos-sdk/testutil/network"
Expand Down Expand Up @@ -56,8 +57,8 @@ func (s *IntegrationTestSuite) TestGetQueryCmd() {
},
}

for name, tc := range testCases {
tc := tc
for _, name := range maps.Keys(testCases) {
tc := testCases[name]

s.Run(name, func() {
cmd := cli.GetQueryCmd()
Expand Down
2 changes: 1 addition & 1 deletion types/module/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ func (m *Manager) EndBlock(ctx sdk.Context, req abci.RequestEndBlock) abci.Respo
// GetVersionMap gets consensus version from all modules
func (m *Manager) GetVersionMap() VersionMap {
vermap := make(VersionMap)
for _, v := range m.Modules {
for _, v := range maps.Values(m.Modules) {
version := v.ConsensusVersion()
name := v.Name()
vermap[name] = version
Expand Down
9 changes: 8 additions & 1 deletion x/authz/migrations/v046/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package v046

import (
"sort"

"golang.org/x/exp/maps"

"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/internal/conv"
"github.com/cosmos/cosmos-sdk/store/prefix"
Expand Down Expand Up @@ -58,7 +62,10 @@ func addExpiredGrantsIndex(ctx sdk.Context, store storetypes.KVStore, cdc codec.
}
}

for key, v := range queueItems {
keys := maps.Keys(queueItems)
sort.Strings(keys)
for _, key := range keys {
Comment on lines +65 to +67
Copy link

Choose a reason for hiding this comment

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

There should be no reason to sort the return value of maps.Keys.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is an absolute reason to, maps.Keys returns them in non-deterministic order and the source code shows it https://cs.opensource.google/go/x/exp/+/32f3d567:maps/maps.go;l=10
Screen Shot 2022-10-28 at 9 16 39 PM

Copy link

Choose a reason for hiding this comment

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

Surprising! But if map.Keys doesn't sort the keys, then the dependency provides very little value, right? It would be better to define your own package map with a Keys that returned ordered keys, I guess?

v := queueItems[key]
bz, err := cdc.Marshal(&authz.GrantQueueItem{
MsgTypeUrls: v,
})
Expand Down
4 changes: 3 additions & 1 deletion x/gov/keeper/tally.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
v1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
"golang.org/x/exp/maps"
)

// TODO: Break into several smaller functions for clarity
Expand Down Expand Up @@ -73,7 +74,8 @@ func (keeper Keeper) Tally(ctx sdk.Context, proposal v1.Proposal) (passes bool,
})

// iterate over the validators again to tally their voting power
for _, val := range currValidators {
valL := maps.Values(currValidators)
for _, val := range valL {
if len(val.Vote) == 0 {
continue
}
Comment on lines -76 to 81
Copy link

Choose a reason for hiding this comment

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

This code isn't sorting the values, so AFAICT doesn't produce behavior any different than ranging over the map directly?

Expand Down
7 changes: 2 additions & 5 deletions x/params/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,9 @@ func (k Keeper) GetSubspace(s string) (types.Subspace, bool) {

// GetSubspaces returns all the registered subspaces.
func (k Keeper) GetSubspaces() []types.Subspace {
spaces := make([]types.Subspace, len(k.spaces))
i := 0
spaces := make([]types.Subspace, 0, len(k.spaces))
for _, ss := range k.spaces {
spaces[i] = *ss
i++
spaces = append(spaces, *ss)
}

return spaces
}
4 changes: 3 additions & 1 deletion x/params/types/table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package types
import (
"reflect"

"golang.org/x/exp/maps"

sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand Down Expand Up @@ -72,7 +74,7 @@ func (t KeyTable) RegisterParamSet(ps ParamSet) KeyTable {
}

func (t KeyTable) maxKeyLength() (res int) {
for k := range t.m {
for _, k := range maps.Keys(t.m) {
l := len(k)
if l > res {
Comment on lines 76 to 79
Copy link

Choose a reason for hiding this comment

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

This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?

res = l
Expand Down
4 changes: 3 additions & 1 deletion x/staking/keeper/val_state_change.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"cosmossdk.io/math"
gogotypes "github.com/cosmos/gogoproto/types"
abci "github.com/tendermint/tendermint/abci/types"
"golang.org/x/exp/maps"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/staking/types"
Expand Down Expand Up @@ -399,7 +400,8 @@ func sortNoLongerBonded(last validatorsByAddr) ([][]byte, error) {
noLongerBonded := make([][]byte, len(last))
index := 0

for valAddrStr := range last {
valAddrL := maps.Keys(last)
for _, valAddrStr := range valAddrL {
Comment on lines -402 to +404
Copy link

Choose a reason for hiding this comment

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

This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?

valAddrBytes, err := sdk.ValAddressFromBech32(valAddrStr)
if err != nil {
return nil, err
Expand Down
14 changes: 12 additions & 2 deletions x/upgrade/plan/info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@ import (
"os"
"path/filepath"
"regexp"
"sort"
"strings"

"golang.org/x/exp/maps"

"github.com/cosmos/cosmos-sdk/internal/conv"
)

Expand Down Expand Up @@ -77,10 +80,12 @@ func (m BinaryDownloadURLMap) ValidateBasic() error {
}

osArchRx := regexp.MustCompile(`[a-zA-Z0-9]+/[a-zA-Z0-9]+`)
for key, val := range m {
for _, key := range maps.Keys(m) {
if key != "any" && !osArchRx.MatchString(key) {
return fmt.Errorf("invalid os/arch format in key \"%s\"", key)
}

val := m[key]
Comment on lines -80 to +88
Copy link

Choose a reason for hiding this comment

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

This code isn't sorting the keys, so AFAICT doesn't produce behavior any different than ranging over the map directly?

if err := ValidateIsURLWithChecksum(val); err != nil {
return fmt.Errorf("invalid url \"%s\" in binaries[%s]: %v", val, key, err)
}
Expand All @@ -99,7 +104,12 @@ func (m BinaryDownloadURLMap) CheckURLs(daemonName string) error {
return fmt.Errorf("could not create temp directory: %w", err)
}
defer os.RemoveAll(tempDir)
for osArch, url := range m {

// Ensure that downloads proceed in a deterministic pattern.
osArches := maps.Keys(m)
sort.Strings(osArches)
Comment on lines +108 to +110
Copy link

Choose a reason for hiding this comment

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

Likewise above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, we need these deterministic.

Copy link

Choose a reason for hiding this comment

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

Is that requirement documented somewhere?

for _, osArch := range osArches {
url := m[osArch]
Comment on lines +108 to +112
Copy link

Choose a reason for hiding this comment

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

Suggested change
// Ensure that downloads proceed in a deterministic pattern.
osArches := maps.Keys(m)
sort.Strings(osArches)
for _, osArch := range osArches {
url := m[osArch]
for _, k := range maps.Keys(m) {
url := m[k]

dstRoot := filepath.Join(tempDir, strings.ReplaceAll(osArch, "/", "-"))
if err = DownloadUpgrade(dstRoot, url, daemonName); err != nil {
return fmt.Errorf("error downloading binary for os/arch %s: %v", osArch, err)
Expand Down