TxPreChecker: report filtered txs to filtering-report service#4653
Open
MishkaRogachev wants to merge 6 commits intomasterfrom
Open
TxPreChecker: report filtered txs to filtering-report service#4653MishkaRogachev wants to merge 6 commits intomasterfrom
MishkaRogachev wants to merge 6 commits intomasterfrom
Conversation
MishkaRogachev
changed the base branch from
master
to
add-report-filtered-txn-api
MishkaRogachev
changed the base branch from
add-report-filtered-txn-api
to
master
Contributor
There was a problem hiding this comment.
Pull request overview
Adds reporting of address-filtered transactions from TxPreChecker to the filtering-report service (per NIT-4645), including a system test to validate the end-to-end RPC call.
Changes:
- Extend
TxPreCheckerto emitFilteredTxReportviaFilteringReportRPCClientwhen dry-run address filtering rejects a transaction. - Update tx filtering plumbing to return filtered address records (for inclusion in reports).
- Add a system test with a local JSON-RPC server to capture and assert reported filtered transactions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| system_tests/prechecker_filter_test.go | Adds an end-to-end test server/collector and a new test asserting reports are emitted for filtered txs. |
| execution/gethexec/tx_pre_checker.go | Wires a filtering-report client into TxPreChecker and builds/sends FilteredTxReport on filter rejection. |
| execution/gethexec/tx_filterer.go | Changes CheckFiltered to return filtered address records along with the filtering error. |
| execution/gethexec/node.go | Passes the filtering-report client into the TxPreChecker constructor. |
| changelog/mrogachev-nit-4645.md | Adds a changelog entry describing the new reporting behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+355
to
+360
| report := addressfilter.FilteredTxReport{ | ||
| ID: uuid.Must(uuid.NewV7()).String(), | ||
| TxHash: tx.Hash(), | ||
| TxRLP: txRLP, | ||
| FilteredAddresses: filteredAddresses, | ||
| BlockNumber: header.Number.Uint64(), |
There was a problem hiding this comment.
addressfilter.FilteredTxReport.TxRLP is hexutil.Bytes, but tx.MarshalBinary() returns a []byte. Assigning txRLP directly to TxRLP won't compile; convert it to hexutil.Bytes (or change txRLP variable type) before assigning.
Comment on lines
+355
to
+356
| report := addressfilter.FilteredTxReport{ | ||
| ID: uuid.Must(uuid.NewV7()).String(), |
There was a problem hiding this comment.
uuid.Must(uuid.NewV7()) can panic if UUID generation fails. Since this runs on the tx submission path, a panic would take down the process; handle the error and either skip reporting (log at debug/error) or fall back to a different ID generation strategy.
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
+367
to
+369
| promise := c.filteringReportRPCClient.ReportFilteredTransactions([]addressfilter.FilteredTxReport{report}) | ||
| go func(txHash common.Hash) { | ||
| if _, err := promise.Await(context.Background()); err != nil { |
There was a problem hiding this comment.
The goroutine waiting on promise.Await(context.Background()) has no timeout/cancellation tied to the caller, so if the RPC call stalls for an extended time this can accumulate background goroutines under load. Consider awaiting with a bounded context (e.g., a short timeout) or logging errors via the promise thread without spawning an extra goroutine.
Suggested change
| promise := c.filteringReportRPCClient.ReportFilteredTransactions([]addressfilter.FilteredTxReport{report}) | |
| go func(txHash common.Hash) { | |
| if _, err := promise.Await(context.Background()); err != nil { | |
| const reportAwaitTimeout = 5 * time.Second | |
| promise := c.filteringReportRPCClient.ReportFilteredTransactions([]addressfilter.FilteredTxReport{report}) | |
| go func(txHash common.Hash) { | |
| reportCtx, cancel := context.WithTimeout(context.Background(), reportAwaitTimeout) | |
| defer cancel() | |
| if _, err := promise.Await(reportCtx); err != nil { |
Comment on lines
+95
to
+97
| func (c *TxPreChecker) SetFilteringReportRPCClient(client *FilteringReportRPCClient) { | ||
| c.filteringReportRPCClient = client | ||
| } |
There was a problem hiding this comment.
SetFilteringReportRPCClient mutates TxPreChecker.filteringReportRPCClient without synchronization. Since TxPreChecker methods can be called concurrently, calling this setter after the prechecker is in use would introduce a data race. If runtime mutation is required, guard it (mutex/atomic) or document/enforce that it must only be called during initialization before any tx handling starts.
Contributor
Author
There was a problem hiding this comment.
called just once durning node startup
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4653 +/- ##
===========================================
- Coverage 52.85% 33.67% -19.19%
===========================================
Files 501 501
Lines 60225 60251 +26
===========================================
- Hits 31834 20290 -11544
- Misses 23277 36400 +13123
+ Partials 5114 3561 -1553 |
Contributor
❌ 15 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
MishkaRogachev
requested review from
diegoximenes,
ganeshvanahalli and
mahdy-nasr
diegoximenes
requested changes
Apr 20, 2026
diegoximenes
requested changes
Apr 20, 2026
Contributor
diegoximenes
left a comment
diegoximenes
left a comment
There was a problem hiding this comment.
Let's also add the tests discussed out of GitHub.
| promise := c.filteringReportRPCClient.ReportFilteredTransactions([]addressfilter.FilteredTxReport{report}) | ||
| txHash := tx.Hash() | ||
| c.filteringReportRPCClient.LaunchThread(func(ctx context.Context) { | ||
| awaitCtx, cancel := context.WithTimeout(ctx, filteredTxReportDeliveryTimeout) |
Contributor
There was a problem hiding this comment.
RPC client, inside of the filteringReportRPCClient, already have a timeout procedure that takes into consideration a CLI config, no need for this one.
| Backend: c.backend, | ||
| RunScheduledTxes: retryables.RunScheduledTxes, | ||
| ReportFilteredTx: func(_ context.Context, records []filter.FilteredAddressRecord) { | ||
| if reportErr := c.reportFilteredTx(tx, header, records); reportErr != nil { |
Contributor
There was a problem hiding this comment.
nitpick: c.reportFilteredTx could not return an error, and this log would be handled in c.reportFilteredTx instead.
diegoximenes
requested changes
Apr 22, 2026
Contributor
diegoximenes
left a comment
diegoximenes
left a comment
There was a problem hiding this comment.
I didn't do a full review round this time, but as discussed here and in a meeting, since design review can take a while, let's implement the discussed test scenarios in this PR
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#657
TxPreChecker now reports filtered transactions to the filtering-report service via FilteringReportRPCClient when its address-filter dry-run rejects a tx. Records flow back through gasestimator.Run, avoiding a second statedb query. eth_estimateGas does not pass through TxPreChecker, so it won't trigger reports.