Closes #20582. Non-executable transactions should not be evicted. #21119

villanuevawill · 2020-05-24T21:34:56Z

Closes #20582.

Description

Queued transactions should not be evicted until after evictionInterval has passed. Successful promotion of transactions via promoteExecutables, pending replacement via add(...), or insertion into the queue via enqueueTx should update the last heartbeat. The heartbeat is no longer removed when the pending list is emptied or after a minute has passed in the future queue.

Tests are expanded to ensure queued transactions are not removed before the evictionInterval duration has passed. Additionally, tests are expanded to ensure a reset or emptied pending list does not clear out the heart beat.

Logging is expanded to detect evictions as a method to prevent/monitor issues in the future.

Monitoring Before & After

We see a 5x reduction in queue evictions and the queue is able to reach capacity. Previously, I was seeing 5 tx/s evicted from the queue, that has been reduced to 1 tx/s evicted. Additionally, the queue is able to grow to its limit, from ~500 to 1024 in the case of my config.

Before:

After:

Questions

There could be a case for removing heartbeat updates on pending replaced and in promote executables, however, it seems optimal to keep the queue fresh when new activity is seen and to manage ordering appropriately on truncation.

villanuevawill · 2020-05-27T15:57:06Z

@karalabe issue #20582 was originally assigned to you it looks like, so tagging you on here. I've been working in txpool code due to our work on account abstraction, so wanted to fix issues along the way. lmk if anything you'd like updated here.

holiman · 2020-05-27T17:00:52Z

@karalabe - I'm thinking it may be a good idea to spin this up on one of the bootnodes. Want me to do that?

karalabe · 2020-05-27T17:05:40Z

@holiman Pls do. The PR seems tiny enough, will try to review tomorrow.

holiman · 2020-05-27T17:10:36Z

I'll rebase it first

holiman · 2020-05-27T17:20:34Z

aws-eu-central, last six hours before redeploying with this PR

and the last 15 minutes

holiman · 2020-05-27T19:15:15Z

After switching to this PR, 15 minutes:

Here are a couple of hours, capturing both before and after switch

Zooming in a bit

holiman · 2020-05-27T19:17:31Z

Not sure why this graph looks like this though: (same time-segment as above)

holiman · 2020-06-02T09:44:33Z

This PR lgtm, has been running stable for a while now

holiman

LGTM

rjl493456442 · 2020-06-04T07:14:59Z

the beats map is a little confusing for me. It seems to represent the Last heartbeat from each known account(from the comment) but actually it's not in the current code or PR.

