-
Notifications
You must be signed in to change notification settings - Fork 117
feat(mempool): Add TxTracker support for handling temporary rejections #646
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
base: main
Are you sure you want to change the base?
Conversation
| for _, txs := range all { | ||
| for _, tx := range txs { | ||
| if err = rlp.Encode(replacement, tx); err != nil { | ||
| replacement.Close() | ||
| return err | ||
| } | ||
| } | ||
| journaled += len(txs) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| for _, txs := range all { | ||
| for _, tx := range txs { | ||
| if err = rlp.Encode(replacement, tx); err != nil { | ||
| replacement.Close() |
Check warning
Code scanning / CodeQL
Writable file handle closed without error handling Warning
call to OpenFile
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 9 hours ago
To correctly handle potential data loss or errors that occur upon closing a writable file handle, we must check and handle the error returned from replacement.Close() in all places it is called. Specifically:
- In the loop on line 158, after a failed
rlp.Encode(replacement, tx), the code callsreplacement.Close()but ignores the error. We should close the file, check the return value, and if an error occurs, wrap it (or ensure it is reported). If bothEncodeandClosefail, we should not lose either error—often wrapping or combining is best. - Similarly, after the write loop on line 164, the call to
replacement.Close()is made, but its error is ignored; we should check this and return the error if it happens.
The cleanest Go idiom is to set up a named return value, and when closing after an error, either wrap or combine the errors (e.g., use fmt.Errorf("...: %w", err)). In the normal path, if replacement.Close() fails, return its error.
No new imports are required, as fmt.Errorf is already available.
All changes occur in mempool/txpool/locals/journal.go, in the rotate function.
-
Copy modified lines R158-R161 -
Copy modified lines R167-R169
| @@ -155,13 +155,18 @@ | ||
| for _, txs := range all { | ||
| for _, tx := range txs { | ||
| if err = rlp.Encode(replacement, tx); err != nil { | ||
| replacement.Close() | ||
| closeErr := replacement.Close() | ||
| if closeErr != nil { | ||
| return fmt.Errorf("failed to encode tx and failed to close file: encode error: %w, close error: %v", err, closeErr) | ||
| } | ||
| return err | ||
| } | ||
| } | ||
| journaled += len(txs) | ||
| } | ||
| replacement.Close() | ||
| if err := replacement.Close(); err != nil { | ||
| return fmt.Errorf("failed to close replacement journal file: %w", err) | ||
| } | ||
|
|
||
| // Replace the live journal with the newly generated one | ||
| if err = os.Rename(journal.path+".new", journal.path); err != nil { |
| } | ||
| journaled += len(txs) | ||
| } | ||
| replacement.Close() |
Check warning
Code scanning / CodeQL
Writable file handle closed without error handling Warning
call to OpenFile
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 9 hours ago
The best way to fix this issue is to explicitly check the error returned by replacement.Close() after all write operations in the journal rotation process. This involves modifying the code region around line 164 to assign the result of Close() to an error variable, and handle it appropriately (return it if it occurs and there’s no prior error). Since this code is currently after a loop of writes, we must ensure that a failure in Close is treated as a failed journal rotation. The fix should only change this portion of code, add no new functionality, retain all code structure, and avoid modifying file-level logic elsewhere.
This does not require new imports, but does require updating error assignment/handling in the rotate method.
-
Copy modified lines R164-R166
| @@ -161,7 +161,9 @@ | ||
| } | ||
| journaled += len(txs) | ||
| } | ||
| replacement.Close() | ||
| if err := replacement.Close(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Replace the live journal with the newly generated one | ||
| if err = os.Rename(journal.path+".new", journal.path); err != nil { |
| for sender, txs := range tracker.byAddr { | ||
| // Wipe the stales | ||
| stales := txs.Forward(tracker.pool.Nonce(sender)) | ||
| for _, tx := range stales { | ||
| delete(tracker.all, tx.Hash()) | ||
| } | ||
| numStales += len(stales) | ||
|
|
||
| // Check the non-stale | ||
| for _, tx := range txs.Flatten() { | ||
| if tracker.pool.Has(tx.Hash()) { | ||
| numOk++ | ||
| continue | ||
| } | ||
| resubmits = append(resubmits, tx) | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| for _, tx := range tracker.all { | ||
| addr, _ := types.Sender(tracker.signer, tx) | ||
| rejournal[addr] = append(rejournal[addr], tx) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| for _, list := range rejournal { | ||
| // cmp(a, b) should return a negative number when a < b, | ||
| slices.SortFunc(list, func(a, b *types.Transaction) int { | ||
| return int(a.Nonce() - b.Nonce()) | ||
| }) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| // layer was also initialized to spawn any goroutines required by the service. | ||
| func (tracker *TxTracker) Start() error { | ||
| tracker.wg.Add(1) | ||
| go tracker.loop() |
Check notice
Code scanning / CodeQL
Spawning a Go routine Note
| defer tracker.journal.close() | ||
| } | ||
| var ( | ||
| lastJournal = time.Now() |
Check warning
Code scanning / CodeQL
Calling the system time Warning
| if checkJournal { | ||
| // Lock to prevent journal.rotate <-> journal.insert (via TrackAll) conflicts | ||
| tracker.mu.Lock() | ||
| lastJournal = time.Now() |
Check warning
Code scanning / CodeQL
Calling the system time Warning
|
Could you add more information in the PR about the different effects of this PR? (i.e. new config that's available, the fact that we save a new file to disk, etc.) |
|
Could we add a system and integration test for this as well? |
|
I think we can also merge the logic in for #494 (comment) to make this fully functional. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #646 +/- ##
==========================================
- Coverage 64.96% 63.30% -1.67%
==========================================
Files 317 320 +3
Lines 21604 22029 +425
==========================================
- Hits 14036 13946 -90
- Misses 6349 6630 +281
- Partials 1219 1453 +234
🚀 New features to boost your workflow:
|
fix(mempool): align temporary rejection check with TxTracker
Description
This PR adds the tracking of priority transactions via
TxTracker, a local queue. This is populated by checking for temporary rejections (such as in the case of nonce gaps or underpriced tx), and adding it to theTxTrackerif that is the case. This is a separate state and worker that runs in a goroutine and periodically (initially after 10s) reevaluates its contents. No longer valid txs are dropped, newly valid txs are queued, and the rest remain in the local queue.If the home directory is configured and not blank, then the contents of the TxTracker are journaled and persisted to disk (by default in
~/.evmd/data/txpool/transactions.rlp). This is picked up between node restarts so that those local txs are not lost.This PR also adds the 4 new fields (
locals,no-locals,journal,rejournal) that is used byTxTrackertotoml.goand server flags underevm.mempool. TheconfigureEVMMempool/GetLegacyPoolConfighelpers have also been updated to make use of the toml and/or cli flag values.Closes: #500
Author Checklist
I have...
mainbranch