perf(core): Hand-rolled UnmarshalBinary for AggregatedBloomFilter

[brbrr]

Reduces allocations and the GC pressure for a getEvents call

Bench results:

Metric	main (old decode)	branch (new decode)	Change
time/op	6.278 ms	1.340 ms	−78.65%
B/op	25.75 MiB	8.25 MiB	−67.96%
allocs/op	49,157	2	−100.00%

[github-actions]

Claude finished @brbrr's task in 4m 49s —— View job

---

Review: Hand-rolled UnmarshalBinary for AggregatedBloomFilter

• Read PR diff (core/aggregated_bloom_filter.go, tests)
• Verify wire-format compatibility vs. old bitset.UnmarshalBinary path
• Check bounds/overflow, anti-OOM guard, panic-safety on corrupt input
• Review shared-backing-array aliasing safety
• Assess test coverage

---

Verdict: clean, ship it. ✅ No blockers, no important issues. This is a well-executed hot-path optimization with unusually thorough tests.

What I checked

Wire-format compatibility — The decode faithfully mirrors bits-and-blooms/bitset v1.24.5's MarshalBinary: uint64 bit-length + nWords big-endian uint64 words, all binary.BigEndian. Reconstructing via FromWithLength(NumBlocksPerFilter, row) reproduces the original BitSet, and the round-trip require.Equal(filter, decoded) tests (including the non-zero-range case that would expose header byte-offset bugs) confirm it.

Bounds / anti-OOM — The count > (len(data)-headerSize)/rowSize guard (aggregated_bloom_filter.go:284) correctly rejects a corrupt count before the make([]uint64, count*wordsPerFilterRow) allocation, since every valid row is exactly rowSize bytes. Per-row checks (off+rowLenSize, blobLen < bitLenSize, off+blobLen > len(data)) make every subsequent binary.BigEndian.Uint64 read provably in-bounds. The FuzzAggregatedBloomFilterUnmarshal test with the 0xFFFFFFFF count seed pins this. No path can panic on untrusted DB bytes.

Shared-backing aliasing — All 128-word rows are subslices of one backing array, but the three-index slice backing[lo : lo+wordsPerFilterRow : lo+wordsPerFilterRow] (:310) caps len==cap==128, so any bitset op that would grow reallocates instead of corrupting neighbors. Since 8192 bits fit exactly in 128 words, no in-range Set/Union ever needs to grow. Correct and deliberate.

Nits (non-blocking, no action required)

• Stricter validation than the old path (:303): decode now hard-requires bitLen == NumBlocksPerFilter, whereas bitset.UnmarshalBinary accepted any length. This is safe — every row is written with the fixed NumBlocksPerFilter=8192 constant, so all persisted data satisfies it — and it's arguably an improvement. Just flagging that it tightens the accepted input set, so a future change to NumBlocksPerFilter would reject old DB data (which would be a migration concern regardless).
• On a mid-row error the receiver f is left partially mutated (header set, bitmap partially filled). This matches the old behavior — callers must discard f on error — so it's not a regression.

Nice work on the benchmark (49,157 → 2 allocs/op) and especially the test suite — the header-layout pin, corruption cases, fuzz target, and alloc-count assertion cover the risk surface well.

Note: I could not execute go test/go run in this sandbox (only go build, which passed), so the above is from static analysis of the diff and the library format rather than a live test run.

[Copilot]

Pull request overview

This PR optimizes core.AggregatedBloomFilter.UnmarshalBinary by replacing the generic binary.Read + per-row allocations with a hand-rolled decoder that reuses a single []uint64 backing buffer, reducing allocations/GC pressure in getEvents-related paths.

Changes:

