Add set filter ID to the hashData for address filter - NIT-4688

[mahdy-nasr]

Address linear NIT-4688.

Propagate addresses-set-id into our addressfilter project

[codecov]

Codecov Report

❌ Patch coverage is 0% with 103 lines in your changes missing coverage. Please review.
✅ Project coverage is 34.05%. Comparing base (dd416d9) to head (c238364).
⚠️ Report is 24 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4594      +/-   ##
==========================================
- Coverage   34.97%   34.05%   -0.92%     
==========================================
  Files         495      497       +2     
  Lines       59049    59155     +106     
==========================================
- Hits        20651    20148     -503     
- Misses      34782    35443     +661     
+ Partials     3616     3564      -52     

[github-actions]

❌ 10 Tests Failed:

Tests completed	Failed	Passed	Skipped
4812	10	4802	0

View the top 3 failed tests by shortest run time

TestAliasingFlaky

Stack Traces | -0.000s run time

=== RUN   TestAliasingFlaky
=== PAUSE TestAliasingFlaky
=== CONT  TestAliasingFlaky
    common_test.go:777: BuildL1 deployConfig: DeployBold=true, DeployReferenceDAContracts=false
INFO [04-08|14:22:22.949] Started P2P networking                   self=enode://89c7a1ed7de00885d7abd1ce40878f2f794cf43ff4bcacd70e88866f0b18bbc26752f493ac67e98d22f4d81cb4d7770b64237c21a37781d3eb6f787db1c8cc14@127.0.0.1:0
WARN [04-08|14:22:22.949] Getting file info                        dir= error="stat : no such file or directory"
INFO [04-08|14:22:22.949] Started log indexer

TestPruningDBSizeReduction

Stack Traces | 0.000s run time

=== RUN   TestPruningDBSizeReduction
--- FAIL: TestPruningDBSizeReduction (0.00s)

TestBatchPosterL1SurplusMatchesBatchGasFlaky

Stack Traces | 0.560s run time

... [CONTENT TRUNCATED: Keeping last 20 lines]
panic: runtime error: invalid memory address or nil pointer dereference [recovered, repanicked]
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x20f3452]

goroutine 67 [running]:
testing.tRunner.func1.2({0x3866ac0, 0x62fd9b0})
	/opt/hostedtoolcache/go/1.25.8/x64/src/testing/testing.go:1872 +0x237
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.25.8/x64/src/testing/testing.go:1875 +0x35b
panic({0x3866ac0?, 0x62fd9b0?})
	/opt/hostedtoolcache/go/1.25.8/x64/src/runtime/panic.go:783 +0x132
github.com/offchainlabs/nitro/arbnode.(*InboxTracker).GetBatchCount(0x17d4b900?)
	/home/runner/work/nitro/nitro/arbnode/inbox_tracker.go:210 +0x12
github.com/offchainlabs/nitro/arbnode.(*InboxTracker).FindInboxBatchContainingMessage(0x0, 0x8)
	/home/runner/work/nitro/nitro/arbnode/inbox_tracker.go:225 +0x2f
github.com/offchainlabs/nitro/system_tests.TestBatchPosterL1SurplusMatchesBatchGasFlaky(0xc00038d880)
	/home/runner/work/nitro/nitro/system_tests/batch_poster_test.go:839 +0x725
testing.tRunner(0xc00038d880, 0x42425f8)
	/opt/hostedtoolcache/go/1.25.8/x64/src/testing/testing.go:1934 +0xea
created by testing.(*T).Run in goroutine 1
	/opt/hostedtoolcache/go/1.25.8/x64/src/testing/testing.go:1997 +0x465

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

[joshuacolvin0]

PR Review Summary: add-filter-set-id-hashstore

All findings verified by reading the code and running tests.

Critical Issues (2)

1. TestParseHashListJSON is broken — tests fail

parseHashListJSON now requires a valid "id" UUID field, but 6 of 8 sub-cases omit it. Confirmed by running the test:

service_test.go:263: failed to parse JSON with Sha256 hashing_scheme: invalid filter set ID UUID: invalid UUID length: 0

