Skip to content

Add hashed address filter #4235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
MishkaRogachev wants to merge 21 commits into
base: master
Choose a base branch
from
Draft

Add hashed address filter #4235

MishkaRogachev wants to merge 21 commits into from
+1,376 −71

Conversation

@MishkaRogachev
Copy link
Contributor

@MishkaRogachev MishkaRogachev commented Jan 13, 2026 •
edited
Loading

Close NIT-4299
Waits: #4234

Changes:

@codecov
Copy link

codecov bot commented Jan 13, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 85.36585% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.96%. Comparing base (78ac7d5) to head (cf1115d).

Additional details and impacted files 
@@                      Coverage Diff                      @@
##           s3-scensorship-resistant    #4235       +/-   ##
=============================================================
+ Coverage                     32.36%   56.96%   +24.60%     
=============================================================
  Files                           477      476        -1     
  Lines                         56865    56808       -57     
=============================================================
+ Hits                          18402    32363    +13961     
+ Misses                        35228    19597    -15631     
- Partials                       3235     4848     +1613

@MishkaRogachev MishkaRogachev force-pushed the transaction-address-filter-hashed-address-filter branch from ae96608 to 9f01974 Compare January 13, 2026 13:47
@github-actions
Copy link
Contributor

github-actions bot commented Jan 13, 2026 •
edited
Loading

❌ 17 Tests Failed:

Tests completed Failed Passed Skipped
2776 17 2759 0
View the top 3 failed tests by shortest run time 
TestMain
Stack Traces | 0.000s run time 
FAIL	github.com/offchainlabs/nitro/addressfilter [build failed]
FAIL	github.com/offchainlabs/nitro/addressfilter [build failed]

TestMain
Stack Traces | 0.000s run time 
FAIL	github.com/offchainlabs/nitro/system_tests [build failed]
FAIL	github.com/offchainlabs/nitro/system_tests [build failed]

TestMain
Stack Traces | 0.000s run time 
FAIL	github.com/offchainlabs/nitro/addressfilter [build failed]
FAIL	github.com/offchainlabs/nitro/addressfilter [build failed]

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

@MishkaRogachev MishkaRogachev force-pushed the transaction-address-filter-hashed-address-filter branch from 9f01974 to 041fbdf Compare January 14, 2026 14:25
@MishkaRogachev MishkaRogachev changed the base branch from master to s3-scensorship-resistant January 14, 2026 14:26
@MishkaRogachev MishkaRogachev force-pushed the transaction-address-filter-hashed-address-filter branch from 041fbdf to e5eb39c Compare January 15, 2026 13:32
@mahdy-nasr mahdy-nasr force-pushed the s3-scensorship-resistant branch from 635f047 to f0ee6c2 Compare January 16, 2026 16:54
@mahdy-nasr mahdy-nasr force-pushed the s3-scensorship-resistant branch from 6235f36 to 064f48a Compare January 20, 2026 19:43
@MishkaRogachev MishkaRogachev force-pushed the transaction-address-filter-hashed-address-filter branch 2 times, most recently from e0c620d to d996b94 Compare January 21, 2026 15:50
@MishkaRogachev MishkaRogachev marked this pull request as ready for review January 21, 2026 15:51
@MishkaRogachev MishkaRogachev force-pushed the transaction-address-filter-hashed-address-filter branch from d996b94 to 2370090 Compare January 22, 2026 11:26
mahdy-nasr
mahdy-nasr reviewed Jan 22, 2026
View reviewed changes
addressfilter/address_checker.go Outdated
Comment on lines 14 to 17
// Default parameters for HashedAddressChecker, used in NewDefaultHashedAddressChecker
const (
restrictedAddrWorkerCount = 4
restrictedAddrQueueSize = 1024
Copy link
Contributor

@mahdy-nasr mahdy-nasr Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think their names could be suffixed/prefixed by "default" which make them clearer.

mahdy-nasr
mahdy-nasr reviewed Jan 22, 2026
View reviewed changes
addressfilter/address_checker.go Outdated
Comment on lines 93 to 94
// queue full: drop check (fail-open, false negative possible)
s.pending.Done()
Copy link
Contributor

@mahdy-nasr mahdy-nasr Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why fail-open here? is there a policy or something allows false negatives here ?

Copy link
Contributor Author

@MishkaRogachev MishkaRogachev Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. This kind of trade off: block the whole execution vs. miss some addresses. After reconsideration, I think missing some addresses is not acceptable here so I've changed it to block instead. I also increased queue size to 8192, assuming address lookups should be fast enough .
@tristan fyi

@MishkaRogachev MishkaRogachev marked this pull request as draft January 22, 2026 16:55
@MishkaRogachev MishkaRogachev marked this pull request as ready for review January 22, 2026 17:56
Tristan-Wilson
Tristan-Wilson requested changes Jan 26, 2026
View reviewed changes
Copy link
Member

@Tristan-Wilson Tristan-Wilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at the StopWaiter utility we're using elsewhere in the code for managing spawning goroutines and graceful shutdown.

@MishkaRogachev
Copy link
Contributor Author

MishkaRogachev commented Jan 26, 2026

@Tristan-Wilson II added StopWaiter, but now I think the checker instance should be moved to FilterService. I will wait until FilterService is merged with the master, and then I will make the final fixes. Also, i removed the previous implementation

@MishkaRogachev MishkaRogachev marked this pull request as draft January 28, 2026 16:08
Base automatically changed from s3-scensorship-resistant to master January 28, 2026 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mahdy-nasr mahdy-nasr mahdy-nasr left review comments

@Tristan-Wilson Tristan-Wilson Tristan-Wilson requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@MishkaRogachev MishkaRogachev

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MishkaRogachev @mahdy-nasr @Tristan-Wilson