Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: fix race conditions in txpool #23474

Merged
merged 6 commits into from Aug 31, 2021

Conversation

MariusVanDerWijden
Copy link
Member

This PR fixes several race conditions in the txpool:

  • A race between txList.SetBasefee(), txlist.Removed() and txlist.Reheap() on who is allowed to reheap the list
  • A race between several methods in txList on updating the txlist.stales counter
  • A race during testing between the txpool.loop() and reseting the txpool's blockchain between tests
  • A race during testing when updating txpool.chain.gaslimit from within a test

core/tx_list.go Show resolved Hide resolved
core/tx_list.go Outdated
}

// reheap forcibly rebuilds the heap based on the current remote transaction set.
// Expects the reheap mutex to be held
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the mu is only there to protect the reheap operation, perhaps rename it to reheapMu and do the lock/unlock inside of reheap instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed and moved

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, now you can revert another change too: remove reheap again and have only Reheap.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -123,6 +124,8 @@ func setupTxPoolWithConfig(config *params.ChainConfig) (*TxPool, *ecdsa.PrivateK
key, _ := crypto.GenerateKey()
pool := NewTxPool(testTxPoolConfig, config, blockchain)

// wait for the pool to initialize
<-pool.initDoneCh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change. What's the point of waiting the loop function to start up here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise the resetState function race with the loop function

@holiman holiman merged commit 067084f into ethereum:master Aug 31, 2021
@holiman holiman added this to the 1.10.9 milestone Aug 31, 2021
sidhujag pushed a commit to sidhujag/go-ethereum that referenced this pull request Sep 1, 2021
* core: fix race conditions in txpool

* core: fixed races in the txpool

* core: rebased on master

* core: move reheap mutex

* core: renamed mutex

* core: revert Reheap changes
@MariusVanDerWijden MariusVanDerWijden deleted the txpool-race branch November 30, 2021 15:26
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
* core: fix race conditions in txpool

* core: fixed races in the txpool

* core: rebased on master

* core: move reheap mutex

* core: renamed mutex

* core: revert Reheap changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants