core: fix queued transaction eviction #21300
Conversation
rjl493456442
force-pushed
the
txpool-fix-queued-evictions
branch
2 times, most recently
from
5c0946a to
534ba56
Compare
|
@rjl493456442 I'd be curious to understand the progression here. This is mostly identical to the initial solution I included, however, this only updates the beat when the first transaction is queued (on promotion as well, which was in the initial PR). So if an account has no promotions for the lifetime, all new queued transactions will be ejected based on when the first queue item was included. Is this the intended behavior? If we have no recent promotions, this is essentially what will happen now:
This does fix the initial issue, but doesn't seem to be as consistent in its behavior. The initial solution which this PR follows is in this commit: 6f8a7f6 |
|
Gary and I discussed it a bit, before this PR was made. So the original PR pivoted on this point
And then per-tx timestamps were added. And suddenly there was both per-tx timestamp and per-account timestamp, which seemed kind of odd. And then Gary removed the per-account timestamp, to have only the per-tx ones. However, the effect of that was that it added a lot of processing, since the tx handling has to flatten the lists and iterate all txs in the loop, instead of just checking one timestamp. So we agreed that maybe it would be better to focus on the original implementation by Peter, which used only one timestamp per account. And yeah, we seem to have circled back to your original PR :) (more or less -- they're not identical, but they seem to be very similar). |
Makes sense - hopefully, I was helpful here. I'll likely put up some other PRs/bugfixes in the near term (looking at the backlog). |
|
@villanuevawill Hi, I would like to apologize first for the first suggestions I made. Recording the timestamp for each transaction is not a good idea. After reworking based on your code, I think I understand the original mechanism better so I choose to come back to the initial version(+ some fixes).
We really appreciate your contribution, thanks for digging out the issue and open the initial fix PR! |
rjl493456442
force-pushed
the
txpool-fix-queued-evictions
branch
from
2fca74d to
f8aaa0e
Compare
core/tx_pool.go
Outdated
| pool.removeTx(tx.Hash(), true) | ||
| } | ||
| queuedEvictionMeter.Mark(int64(size)) |
There was a problem hiding this comment.
Nit, please inline int64(len(list)). Seems weird to pre-calculate this field and store it. I was expecting something else to be done with it later.
core/tx_pool.go
Outdated
| @@ -618,6 +622,9 @@ func (pool *TxPool) add(tx *types.Transaction, local bool) (replaced bool, err e | |||
| pool.journalTx(from, tx) | |||
| pool.queueTxEvent(tx) | |||
| log.Trace("Pooled new executable transaction", "hash", hash, "from", from, "to", tx.To()) | |||
|
|
|||
| // Successfully promotion, bump the heartbeat | |||
There was a problem hiding this comment.
"Successfully" -> "Successful"
core/tx_pool.go
Outdated
| pool.pendingNonces.set(addr, tx.Nonce()+1) | ||
|
|
||
| // Successfully promotion, bump the heartbeat |
There was a problem hiding this comment.
"Successfully" -> "Successful"
Solves issue#20582. Non-executable transactions should not be evicted on each tick if there are no promote transactions or if a pending/reset empties the pending list. Tests and logging expanded to handle these cases in the future. core/tx_pool: use a ts for each tx in the queue, but only update the heartbeat on promotion or pending replaced queuedTs proper naming
core: address comment
karalabe
force-pushed
the
txpool-fix-queued-evictions
branch
from
8f31499 to
5413df1
Compare
This PR is the successor of #21119 by dropping the
beatsfield in txpool.@holiman @villanuevawill Please take a look if the PR is good to you guys.
If we want to merge this PR, please don't squash into a single commit, would be nice to keep the commit from vill