[CON-126] Do not return error string on precompile error (#2563)

## Describe your changes and provide context

### Context

Different precompile error messages lead to app hash.
Reproduction suite:
https://github.com/sei-protocol/sei-chain/pull/2545/files

### Mechanism
`resultsHash` which is part of consensus is derived by hashing
marshalled transaction results, only deterministic fields should be
included during the marshalling
[operation](https://github.com/sei-protocol/sei-chain/blob/fe95a1ff76a108ff37347a16ee6139509322e058/sei-tendermint/abci/types/types.go#L199-L214)
- [error
code](https://github.com/sei-protocol/sei-chain/blob/fe95a1ff76a108ff37347a16ee6139509322e058/sei-tendermint/proto/tendermint/abci/types.proto#L429),
[data](https://github.com/sei-protocol/sei-chain/blob/fe95a1ff76a108ff37347a16ee6139509322e058/sei-tendermint/proto/tendermint/abci/types.proto#L430),
[gas_wanted](https://github.com/sei-protocol/sei-chain/blob/fe95a1ff76a108ff37347a16ee6139509322e058/sei-tendermint/proto/tendermint/abci/types.proto#L433),
[gas_used](https://github.com/sei-protocol/sei-chain/blob/fe95a1ff76a108ff37347a16ee6139509322e058/sei-tendermint/proto/tendermint/abci/types.proto#L434).

Initially I thought that the error code is being [decoded
indeterministically](https://github.com/sei-protocol/sei-chain/blob/06a4e242bf80fff303be607734e121bd2f0f6916/sei-cosmos/types/errors/abci.go#L39)
which turned out
[untrue](https://github.com/sei-protocol/sei-chain/blob/fe95a1ff76a108ff37347a16ee6139509322e058/sei-cosmos/baseapp/abci.go#L283-L289).
It turned out that the data field is indeterministic.

Return data gets [populated with a stringified error
](https://github.com/sei-protocol/sei-chain/blob/07441d7bfcd7f9fc69119cf3002be7d6912b3a87/precompiles/common/precompiles.go#L159-L166)if
the precompile errors out, this bubbles up as aforementioned
[data](https://github.com/sei-protocol/sei-chain/blob/fe95a1ff76a108ff37347a16ee6139509322e058/sei-tendermint/proto/tendermint/abci/types.proto#L430),
stringified error can among other things contain the path to executable
by way of including the call stack.

PR that introduced the issue -
#1757.

### Potential Solutions
One of the reasons ABCI decodes errors and excludes
[log](https://github.com/sei-protocol/sei-chain/blob/fe95a1ff76a108ff37347a16ee6139509322e058/sei-tendermint/proto/tendermint/abci/types.proto#L293),
[info](https://github.com/sei-protocol/sei-chain/blob/fe95a1ff76a108ff37347a16ee6139509322e058/sei-tendermint/proto/tendermint/abci/types.proto#L294),
[events](https://github.com/sei-protocol/sei-chain/blob/fe95a1ff76a108ff37347a16ee6139509322e058/sei-tendermint/proto/tendermint/abci/types.proto#L297)
fields from consensus is to guard from indeterminism.
Ethereum on the other side discerns between two types of errors during
transaction execution - vmerrors and consensus/client errors - vmerrors
are not part of consensus in any way and errors coming from precompile
runs fall into this category.
Broadly there are a couple of ways to solve this:
1. Remove all inderterminism from errors - do not wrap errors, etc. -
short term this will work for the specific scenario we encountered, long
term we will have exactly the same issue because we will forget that
changing an error message is an app hash break
2. Populate return data deterministically if a precompile errors out -
this approach would be similar to what ABCI does by reducing errors to
codes/codespaces by decoding them
3. Do not populate return data if a precompile errors out - precompiles
that error out should never have side effects (to be confirmed) which
makes this approach viable and it is the most right/Ethereum equivalent
way of doing things

### Solution

[1.] is a hotfix - which we already have - but not a long running
solution as every precompile error message diff would lead to app hash
break.

[2.] does work but we've got no additional benefits from reducing
precompile errors to error codes/spaces.

Therefore picked [3.]

What we lose by this solution is visibility into the specific error that
happened - we can retain that by concatenate the error string to the
execution reverted error.

Comment on security concerns (proof by negation): 
This won't affect security - the only way in which including a
precompile error message into consensus can boost security is by making
sure the precompile run of every actor ended at exactly the same point -
this can easily be bypassed (both with or without this PR) by a
malicious actor executing the actual precompile and additional code
besides that so guardrails should be elsewhere.

## Testing performed to validate your change

Ran a local chain against the testing suite, also CI/CD.

Author: pdrobnjak

precompiles/addr/addr_test.go

@@ -167,7 +167,7 @@ func TestAssociatePubKey(t *testing.T) {
 			}
 			if err != nil {
 				require.Equal(t, vm.ErrExecutionReverted, err)
-				require.Equal(t, tt.wantErrMsg, string(ret))
+				require.Nil(t, ret)
 			} else if tt.wrongRet {
 				// tt.wrongRet is set if we expect a return value that's different from the happy path. This means that the wrong addresses were associated.
 				require.NotEqual(t, tt.wantRet, ret)
@@ -358,7 +358,7 @@ func TestAssociate(t *testing.T) {
 			}
 			if err != nil {
 				require.Equal(t, vm.ErrExecutionReverted, err)
-				require.Equal(t, tt.wantErrMsg, string(ret))
+				require.Nil(t, ret)
 			} else if tt.wrongRet {
 				// tt.wrongRet is set if we expect a return value that's different from the happy path. This means that the wrong addresses were associated.
 				require.NotEqual(t, tt.wantRet, ret)

precompiles/common/precompiles.go

@@ -66,7 +66,6 @@ func (p Precompile) Run(evm *vm.EVM, caller common.Address, callingContract comm
 	defer func() {
 		HandlePrecompileError(err, evm, operation)
 		if err != nil {
-			bz = []byte(err.Error())
 			err = vm.ErrExecutionReverted
 		}
 	}()
@@ -159,7 +158,6 @@ func (d DynamicGasPrecompile) RunAndCalculateGas(evm *vm.EVM, caller common.Addr
 	defer func() {
 		HandlePrecompileError(err, evm, operation)
 		if err != nil {
-			ret = []byte(err.Error())
 			fmt.Printf("precompile %s encountered error: %v\n", d.name, err)
 			err = vm.ErrExecutionReverted
 		}

precompiles/common/precompiles_test.go

@@ -86,7 +86,7 @@ func TestPrecompileRun(t *testing.T) {
 	stateDB.WithCtx(ctx.WithEventManager(sdk.NewEventManager()))
 	precompile = common.NewPrecompile(newAbi, &MockPrecompileExecutor{throw: true}, ethcommon.Address{}, "test")
 	res, err = precompile.Run(&vm.EVM{StateDB: stateDB}, ethcommon.Address{}, ethcommon.Address{}, input, big.NewInt(0), false, false, nil)
-	require.NotNil(t, res)
+	require.Nil(t, res)
 	require.NotNil(t, err)
 	// should not emit any event
 	require.Empty(t, stateDB.Ctx().EventManager().Events())
@@ -127,7 +127,7 @@ func TestDynamicGasPrecompileRun(t *testing.T) {
 	stateDB.WithCtx(ctx.WithEventManager(sdk.NewEventManager()))
 	precompile = common.NewDynamicGasPrecompile(newAbi, &MockDynamicGasPrecompileExecutor{throw: true, evmKeeper: k}, ethcommon.Address{}, "test")
 	res, _, err = precompile.RunAndCalculateGas(&vm.EVM{StateDB: stateDB}, ethcommon.Address{}, ethcommon.Address{}, input, 0, big.NewInt(0), nil, false, false)
-	require.NotNil(t, res)
+	require.Nil(t, res)
 	require.NotNil(t, err)
 	// should not emit any event
 	require.Empty(t, stateDB.Ctx().EventManager().Events())

precompiles/distribution/distribution_test.go

@@ -502,7 +502,7 @@ func TestPrecompile_RunAndCalculateGas_WithdrawDelegationRewards(t *testing.T) {
 			}
 			if err != nil {
 				require.Equal(t, vm.ErrExecutionReverted, err)
-				require.Equal(t, tt.wantErrMsg, string(gotRet))
+				require.Nil(t, gotRet)
 			} else if !reflect.DeepEqual(gotRet, tt.wantRet) {
 				t.Errorf("RunAndCalculateGas() gotRet = %v, want %v", gotRet, tt.wantRet)
 			}
@@ -663,7 +663,7 @@ func TestPrecompile_RunAndCalculateGas_WithdrawMultipleDelegationRewards(t *test
 			}
 			if err != nil {
 				require.Equal(t, vm.ErrExecutionReverted, err)
-				require.Equal(t, tt.wantErrMsg, string(gotRet))
+				require.Nil(t, gotRet)
 			} else if !reflect.DeepEqual(gotRet, tt.wantRet) {
 				t.Errorf("RunAndCalculateGas() gotRet = %v, want %v", gotRet, tt.wantRet)
 			}
@@ -839,7 +839,7 @@ func TestPrecompile_RunAndCalculateGas_SetWithdrawAddress(t *testing.T) {
 			}
 			if err != nil {
 				require.Equal(t, vm.ErrExecutionReverted, err)
-				require.Equal(t, tt.wantErrMsg, string(gotRet))
+				require.Nil(t, gotRet)
 			} else if !reflect.DeepEqual(gotRet, tt.wantRet) {
 				t.Errorf("RunAndCalculateGas() gotRet = %v, want %v", gotRet, tt.wantRet)
 			}
@@ -1136,7 +1136,7 @@ func TestPrecompile_RunAndCalculateGas_Rewards(t *testing.T) {
 			}
 			if err != nil {
 				require.Equal(t, vm.ErrExecutionReverted, err)
-				require.Equal(t, tt.wantErrMsg, string(gotRet))
+				require.Nil(t, gotRet)
 			} else if !reflect.DeepEqual(gotRet, tt.wantRet) {
 				t.Errorf("RunAndCalculateGas() gotRet = %v, want %v", gotRet, tt.wantRet)
 			}
@@ -1235,7 +1235,7 @@ func TestWithdrawValidatorCommission_noCommissionToWithdrawRightAfterDelegation(
 	ante.Preprocess(ctx, req, k.ChainID(ctx), false)
 	res, err = msgServer.EVMTransaction(sdk.WrapSDKContext(ctx), req)
 	require.Nil(t, err)
-	require.Equal(t, "no validator commission to withdraw", string(res.ReturnData))
+	require.Nil(t, res.ReturnData)
 
 }
 
@@ -1376,7 +1376,7 @@ func TestWithdrawValidatorCommission_InputValidation(t *testing.T) {
 
 			if tc.wantError {
 				require.NotNil(t, err, "Expected error for test case: %s", tc.name)
-				require.Equal(t, tc.wantErrMsg, string(ret))
+				require.Nil(t, ret)
 			} else {
 				require.Nil(t, err, "Expected no error for test case: %s", tc.name)
 				require.Greater(t, remainingGas, uint64(0), "Should have remaining gas")

precompiles/gov/gov_test.go

@@ -312,7 +312,7 @@ func TestGovPrecompile(t *testing.T) {
 			res, err := msgServer.EVMTransaction(sdk.WrapSDKContext(ctx), req)
 			if tt.wantErr {
 				require.NotEmpty(t, res.VmError)
-				require.Contains(t, string(res.ReturnData), tt.wantErrMsgToContain)
+				require.Nil(t, res.ReturnData)
 			} else {
 				require.Nil(t, err)
 				require.Empty(t, res.VmError)
@@ -751,7 +751,7 @@ func TestPrecompileExecutor_submitProposal(t *testing.T) {
 
 			if tt.wantErr {
 				require.NotEmpty(t, gotRet.VmError)
-				require.Equal(t, string(gotRet.ReturnData), tt.wantErrMsg)
+				require.Nil(t, gotRet.ReturnData)
 			} else {
 				require.Empty(t, gotRet.VmError)
 				require.Nil(t, err)

precompiles/ibc/ibc_test.go

@@ -337,7 +337,7 @@ func TestPrecompile_Run(t *testing.T) {
 			}
 			if err != nil {
 				require.Equal(t, vm.ErrExecutionReverted, err)
-				require.Equal(t, tt.wantErrMsg, string(gotBz))
+				require.Nil(t, gotBz)
 			} else if !reflect.DeepEqual(gotBz, tt.wantBz) {
 				t.Errorf("Run() gotRet = %v, want %v", gotBz, tt.wantBz)
 			}
@@ -538,7 +538,7 @@ func TestTransferWithDefaultTimeoutPrecompile_Run(t *testing.T) {
 			}
 			if err != nil {
 				require.Equal(t, vm.ErrExecutionReverted, err)
-				require.Equal(t, tt.wantErrMsg, string(gotBz))
+				require.Nil(t, gotBz)
 			} else if !reflect.DeepEqual(gotBz, tt.wantBz) {
 				t.Errorf("Run() gotRet = %v, want %v", gotBz, tt.wantBz)
 			}

precompiles/json/json_test.go

@@ -281,7 +281,7 @@ func TestPrecompileExecutor_extractAsBytesFromArray(t *testing.T) {
 
 			if tt.wantErr {
 				require.Error(t, err)
-				require.Equal(t, tt.wantErrMsg, string(res))
+				require.Nil(t, res)
 				return
 			} else {
 				output, err := method.Outputs.Unpack(res)

precompiles/staking/staking_test.go

@@ -473,7 +473,7 @@ func TestPrecompile_Run_Delegation(t *testing.T) {
 			}
 			if err != nil {
 				require.Equal(t, vm.ErrExecutionReverted, err)
-				require.Equal(t, tt.wantErrMsg, string(gotRet))
+				require.Nil(t, gotRet)
 			} else if !reflect.DeepEqual(gotRet, tt.wantRet) {
 				t.Errorf("Run() gotRet = %v, want %v", gotRet, tt.wantRet)
 			}
@@ -691,7 +691,7 @@ func TestCreateValidator(t *testing.T) {
 
 			if tt.wantErr {
 				require.NotEmpty(t, res.VmError, "Expected error but transaction succeeded")
-				require.Equal(t, tt.wantErrMsg, string(res.ReturnData), "Expected error: %s", res.VmError)
+				require.Nil(t, res.ReturnData)
 			} else {
 				require.Empty(t, res.VmError, "Unexpected error: %s", res.VmError)
 				// Additional validation for successful cases
@@ -752,7 +752,7 @@ func TestCreateValidator_UnassociatedAddress(t *testing.T) {
 	res, err := setup.msgServer.EVMTransaction(sdk.WrapSDKContext(setup.ctx), req)
 	require.NoError(t, err)
 	require.NotEmpty(t, res.VmError, "Should fail with unassociated address")
-	require.Equal(t, "address "+unassociatedEvmAddr.String()+" is not linked", string(res.ReturnData), "Should fail with unassociated address")
+	require.Nil(t, res.ReturnData)
 }
 
 func TestEditValidator_ErorrIfDoesNotExist(t *testing.T) {
@@ -809,7 +809,7 @@ func TestEditValidator_ErorrIfDoesNotExist(t *testing.T) {
 	require.NoError(t, err)
 	// Should fail because validator doesn't exist
 	require.NotEmpty(t, res.VmError, "Should fail because validator doesn't exist")
-	require.Contains(t, string(res.ReturnData), "validator does not exist")
+	require.Nil(t, res.ReturnData)
 }
 
 func TestEditValidator(t *testing.T) {

precompiles/wasmd/wasmd_test.go

@@ -156,7 +156,7 @@ func TestExecute(t *testing.T) {
 	statedb.WithCtx(statedb.Ctx().WithIsEVM(false))
 	testApp.EvmKeeper.SetCode(statedb.Ctx(), mockEVMAddr, []byte{1, 2, 3})
 	res, _, err := p.RunAndCalculateGas(&evm, mockEVMAddr, mockEVMAddr, append(p.GetExecutor().(*wasmd.PrecompileExecutor).ExecuteID, args...), suppliedGas, big.NewInt(1000_000_000_000_000), nil, false, false)
-	require.Equal(t, "sei does not support CW->EVM->CW call pattern", string(res))
+	require.Nil(t, res)
 	require.Equal(t, vm.ErrExecutionReverted, err)
 	statedb.WithCtx(statedb.Ctx().WithIsEVM(true))
 	res, g, err := p.RunAndCalculateGas(&evm, mockEVMAddr, mockEVMAddr, append(p.GetExecutor().(*wasmd.PrecompileExecutor).ExecuteID, args...), suppliedGas, big.NewInt(1000_000_000_000_000), nil, false, false)