Check recovered sender address before including txs in the TransactionsByPriceAndNonce list

[jimthematrix]

This fixes a panic error that can happen when externally signed EIP155 transactions are submitted before the EIP155 block according to the chain config.

The panic of the minter:

panic: runtime error: index out of range

goroutine 723 [running]:
github.com/ethereum/go-ethereum/core/types.NewTransactionsByPriceAndNonce(0x4e6a8e0, 0x57a41e8, 0xc42407ad80, 0x57a41e8)
	/Users/jimzhang/workspace/src/github.com/kaleido-io/quorum/build/_workspace/src/github.com/ethereum/go-ethereum/core/types/transaction.go:407 +0x823
github.com/ethereum/go-ethereum/raft.(*minter).getTransactions(0xc420404500, 0xc42407ad20)
	/Users/jimzhang/workspace/src/github.com/kaleido-io/quorum/build/_workspace/src/github.com/ethereum/go-ethereum/raft/minter.go:291 +0x1cf
github.com/ethereum/go-ethereum/raft.(*minter).mintNewBlock(0xc420404500)
	/Users/jimzhang/workspace/src/github.com/kaleido-io/quorum/build/_workspace/src/github.com/ethereum/go-ethereum/raft/minter.go:314 +0xad
github.com/ethereum/go-ethereum/raft.(*minter).mintingLoop.func1()
	/Users/jimzhang/workspace/src/github.com/kaleido-io/quorum/build/_workspace/src/github.com/ethereum/go-ethereum/raft/minter.go:234 +0x3c
created by github.com/ethereum/go-ethereum/raft.throttle.func1
	/Users/jimzhang/workspace/src/github.com/kaleido-io/quorum/build/_workspace/src/github.com/ethereum/go-ethereum/raft/minter.go:216 +0xc0

For RAFT there is a very good chance that Quorum chains may be set up either without EIP155 configuration or EIP155 block is set to a number higher than 1, such that an externally signed transactions with EIP155 signer can cause the chain to be broken forever. there's no recovery from this state as far as we are aware.

The fix is to check the returned error by Sender() method and exclude the offending transaction list from the TransactionsByPriceAndNonce list used by the minter. The tx will be stuck in the txpool but future transactions, non EIP155 txs on any block or EIP155 txs after the EIP155 blocks, will get minted properly

[jbhurat]

Hi @jimthematrix, I did some testing after applying the fix, and the changes look good when config.eip155Block is set in genesis json. However, when config.eip155Block is missing from genesis json, submitting externally eip155 signed transaction, the other nodes no longer panics, but all the subsequent transactions whether or not eip155 signed, go to pending queue on that node.

[jimthematrix]

@jbhurat thanks for the feedback, my understanding is that the panic should still happen on the minter node (raft leader) if EIP155 is not configured. the pending queue is most likely due to a failing tx from the previous tx (since an EIP155 signed tx can't pass pre-check validation on a non-EIP155 chain due to signature containing the chainId), meaning to recover you would have to send in a new tx with the same nonce and higher gas price.

[jbhurat]

When EIP155 configuration is missing I no longer see the panic on the minter node, instead if EIP155 signed transaction is submitted on minter, the transaction is lost. Subsequent non-EIP155 signed transaction work fine. On verifier nodes, EIP155 signed transactions and any subsequent non-EIP155 signed transaction stuck in pending queue. I am going to open a new issue on how to handle missing EIP155 configuration

[jpmsam]

@jimthematrix thanks for your PR!

my understanding is that the panic should still happen on the minter node (raft leader) if EIP155 is not configured

The leader node shouldn't panic regardless of the type of the transaction.

@jbhurat

However, when config.eip155Block is missing from genesis json, submitting externally eip155 signed transaction, the other nodes no longer panics, but all the subsequent transactions whether or not eip155 signed, go to pending queue on that node.

Can we add a validation so that the transaction fails and it isn't stuck in the pending queue on the originating node ?

[jimthematrix]

@jbhurat @jpmsam thanks for the details, I realized my comment could be misleading, I meant to say that without the fix the minter should still panic on an EIP155 signed tx submission, if EIP155 is not configured in the genesis block at all. with the fix that definitely should not happen any longer. sorry about the confusion.

thanks for identifying issue #672, I agree that should be a good enhancement