[Bug]: Authz MsgExecResponse.Results encoding

[bryanchriswhite]

Is there an existing issue for this?

• I have searched the existing issues

What happened?

Background

While working on pokt-network/poktroll#826, I experienced issues when adding test assertions around the MsgExecResponse.Results field.

I've outlined the codepaths that I followed to bulid context and formulate my expectation in a comment in the test in question:

// TODO_INVESTIGATE: It seems like the response objects are encoded in
// an unexpected way. It's unclear whether this is the result of being
// executed via authz. Looking at the code, it seems like authz utilizes the
// sdk.Result#Data field of the result which is returned from the message handler.
// These result byte slices are accumulated for each message in the MsgExec and
// set on the MsgExecResponse#Results field.
//
// - https://github.com/cosmos/cosmos-sdk/blob/v0.50.9/x/authz/keeper/msg_server.go#L120
// - https://github.com/cosmos/cosmos-sdk/blob/v0.50.9/x/authz/keeper/keeper.go#L166
// - https://github.com/cosmos/cosmos-sdk/blob/v0.50.9/baseapp/msg_service_router.go#L55
// - https://github.com/cosmos/cosmos-sdk/blob/v0.50.9/baseapp/msg_service_router.go#L198
// - https://github.com/cosmos/cosmos-sdk/blob/v0.50.9/types/result.go#L213
//
// I (@bryanchriswhite) would've expected the following to work, but it does not:

updateResValue := reflect.New(reflect.TypeOf(moduleCfg.ParamsMsgs.MsgUpdateParamResponse))
// NB: using proto.Unmarshal here because authz seems to use
// proto.Marshal to serialize each message response.
err = proto.Unmarshal(updateResBz, updateResValue.Interface().(cosmostypes.Msg))
require.NoError(t, err)
updateResParamValue := updateResValue.Elem().FieldByName("Params").Elem().FieldByName(fieldName)
require.Equal(t, fieldExpectedValue.Interface(), updateResParamValue.Interface())

This snippet above is an excerpt from a test which can be summarized like so:

Given some cosmos-sdk appchain modules, "Modules"
And each module is configured with some authority, "Authority"
And each module supports a distinct `MsgUpdateParam` message
And an authz grant exists which authorizes "Authorized" to execute any module's `MsgUpdateParam` message on behalf of "Authority"
When an authz `MsgExec` which includes a `MsgUpdateParam` message and is sent from "Authorized" is handled
Then the `MsgExecResponse.Results` should be a slice of serialized message response objects corresponding to the messages which were executed in the `MsgExec` (e.g. `MsgUpdateParamResponse`)
When the params are queried next
Then the params should have been updated

Relevant modules

Currently, only some modules have any params. Support for MsgUpdateParam is limted to modules which do:

• shared
• proof
• service

Due diligence

Each module does indeed include a Params field in its respective MsgUpdateParamResponse type, assign a non-zero-value value to it in their respective message handlers, and register the correct type as the return for the UpdateParam rpc definition:

• sharedtypes.MsgUpdateParamResponse
  • params field
  • rpc signature
  • handler assignment & return
• prooftypes.MsgUpdateParamResponse
  • params field
  • rpc signature
  • handler assignment & return
• servicetypes.MsgUpdateParamResponse
  • params field
  • rpc signature
  • handler

Failure modes

The final assertions from the test summary hold:

...
When the params are queried next
Then the params should have been updated

It is only the assertion(s) around the MsgExecResponse.Results which are problematic:

...
Then the `MsgExecResponse.Results` should be a slice of serialized message response objects corresponding to the messages which were executed in the `MsgExec` (e.g. `MsgUpdateParamResponse`)
...

wrong wireType

In the case of the shared module, an error is returned when attempting to decode the first (and only) serialized message response (i.e. MsgExec.Results[0]):

=== RUN   TestUpdateParamSuite/TestAuthorizedMsgUpdateParamSucceeds/shared_NumBlocksPerSession
    update_param_test.go:128: 
        	Error Trace:	/home/bwhite/Projects/pokt/poktroll/tests/integration/params/update_param_test.go:128
        	Error:      	Received unexpected error:
        	            	proto: wrong wireType = 2 for field NumBlocksPerSession
        	Test:       	TestUpdateParamSuite/TestAuthorizedMsgUpdateParamSucceeds/shared_NumBlocksPerSession
--- FAIL: TestUpdateParamSuite/TestAuthorizedMsgUpdateParamSucceeds/shared_NumBlocksPerSession (0.02s)

Unexpected decoded values

In the case of the proof and service modules, decoding as described doesn't cause an error. Instead, the first service and proof module params values seem to hold values that contain the entire params struct data (i.e. a serialize Param object, rather than a particular field value thereof):

