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

Test concurrent feecurrency tx handling in tx pool #2333

Merged

Conversation

piersy
Copy link
Contributor

@piersy piersy commented Nov 1, 2024

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.current was 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.

Copy link

github-actions bot commented Nov 1, 2024

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 2dca825

coverage: 55.1% of statements across all listed packages
coverage:  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

Copy link

github-actions bot commented Nov 1, 2024

5889 passed, 1 failed, 45 skipped

Test failures:
  TestPriorityClient: geth

Failed
    les_test.go:121: Initializing geth: [--networkid=42 init ./testdata/clique.json] 
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.242] Maximum peer count                       ETH=175 LES=0 total=175
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.244] Set global gas inflation rate            rate=1.300
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.244] Set global gas cap                       cap=25,000,000
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.244] Allocated cache and file handles         database=/tmp/geth-test1105644710/celo/chaindata cache=16.00MiB handles=16
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.303] Writing custom genesis block 
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.304] Persisted trie from memory database      nodes=4 size=566.00B time="102.441µs" gcnodes=0 gcsize=0.00B gctime=0s livenodes=1 livesize=-82.00B
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.305] Successfully wrote genesis state         database=chaindata                               hash=9195f2..122dcd
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.305] Allocated cache and file handles         database=/tmp/geth-test1105644710/celo/lightchaindata cache=16.00MiB handles=16
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.314] Writing custom genesis block 
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.314] Persisted trie from memory database      nodes=4 size=566.00B time="50.593µs"  gcnodes=0 gcsize=0.00B gctime=0s livenodes=1 livesize=-82.00B
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.315] Successfully wrote genesis state         database=lightchaindata                          hash=9195f2..122dcd
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.315] Allocated cache and file handles         database=/tmp/geth-test1105644710/celo/lightestchaindata cache=16.00MiB handles=16
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.347] Writing custom genesis block 
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.348] Persisted trie from memory database      nodes=4 size=566.00B time="37.433µs"  gcnodes=0 gcsize=0.00B gctime=0s livenodes=1 livesize=-82.00B
test_cmd.go:262: (stderr:31) INFO [11-26|15:31:52.348] Successfully wrote genesis state         database=lightestchaindata                       hash=9195f2..122dcd
les_test.go:130: Importing keys to geth
test_cmd.go:262: (stderr:32) INFO [11-26|15:31:52.393] Maximum peer count                       ETH=175 LES=0 total=175
test_cmd.go:262: (stderr:32) INFO [11-26|15:31:52.394] Set global gas inflation rate            rate=1.300
test_cmd.go:262: (stderr:32) INFO [11-26|15:31:52.394] Set global gas cap                       cap=25,000,000
les_test.go:99: Starting lightserver with rpc: [--networkid=42 --port=0 --ipcpath geth-1.ipc --allow-insecure-unlock --datadir /tmp/geth-test1105644710 --password ./testdata/password.txt --unlock 0x02f0d131f1f97aef08aec6e3291b957d9efe7105 --mine --miner.validator 0x02f0d131f1f97aef08aec6e3291b957d9efe7105 --tx-fee-recipient 0x02f0d131f1f97aef08aec6e3291b957d9efe7105 --light.serve=100 --light.maxpeers=1 --nodiscover --nat=extip:127.0.0.1 --verbosity=4]
test_cmd.go:262: (stderr:33) INFO [11-26|15:31:52.668] Maximum peer count                       ETH=175 LES=1 total=176
test_cmd.go:262: (stderr:33) DEBUG[11-26|15:31:52.669] FS scan times                            list="264.054µs" set="7.371µs" diff="2.921µs"
test_cmd.go:262: (stderr:33) WARN [11-26|15:31:52.669] LES server cannot serve old transaction status and cannot connect below les/4 protocol version if transaction lookup index is limited 
test_cmd.go:262: (stderr:33) DEBUG[11-26|15:31:52.670] Sanitizing Go's GC trigger               percent=100
test_cmd.go:262: (stderr:33) INFO [11-26|15:31:52.670] Set global gas inflation rate            rate=1.300
test_cmd.go:262: (stderr:33) INFO [11-26|15:31:52.670] Set global gas cap                       cap=25,000,000
test_cmd.go:262: (stderr:33) INFO [11-26|15:31:52.670] Allocated trie memory caches             clean=154.00MiB dirty=256.00MiB
test_cmd.go:262: (stderr:33) INFO [11-26|15:31:52.670] Allocated cache and file handles         database=/tmp/geth-test1105644710/celo/chaindata cache=512.00MiB handles=524,288
test_cmd.go:262: (stderr:33) DEBUG[11-26|15:31:54.269] Chain freezer table opened               database=/tmp/geth-test1105644710/celo/chaindata/ancient table=bodies items=0 size=0.00B
test_cmd.go:262: (stderr:33) DEBUG[11-26|15:31:54.492] Chain freezer table opened               database=/tmp/geth-test1105644710/celo/chaindata/ancient table=receipts items=0 size=0.00B
test_cmd.go:262: (stderr:33) DEBUG[11-26|15:31:54.740] Chain freezer table opened               database=/tmp/geth-test1105644710/celo/chaindata/ancient table=diffs    items=0 size=0.00B
test_cmd.go:262: (stderr:33) DEBUG[11-26|15:31:55.120] Chain freezer table opened               database=/tmp/geth-test1105644710/celo/chaindata/ancient table=headers  items=0 size=0.00B
test_cmd.go:262: (stderr:33) DEBUG[11-26|15:31:55.364] Chain freezer table opened               database=/tmp/geth-test1105644710/celo/chaindata/ancient table=hashes   items=0 size=0.00B
test_cmd.go:262: (stderr:33) INFO [11-26|15:31:55.364] Opened ancient database                  database=/tmp/geth-test1105644710/celo/chaindata/ancient readonly=false
test_cmd.go:262: (stderr:33) DEBUG[11-26|15:31:55.386] Current full block not old enough        number=0 hash=9195f2..122dcd delay=90000
test_cmd.go:262: (stderr:33) INFO [11-26|15:31:55.387] Initialised chain configuration          config="{ChainID: 15 Homestead: 0 DAO: <nil> DAOSupport: false EIP150: 0 EIP155: 0 EIP158: 0 Byzantium: 0 Constantinople: 0 Petersburg: 0 Istanbul: <nil> Churrito: <nil>, Donut: <nil>, Espresso: <nil>, Gingerbread: <nil>, Gingerbread P2: <nil>, HForkBlock: <nil>, Engine: istanbul}"
test_cmd.go:262: (stderr:33) DEBUG[11-26|15:31:55.387] Setting up Istanbul consensus engine 
les_test.go:115: lightserver rpc connect to /tmp/geth-test1105644710/geth-1.ipc: dial unix /tmp/geth-test1105644710/geth-1.ipc: connect: no such file or directory</code></pre></td></tr>
This test report was produced by the test-summary action.  Made with ❤️ in Cambridge.

Copy link
Contributor

@gastonponti gastonponti left a comment

Choose a reason for hiding this comment

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

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?

@piersy
Copy link
Contributor Author

piersy commented Nov 1, 2024

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

AccountSlots uint64 // Number of executable transaction slots guaranteed per account

Increased time between transactions sending so that accounts don't send
more than 64 txs within the block period.
@palango
Copy link
Contributor

palango commented Nov 26, 2024

@piersy Do we still want to merge this?

@piersy piersy merged commit 2dca825 into master Nov 26, 2024
27 checks passed
@piersy piersy deleted the piersy/test-concurrent-feecurrency-tx-handling-in-tx-pool branch November 26, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants