Questions about sig verification decorator `panic`

[rootulp]

Context

Celestia is upgrading from Cosmos SDK v0.46.x to v0.50.x

Problem

In Cosmos SDK v0.46.x, when a user submit a tx with a nil public key in the signature data, it would fail in the sig verify decorator (confirmed in this test).

In Cosmos SDK v0.50.x, when a user submits the same tx with a nil public key in the signature data, it would result in a panic. See this issue.

These lines were added recently to main

cosmos-sdk/x/auth/tx/adapter.go

Lines 64 to 66 in b862752

	if signerInfo.PublicKey == nil {
	panic("signerInfo.PublicKey cannot be nil")
	}

which make the panic message more useful to end users. I'm porting that change to Celestia's Cosmos SDK fork but I have a few questions:

1. If a tx's signature data has a nil public key, is that always considered invalid?
2. Instead of hitting this panic, can we introduce some stateless checks on the signature earlier in the antehandler flow in order to throw an error?

[linear]

SDK-356 Questions about sig verification decorator `panic`

[aljo242]

Yes, the signature is required to have a public key, so I agree with 2. that we should add a nil check and return a specific error for that condition

[evan-forbes]

This feels weird, since the state machine definitely does not require that a public key be included

when we set the pubkey in the setup pubkey decorator, we first check if the pub key is in the state and use that instead of what is included in the transaction

https://github.com/celestiaorg/cosmos-sdk/blob/57e56239bdfd8ec9e92cd4eb6f06241297fa345b/x/auth/ante/sigverify.go#L103-L106

the existing tx builder also doesn't require it https://github.com/celestiaorg/cosmos-sdk/blob/57e56239bdfd8ec9e92cd4eb6f06241297fa345b/x/auth/tx/builder.go#L166-L170

the first transaction must have a public key, which is why that test fails @rootulp afaict, however we have many transactions that don't have public keys that operate normally

look at the raw json of this tx, there's no public key https://celestia.explorers.guru/transaction/06972902E92EC39E9E80DD9453B4E76362F12249728812B660236FF2D5733816?height=5973642

[rootulp]

however we have many transactions that don't have public keys that operate normally

Can you point to such a transaction on Cosmos SDK v0.50.x? When I reproduced celestiaorg/celestia-app#4847 I observed that the PubKey needs to be included in the signature data because if it isn't a panic is encountered.

[evan-forbes]

Can you point to such a transaction on Cosmos SDK v0.50.x?

apologies you are correct, we can't on v0.50 which seems like an accident considering the points made above, however we definitely can on v0.46 and v0.47 (see tx and tests that submit multiple txs from the same account)

it's ofc too late to change this behavior since it would be consensus breaking, and commented here to try and get a better understanding of if this was in fact a simple mistake or was done on purpose
https://github.com/cosmos/cosmos-sdk/pull/15910/files#r2150875680

[evan-forbes]

this seems like a mistake considering #19651

(which should have never been included in a non-consensus-breaking release!!)

this was also patched on the celestia fork celestiaorg#371