Parameter JSON serialization must be deterministic (#1614)

This is a fix for a regression introduced by Per-chain RTTM (#1580).
When we store a map-type parameter, we use `Codec.MarshalJSON` that
doen't guarantee the order of fields in a map. This causes consensus
failure because a different order of fields in the parameter store
generates a different merkle tree (= AppHash), resulting in chainhalt.

The proposed fix is to canonicalize the marshaled bytes by calling
`MustSortJSON` before storing it in the tree.

Changing `Subspace.Set` is enough and a change in `
Subspace.SetWithSubkey` won't be necessary because it's not used from
anywhere, but we should keep consistency.

reviewpad:summary

types/param.go

@@ -212,11 +212,11 @@ func (s Subspace) checkType(store KVStore, key []byte, param interface{}) {
 func (s Subspace) Set(ctx Ctx, key []byte, param interface{}) {
 	store := s.kvStore(ctx)
 	s.checkType(store, key, param)
-	bz, err := s.cdc.MarshalJSON(param)
-	if err != nil {
-		panic(err)
-	}
+
+	// Sort marshaled JSON to make sure a map param value is canonicalized.
+	bz := MustSortJSON(s.cdc.MustMarshalJSON(param))
 	_ = store.Set(key, bz)
+
 	tstore := s.transientStore(ctx)
 	_ = tstore.Set(key, []byte{})
 }
@@ -254,10 +254,8 @@ func (s Subspace) SetWithSubkey(ctx Ctx, key []byte, subkey []byte, param interf
 
 	newkey := concatKeys(key, subkey)
 
-	bz, err := s.cdc.MarshalJSON(param)
-	if err != nil {
-		panic(err)
-	}
+	// Sort marshaled JSON to make sure a map param value is canonicalized.
+	bz := MustSortJSON(s.cdc.MustMarshalJSON(param))
 	_ = store.Set(newkey, bz)
 
 	tstore := s.transientStore(ctx)

types/utils.go

@@ -195,3 +195,19 @@ func CompareVersionStrings(verStr1, verStr2 string) (int, error) {
 
 	return 0, nil
 }
+
+// True if two maps are equivalent.
+// Nil is considered to be the same as an empty map.
+func CompareStringMaps[T comparable](a, b map[string]T) bool {
+	if len(a) != len(b) {
+		return false
+	}
+
+	for k, v := range a {
+		if v != b[k] {
+			return false
+		}
+	}
+
+	return true
+}

types/utils_test.go

@@ -29,6 +29,14 @@ func TestSortJSON(t *testing.T) {
 		{unsortedJSON: `{"chain_id":"test-chain-1","sequence":1,"fee_bytes":{"amount":[{"amount":5,"denom":"photon"}],"gas":10000},"msg_bytes":{"inputs":[{"address":"696E707574","coins":[{"amount":10,"denom":"atom"}]}],"outputs":[{"address":"6F7574707574","coins":[{"amount":10,"denom":"atom"}]}]},"alt_bytes":null}`,
 			want:    `{"alt_bytes":null,"chain_id":"test-chain-1","fee_bytes":{"amount":[{"amount":5,"denom":"photon"}],"gas":10000},"msg_bytes":{"inputs":[{"address":"696E707574","coins":[{"amount":10,"denom":"atom"}]}],"outputs":[{"address":"6F7574707574","coins":[{"amount":10,"denom":"atom"}]}]},"sequence":1}`,
 			wantErr: false},
+
+		// non-object values
+		{unsortedJSON: "null", want: "null"},
+		{unsortedJSON: "42", want: "42"},
+		{unsortedJSON: "-3.14", want: "-3.14"},
+		{unsortedJSON: "\"str\"", want: "\"str\""},
+		{unsortedJSON: "[\"string\",12345,null]", want: "[\"string\",12345,null]"},
+		{unsortedJSON: "[[3,1,2],{},[1,2,3]]", want: "[[3,1,2],{},[1,2,3]]"},
 	}
 
 	for tcIndex, tc := range cases {
@@ -92,3 +100,31 @@ func Test_CompareVersionStrings(t *testing.T) {
 	comp, err = CompareVersionStrings("", "1")
 	assert.NotNil(t, err)
 }
+
+func TestCompareStringMaps(t *testing.T) {
+	m1 := map[string]int{}
+	m2 := map[string]int{}
+	assert.True(t, CompareStringMaps(m1, m2))
+
+	// m1 is non-empty and m2 is empty
+	m1["a"] = 10
+	m1["b"] = 100
+	assert.False(t, CompareStringMaps(m1, m2))
+
+	// m1 and m2 are not empty and identical
+	m2["b"] = 100
+	m2["a"] = 10
+	assert.True(t, CompareStringMaps(m2, m1))
+
+	// m1 is non-empty and m2 is nil
+	m2 = nil
+	assert.False(t, CompareStringMaps(m1, m2))
+	assert.False(t, CompareStringMaps(nil, m1))
+
+	// m1 and m2 are both nil
+	m1 = nil
+	assert.True(t, CompareStringMaps(m1, m2))
+
+	// Empty and nil maps are identical
+	assert.True(t, CompareStringMaps(nil, map[string]int{}))
+}

x/gov/keeper/common_test.go

@@ -1,11 +1,11 @@
 package keeper
 
 import (
-	"github.com/pokt-network/pocket-core/codec/types"
 	"math/rand"
 	"testing"
 
 	"github.com/pokt-network/pocket-core/codec"
+	"github.com/pokt-network/pocket-core/codec/types"
 	"github.com/pokt-network/pocket-core/crypto"
 	"github.com/pokt-network/pocket-core/store"
 	sdk "github.com/pokt-network/pocket-core/types"
@@ -50,8 +50,11 @@ func getRandomValidatorAddress() sdk.Address {
 	return sdk.Address(getRandomPubKey().Address())
 }
 
-// nolint: deadcode unused
-func createTestKeeperAndContext(t *testing.T, isCheckTx bool) (sdk.Context, Keeper) {
+func createTestKeeperAndContext(
+	t *testing.T,
+	isCheckTx bool,
+	subspaces ...sdk.Subspace,
+) (sdk.Context, Keeper) {
 	keyAcc := sdk.NewKVStoreKey(auth.StoreKey)
 	db := dbm.NewMemDB()
 	ms := store.NewCommitMultiStore(db, false, 5000000)
@@ -82,7 +85,14 @@ func createTestKeeperAndContext(t *testing.T, isCheckTx bool) (sdk.Context, Keep
 	akSubspace := sdk.NewSubspace(auth.DefaultParamspace)
 	ak := keeper.NewKeeper(cdc, keyAcc, akSubspace, maccPerms)
 	ak.GetModuleAccount(ctx, "FAKE")
-	pk := NewKeeper(cdc, sdk.ParamsKey, sdk.ParamsTKey, govTypes.DefaultParamspace, ak, akSubspace)
+	pk := NewKeeper(
+		cdc,
+		sdk.ParamsKey,
+		sdk.ParamsTKey,
+		govTypes.DefaultParamspace,
+		ak,
+		append(subspaces, akSubspace)...,
+	)
 	moduleManager := module.NewManager(
 		auth.NewAppModule(ak),
 	)

x/gov/keeper/subspace_test.go

@@ -1,14 +1,30 @@
 package keeper
 
 import (
+	"testing"
+
 	sdk "github.com/pokt-network/pocket-core/types"
 	"github.com/pokt-network/pocket-core/x/gov/types"
 	"github.com/stretchr/testify/assert"
 	"github.com/tendermint/go-amino"
 	abci "github.com/tendermint/tendermint/abci/types"
-	"testing"
 )
 
+const (
+	testSubSpaceName  = "testParamSet"
+	testParamFieldKey = "field"
+)
+
+type testParamSet struct {
+	m map[string]string
+}
+
+func (p *testParamSet) ParamSetPairs() sdk.ParamSetPairs {
+	return sdk.ParamSetPairs{
+		{Key: []byte(testParamFieldKey), Value: &p.m},
+	}
+}
+
 func TestModifyParam(t *testing.T) {
 	addr := getRandomValidatorAddress()
 	var aclKey = types.NewACLKey(types.ModuleName, string(types.DAOOwnerKey))
@@ -37,3 +53,64 @@ func TestModifyParam(t *testing.T) {
 		),
 	)
 }
+
+func Test_ModifyParam_MapValue(t *testing.T) {
+	subspace := sdk.NewSubspace(testSubSpaceName).WithKeyTable(
+		sdk.NewKeyTable().RegisterParamSet(&testParamSet{}),
+	)
+	ctx, k := createTestKeeperAndContext(t, false, subspace)
+
+	var (
+		expectedMapValue = map[string]string{"2": "2", "3": "3", "1": "1"}
+
+		// These strings represent the same value as `expectedMapValue` above
+		mapValueVariations = []string{
+			"{\"1\":\"1\", \"2\":\"2\", \"3\":\"3\"}",
+			"{\"1\":\"1\", \"3\":\"3\", \"2\":\"2\"}",
+			"{\"2\":\"2\", \"3\":\"3\", \"1\":\"1\"}",
+			"{\"2\":\"2\", \"1\":\"1\", \"3\":\"3\"}",
+			"{\"3\":\"3\", \"1\":\"1\", \"2\":\"2\"}",
+			"{\"3\":\"3\", \"2\":\"2\", \"1\":\"1\"}",
+		}
+
+		// These strings are incompatible with testParamFieldKey
+		invalidParamValues = []string{
+			"{\"3\":3, \"1\":1, \"1\":13}", // wrong type
+			"[\"123\":\"123\"]",            // wrong json
+		}
+	)
+
+	aclKey := types.NewACLKey(testSubSpaceName, testParamFieldKey)
+	aclOwner := k.GetACL(ctx).GetOwner(aclKey)
+
+	for _, mapValue := range mapValueVariations {
+		res := k.ModifyParam(ctx, aclKey, []byte(mapValue), aclOwner)
+		assert.Zero(t, res.Code)
+
+		s, ok := k.GetSubspace(testSubSpaceName)
+		assert.True(t, ok)
+
+		var value map[string]string
+		s.Get(ctx, []byte(testParamFieldKey), &value)
+
+		// Make sure the value is expected regardless of the order of fields in
+		// `mapValue`.  Ideally we should verify the merkle root of the tree to
+		// make sure binary representation of a param leaf is the same, but there
+		// is no easy way to retrieve the merkle root from `ctx`.
+		assert.True(t, sdk.CompareStringMaps(value, expectedMapValue))
+	}
+
+	for _, mapValue := range invalidParamValues {
+		// Setting an invalid value doesn't fail
+		res := k.ModifyParam(ctx, aclKey, []byte(mapValue), aclOwner)
+		assert.Zero(t, res.Code)
+
+		s, ok := k.GetSubspace(testSubSpaceName)
+		assert.True(t, ok)
+
+		// Value is not updated
+		var value map[string]string
+		s.Get(ctx, []byte(testParamFieldKey), &value)
+		assert.True(t, sdk.CompareStringMaps(value, expectedMapValue))
+	}
+}

x/nodes/keeper/valStateChanges.go

@@ -268,7 +268,7 @@ func (k Keeper) ValidateEditStake(ctx sdk.Ctx, currentValidator, newValidtor typ
 		// For more details, see
 		// https://forum.pokt.network/t/pip32-unleashing-the-potential-of-non-custodial-node-running/4796
 		if k.Cdc.IsAfterRewardDelegatorUpgrade(ctx.BlockHeight()) &&
-			!types.CompareStringMaps(
+			!sdk.CompareStringMaps(
 				currentValidator.RewardDelegators,
 				newValidtor.RewardDelegators,
 			) &&

x/nodes/keeper/valStateChanges_test.go

@@ -569,7 +569,7 @@ func TestValidatorStateChange_Delegators(t *testing.T) {
 	assert.True(t, found)
 	assert.True(
 		t,
-		types.CompareStringMaps(validatorCur.RewardDelegators, singleDelegator),
+		sdk.CompareStringMaps(validatorCur.RewardDelegators, singleDelegator),
 	)
 
 	// Attempt to reset the delegators with operator's signature --> Success
@@ -586,7 +586,7 @@ func TestValidatorStateChange_Delegators(t *testing.T) {
 	assert.True(t, found)
 	assert.True(
 		t,
-		types.CompareStringMaps(validatorCur.RewardDelegators, singleDelegator),
+		sdk.CompareStringMaps(validatorCur.RewardDelegators, singleDelegator),
 	)
 }
 

x/nodes/keeper/validator_test.go

@@ -262,7 +262,7 @@ func TestValidator_Proto_MarshalingCompatibility(t *testing.T) {
 	assert.NotNil(t, val_2.RewardDelegators)
 	assert.True(
 		t,
-		types.CompareStringMaps(val_2.RewardDelegators, val_1.RewardDelegators),
+		sdk.CompareStringMaps(val_2.RewardDelegators, val_1.RewardDelegators),
 	)
 
 	// Validator --> []byte --> LegacyValidator

x/nodes/types/util.go

@@ -80,19 +80,3 @@ func CompareSlices[T comparable](a, b []T) bool {
 
 	return true
 }
-
-// True if two maps are equivalent.
-// Nil is considered to be the same as an empty map.
-func CompareStringMaps[T comparable](a, b map[string]T) bool {
-	if len(a) != len(b) {
-		return false
-	}
-
-	for k, v := range a {
-		if v != b[k] {
-			return false
-		}
-	}
-
-	return true
-}

x/nodes/types/util_test.go

@@ -37,31 +37,3 @@ func TestCompareSlices(t *testing.T) {
 	assert.True(t, CompareSlices(nil, []int{}))
 	assert.True(t, CompareSlices([]int{}, []int{}))
 }
-
-func TestCompareStringMaps(t *testing.T) {
-	m1 := map[string]int{}
-	m2 := map[string]int{}
-	assert.True(t, CompareStringMaps(m1, m2))
-
-	// m1 is non-empty and m2 is empty
-	m1["a"] = 10
-	m1["b"] = 100
-	assert.False(t, CompareStringMaps(m1, m2))
-
-	// m1 and m2 are not empty and identical
-	m2["b"] = 100
-	m2["a"] = 10
-	assert.True(t, CompareStringMaps(m2, m1))
-
-	// m1 is non-empty and m2 is nil
-	m2 = nil
-	assert.False(t, CompareStringMaps(m1, m2))
-	assert.False(t, CompareStringMaps(nil, m1))
-
-	// m1 and m2 are both nil
-	m1 = nil
-	assert.True(t, CompareStringMaps(m1, m2))
-
-	// Empty and nil maps are identical
-	assert.True(t, CompareStringMaps(nil, map[string]int{}))
-}