[Bug]: Gas simulation uses incorrect key

[outofforest]

Is there an existing issue for this?

• I have searched the existing issues

What happened?

According to what I've found here:

cosmos-sdk/client/tx/factory.go

Line 446 in 1a62d77

func (f Factory) getSimPK() (cryptotypes.PubKey, error) {

When gas is simulated (using --gas=auto) the first public key found in the keystore is used.
This behavior might break the simulation process giving this or similar error:

Error executing cmd, err: rpc error: code = Unknown desc = 
github.com/cosmos/cosmos-sdk/baseapp.gRPCErrorToSDKError
	github.com/cosmos/[email protected]/baseapp/abci.go:720
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).handleQueryGRPC
	github.com/cosmos/[email protected]/baseapp/abci.go:696
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).Query
	github.com/cosmos/[email protected]/baseapp/abci.go:541
github.com/cometbft/cometbft/abci/client.(*localClient).QuerySync
	github.com/cometbft/[email protected]/abci/client/local_client.go:259
github.com/cometbft/cometbft/proxy.(*appConnQuery).QuerySync
	github.com/cometbft/[email protected]/proxy/app_conn.go:193
github.com/cometbft/cometbft/rpc/core.ABCIQuery
	github.com/cometbft/[email protected]/rpc/core/abci.go:20
reflect.Value.call
	reflect/value.go:596
reflect.Value.Call
	reflect/value.go:380
github.com/cometbft/cometbft/rpc/jsonrpc/server.RegisterRPCFuncs.makeJSONRPCHandler.func3
	github.com/cometbft/[email protected]/rpc/jsonrpc/server/http_json_handler.go:108
github.com/cometbft/cometbft/rpc/jsonrpc/server.RegisterRPCFuncs.handleInvalidJSONRPCPaths.func4
	github.com/cometbft/[email protected]/rpc/jsonrpc/server/http_json_handler.go:140
net/http.HandlerFunc.ServeHTTP
	net/http/server.go:2136
net/http.(*ServeMux).ServeHTTP
	net/http/server.go:2514
github.com/cometbft/cometbft/rpc/jsonrpc/server.maxBytesHandler.ServeHTTP
	github.com/cometbft/[email protected]/rpc/jsonrpc/server/http_server.go:256
github.com/cometbft/cometbft/rpc/jsonrpc/server.Serve.RecoverAndLogHandler.func1
	github.com/cometbft/[email protected]/rpc/jsonrpc/server/http_server.go:229
net/http.HandlerFunc.ServeHTTP
	net/http/server.go:2136
net/http.serverHandler.ServeHTTP
	net/http/server.go:2938
net/http.(*conn).serve
	net/http/server.go:2009
rpc error: code = Unknown desc = expected *signing.MultiSignatureData, got, *signing.SingleSignatureData With gas wanted: '18446744073709551615' and gas used: '14930' : unknown requestpanic: errors: target must be a non-nil pointer

I was able to trigger this by:

1. adding key named aaa to the keystore
2. adding multisig key named bbb to the keystore`
3. simulating transaction sending funds from bbb (multisig account)

In this case transaction is simulated using aaa public key, which is wrong because the node expects multisig public key.

Same things applies the other way:

1. add multisig aaa key
2. add normal bbb key
3. simulate transaction sending funds from bbb (normal account)

This time multisig key aaa is used for simulation, which is wrong again, because node expects normal public key.

How to solve it:
Instead of taking the first key from the store, put the right empty signature there.
I've already implemented possible fix in the past, it's available here: https://github.com/CoreumFoundation/coreum/blob/3634317320c464f3eae766929952ae943205cce2/pkg/client/tx.go#L145

Cosmos SDK Version

0.47

How to reproduce?

No response

[alexanderbez]

Taking a key from the keystore for gas simulation doesn't seem correct to me. Unless something changed, gas estimation used a fake sentinel public key: https://github.com/cosmos/cosmos-sdk/blob/main/x/auth/ante/sigverify.go#L85-L91

[outofforest]

But the code I linked does exactly that - it takes the key from the keystore

[alexanderbez]

Interesting. I had no idea we did that and it seems new-ish. On the original v0.45 version we didn't do that: https://github.com/cosmos/cosmos-sdk/blob/v0.45.0/client/tx/tx.go#L264

I'd have to defer to the author or someone who's more familiar. It seems to have been added in this PR: #11282

[outofforest]

Agree that approach taken there is "intriguing"

[tac0turtle]

Interesting. I had no idea we did that and it seems new-ish. On the original v0.45 version we didn't do that: https://github.com/cosmos/cosmos-sdk/blob/v0.45.0/client/tx/tx.go#L264

I'd have to defer to the author or someone who's more familiar. It seems to have been added in this PR: #11282

could you look into this @alexanderbez otherwise this will sit and never get closed until Julien or I have time.

[alexanderbez]

Yeah sure.

My proposal is to revert #11282 (sorry @fedekunze). This isn't a clean solution IMO. You shouldn't have to rely on the keyring to do this (hence this issue!).

I instead propose we add a WithSimPubKey(pk PubKey) to Factory. Then BuildSimTx will simply use that field. LMK what you think.

[educlerici-zondax]

@tac0turtle can we take this issue?

[alexanderbez]

That would be 🔥 @educlerici-zondax

[JulianToledano]

I instead propose we add a WithSimPubKey(pk PubKey) to Factory. Then BuildSimTx will simply use that field. LMK what you think.

hey! @alexanderbez

would adding a fromName field to the Factory populated at construction time from clientCtx work? Then we can get the Pubkey from keyring

[alexanderbez]

TBH, I'm not sure, but possibly, yes. I didn't spend a ton of time looking into the specifics of my proposed solution, but the general idea should work, yeah.