Replace TransactionFiltererAPI mutex with a channel#4428
Open
MishkaRogachev wants to merge 3 commits intomasterfrom
Open
Replace TransactionFiltererAPI mutex with a channel#4428MishkaRogachev wants to merge 3 commits intomasterfrom
MishkaRogachev wants to merge 3 commits intomasterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4428 +/- ##
==========================================
- Coverage 33.03% 32.32% -0.71%
==========================================
Files 493 493
Lines 58290 58315 +25
==========================================
- Hits 19256 18853 -403
- Misses 35675 36109 +434
+ Partials 3359 3353 -6 |
MishkaRogachev
force-pushed
the
filterer-use-a-channel-instead-of-mutex
branch
from
2c947df to
9869061
Compare
Contributor
❌ 8 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
Tristan-Wilson
requested changes
Feb 27, 2026
Member
Tristan-Wilson
left a comment
Tristan-Wilson
left a comment
There was a problem hiding this comment.
I think the old code with the mutex was clearer and easier to reason about.
cmd/transaction-filterer/api/api.go
Outdated
| t.apiMutex.Lock() | ||
| defer t.apiMutex.Unlock() | ||
| t.arbFilteredTransactionsManager = arbFilteredTransactionsManager | ||
| t.queue <- func() { |
Member
There was a problem hiding this comment.
Should also select for ctx.Done
cmd/transaction-filterer/api/api.go
Outdated
Comment on lines
+59
to
+77
| case t.queue <- func() { | ||
| if t.arbFilteredTransactionsManager == nil { | ||
| reply <- errors.New("sequencer client not set yet") | ||
| return | ||
| } | ||
| txOpts := *t.txOpts | ||
| txOpts.Context = ctx | ||
| log.Info("Received call to filter transaction", "txHashToFilter", txHashToFilter.Hex()) | ||
| tx, err := t.arbFilteredTransactionsManager.AddFilteredTransaction(&txOpts, txHashToFilter) | ||
| if err != nil { | ||
| log.Warn("Failed to filter transaction", "txHashToFilter", txHashToFilter.Hex(), "err", err) | ||
| reply <- err | ||
| return | ||
| } | ||
| log.Info("Submitted filter transaction", "txHashToFilter", txHashToFilter.Hex(), "txHash", tx.Hash().Hex()) | ||
| return tx.Hash(), nil | ||
| txHash = tx.Hash() | ||
| reply <- nil | ||
| }: | ||
| case <-ctx.Done(): |
Member
There was a problem hiding this comment.
I think there is a situation where this is called and the closure is enqueued, and then later its context is canceled, and we return an error to the caller at line 84, but the closure is still enqueued and then will execute later, which will fail immediately inside the contract binding call and probably fail immediately due to the canceled context and log an error. At least it won't hang forever since the reply chan has size 1. I think the decoupling introduced by the channel isn't worth it here.
Contributor
Author
There was a problem hiding this comment.
I made another attempt with the channel, and now it should be a bit clearer
MishkaRogachev
assigned
Tristan-Wilson and
diegoximenes and unassigned
MishkaRogachev and
Tristan-Wilson
diegoximenes
requested changes
Mar 5, 2026
Contributor
diegoximenes
left a comment
diegoximenes
left a comment
There was a problem hiding this comment.
One issue with the mutex approach is that, if the incoming call is aborted, the apiMutex.Lock call will not be canceled.
Current strategy solves that.
However this is not a big issue given the current architecture, e.g., a single sequencer replica calling this API sequentially.
This kind of change can make it easier to, in the future, adding retry logic when calling the sequencer, etc.
But the extra complexities introduced here are not worth it.
You can simplify mutex usage by making TransactionFiltererAPI.Filter not return a transaction hash, we don't use this response today.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fixes NIT-4499
Address #4294 (comment)