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

fix: txpool race condition #12322

Closed
wants to merge 2 commits into from
Closed

fix: txpool race condition #12322

wants to merge 2 commits into from

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Nov 5, 2024

Closes #12287
Closes #12286

Changes in this PR:

  • TransactionValidator now accepts at: B256 argument to functions performing validation which should be used as a block at which transaction should be validated. We are passing latest block seen by the pool to it to ensure that on_chain_nonce/on_chain_balance values are correct with the current view of the pool
  • transaction_import_lock field is added to pool which is a simple RwLock. It is used to ensure that pool is not processing canonical chain update while importing transactions. This allows us to make sure that transaction validation result is always valid for tx insertion
  • PendingPool::add_transaction is changed to not make any assumptions on order if which transactions are being added. Earlier it would assume that it receives transaction in ascening nonce order. It is done by keeping a mapping from SenderId to -> highest/lowest nonce.

@github-actions github-actions bot added the C-bug An unexpected or incorrect behavior label Nov 5, 2024
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

could we apply the PendingPool::add_transaction fixes separately.

I think the update lock is also a good idea, need to think about the validation at hash a bit more

@@ -327,7 +333,7 @@ where

let account = match self
.client
.latest()
.state_by_block_hash(at)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is now slightly problematic, because by the time this is executed, the hash could have been reorged and could result in an error

@mattsse mattsse closed this Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug An unexpected or incorrect behavior
Projects
None yet
2 participants