8000 fix(mempool/lanes): Do not reuse iterator when reaping by hvanz · Pull Request #3996 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(mempool/lanes): Do not reuse iterator when reaping #3996

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

Merged

Conversation

hvanz
Copy link
Member
@hvanz hvanz commented Sep 4, 2024

There's a risk of race conditions on the iterator for reaping, as shown in the failed test TestBroadcastTxSync. We assumed that Reap methods are only called by BlockExecutor, but this is not true. In particular, ReapMaxTxs is called by the RPC endpoint UnconfirmedTxs.

This PR creates a new iterator on every Reap call, instead of reusing only one.

Benchmarks with one reusable iterator and with an iterator created on each Reap call (this PR).

BenchmarkReap-8             1272            894477 ns/op          485765 B/op      10001 allocs/op
BenchmarkReap-8             1269            928269 ns/op          486134 B/op      10006 allocs/op

@hvanz hvanz added the mempool label Sep 4, 2024
@hvanz hvanz self-assigned this Sep 4, 2024
@hvanz hvanz requested review from a team as code owners September 4, 2024 06:16
@hvanz hvanz mentioned this pull request Sep 4, 2024
4 tasks
Copy link
Contributor
@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

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

LGTM

@hvanz hvanz merged commit 1bde84a into jasmina/3484-add-lanes-to-mempool Sep 4, 2024
34 checks passed
@hvanz hvanz deleted the hvanz/lanes-dont-reuse-iter branch September 4, 2024 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants
0