Active sequncer periodic report of filter-set-id NIT-4689#4598
Open
mahdy-nasr wants to merge 9 commits intomasterfrom
Open
Active sequncer periodic report of filter-set-id NIT-4689#4598mahdy-nasr wants to merge 9 commits intomasterfrom
mahdy-nasr wants to merge 9 commits intomasterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4598 +/- ##
==========================================
- Coverage 33.82% 33.64% -0.18%
==========================================
Files 499 500 +1
Lines 60170 60262 +92
==========================================
- Hits 20350 20277 -73
- Misses 36272 36432 +160
- Partials 3548 3553 +5 |
Contributor
❌ 10 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
Member
|
Unit tests are failing. Asking copilot, it stated that failures are likely caused by this PR. Critical Issues (2 found)
Important Issues (6 found)
Suggestions (6 found)
Strengths
Recommended Action
|
mahdy-nasr
force-pushed
the
sequncer-periodically-report-set-id-addressfiltering
branch
from
68c5864 to
9a47028
Compare
Contributor
Author
|
@joshuacolvin0 unit test was failing by parent branch. fixed there and rebased here. |
mahdy-nasr
force-pushed
the
add-filter-set-id-hashstore
branch
2 times, most recently
from
f63cf0f to
38f8b72
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Implements NIT-4689 by adding a periodic “current address-filter set ID” report from the active sequencer to an external HTTP endpoint, routed through the transaction-filterer JSON-RPC API. This is supported by propagating a filter-set ID through the address-filter hash list payload and storage.
Changes:
- Added sequencer-side periodic reporting (configurable interval) that calls
transactionfilterer_reportCurrentFilterSetId. - Extended address-filter hash list format + in-memory store to track a filter-set ID and expose it to the sequencer.
- Added transaction-filterer API support to POST the filter-set ID to a configured external endpoint, plus a system test covering the end-to-end flow.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| system_tests/tx_address_filter_test.go | Updates test helpers for new hash store signature; adds an end-to-end periodic reporting system test. |
| system_tests/delayed_message_filter_test.go | Updates transaction-filterer stack creation to new NewStack signature. |
| execution/gethexec/transaction_filterer_client.go | Adds RPC client method to call reportCurrentFilterSetId. |
| execution/gethexec/sequencer.go | Adds config + periodic reporting loop; updates test helper to store filter rules with an ID. |
| execution/gethexec/executionengine.go | Exposes getter for the transaction-filterer RPC client. |
| execution/gethexec/addressfilter/service.go | Exposes current filter-set ID from the address-filter service. |
| execution/gethexec/addressfilter/service_test.go | Updates hash store calls + parsing tests for the new payload shape. |
| execution/gethexec/addressfilter/s3_sync.go | Parses id from the S3 payload and stores it in the hash store. |
| execution/gethexec/addressfilter/hash_store.go | Stores filter-set ID alongside salt/hashes and exposes Id(). |
| execution/gethexec/addressfilter/address_checker_test.go | Updates hash store calls for new signature. |
| cmd/transaction-filterer/main.go | Adds CLI/config for filter-set-reporting-endpoint and wires it into the API stack. |
| cmd/transaction-filterer/api/api.go | Adds ReportCurrentFilterSetId RPC method that POSTs to an external endpoint; extends NewStack to accept endpoint. |
| cmd/transaction-filterer/api/api_test.go | Updates stack creation to new NewStack signature. |
| changelog/mnasr-nit-4689.md | Adds changelog entry for periodic filter-set ID reporting. |
| changelog/mnasr-nit-4688.md | Adds changelog entry for filter-set ID support in address filter reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+97
to
+99
| id, err := uuid.Parse(payload.Id) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid filter set ID UUID: %w", err) |
There was a problem hiding this comment.
parseHashListJSON now requires an id field and fails if it’s missing. Previously the payload only required salt + address_hashes, so upgrading could cause nodes to fail initialization if the S3 object hasn’t been updated yet. Consider making id optional (e.g., default to uuid.Nil or derive a stable ID from digest) so filtering continues to work even when id is absent, and only reporting is skipped.
Suggested change
| id, err := uuid.Parse(payload.Id) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid filter set ID UUID: %w", err) | |
| id := uuid.Nil | |
| if strings.TrimSpace(payload.Id) != "" { | |
| id, err = uuid.Parse(payload.Id) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid filter set ID UUID: %w", err) | |
| } |
Comment on lines
+1746
to
+1751
| if s.execEngine.GetTransactionFiltererRPCClient() != nil { | ||
| s.CallIteratively(func(ctx context.Context) time.Duration { | ||
| reportingInterval := s.config().TransactionFiltering.FilterSetReportingInterval | ||
| if reportingInterval <= 0 { | ||
| return 5 * time.Minute | ||
| } |
There was a problem hiding this comment.
The interval handling here is ambiguous: filter-set-reporting-interval <= 0 silently falls back to 5 minutes, which makes it impossible to disable reporting by setting 0 and also contradicts the configured value. Consider either (a) treating <=0 as “disabled” (return 0 and skip scheduling) or (b) rejecting non-positive values in config validation / flag parsing so behavior matches user intent.
Suggested change
| if s.execEngine.GetTransactionFiltererRPCClient() != nil { | |
| s.CallIteratively(func(ctx context.Context) time.Duration { | |
| reportingInterval := s.config().TransactionFiltering.FilterSetReportingInterval | |
| if reportingInterval <= 0 { | |
| return 5 * time.Minute | |
| } | |
| reportingInterval := s.config().TransactionFiltering.FilterSetReportingInterval | |
| if s.execEngine.GetTransactionFiltererRPCClient() != nil && reportingInterval > 0 { | |
| s.CallIteratively(func(ctx context.Context) time.Duration { |
Comment on lines
+62
to
+67
| // add timeout to context to avoid hanging if the endpoint is unresponsive | ||
| ctx, cancel := context.WithTimeout(ctx, 5*time.Second) | ||
| defer cancel() | ||
| if t.filterSetReportingEndpoint == "" { | ||
| return errors.New("filter set reporting endpoint not configured") | ||
| } |
There was a problem hiding this comment.
Returning an error when filterSetReportingEndpoint is empty will surface as repeated warnings on the sequencer side every reporting interval (since the RPC method exists but can never succeed). If the endpoint is meant to be optional, consider making this a no-op (return nil) when not configured, or otherwise fail fast at startup / config validation so the misconfiguration is caught once rather than producing ongoing noise.
Suggested change
| // add timeout to context to avoid hanging if the endpoint is unresponsive | |
| ctx, cancel := context.WithTimeout(ctx, 5*time.Second) | |
| defer cancel() | |
| if t.filterSetReportingEndpoint == "" { | |
| return errors.New("filter set reporting endpoint not configured") | |
| } | |
| if t.filterSetReportingEndpoint == "" { | |
| return nil | |
| } | |
| // add timeout to context to avoid hanging if the endpoint is unresponsive | |
| ctx, cancel := context.WithTimeout(ctx, 5*time.Second) | |
| defer cancel() |
| @@ -0,0 +1,2 @@ | |||
| ### Added | |||
| - Added periodic reporting to filtered addresses filter set id No newline at end of file | |||
There was a problem hiding this comment.
Changelog entry wording is unclear/grammatically incorrect: “reporting to filtered addresses filter set id”. Consider rephrasing to something like “periodic reporting of the address-filter set ID” (and use “ID” capitalization).
Suggested change
| - Added periodic reporting to filtered addresses filter set id | |
| - Added periodic reporting of the address-filter set ID for filtered addresses |
| @@ -0,0 +1,2 @@ | |||
| ### Added | |||
| - Added support for id-set-filter for address filter reporting No newline at end of file | |||
There was a problem hiding this comment.
Changelog entry uses unclear phrasing (“id-set-filter”) and inconsistent capitalization (“id”). Consider clarifying what was added (e.g., “support for a filter set ID in the address filter payload”) and use “ID” capitalization for readability.
Suggested change
| - Added support for id-set-filter for address filter reporting | |
| - Added support for a filter set ID in the address filter reporting payload |
Contributor
Author
|
Now this API fully moved to new filter-reporting cmd service. |
Contributor
MishkaRogachev
left a comment
MishkaRogachev
left a comment
There was a problem hiding this comment.
Looks solid, just a few questions.
Also, in the addressfilter/service_test.go the invalidHashPayload and wrongLenPayload test cases do not include an id field so they test wrong error path
| s.CallIteratively(func(ctx context.Context) time.Duration { | ||
| reportingInterval := s.filteringConfigFetcher().FilterSetReportingInterval | ||
| if reportingInterval == 0 { | ||
| return reportingInterval |
Contributor
There was a problem hiding this comment.
That will retry immediately, so if it's misconfigured to 0 it will eat the whole core. Probably define a const with ~10ms delay
Contributor
There was a problem hiding this comment.
We can just have a reasonable CLI default, and then just trust user's input
| if reportingInterval == 0 { | ||
| return reportingInterval | ||
| } | ||
| if s.addressFilterService == nil { |
Contributor
There was a problem hiding this comment.
this check could be done just once before looping
diegoximenes
requested changes
Apr 17, 2026
| IPC genericconf.IPCConfig `koanf:"ipc"` | ||
| Auth genericconf.AuthRPCConfig `koanf:"auth"` | ||
|
|
||
| FilterSetReportingEndpoint string `koanf:"filter-set-reporting-endpoint"` |
Contributor
There was a problem hiding this comment.
Let's use the genericconf.HTTPClientConfig strategy that #4620 is using.
genericconf.HTTPClientConfig was introduced by the SQS PR, which has precedence over this one.
So it is OK to use SQS's PR related branch as the base branch of this one.
|
|
||
| type FilteringReportAPI struct{} | ||
| type FilteringReportAPI struct { | ||
| filterSetReportingEndpoint string |
Contributor
There was a problem hiding this comment.
Related to https://github.com/OffchainLabs/nitro/pull/4598/changes#r3100471692
Let's hold an http client here instead
| if a.filterSetReportingEndpoint == "" { | ||
| return errors.New("filter set reporting endpoint not configured") | ||
| } | ||
| payload, err := json.Marshal(map[string]string{"filterSetId": filterSetId}) |
Contributor
There was a problem hiding this comment.
Let's also add the chain id to the payload
| AddressFilter: addressfilter.DefaultConfig, | ||
| TransactionFiltererRPCClient: DefaultTransactionFiltererRPCClientConfig, | ||
| FilteringReportRPCClient: DefaultFilteringReportRPCClientConfig, | ||
| FilterSetReportingInterval: 5 * time.Minute, |
Contributor
There was a problem hiding this comment.
Let's reduce this to 1 minute
| s.CallIteratively(func(ctx context.Context) time.Duration { | ||
| reportingInterval := s.filteringConfigFetcher().FilterSetReportingInterval | ||
| if reportingInterval == 0 { | ||
| return reportingInterval |
Contributor
There was a problem hiding this comment.
We can just have a reasonable CLI default, and then just trust user's input
| w.WriteHeader(http.StatusInternalServerError) | ||
| return | ||
| } | ||
| var payload map[string]string |
Contributor
There was a problem hiding this comment.
Let's use the struct related to https://github.com/OffchainLabs/nitro/pull/4598/changes#r3100627930 here
| builder.execConfig.TransactionFiltering.FilteringReportRPCClient.URL = filteringReportStack.HTTPEndpoint() | ||
| builder.execConfig.TransactionFiltering.FilterSetReportingInterval = 500 * time.Millisecond | ||
|
|
||
| // Use large MsgLag and SyncInterval to prevent ConsensusExecutionSyncer from overwriting it. |
| execNode.Sequencer.SetAddressFilterServiceForTest(t, filterService) | ||
|
|
||
| // Store filter rules with a known ID so periodic reporting has something to report | ||
| execNode.Sequencer.StoreFilterRulesForTest(t, expectedId, uuid.New(), nil, "test-digest") |
Contributor
There was a problem hiding this comment.
Why not just calling GetHashStore directly from the filterService just created above?
| case <-deadline: | ||
| t.Fatalf("timed out waiting for 2 periodic reports, got %d (total count: %d)", len(reports), reportCount.Load()) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Also add a scenario in which the filterSetId is updated
| if s.addressFilterService == nil { | ||
| return reportingInterval | ||
| } | ||
| // Only report if active sequencer |
Contributor
There was a problem hiding this comment.
Create a func (s *Sequencer) isActiveSequencer to improve code readability.
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
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.
Implements NIT-4689. The active sequencer periodically POSTs the current address filter set ID to an external endpoint via the transaction-filterer API.