Add ErrAddressFiltered error type to store filtered records#4642
Draft
MishkaRogachev wants to merge 11 commits intomasterfrom
Draft
Add ErrAddressFiltered error type to store filtered records#4642MishkaRogachev wants to merge 11 commits intomasterfrom
MishkaRogachev wants to merge 11 commits intomasterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## add-report-filtered-txn-api-2 #4642 +/- ##
=================================================================
+ Coverage 33.90% 35.49% +1.59%
=================================================================
Files 498 501 +3
Lines 59879 60253 +374
=================================================================
+ Hits 20303 21389 +1086
+ Misses 36022 35067 -955
- Partials 3554 3797 +243 |
Contributor
❌ 7 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end reporting of address-filtered transactions from TxPreChecker to an external “filtering-report” JSON-RPC service, along with a system test to validate the reporting behavior (Fixes NIT-4645).
Changes:
- Add a
FilteringReportRPCClienthook toTxPreCheckerand send aFilteredTxReportwhen a transaction is rejected due to address filtering. - Extend
txFiltererto capture and expose filtered address records for inclusion in the report payload. - Add a system test that spins up a local JSON-RPC server to collect and assert reported filtering events.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| system_tests/prechecker_filter_test.go | Adds a JSON-RPC test server and a new system test asserting reports are emitted for filtered txs |
| execution/gethexec/tx_pre_checker.go | Adds filtering-report client wiring and report generation on ErrArbTxFilter |
| execution/gethexec/tx_filterer.go | Captures filtered address records from StateDB.IsAddressFiltered() for reporting |
| execution/gethexec/node.go | Wires an optional FilteringReportRPCClient into the TxPreChecker |
| changelog/mrogachev-nit-4645.md | Documents the new filtered-transaction reporting behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| go httpServer.Serve(listener) //nolint:errcheck | ||
| go func() { | ||
| <-ctx.Done() | ||
| httpServer.Shutdown(context.Background()) //nolint:errcheck |
There was a problem hiding this comment.
httpServer.Shutdown(context.Background()) can block indefinitely if connections don’t close. In tests this can hang the suite; use a context with timeout (e.g., context.WithTimeout) for shutdown.
Suggested change
| httpServer.Shutdown(context.Background()) //nolint:errcheck | |
| shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| httpServer.Shutdown(shutdownCtx) //nolint:errcheck |
Comment on lines
18
to
22
| type txFilterer struct { | ||
| execEngine *ExecutionEngine | ||
| eventFilter *eventfilter.EventFilter | ||
| execEngine *ExecutionEngine | ||
| eventFilter *eventfilter.EventFilter | ||
| filteredRecords []filter.FilteredAddressRecord | ||
| } |
There was a problem hiding this comment.
txFilterer now stores filteredRecords on the struct, but the same txFilterer instance is created once in execution/gethexec/node.go and used by the backend for RPC calls. This introduces a potential data race and cross-request contamination when concurrent calls (e.g., eth_call / estimateGas / precheck) invoke CheckFiltered/FilteredRecords. Prefer returning the filtered records via an error value (e.g., a typed ErrAddressFiltered carrying records) or storing them in request-local state instead of mutating shared struct fields.
Comment on lines
+357
to
+358
| report := addressfilter.FilteredTxReport{ | ||
| ID: uuid.Must(uuid.NewV7()).String(), |
There was a problem hiding this comment.
uuid.Must(uuid.NewV7()) will panic if UUID generation fails (e.g., entropy/rand read failure). Panicking inside TxPreChecker can crash the node; generate the UUID and return/log the error instead of using Must.
Suggested change
| report := addressfilter.FilteredTxReport{ | |
| ID: uuid.Must(uuid.NewV7()).String(), | |
| reportID, err := uuid.NewV7() | |
| if err != nil { | |
| return fmt.Errorf("failed to generate filtered tx report id: %w", err) | |
| } | |
| report := addressfilter.FilteredTxReport{ | |
| ID: reportID.String(), |
Comment on lines
+369
to
+370
| // Non-blocking: ReportFilteredTransactions uses LaunchPromiseThread internally. | ||
| c.filteringReportRPCClient.ReportFilteredTransactions([]addressfilter.FilteredTxReport{report}) |
There was a problem hiding this comment.
ReportFilteredTransactions returns a Promise containing the RPC call error, but the result is currently ignored. This makes report delivery failures silent and also makes the log message above misleading (it only catches build-time errors). Consider awaiting the promise in a background goroutine (similar to executionengine.go’s pattern) and logging any RPC error.
Suggested change
| // Non-blocking: ReportFilteredTransactions uses LaunchPromiseThread internally. | |
| c.filteringReportRPCClient.ReportFilteredTransactions([]addressfilter.FilteredTxReport{report}) | |
| // Non-blocking: await the async RPC result in a background goroutine so | |
| // report delivery failures are not silently ignored. | |
| promise := c.filteringReportRPCClient.ReportFilteredTransactions([]addressfilter.FilteredTxReport{report}) | |
| go func(txHash common.Hash) { | |
| if err := promise.Await(); err != nil { | |
| log.Error("failed to deliver filtered tx report", "txHash", txHash, "err", err) | |
| } | |
| }(tx.Hash()) |
Comment on lines
+335
to
+338
| var filteredAddresses []filter.FilteredAddressRecord | ||
| if tf, ok := c.backend.TxFilter().(*txFilterer); ok { | ||
| filteredAddresses = tf.FilteredRecords() | ||
| } |
There was a problem hiding this comment.
This code path depends on c.backend.TxFilter() being the concrete *txFilterer to retrieve filtered address records. Besides coupling TxPreChecker to a specific backend implementation, this also relies on shared mutable state inside txFilterer (see tx_filterer.go) which can be overwritten by concurrent requests. Prefer extracting filtered records from the filtering error itself (errors.As on a typed error carrying records) or another request-local mechanism.
…dfiltering-report-to-report-that-a
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.
Fixes NIT-4645
Pulls in OffchainLabs/go-ethereum#656