core: make txpool operate on immutable state #15085

karalabe · 2017-09-04T19:36:33Z

Apparently the transaction pool (and handling in general) had 3 different update pathways:

On chain head events (and most notably reorgs), the pool's internal state was reset to the current chain state (and transactions filtered accordingly).
On transaction removals events (caused by reorgs), the old transactions were rescheduled.
On miner invalidations, transactions we're yet again concurrently dropped from the pool.

Unfortunately, the above is a very very bad idea from multiple perspectives:

During reorgs, the pool absolutely surely goes out of sync with itself, because its tracked nonces are updated based on head events, but its tracked transactions are updated based on removal events. These by their very nature happen concurrently, so any interaction with the transaction pool in between is undefined.
The exact same data race happens within the miner too, which was "hacked" to handle it by detecting these "strange" out-of-sync transactions and explicitly removing them from the transaction pool. However this again was a very bad idea, because now there's a third event racing to update the pool, which plays horribly with the reorg corruption window (and happens in the exact same scenario).

This PR tries to fix all these races via reducing the transaction pool's API surface to one single entry point: chain head events.

When a chain head event is received, we reset the pool's internal nonces to the current state as currently, but afterwards we gather the reorged transactions within the pool itself and apply any diffs before releasing the lock. This ensures that nonces, states and transactions are updates atomically and cannot go out of sync between each other.
A direct extension of the above is that all method calls from the transaction pool that accessed the blockchain/statedb/gaslimit directly is now gone, and instead only the chain head header is used to derive all needed fields (and StateAt(head.Root)). This ensures that even if the blockchain changes concurrently with the transaction pool, no changes leak into the pool without an isolated head event.
Lastly, the functionality to remove transactions from the pool from the outside was altogether removed. The miner was made a bit smarter so it can differentiate between a few transaction processing errors that it can safely ignore because the pool will filter on it own (nonce too low/high). This allows the last update path that poked holes into the transaction pool to be removed.

holiman · 2017-09-05T06:51:13Z

core/tx_pool.go

+ for rem.NumberU64() > add.NumberU64() {
+ discarded = append(discarded, rem.Transactions()...)
+ if rem = pool.chain.GetBlock(rem.ParentHash(), rem.NumberU64()-1); rem == nil {
+ return


This return and the ones below - these signify some kind of error, right, that we've been told about a new chain head which somehow lacks parent? Shouldn't that be reported rather than just silently ignored?

Yes. I'll add a warning to it.

I don't see it anywhere?

Are all these returns error states? It seems like they should at least log at level ERROR, if not abort entirely.

holiman

LGTM

Arachnid · 2017-09-05T10:27:31Z

core/tx_pool.go

+ for rem.NumberU64() > add.NumberU64() {
+ discarded = append(discarded, rem.Transactions()...)
+ if rem = pool.chain.GetBlock(rem.ParentHash(), rem.NumberU64()-1); rem == nil {
+ return


I don't see it anywhere?

Arachnid · 2017-09-05T10:28:33Z

core/tx_pool.go

+ for rem.NumberU64() > add.NumberU64() {
+ discarded = append(discarded, rem.Transactions()...)
+ if rem = pool.chain.GetBlock(rem.ParentHash(), rem.NumberU64()-1); rem == nil {
+ return


Are all these returns error states? It seems like they should at least log at level ERROR, if not abort entirely.

Arachnid · 2017-09-05T10:29:45Z

core/tx_pool.go

+ if list.Len() > 0 && list.txs.Get(nonce) == nil {
+ for _, tx := range list.Cap(0) {
+ hash := tx.Hash()
+ log.Warn("Demoting invalidated transaction", "hash", hash)


I think all 'should never happen' errors should be level ERROR.

Agreed, will fix

Fixed, ptal

karalabe force-pushed the txpool-immutable branch from 568076c to 5b6b535 Compare September 4, 2017 22:53

ethereum deleted a comment from GitCop Sep 4, 2017

karalabe added this to the 1.7.0 milestone Sep 4, 2017

karalabe requested review from fjl and holiman September 4, 2017 23:11

holiman reviewed Sep 5, 2017

View reviewed changes

karalabe force-pushed the txpool-immutable branch from 5b6b535 to f192220 Compare September 5, 2017 07:47

holiman approved these changes Sep 5, 2017

View reviewed changes

Arachnid approved these changes Sep 5, 2017

View reviewed changes

karalabe force-pushed the txpool-immutable branch from f192220 to 40bbde1 Compare September 5, 2017 10:33

core: make txpool operate on immutable state

da7d57e

karalabe force-pushed the txpool-immutable branch from 40bbde1 to da7d57e Compare September 5, 2017 10:34

karalabe merged commit c91f7be into ethereum:master Sep 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: make txpool operate on immutable state #15085

core: make txpool operate on immutable state #15085

karalabe commented Sep 4, 2017 •

edited

Loading

holiman Sep 5, 2017

karalabe Sep 5, 2017

karalabe Sep 5, 2017

Arachnid Sep 5, 2017

Arachnid Sep 5, 2017

holiman left a comment

Arachnid Sep 5, 2017

Arachnid Sep 5, 2017

Arachnid Sep 5, 2017

karalabe Sep 5, 2017

karalabe Sep 5, 2017

core: make txpool operate on immutable state #15085

core: make txpool operate on immutable state #15085

Conversation

karalabe commented Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karalabe commented Sep 4, 2017 •

edited

Loading