=== RUN   TestUpdateParamSuite/TestAuthorizedMsgUpdateParamSucceeds/proof_RelayDifficultyTargetHash
    update_param_test.go:130: 
        	Error Trace:	/home/bwhite/Projects/pokt/poktroll/tests/integration/params/update_param_test.go:130
        	Error:      	Not equal: 
        	            	expected: []byte{0x0, 0x0, 0x0, 0x0, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}
        	            	actual  : []byte{0xa, 0x20, 0x0, 0x0, 0x0, 0x0, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x15, 0x0, 0x0, 0x80, 0x3e, 0x1a, 0x11, 0xa, 0x5, 0x75, 0x70, 0x6f, 0x6b, 0x74, 0x12, 0x8, 0x32, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x22, 0x12, 0xa, 0x5, 0x75, 0x70, 0x6f, 0x6b, 0x74, 0x12, 0x9, 0x33, 0x32, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x2a, 0x10, 0xa, 0x5, 0x75, 0x70, 0x6f, 0x6b, 0x74, 0x12, 0x7, 0x31, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,4 +1,8 @@
        	            	-([]uint8) (len=32) {
        	            	- 00000000  00 00 00 00 ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
        	            	+([]uint8) (len=96) {
        	            	+ 00000000  0a 20 00 00 00 00 ff ff  ff ff ff ff ff ff ff ff  |. ..............|
        	            	  00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
        	            	+ 00000020  ff ff 15 00 00 80 3e 1a  11 0a 05 75 70 6f 6b 74  |......>....upokt|
        	            	+ 00000030  12 08 32 30 30 30 30 30  30 30 22 12 0a 05 75 70  |..20000000"...up|
        	            	+ 00000040  6f 6b 74 12 09 33 32 30  30 30 30 30 30 30 2a 10  |okt..320000000*.|
        	            	+ 00000050  0a 05 75 70 6f 6b 74 12  07 31 30 30 30 30 30 30  |..upokt..1000000|
        	            	 }
        	Test:       	TestUpdateParamSuite/TestAuthorizedMsgUpdateParamSucceeds/proof_RelayDifficultyTargetHash
--- FAIL: TestUpdateParamSuite/TestAuthorizedMsgUpdateParamSucceeds/proof_RelayDifficultyTargetHash (0.03s)

Expected :[]byte{0x0, 0x0, 0x0, 0x0, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}
Actual   :[]byte{0xa, 0x20, 0x0, 0x0, 0x0, 0x0, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x15, 0x0, 0x0, 0x80, 0x3e, 0x1a, 0x11, 0xa, ...

E.g., proof_RelayDifficultyTargetHash

=== RUN   TestUpdateParamSuite/TestAuthorizedMsgUpdateParamSucceeds/service_AddServiceFee
    update_param_test.go:130: 
        	Error Trace:	/home/bwhite/Projects/pokt/poktroll/tests/integration/params/update_param_test.go:130
        	Error:      	Not equal: 
        	            	expected: &types.Coin{Denom:"upokt", Amount:math.Int{i:(*big.Int)(0xc00123a120)}}
        	            	actual  : &types.Coin{Denom:"\n\x05upokt\x12\n1000000001", Amount:math.Int{i:(*big.Int)(nil)}}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,10 +1,5 @@
        	            	 (*types.Coin)({
        	            	- Denom: (string) (len=5) "upokt",
        	            	+ Denom: (string) (len=19) "\n\x05upokt\x12\n1000000001",
        	            	  Amount: (math.Int) {
        	            	-  i: (*big.Int)({
        	            	-   neg: (bool) false,
        	            	-   abs: (big.nat) (len=1) {
        	            	-    (big.Word) 1000000001
        	            	-   }
        	            	-  })
        	            	+  i: (*big.Int)(<nil>)
        	            	  }
        	Test:       	TestUpdateParamSuite/TestAuthorizedMsgUpdateParamSucceeds/service_AddServiceFee
--- FAIL: TestUpdateParamSuite/TestAuthorizedMsgUpdateParamSucceeds/service_AddServiceFee (0.04s)

Expected :&types.Coin{Denom:"upokt", Amount:math.Int{i:(*big.Int)(0xc00123a120)}}
Actual   :&types.Coin{Denom:"\n\x05upokt\x12\n1000000001", Amount:math.Int{i:(*big.Int)(nil)}}

E.g., service_AddServiceFee

Nil or zero-value field values

In the case of the proof module, it seems that when asserting on any field other than the first, the resulting concrete object is its type's zero-value or a nil pointer. This can be observed by adding the following line above the failing assertion (in the excerpt above), which causes 4 of the proof module's tests cases to fail for this reason:

switch updateResParamValue.Kind() {
case reflect.Ptr:
	require.False(t, updateResParamValue.IsNil())
default:
	require.False(t, updateResParamValue.IsZero())
}

This observation might apply generally as the service module's response params only include one value which results in the second failure mode. It would stand to reason that if additional fields were present, they would result in this failure mode.

Cosmos SDK Version

0.50.9

How to reproduce?

git clone https://github.com/pokt-network/poktroll

# See: https://dev.poktroll.com/develop/developer_guide/quickstart#0-install-dependencies
make install_ci_deps
make go_develop

# Uncomment the assertions from the excerpt above; i.e., `tests/integration/params/update_param_test.go:124-130.

go test -v ./tests/integration/params/update_param_test.go