Update TouchAddress and IsFiltered to propagate filtering reasons#4557
Update TouchAddress and IsFiltered to propagate filtering reasons#4557MishkaRogachev wants to merge 6 commits intomasterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4557 +/- ##
==========================================
+ Coverage 34.30% 34.54% +0.24%
==========================================
Files 498 498
Lines 59096 59121 +25
==========================================
+ Hits 20270 20424 +154
+ Misses 35238 35077 -161
- Partials 3588 3620 +32 |
MishkaRogachev
force-pushed
the
update-touchaddress-and-hashedaddresscheckerstateisfiltered
branch
from
4081e00 to
d5ebca5
Compare
❌ 7 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
There was a problem hiding this comment.
Pull request overview
This PR updates Nitro’s transaction/address filtering plumbing so that touched addresses carry an explicit filtering reason and filtering queries can return the relevant records, enabling richer downstream filtering reports (NIT-4642; pulls in OffchainLabs/go-ethereum#642).
Changes:
- Update
TouchAddresscall sites to pass aFilteredAddressRecordannotated with a concrete reason (from/to/event rules/retryable fields/contract context). - Update address-filter checker state to aggregate and return filtered address records (not just a boolean).
- Extend unit tests and add a changelog entry documenting the API behavior change.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
execution/gethexec/sequencer.go |
Annotate touched addresses (from/to/event_rule) with filtering reasons; adapt to IsAddressFiltered() returning a tuple. |
execution/gethexec/executionengine.go |
Propagate filtering reasons through delayed sequencing hooks, event filter touches, and retryable-related touches; adapt IsAddressFiltered() usage. |
execution/gethexec/addressfilter/address_checker_test.go |
Update tests to pass filtering reasons and assert returned filtered records. |
execution/gethexec/addressfilter/address_checker.go |
Replace atomic boolean with mutex-gated collection of FilteredAddressRecord results and return them from IsFiltered(). |
changelog/mrogachev-nit-4642.md |
Document the TouchAddress/IsFiltered API change for internal release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
execution/gethexec/sequencer.go
Outdated
| statedb.TouchAddress(addr) | ||
| statedb.TouchAddress(filter.FilteredAddressRecord{ | ||
| Address: addr, | ||
| FilterReason: filter.FilterReason{Reason: filter.ReasonEventRule}, |
There was a problem hiding this comment.
The other fields related to filter.ReasonEventRule will need to be filled here.
| db.TouchAddress(addr) | ||
| db.TouchAddress(filter.FilteredAddressRecord{ | ||
| Address: addr, | ||
| FilterReason: filter.FilterReason{Reason: filter.ReasonEventRule}, |
There was a problem hiding this comment.
The other fields related to filter.ReasonEventRule will need to be filled here.
| case <-s.checker.GetContext().Done(): | ||
| // shutting down, conservatively mark filtered | ||
| s.report(true) | ||
| s.report(record, true) |
There was a problem hiding this comment.
We should not include the record in this case.
We don't want to report to an external system that a transaction was denied because the node was shutting down.
One idea is to report receive a pointer, and here we pass nil.
report will only add to s.filteredAddresses if the pointer is not nil.
| // If the checker is stopped, conservatively mark filtered | ||
| if s.checker.Stopped() { | ||
| s.report(true) | ||
| s.report(record, true) |
There was a problem hiding this comment.
| state3.TouchAddress(filter.FilteredAddressRecord{Address: addrFiltered, FilterReason: filter.FilterReason{Reason: filter.ReasonTo}}) | ||
| filtered3, records3 := state3.IsFiltered() | ||
| assert.True(t, filtered3, "expected transaction with mixed addresses to be filtered") | ||
| require.Len(t, records3, 1) |
There was a problem hiding this comment.
Lets create a test case that returns more than 1 records
There was a problem hiding this comment.
added in tx 4
…-and-hashedaddresscheckerstateisfiltered
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if addressFiltered, _ := statedb.IsAddressFiltered(); statedb.IsTxFiltered() || addressFiltered { | ||
| return state.ErrArbTxFilter |
There was a problem hiding this comment.
IsAddressFiltered() is evaluated unconditionally in the if initializer, which removes the previous short-circuit behavior where IsTxFiltered() could skip the (potentially blocking) address-filter wait. Consider checking IsTxFiltered() first, then only calling IsAddressFiltered() if needed to avoid unnecessary waiting/latency when the tx is already filtered.
There was a problem hiding this comment.
pre-existing behaviour
| if addressFiltered, _ := statedb.IsAddressFiltered(); statedb.IsTxFiltered() || addressFiltered { | ||
| return state.ErrArbTxFilter |
There was a problem hiding this comment.
Same as above: IsAddressFiltered() now runs even when IsTxFiltered() is true due to the initializer form, which can add avoidable blocking/latency. Consider splitting the condition so address filtering is only awaited if tx filtering didn’t already reject.
There was a problem hiding this comment.
pre-existing behaviour
| // Tx 6: queue overflow should not panic and must be conservative | ||
| overflowChecker := NewHashedAddressChecker( | ||
| store, | ||
| /* workerCount */ 1, | ||
| /* queueSize */ 0, | ||
| ) | ||
| overflowChecker.Start(context.Background()) | ||
|
|
||
| // Tx 5: synchronous call | ||
| // Tx 6: synchronous call | ||
| overflowState := mustState(t, overflowChecker.NewTxState()) |
There was a problem hiding this comment.
The comment says “queue overflow”, but queueSize is set to 0, which makes the channel unbuffered/synchronous rather than testing overflow of a bounded queue. Consider updating the comment (and/or adjusting the test to actually exercise the full-queue behavior if that’s what you want to validate).
There was a problem hiding this comment.
fixed comment
Fixes NIT-4642
Pulls in OffchainLabs/go-ethereum#642