• Added a package-level wordsPerFilterRow helper to compute the fixed word width for a NumBlocksPerFilter-bit row.
• Reimplemented AggregatedBloomFilter.UnmarshalBinary to parse the on-disk format directly and build bitsets from a shared backing slice.
• Added compatibility, corruption/bounds, fuzz, and allocation-regression tests for the new decoder.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
core/aggregated_bloom_filter.go	Implements the new manual UnmarshalBinary and shared backing allocation strategy.
core/aggregated_bloom_filter_test.go	Adds tests covering wire-format compatibility, corruption handling, fuzzing, and allocation limits.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

❌ Patch coverage is 89.47368% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.48%. Comparing base (c35a08d) to head (b0c5f6e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
core/aggregated_bloom_filter.go	89.47%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3796      +/-   ##
==========================================
+ Coverage   75.21%   75.48%   +0.27%     
==========================================
  Files         438      438              
  Lines       39577    39521      -56     
==========================================
+ Hits        29767    29832      +65     
+ Misses       7720     7627      -93     
+ Partials     2090     2062      -28     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rodrodros]

@brbrr how were the benchmark results achieved?

[brbrr]

@rodrodros I decided not to commit the bench test, as it's quite simple. Basically I've tested direct unmarshaling and compared it against main branch. Let me know if you think I should commit the bench test. Next week I'll look into it's effects on rpc latency.

func BenchmarkAggregatedBloomFilterUnmarshal(b *testing.B) {
	filter := core.NewAggregatedFilter(0)
	bf := bloom.New(core.EventsBloomLength, core.EventsBloomHashFuncs)
	bf.Add([]byte{0x01})
	require.NoError(b, filter.Insert(bf, 0))

	data, err := filter.MarshalBinary()
	require.NoError(b, err)

	b.ReportAllocs()
	for b.Loop() {
		var decoded core.AggregatedBloomFilter
		if err := decoded.UnmarshalBinary(data); err != nil {
			b.Fatal(err)
		}
	}
}

[rodrodros]

@brbrr yes, please commit the benchmarks in its dedicated bench_test file, it is good to have them

[Ehsan-saradar]

Pulled the branch and reproduced the benchmark independently on a worst-case fully-populated filter (~8.1 MiB blob): allocs 49,157 → 2 and 25.75 MiB → 8.25 MiB match your table exactly, and the ~78% speedup holds on a slower box too. @rodrodros — that should cover the reproducibility question; committing the benchmark would let CI guard it from here on.

The per-row validation is tight, nice. One theme across the inline notes: the decoder fully trusts the three header fields that MarshalBinary always writes consistently (count, toBlock, and the total length), so a corrupt/truncated DB blob can decode "successfully" and blow up on a later query. All corrupt-input hardening, not happy-path — so flagging as a comment, not blocking.

[Ehsan-saradar]

Building on the earlier note about count: since NewAggregatedFilter always creates exactly EventsBloomLength rows, I'd validate the header invariants right here instead of trusting them —

• count == EventsBloomLength: a smaller (or zero) self-consistent count decodes fine, then BlocksForKeys panics index out of range on the next query. A zero-value AggregatedBloomFilter{} marshals to a 20-byte header with count == 0 and hits this too.
• toBlock == fromBlock + NumBlocksPerFilter - 1: otherwise a corrupt toBlock widens the accepted range while the matrix still has 8192 columns — Insert then accepts an out-of-range block, grows a row past 8192 bits, and later intersections silently truncate to the shorter row → wrong results. Verified: toBlock = 1<<40 decodes with no error and Insert(b, 20000) is accepted.

Two cheap guards here close all of it.

[Ehsan-saradar]

Worth asserting off == len(data) before returning. Right now trailing bytes after the last row are silently ignored, so a valid blob with junk appended still decodes (err == nil) — verified by appending 4 bytes. Makes decode canonical and catches framing corruption/truncation.

[Ehsan-saradar]

Minor/readability: a valid row is always exactly rowSize - rowLenSize bytes, so these three conditions collapse to a single blobLen != <expected> — which also makes reading bitLen provably in-bounds and drops nWords entirely.

