-
Notifications
You must be signed in to change notification settings - Fork 139
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 missing transaction in light mempool bug #1239
Fix missing transaction in light mempool bug #1239
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1239 +/- ##
==========================================
+ Coverage 19.36% 25.13% +5.76%
==========================================
Files 164 20 -144
Lines 10812 1134 -9678
==========================================
- Hits 2094 285 -1809
+ Misses 8718 849 -7869 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@Shourya742 I had a quick look at this solution and I have two major points to discuss:
Have you explored the idea about not removing the old txs, but simply updating the mempool (as it's already done) and adding the txs contained in the last declared jobs (if they are not already there in the mempool after the update)? |
Point 1: We are checking if the Point 2: I didn’t know that—let me check on it. I initially thought keeping the JDS mempool as lean as possible was a requirement. However, it does make sense to let transactions persist in the mempool without a specific logic to remove them once a new DMJ is anticipated. In my testing, while we have logic to remove transactions from the old job, it didn’t result in a significant reduction in the number of transactions deleted from the mempool. But if we’re okay with not keeping the JDS mempool lean, then we can follow the approach you suggested. |
f244cf7
to
5d0462d
Compare
Bencher Report
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I would change the [mempool_update_interval]
parameter to 0.1 secs as we tested together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I just left a minor comment. I would say that we could add an integration test for this scenario in the future. I remember we were tracking these issues to be tested by the new integration tests framework somewhere, but I can't find it anymore (cc @plebhash)
05cb2a8
to
b907e7d
Compare
b907e7d
to
7cc0424
Compare
Closes: #840