Now we use beats for two things: (1) pick the least active users to evict queue transaction (2) drop old enough queue transactions. For the latter one, I think the best solution may record the timestamp of each transaction(but tbh not sure whether it's worthwhile to add this complexity).

And for the former, it's even worse. We only record the beat when we successfully promote a transaction. But if a user caches lots of queue transaction but never successfully promote any one. Then the queue transactions belong to this user will never be dropped.

holiman · 2020-06-04T09:11:05Z

Another comparison chart, this PR (green) against another bootnode:

villanuevawill · 2020-06-17T20:36:40Z

Now we use beats for two things: (1) pick the least active users to evict queue transaction (2) drop old enough queue transactions. For the latter one, I think the best solution may record the timestamp of each transaction(but tbh not sure whether it's worthwhile to add this complexity).

@rjl493456442 beats are also used when the queue reaches capacity to determine which transactions to throw out by ordering the set - https://github.com/ethereum/go-ethereum/blob/master/core/tx_pool.go#L1341

For the latter one, I think the best solution may record the timestamp of each transaction(but tbh not sure whether it's worthwhile to add this complexity).

Yes, this would be ideal.

We only record the beat when we successfully promote a transaction. But if a user caches lots of queue transaction but never successfully promote any one. Then the queue transactions belong to this user will never be dropped.

You are right, if an account continues to have enqueued transactions yet it never makes it to pending, then these queued transactions will remain while they continue to actively include new queued transactions. However, in the current scenario, these just get dropped on every eviction tick.

A more optimal solution would be to:

Have a timestamp for each separate tx as you mentioned
Update the current heartbeat on each promotion or pending replaced (not on enqueue)
Only evict queue items past their timestamp

If you would like, I'm happy to put up a PR on this optimal solution (I've been in the txpool code for a while at this point and wouldn't mind).

villanuevawill · 2020-06-17T21:06:17Z

For the latter one, I think the best solution may record the timestamp of each transaction(but tbh not sure whether it's worthwhile to add this complexity).

I'll update the PR soon with the optimal solution and an updated explanation - it doesn't appear to add much complexity. Sorry for the late reply on this thread - I missed the notifications.

villanuevawill · 2020-06-23T03:38:58Z

@holiman & @rjl493456442 I've made the updates described above and this is ready for review.

The pool is stable locally with the updates. I've updated to:

Only update the heartbeat on promotion or pending replaced (not on enqueue)
Clear the heartbeat when pending is removed
Add a ts for each transaction hash
Evict a transaction from the queue only if the heartbeat is past the lifetime and the transaction ts is past the lifetime

I've added thorough testing to check the edge cases. Also, I updated validateTxPoolInternals in the tests to verify internals are correct and that the length of ts entries is always equal to the # of transactions in the queue: https://github.com/ethereum/go-ethereum/pull/21119/files#diff-59daeee0a5084a262da2e62280567af0R111 - this adds additional security to the changes and tests this thoroughly.

holiman · 2020-06-23T06:58:35Z

core/tx_pool.go

@@ -363,7 +366,10 @@ func (pool *TxPool) loop() {
 				// Any non-locals old enough should be removed
 				if time.Since(pool.beats[addr]) > pool.config.Lifetime {
 					for _, tx := range pool.queue[addr].Flatten() {
-						pool.removeTx(tx.Hash(), true)
+						if time.Since(pool.queued_ts[tx.Hash()]) > pool.config.Lifetime {


If we get a mismatch between what's in pool.queued_tx and other pools, the pool.queued_ts[tx.Hash()] will return 0, and flush out the tx. I suppose that is indeed the correct way to handle that error.
For testing though, it might be interesting to make it panic, and run it for a few days with a panic in there, just to see that we don't have any inconsistencies.

I just updated this to panic, thanks!

Sorry, I was a bit unclear. We can't merge it to master with a panic in there, it was more of a note-to-self (or whoever puts it on a prod server) to put a panic check there.

I'll drop the last commit and rebase it on master, ok?

villanuevawill · 2020-06-23T16:46:44Z

core/tx_pool.go

@@ -363,7 +367,13 @@ func (pool *TxPool) loop() {
 				// Any non-locals old enough should be removed
 				if time.Since(pool.beats[addr]) > pool.config.Lifetime {
 					for _, tx := range pool.queue[addr].Flatten() {
-						pool.removeTx(tx.Hash(), true)
+						if pool.queued_ts[tx.Hash()].IsZero() {


@holiman here we include the check and panic.

… 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.

…heartbeat on promotion or pending replaced

core/tx_pool.go

villanuevawill · 2020-07-06T18:23:17Z

@holiman updates applied 💯

holiman · 2020-07-06T18:36:32Z

Great! Did you by any chance run it with panics enabled yourself?

villanuevawill · 2020-07-06T19:04:41Z

Great! Did you by any chance run it with panics enabled yourself?

Yes I did for a few days and it didn't panic.

holiman · 2020-07-24T11:45:14Z

Closed in favour of #21300

villanuevawill requested review from holiman, karalabe and rjl493456442 as code owners May 24, 2020 21:34

holiman force-pushed the eth/txpool-fix-queued-evictions branch from 953b9fe to 72ece8e Compare May 27, 2020 17:11

adamschmideg added the status:triage label Jun 2, 2020

holiman approved these changes Jun 2, 2020

View reviewed changes

adamschmideg assigned karalabe Jun 16, 2020

adamschmideg removed the status:triage label Jun 16, 2020

villanuevawill force-pushed the eth/txpool-fix-queued-evictions branch 2 times, most recently from 4174a3a to 5cee963 Compare June 23, 2020 03:32

villanuevawill requested a review from holiman June 23, 2020 03:41

holiman reviewed Jun 23, 2020

View reviewed changes

villanuevawill commented Jun 23, 2020

View reviewed changes

villanuevawill added 2 commits July 6, 2020 15:14

Solves issue#20582. Non-executable transactions should not be evicted…

6f8a7f6

… 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 …

45334d9

…heartbeat on promotion or pending replaced

holiman force-pushed the eth/txpool-fix-queued-evictions branch from 2423562 to 45334d9 Compare July 6, 2020 13:18

holiman reviewed Jul 6, 2020

View reviewed changes

core/tx_pool.go Outdated Show resolved Hide resolved

queuedTs proper naming

9978836

rjl493456442 mentioned this pull request Jul 7, 2020

core: fix queued transaction eviction #21300

Merged

holiman closed this Jul 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #20582. Non-executable transactions should not be evicted. #21119

Closes #20582. Non-executable transactions should not be evicted. #21119

villanuevawill commented May 24, 2020

villanuevawill commented May 27, 2020 •

edited

Loading

holiman commented May 27, 2020

karalabe commented May 27, 2020

holiman commented May 27, 2020

holiman commented May 27, 2020 •

edited

Loading

holiman commented May 27, 2020

holiman commented May 27, 2020

holiman commented Jun 2, 2020

holiman left a comment

rjl493456442 commented Jun 4, 2020 •

edited

Loading

holiman commented Jun 4, 2020

villanuevawill commented Jun 17, 2020 •

edited

Loading

villanuevawill commented Jun 17, 2020 •

edited

Loading

villanuevawill commented Jun 23, 2020 •

edited

Loading

holiman Jun 23, 2020

villanuevawill Jun 23, 2020

holiman Jul 6, 2020

holiman Jul 6, 2020

villanuevawill Jun 23, 2020

villanuevawill commented Jul 6, 2020

holiman commented Jul 6, 2020 via email

villanuevawill commented Jul 6, 2020 •

edited

Loading

holiman commented Jul 24, 2020

Closes #20582. Non-executable transactions should not be evicted. #21119

Closes #20582. Non-executable transactions should not be evicted. #21119

Conversation

villanuevawill commented May 24, 2020

Description

Monitoring Before & After

Questions

villanuevawill commented May 27, 2020 • edited Loading

holiman commented May 27, 2020

karalabe commented May 27, 2020

holiman commented May 27, 2020

holiman commented May 27, 2020 • edited Loading

holiman commented May 27, 2020

holiman commented May 27, 2020

holiman commented Jun 2, 2020

holiman left a comment

Choose a reason for hiding this comment

rjl493456442 commented Jun 4, 2020 • edited Loading

holiman commented Jun 4, 2020

villanuevawill commented Jun 17, 2020 • edited Loading

villanuevawill commented Jun 17, 2020 • edited Loading

villanuevawill commented Jun 23, 2020 • edited Loading

holiman Jun 23, 2020

Choose a reason for hiding this comment

villanuevawill Jun 23, 2020

Choose a reason for hiding this comment

holiman Jul 6, 2020

Choose a reason for hiding this comment

holiman Jul 6, 2020

Choose a reason for hiding this comment

villanuevawill Jun 23, 2020

Choose a reason for hiding this comment

villanuevawill commented Jul 6, 2020

holiman commented Jul 6, 2020 via email

villanuevawill commented Jul 6, 2020 • edited Loading

holiman commented Jul 24, 2020

villanuevawill commented May 27, 2020 •

edited

Loading

holiman commented May 27, 2020 •

edited

Loading

rjl493456442 commented Jun 4, 2020 •

edited

Loading

villanuevawill commented Jun 17, 2020 •

edited

Loading

villanuevawill commented Jun 17, 2020 •

edited

Loading

villanuevawill commented Jun 23, 2020 •

edited

Loading

villanuevawill commented Jul 6, 2020 •

edited

Loading