core/types: correct chainId check for pragueSigner

[islishude]

Use zero value check for the pragueSigner

This aligns with cancunSigner and londonSigner as well.

[lightclient]

We should change this is all signers since 2930 if we want to make this change. It seems like the right thing to do -- @fjl added this line in dd2483c, maybe he can chime in if it's okay to switch over all other instances of this?

IIUC, this new check avoids derefing a nil chain id and disallows chain id 0. Since SignatureValues is only used by WithSignature, this shouldn't affect any aspects of consensus. But since chain id 0 txs are not allowed post EIP-155, it seems correct to make this update.

[fjl]

I have checked this, and it's an API change for users. Block processing can never hit a tx with ChainID == nil because the decoding of transactions doesn't allow it (and SignatureValues isn't used as you say). It can only happen for transactions which are being signed by a user. There, we specifically allow ChainID == 0 to mean that you want the chain ID to be auto-set during signing. It's OK to also allow nil for this, but the intended behavior needs to remain in place.

The pragueSigner handles this incorrectly right now and we should change it to restore this behavior.

[islishude]

I prefer to use nil check, because types.SignTx always set the chainId internally

signer := types.NewCancunSigner(chainId)
tx := types.NewTx(&types.BlobTx{
        // whatever the chainId here is, nil or non-nil, it will be set by the signer
	ChainID:    uint256.MustFromBig(chainId), 
})
signedTx, err := types.SignTx(tx, signer, privateKey) // it will set the chainId internally
if err != nil {
	panic(err)
}

[islishude]

using the zero value check is not a bad idea since it's not a breaking change.

Just reverted the change, and updated the pragueSigner

[MariusVanDerWijden]

SGTM