Test concurrent feecurrency tx handling in tx pool #2333
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When handling fee currency transactions.
|
Coverage from tests in coverage: 55.1% of statements across all listed packagescoverage: 68.4% of statements in consensus/istanbul coverage: 63.3% of statements in consensus/istanbul/announce coverage: 57.0% of statements in consensus/istanbul/backend coverage: 0.0% of statements in consensus/istanbul/backend/backendtest coverage: 24.3% of statements in consensus/istanbul/backend/internal/replica coverage: 66.4% of statements in consensus/istanbul/core coverage: 50.0% of statements in consensus/istanbul/db coverage: 0.0% of statements in consensus/istanbul/proxy coverage: 64.2% of statements in consensus/istanbul/uptime coverage: 52.4% of statements in consensus/istanbul/validator coverage: 79.2% of statements in consensus/istanbul/validator/random |
|
gastonponti
approved these changes
Nov 1, 2024
There was a problem hiding this comment.
LGTM
The only doubt I have, is that you are sending the txs from the same N accounts, and I thought that every txpool had a maximum amount of txs per account in a pending state, so I would have guess that it was going to fail due to that limitation, but it seems that works, so maybe was configured to not have a cap before?
|
@gastonponti Good point, that is strange there is a limit (see below), and the default which is used in the test is 64, so I'd really expect this test to at least be flaky if not fail all the time, but even running with a block time of 20 seconds, which means all txs will have been sent and in the tx pool before a block is constructed (300 txs per account) it still passes! So I guess there is some understanding we are missing, anyway I've updated the tx sending to try and not overload the account queue. celo-blockchain/core/tx_pool.go Line 193 in ff274ca |
Increased time between transactions sending so that accounts don't send more than 64 txs within the block period.
|
@piersy Do we still want to merge this? |
piersy
deleted the
piersy/test-concurrent-feecurrency-tx-handling-in-tx-pool
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a test to verify that the tx pool is executing concurrent fee currency checks safely.
I created this to see if I could reproduce #2318 but I was unable to reproduce that issue. It did however uncover a different race condition, which was that
core.currentwas being set in one goroutine and accessed from others without synchronisation. This was easy to fix by just locking around the point where it is set.