[github-actions]

Claude finished @brbrr's task in 2m 15s —— View job

---

Review: Hand-rolled UnmarshalBinary for AggregatedBloomFilter

• Re-read core/aggregated_bloom_filter.go decode path + call sites
• Verify the count / toBlock corruption concerns raised by reviewers
• Review the newly-committed benchmark
• Post findings

The hot-path optimization itself is solid — the wire format faithfully mirrors bitset.MarshalBinary, the shared backing array with the three-index slice (:310) is correctly capped so no in-range op can grow into a neighbor, and the per-row bounds checks are airtight. The headline win (49,157 → 2 allocs/op) is real and worth having.

My prior automated review called this "clean" but missed a genuine hardening gap that @Ehsan-saradar and Copilot correctly flagged. Confirming it here:

Important

• Header fields (count, toBlock) are trusted, not validated (aggregated_bloom_filter.go:276). Every consumer indexes the matrix with the fixed constant — BlocksForKeys uses f.bitmap[index%EventsBloomLength] (:174) and clear loops range EventsBloomLength (:153) — but the decoder accepts any count. A corrupt/truncated blob with a self-consistent count < EventsBloomLength (e.g. a zero-value filter → 20-byte header, count == 0) decodes with err == nil and then panics index out of range on the next query. Likewise a widened toBlock lets Insert accept an out-of-range block and grow a row past 8192 bits. Two cheap guards (count != EventsBloomLength, toBlock != fromBlock+NumBlocksPerFilter-1) close both. Not a regression — the old binary.Read path was equally trusting — but the hand-rolled decoder is the right place to make decode total. Details inline.

Nit

• Trailing bytes silently ignored (:318): a valid blob with junk appended still decodes. A final off == len(data) assertion makes decode canonical. Inline.
• Row-length check could collapse (:303): since a valid row is always exactly rowSize - rowLenSize bytes, the three conditions reduce to a single blobLen != expected, dropping nWords. Purely readability — current form is correct.
• Benchmark (aggregated_bloom_filter_bench_test.go): good to have it committed and CI-guarded. It exercises the full 8192-row decode (allocs scale with row count, not set bits), so it's representative of the reported numbers even with a sparse filter. 👍

Nothing here blocks merge on its own — the validation gap matches pre-existing behavior — but since you're rewriting the decoder, adding the two header guards is the highest-value follow-up. I've left the concrete guard code inline. Want me to push those changes to this branch? Just say the word.

[github-actions]

[important] Validate the header invariants instead of trusting count / toBlock.

The decoder accepts any count, but every consumer indexes the matrix with the fixed constant — e.g. BlocksForKeys does f.bitmap[index%EventsBloomLength] (line 174) and clear loops for index := range EventsBloomLength. A corrupt/truncated blob encoding a self-consistent count < EventsBloomLength (a zero-value filter marshals to a 20-byte header with count == 0) decodes with err == nil and then panics index out of range on the next query — a DoS on corrupt DB bytes.

Similarly toBlock is stored verbatim; a widened toBlock makes Insert accept an out-of-range block and grow a row past 8192 bits, silently corrupting later intersections.

Both are cheap to close right here, since NewAggregatedFilter always produces exactly EventsBloomLength rows spanning NumBlocksPerFilter blocks:

if count != EventsBloomLength {
	return ErrBloomFilterSizeMismatch
}
if f.toBlock != f.fromBlock+NumBlocksPerFilter-1 {
	return ErrBloomFilterSizeMismatch
}

Note this isn't a regression (the old binary.Read path was equally trusting), but since you're hand-rolling the decoder it's a good time to make it total.

[github-actions]

[nit] Assert off == len(data) before returning nil. Trailing bytes after the last row are currently ignored, so a valid blob with junk appended still decodes successfully. A final if off != len(data) { return io.ErrUnexpectedEOF } makes the decode canonical and catches framing/truncation corruption.