txpool: Respect updates to block gas limit

[jyellick]

There is a bug in the txpool which can manifest either as a race condition, or in more novel network configurations. When a transaction is classified as either having NotTooMuchGas or its complement, the classification will never change (unless a change to the sender account triggers reevaluation of that transaction).

If the block gas limit changes upwards, then any transactions previously not marked as NotTooMuchGas will remain ineligible for promotion. Similarly, if the block gas limit changes downwards, then any transactions previously marked as NotTooMuchGas may inappropriately remain as pending.

Although block gas limits may be relatively static in practice, there is additionally a race condition which can cause this bug to manifest. When the tx pool is first constructed, the gas limit is implicitly set to 0. Until the first update arrives with the configured gas limit all transactions received will be categorized as using too much gas and will never recover. This race condition was occurring in some integration tests we run.

This change adds a check in the OnNewBlock path which checks if the gas limit has changed, and if so triggers the re-evaluation of the NotTooMuchGas status for all transactions.

I am not an expert in the locking code for this path, but it looked like a lock was held and so these modifications should be safe. But please ensure that aspect is adequately reviewed.

[AskAlexSharov]

txs are added a bit later - in: OnNewBlock -> addTxsOnNewBlock -> addLocked probably you need to fix there. (and maybe in onSenderStateChange)

[jyellick]

txs are added a bit later - in: OnNewBlock -> addTxsOnNewBlock -> addLocked probably you need to fix there. (and maybe in onSenderStateChange)

I took a look in this path, but I don't understand your comment. Are you suggesting that the fix needs to be additionally there? Or that the fix should be moved to this path? The onSenderStateChange does already correctly classify transactions according to the current gas limit, but this path is not triggered by gas limit changes.

The general problem is that unlike with onSenderStateChange where changes to a particular account trigger re-processing of transactions for that account, changes to the block gas limit potentially impact every transaction in the pool. And, as best as I can tell there's no real indexing done for the NotTooMuchGas state. I suppose it's possible that maybe we could instead iterate over just the pending or queued pools depending on whether the gas limit has increased or decreased?

[jyellick]

Just rebased as this had gone into conflict -- happy to adapt this if there are changes needed, as the bug it resolves is real for us.

[AskAlexSharov]

yes, seems everything make sense.