Sequencer calls cmd/transaction-filterer by diegoximenes · Pull Request #4294 · OffchainLabs/nitro

diegoximenes · 2026-01-29T17:55:49Z

Sequencer calls cmd/transaction-filterer to filter delayed transaction.

Attention to the base branch.

codecov · 2026-01-29T18:29:21Z

Codecov Report

❌ Patch coverage is 17.27273% with 91 lines in your changes missing coverage. Please review.
✅ Project coverage is 33.05%. Comparing base (c885660) to head (55fbbef).
⚠️ Report is 104 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4294      +/-   ##
==========================================
- Coverage   34.84%   33.05%   -1.79%     
==========================================
  Files         488      489       +1     
  Lines       57870    57944      +74     
==========================================
- Hits        20163    19156    -1007     
- Misses      34134    35423    +1289     
+ Partials     3573     3365     -208

github-actions · 2026-01-29T18:31:51Z

❌ 6 Tests Failed:

Tests completed	Failed	Passed	Skipped
4184	6	4178	0

View the top 3 failed tests by shortest run time

TestDataStreaming_PositiveScenario/Many_senders,_long_messages

Stack Traces | 0.190s run time

... [CONTENT TRUNCATED: Keeping last 20 lines]
        github.com/offchainlabs/nitro/daprovider/data_streaming.testBasic.func1()
        	/home/runner/work/nitro/nitro/daprovider/data_streaming/protocol_test.go:230 +0x19b
        created by github.com/offchainlabs/nitro/daprovider/data_streaming.testBasic in goroutine 126
        	/home/runner/work/nitro/nitro/daprovider/data_streaming/protocol_test.go:223 +0x85
        
    protocol_test.go:230: �[31;1m [] too much time has elapsed since request was signed �[0;0m
WARN [02-10|19:27:45.031] Served datastreaming_start               conn=127.0.0.1:54270 reqid=109 duration="118.322µs" err="too much time has elapsed since request was signed"
INFO [02-10|19:27:45.031] rpc response                             method=datastreaming_start logId=110 err="too much time has elapsed since request was signed" result={} attempt=0 args="[\"0x698b86b0\", \"0x37\", \"0xd9\", \"0x2e04\", \"0xa\", \"0xd94703b5bf808669f6efa582fd60718ad281c0e756277468595e50fd33caa3065acd4278f59cad3b87f60b7ccbd0adc1712fd8962be41bdca034cbdc1ad79d4400\"]" errorData=null
    protocol_test.go:230: goroutine 264 [running]:
        runtime/debug.Stack()
        	/opt/hostedtoolcache/go/1.25.6/x64/src/runtime/debug/stack.go:26 +0x5e
        github.com/offchainlabs/nitro/util/testhelpers.RequireImpl({0x161f990, 0xc0004ac380}, {0x16061e0, 0xc000bfcd80}, {0x0, 0x0, 0x0})
        	/home/runner/work/nitro/nitro/util/testhelpers/testhelpers.go:29 +0x9f
        github.com/offchainlabs/nitro/daprovider/data_streaming.testBasic.func1()
        	/home/runner/work/nitro/nitro/daprovider/data_streaming/protocol_test.go:230 +0x19b
        created by github.com/offchainlabs/nitro/daprovider/data_streaming.testBasic in goroutine 126
        	/home/runner/work/nitro/nitro/daprovider/data_streaming/protocol_test.go:223 +0x85
        
    protocol_test.go:230: �[31;1m [] too much time has elapsed since request was signed �[0;0m
--- FAIL: TestDataStreaming_PositiveScenario/Many_senders,_long_messages (0.19s)

TestDataStreaming_PositiveScenario

Stack Traces | 0.220s run time

=== RUN   TestDataStreaming_PositiveScenario
--- FAIL: TestDataStreaming_PositiveScenario (0.22s)

TestVersion40

Stack Traces | 5.310s run time

... [CONTENT TRUNCATED: Keeping last 20 lines]
        	/home/runner/work/nitro/nitro/system_tests/common_test.go:2065 +0x5d
        github.com/offchainlabs/nitro/system_tests.testPrecompiles(0xc00f4f3180, 0x28, {0xc234477df8, 0x5, 0x39?})
        	/home/runner/work/nitro/nitro/system_tests/precompile_inclusion_test.go:94 +0x371
        github.com/offchainlabs/nitro/system_tests.TestVersion40(0xc00f4f3180?)
        	/home/runner/work/nitro/nitro/system_tests/precompile_inclusion_test.go:71 +0x64b
        testing.tRunner(0xc00f4f3180, 0x3d1eb80)
        	/opt/hostedtoolcache/go/1.25.6/x64/src/testing/testing.go:1934 +0xea
        created by testing.(*T).Run in goroutine 1
        	/opt/hostedtoolcache/go/1.25.6/x64/src/testing/testing.go:1997 +0x465
        
    precompile_inclusion_test.go:94: �[31;1m [] execution aborted (timeout = 5s) �[0;0m
INFO [02-10|16:28:51.321] Stopping work on payload                 id=0x03c1eef7d35dee45 reason=delivery
INFO [02-10|16:28:51.321] Writing cached state to disk             block=1   hash=edf159..2a6be3 root=9d2e2f..b7db94
INFO [02-10|16:28:51.321] Persisted trie from memory database      nodes=23   flushnodes=0 size=3.61KiB   flushsize=0.00B time="121.968µs" flushtime=0s gcnodes=0 gcsize=0.00B gctime="1.472µs"  livenodes=0    livesize=0.00B
INFO [02-10|16:28:51.321] Writing cached state to disk             block=1   hash=edf159..2a6be3 root=9d2e2f..b7db94
INFO [02-10|16:28:51.321] Persisted trie from memory database      nodes=0    flushnodes=0 size=0.00B     flushsize=0.00B time="2.455µs"   flushtime=0s gcnodes=0 gcsize=0.00B gctime=0s         livenodes=0    livesize=0.00B
INFO [02-10|16:28:51.321] Writing snapshot state to disk           root=930b37..bec76f
INFO [02-10|16:28:51.321] Persisted trie from memory database      nodes=0    flushnodes=0 size=0.00B     flushsize=0.00B time="1.232µs"   flushtime=0s gcnodes=0 gcsize=0.00B gctime=0s         livenodes=0    livesize=0.00B
INFO [02-10|16:28:51.322] Imported new potential chain segment     number=6   hash=36a2b5..a69f42 blocks=1  txs=1   mgas=0.569 elapsed="943.876µs"  mgasps=602.904  triediffs=17.81KiB   triedirty=0.00B
--- FAIL: TestVersion40 (5.31s)

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

cmd/transaction-filterer/api/api.go

…Service

MishkaRogachev

A few minor questions

execution/gethexec/transaction_filterer_client.go

execution/gethexec/executionengine.go

execution/gethexec/node.go

tsahee

small comments in the meantime. I think this PR will need to change after some things currently in the merge queue.
Generally good

tsahee · 2026-02-07T01:29:01Z

execution/gethexec/executionengine.go

+func (s *ExecutionEngine) StopAndWait() {
+	s.StopWaiter.StopAndWait()
+
+	if s.transactionFiltererRPCClient != nil {


Do this before the parent stopandwait.
Parent stopAndWait will kill parent context, and you'll end up with a non-ordered shutdown.

Fixed ExecutionEngine StopWaiter start/stop

@tsahee I am noop here regarding this pattern, but have question, why the order should be stopChild then stopParent?
I think parent might be in mid of thread sending request thru child but child already stoped, this will cause error during shutdown?

StopAndWait cancels the context, then waits for all threads to be done.
Child uses a context that's a child of parent context so once you call parent stopandwait, both parent and child context will be closed and all threads will end without particular order.
If parent creates threads that should be closed while child is still active - these threads should be done by a different child.

execution/gethexec/sequencer.go

cmd/transaction-filterer/api/api.go

tsahee

approving with comments, will open separate issues for those

tsahee · 2026-02-10T01:32:29Z

execution/gethexec/node.go

 	if n.ExecEngine.Started() {
 		n.ExecEngine.StopAndWait()
 	}
+	if n.consensusRPCClient != nil {


if there's a reason this moved - document it in a comment because we won't remember that.

This was not moved, it was created in this PR.
consensusRPCClient didn't have a StopAndWait func before this PR.
Given that, adding a code comment for that is still useful?

ah, was confused with the addressfilterservice

tsahee · 2026-02-10T01:55:20Z

cmd/transaction-filterer/api/api.go

-const namespace = "transactionfilterer"
-
 type TransactionFiltererAPI struct {
+	apiMutex sync.Mutex // avoids concurrent transactions with the same nonce


use a channel with a single sink to ensure transactions are treated one by one. We should eventually have a service with some more internal state that could e.g. make sure it's filter requests pass through, retries if needed etc.. no need to catch a lock while handling a request.

If following a strategy like this, then adding a message queue layer between the sequencer and cmd/transaction-filterer is a way to move forward.
But it seems over engineering right now TBH.

agree it can be done later

execution/gethexec/node.go

mahdy-nasr · 2026-02-10T18:16:43Z

cmd/transaction-filterer/api/api.go

+// Only used for testing.
+// Sequencer and TransactionFiltererAPI depend on each other, as a workaround for the egg/chicken problem,
+// we set the sequencer client after both are created.
+func (t *TransactionFiltererAPI) SetSequencerClient(_ *testing.T, sequencerClient *ethclient.Client) error {


Using go testing package in production code is a bit anti-pattern I think. I can see the need for it here from the comment. But the impact of this is that go now will compile testing related package/runtime into production code. increase the binary size. and might cause some issues or affect the speed (I think)

using testing.T input in code that's only used for testing helps make sure it's not used anywhere outside of tests by mistakes. Binary size is irrelevant, having a function that can be called from tests and only from tests is useful.

also, since no member function of testing.T is called I don't believe even binary size will increase

My worries is not only the binary size, but the test-related runtime or execution.

tsahee · 2026-02-10T20:16:18Z

execution/gethexec/node.go

 	if n.ExecEngine.Started() {
 		n.ExecEngine.StopAndWait()
 	}
+	if n.consensusRPCClient != nil {


ah, was confused with the addressfilterservice

tsahee · 2026-02-10T20:19:31Z

cmd/transaction-filterer/api/api.go

+// Only used for testing.
+// Sequencer and TransactionFiltererAPI depend on each other, as a workaround for the egg/chicken problem,
+// we set the sequencer client after both are created.
+func (t *TransactionFiltererAPI) SetSequencerClient(_ *testing.T, sequencerClient *ethclient.Client) error {


using testing.T input in code that's only used for testing helps make sure it's not used anywhere outside of tests by mistakes. Binary size is irrelevant, having a function that can be called from tests and only from tests is useful.

tsahee · 2026-02-10T20:21:19Z

cmd/transaction-filterer/api/api.go

-const namespace = "transactionfilterer"
-
 type TransactionFiltererAPI struct {
+	apiMutex sync.Mutex // avoids concurrent transactions with the same nonce


agree it can be done later

tsahee · 2026-02-10T20:22:30Z

cmd/transaction-filterer/api/api.go

+// Only used for testing.
+// Sequencer and TransactionFiltererAPI depend on each other, as a workaround for the egg/chicken problem,
+// we set the sequencer client after both are created.
+func (t *TransactionFiltererAPI) SetSequencerClient(_ *testing.T, sequencerClient *ethclient.Client) error {


also, since no member function of testing.T is called I don't believe even binary size will increase

tsahee · 2026-02-10T20:25:45Z

execution/gethexec/executionengine.go

+func (s *ExecutionEngine) StopAndWait() {
+	s.StopWaiter.StopAndWait()
+
+	if s.transactionFiltererRPCClient != nil {


StopAndWait cancels the context, then waits for all threads to be done.
Child uses a context that's a child of parent context so once you call parent stopandwait, both parent and child context will be closed and all threads will end without particular order.
If parent creates threads that should be closed while child is still active - these threads should be done by a different child.

diegoximenes force-pushed the sequencer_calls_transaction_filterer branch 2 times, most recently from 0eb6f47 to 04ecb8a Compare January 30, 2026 17:12

diegoximenes commented Jan 30, 2026

View reviewed changes

cmd/transaction-filterer/api/api.go Outdated Show resolved Hide resolved

diegoximenes marked this pull request as ready for review January 30, 2026 18:50

diegoximenes requested review from MishkaRogachev, Tristan-Wilson, mahdy-nasr and tsahee January 30, 2026 18:50

diegoximenes assigned Tristan-Wilson Jan 30, 2026

Base automatically changed from delayed-tx-filtering-2 to master February 2, 2026 13:50

diegoximenes added 16 commits February 2, 2026 11:32

Adds TransactionFiltererRPCClient to SequencerConfig

b93225e

Moves TransactionFiltererRPClient to execution.Config

235cfee

TransactionFiltererRPCClient

e250a63

ExecutionEngine with TransactionFiltererRPCClient

6d37e9b

Guard agains nil transactionFiltererRPCClient

638b7ee

System tests use cmd/transaction-filterer

e95384c

Add comments

c42252f

Rename test setTransactionFiltererRPCClient to setTransactionFilterer…

c9645de

…Service

Simplifies NewTransactionFiltererRPCClient

45d1dd3

changelog

b6eccb4

Fix changelog

801ca8f

Adds missing require.NoError

b244782

Avoids concurrent txs with the same nonce

40ea668

Sequencer calls transaction-filterer sequentially

c5bca58

Adds *testing.T as an argument for SetTransactionFiltererRPCClient

cdd0916

Adds addTxHashToOnChainFilter again

eb88a51

diegoximenes force-pushed the sequencer_calls_transaction_filterer branch from eb20adc to eb88a51 Compare February 2, 2026 15:05

MishkaRogachev reviewed Feb 2, 2026

View reviewed changes

execution/gethexec/transaction_filterer_client.go Outdated Show resolved Hide resolved

execution/gethexec/executionengine.go Outdated Show resolved Hide resolved

execution/gethexec/node.go Outdated Show resolved Hide resolved

execution/gethexec/node.go Outdated Show resolved Hide resolved

MishkaRogachev previously approved these changes Feb 6, 2026

View reviewed changes

tsahee requested changes Feb 7, 2026

View reviewed changes

tsahee assigned diegoximenes and unassigned tsahee Feb 7, 2026

diegoximenes added 2 commits February 9, 2026 12:29

Moves transactionfilterer namespace to gethexec

f00b17c

Fixes ExecutionEngine StopWaiter start/stop

687e1d1

diegoximenes dismissed MishkaRogachev’s stale review via 687e1d1 February 9, 2026 16:01

Merge branch 'master' into sequencer_calls_transaction_filterer

8c207bb

tsahee previously approved these changes Feb 10, 2026

View reviewed changes

tsahee added design-approved and removed design-approved labels Feb 10, 2026

diegoximenes added 2 commits February 10, 2026 10:11

Merge branch 'master' into sequencer_calls_transaction_filterer

d7479ee

Fix import

10e8cf9

diegoximenes dismissed tsahee’s stale review via 10e8cf9 February 10, 2026 13:18

diegoximenes added 5 commits February 10, 2026 10:35

Creates transactionFiltererRPCClient in NewSequencer

a918047

Simplifies NewExecutionEngine

3cffec2

Fixes order of execution and consensus rpc clients StopAndWait calls

7f334f1

Merge branch 'master' into sequencer_calls_transaction_filterer

5221bee

Merge branch 'master' into sequencer_calls_transaction_filterer

55fbbef

diegoximenes assigned tsahee and unassigned diegoximenes Feb 10, 2026

mahdy-nasr reviewed Feb 10, 2026

View reviewed changes

mahdy-nasr assigned diegoximenes Feb 10, 2026

tsahee approved these changes Feb 10, 2026

View reviewed changes

tsahee enabled auto-merge February 10, 2026 20:27

tsahee added the design-approved label Feb 10, 2026

tsahee added this pull request to the merge queue Feb 10, 2026

Merged via the queue into master with commit 1610629 Feb 10, 2026
40 of 41 checks passed

tsahee deleted the sequencer_calls_transaction_filterer branch February 10, 2026 22:11

Conversation

diegoximenes commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 6 Tests Failed:

Uh oh!

Uh oh!

MishkaRogachev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tsahee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tsahee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegoximenes Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

diegoximenes commented Jan 29, 2026 •

edited

Loading

codecov bot commented Jan 29, 2026 •

edited

Loading

github-actions bot commented Jan 29, 2026 •

edited

Loading

diegoximenes Feb 10, 2026 •

edited

Loading