• invalidHashPayload (line 231) — passes but tests wrong error (id parse, not hash validation)
• wrongLenPayload (line 242) — passes but tests wrong error (id parse, not hash length)
• sha256Payload (line 253) — expects success, Fatalf stops test here
• unknownSchemePayload (line 270) — never reached
• prefixedPayload (line 287) — never reached
• noSchemePayload (line 306) — never reached

Note: invalidSaltPayload (line 220) still works correctly because salt is parsed before id.

Fix: Add "id": uuid.New().String() to every payload missing it.

2. No backward compatibility for missing id field in production payloads

s3_sync.go:97 — uuid.Parse("") fails if the provider hasn't added "id" yet, causing node startup failure. Unlike hashing_scheme (which handles missing values gracefully), id is a hard requirement with no fallback.

Fix: Either make id optional or coordinate deployment with the provider.

Important Issues (5)

3. Unwrapped salt UUID parse error — s3_sync.go:92-94

return nil, err is bare, while the id error on line 99 wraps with fmt.Errorf. Inconsistent and harder to debug.

Fix: return nil, fmt.Errorf("invalid salt UUID: %w", err)

4. uuid.Nil salt would silently disable all filtering — hash_store.go:78

uuid.Nil is the "not initialized" sentinel, but nothing prevents storing it as a real salt, which would silently bypass filtering.

Fix: Validate in parseHashListJSON: if salt == uuid.Nil { return nil, fmt.Errorf("salt cannot be nil UUID") }

5. Stale comments referencing old format

• s3_sync.go:79 — doc comment omits required "id" field from format
• service_test.go:178 — same omission
• service_test.go:219,227 — says "invalid salt hex" but salt is now UUID
• service_test.go:286 — says "0x-prefixed salt" but salt is now a UUID string

6. Changelog typo — changelog/mnasr-nit-4688.md:2

"id-set-fitler" should be "id-set-filter".

7. id field stored but never read — hash_store.go:21

The id is parsed (with hard failure on absence) and stored in hashData, but no code reads it — no getter, no logging, no metrics. The log line at s3_sync.go:74 doesn't include it. Consider at minimum logging it.

Suggestions (5)

8. Discarded uuid.Parse errors in tests — multiple files

Many tests use salt, _ := uuid.Parse(...). If a UUID constant is corrupted, the test silently gets uuid.Nil, and per issue 4, filtering is bypassed causing tests to pass for wrong reasons. Some tests already use
require.NoError — should be consistent.

9. Exported fields on unexported type — s3_sync.go:38-42

parsedPayload is unexported but has exported fields Id, Salt, Hashes. These should be lowercase. Also Id should be ID per Go conventions.

10. Missing doc comment on HashWithSalt — hash_store.go:38

Exported function implementing a protocol-level contract (sha256("::0x")). Deserves a doc comment.

11. Missing doc comment on parsedPayload — s3_sync.go:38

No doc comment, unlike its sibling hashListPayload on line 28.

12. Pre-existing: unknown hashing scheme continues silently — s3_sync.go:87-90

If the provider switches algorithms, the code logs a warning but proceeds with SHA-256, silently producing wrong hashes and bypassing filtering.

Strengths

• Provider cross-check test vectors in TestHashedAddressCheckerSimple — excellent for verifying compatibility
• HashWithSalt properly extracted and shared across production and test code
• Atomic swap and concurrency tests properly updated
• Import ordering correct across all files
• Named field initialization used correctly for hashData (6 fields)

Recommended Action

1. Fix the broken TestParseHashListJSON (CI will fail)
2. Decide on id field backward compatibility strategy
3. Wrap salt error, add uuid.Nil validation, fix stale comments/typo
4. Consider the suggestions for polish

[mahdy-nasr]

Merge conflict resolved!

[mahdy-nasr]

rebased on master

[diegoximenes]

Oh, the base branch of the cmd/filtering-report was the branch of this PR.
They are independent PRs, different goals, no need to had this dependency, it complicates the life of the reviewers.
Not sure why you did that.