feat(txpool): only try demoting txns from accounts that were active #1050
Conversation
omerfirmak
force-pushed
the
omerfirmak/txpool-reorg
branch
from
b57d606
to
b33eceb
Compare
| // Iterate over all accounts and demote any non-executable transactions | ||
| for addr, list := range pool.pending { | ||
| if affectedAccounts != nil && !affectedAccounts[addr] { |
There was a problem hiding this comment.
It is true that only these accounts might have higher nonce or lower balance.
But it is possible that a transaction has sufficient balance before, but it's balance is not sufficient anymore, because of L1 data fee. Would not processing these accounts here cause any problem?
There was a problem hiding this comment.
Good point, I missed that. In that case, those txns will linger in the txpool until worker tries to execute them. At that point, worker will encounter an ErrInsufficientFunds error and remove it from the pool manually.
There was a problem hiding this comment.
but it is fine imo. Since L1DataFee increasing is not in control of any user, this should not be a viable attack vector
There was a problem hiding this comment.
or could we just randomly trigger the fallback demoteUnexecutables()? e.g. 1000 times with one that loops all accounts.
There was a problem hiding this comment.
so the affectedAccounts purpose is that: decreases the demoteUnexecutables traversal interval, is it correct?
if true, maybe truncatePending also need to check it
txpool demotes pending txns only if their nonce is now lower than the nonce in the latest state or the account no longer has enough funds to cover the costs. Unless the account in question was active since the last txpool reorg, there is no way that it's nonce changed or balance decreased.
omerfirmak
force-pushed
the
omerfirmak/txpool-reorg
branch
from
b33eceb
to
3112cc1
Compare
| @@ -892,10 +889,6 @@ func (w *worker) commit() (common.Hash, error) { | |||
| } | |||
| } | |||
|
|
|||
| // A new block event will trigger a reorg in the txpool, pause reorgs to defer this until we fetch txns for next block. | |||
| // We may end up trying to process txns that we already included in the previous block, but they will all fail the nonce check | |||
| w.eth.TxPool().PauseReorgs() | |||
There was a problem hiding this comment.
removing as I don't think we will be needing this hack after this
| // If we're reorging an old state, reinject all dropped transactions | ||
| var reinject types.Transactions | ||
| affectedAccounts := make(map[common.Address]bool) |
There was a problem hiding this comment.
| affectedAccounts := make(map[common.Address]bool) | |
| affectedAccounts := make(map[common.Address]struct{}) |
There was a problem hiding this comment.
could save one byte. and in the meanwhile, we don't care about the case that some element is "false" in this map.
| } | ||
| } | ||
| reinject = types.TxDifference(discarded, included) | ||
| collectAffectedAccounts(discarded) |
There was a problem hiding this comment.
| collectAffectedAccounts(discarded) | |
| collectAffectedAccounts(reinject) |
There was a problem hiding this comment.
set {reinject + included} = set {discarded + included}, while # of reinject <= # of discarded.
| } | ||
| pool.currentState = statedb | ||
| pool.pendingNonces = newTxNoncer(statedb) | ||
| pool.currentMaxGas = newHead.GasLimit | ||
| collectAffectedAccounts(pool.chain.GetBlock(newHead.Hash(), newHead.Number.Uint64()).Transactions()) |
There was a problem hiding this comment.
If calculate the included discard to collectAffectedAccounts, maybe here is duplicated.
| // Iterate over all accounts and demote any non-executable transactions | ||
| for addr, list := range pool.pending { | ||
| if affectedAccounts != nil && !affectedAccounts[addr] { |
There was a problem hiding this comment.
so the affectedAccounts purpose is that: decreases the demoteUnexecutables traversal interval, is it correct?
if true, maybe truncatePending also need to check it
txpool demotes pending txns only if their nonce is now lower than the nonce in the latest state or the account no longer has enough funds to cover the costs. Unless the account in question was active since the last txpool reorg, there is no way that it's nonce changed or balance decreased.