[Bug]: Gas simulation uses incorrect key

[outofforest]

Is there an existing issue for this?

• [X] I have searched the existing issues

What happened?

According to what I've found here: https://github.com/cosmos/cosmos-sdk/blob/1a62d774bb3240a7cd8c28fee01a5db369235908/client/tx/factory.go#L446

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/cosmos-sdk@v0.47.4/baseapp/abci.go:720
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).handleQueryGRPC
    github.com/cosmos/cosmos-sdk@v0.47.4/baseapp/abci.go:696
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).Query
    github.com/cosmos/cosmos-sdk@v0.47.4/baseapp/abci.go:541
github.com/cometbft/cometbft/abci/client.(*localClient).QuerySync
    github.com/cometbft/cometbft@v0.37.2/abci/client/local_client.go:259
github.com/cometbft/cometbft/proxy.(*appConnQuery).QuerySync
    github.com/cometbft/cometbft@v0.37.2/proxy/app_conn.go:193
github.com/cometbft/cometbft/rpc/core.ABCIQuery
    github.com/cometbft/cometbft@v0.37.2/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/cometbft@v0.37.2/rpc/jsonrpc/server/http_json_handler.go:108
github.com/cometbft/cometbft/rpc/jsonrpc/server.RegisterRPCFuncs.handleInvalidJSONRPCPaths.func4
    github.com/cometbft/cometbft@v0.37.2/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/cometbft@v0.37.2/rpc/jsonrpc/server/http_server.go:256
github.com/cometbft/cometbft/rpc/jsonrpc/server.Serve.RecoverAndLogHandler.func1
    github.com/cometbft/cometbft@v0.37.2/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: https://github.com/cosmos/cosmos-sdk/pull/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 https://github.com/cosmos/cosmos-sdk/pull/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.