core: fix TestStateProcessorErrors test

[MariusVanDerWijden]

#31032 was merged under the assumption that chainID will never be nil in our code (since that is checked during unmarshalling). However during some hand-constructed tests, we kept the chainID unset, which resulted in a panic'ing test

[fjl]

We should probably add an error case somewhere in package types to catch this condition. I feel like panic is not a good option. If panic is wanted, MustSignTx can be used.

[MariusVanDerWijden]

Okay I fixed it a different way now, which creates a lot more allocations, but I don't think we need to care about that...

In core/types/transaction.go:L277 we have the following comment:

// ChainId returns the EIP155 chain ID of the transaction. The return value will always be
// non-nil. For legacy transactions which are not replay-protected, the return value is
// zero.
func (tx *Transaction) ChainId() *big.Int {
	return tx.inner.chainID()
}

Our implementation broke this assumption, with the fix I added, we are back to that assumption.
I'm not 100% convinced that we need to have the assumption, we could get rid of that in order to save a few allocations on ChainID().

[MariusVanDerWijden]

Alternative:

diff --git a/core/types/transaction_signing.go b/core/types/transaction_signing.go
index 030fc472a0..8fdf3998ee 100644
--- a/core/types/transaction_signing.go
+++ b/core/types/transaction_signing.go
@@ -219,6 +219,9 @@ func (s pragueSigner) SignatureValues(tx *Transaction, sig []byte) (R, S, V *big
        }
        // Check that chain ID of tx matches the signer. We also accept ID zero here,
        // because it indicates that the chain ID was not specified in the tx.
+       if txdata.ChainID == nil {
+               return nil, nil, nil, fmt.Errorf("%w: nil chainID", ErrInvalidChainId)
+       }
        if txdata.ChainID.Sign() != 0 && txdata.ChainID.CmpBig(s.chainId) != 0 {
                return nil, nil, nil, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, txdata.ChainID, s.chainId)
        }
@@ -287,6 +290,9 @@ func (s cancunSigner) SignatureValues(tx *Transaction, sig []byte) (R, S, V *big
        }
        // Check that chain ID of tx matches the signer. We also accept ID zero here,
        // because it indicates that the chain ID was not specified in the tx.
+       if txdata.ChainID == nil {
+               return nil, nil, nil, fmt.Errorf("%w: nil chainID", ErrInvalidChainId)
+       }
        if txdata.ChainID.Sign() != 0 && txdata.ChainID.CmpBig(s.chainId) != 0 {
                return nil, nil, nil, fmt.Errorf("%w: have %d want %d", ErrInvalidChainId, txdata.ChainID, s.chainId)
        }
        ```

[rjl493456442]

I would prefer to fix the test, rather than adding the nil-handling in the chainID function.

Reasons:

• The tx with nil chainID will be rejected during the unmarshalling
• The tx with nil chainID will be set with zero chain ID in rlp decoding

Therefore, it's impossible to have a nil chainID except the tests, if I understand correctly?

[fjl]

The modified function SignatureValues is only called during transaction signing. We have two functions for signing: SignTx and MustSignTx. Only MustSignTx should panic on failure.

[rjl493456442]

it's impossible to have nil chainID if the tx is constructed from the json unmarshal or rlp decode, right?

it's only possible for the manual constructed one.

maybe the default zero value is unnecessary

[MariusVanDerWijden]

Yep its only possible for hand-created transactions, I added it, because we have this comment here:

// ChainId returns the EIP155 chain ID of the transaction. The return value will always be
// non-nil. For legacy transactions which are not replay-protected, the return value is
// zero.
func (tx *Transaction) ChainId() *big.Int {
	return tx.inner.chainID()
}

I can revert, but then I would also remove this comment

[islishude]

Hey, I have another approach, just let it be consistent with blobTx

#31054

[fjl]

Closing because this got merged: #31054