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 86.36364% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.28%. Comparing base (c35a08d) to head (80fc145).
⚠️ Report is 16 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3796      +/-   ##
==========================================
+ Coverage   75.21%   75.28%   +0.06%     
==========================================
  Files         438      438              
  Lines       39577    39355     -222     
==========================================
- Hits        29767    29627     -140     
+ Misses       7720     7654      -66     
+ Partials     2090     2074      -16     

☔ 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.

[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.

[RafaelGranza]

LGTM. But please address the changes I considered to be fair.
Just as a guarantee, I ran the fuzzer for 1M execs and saw no panics or crashes.

[github-actions]

Claude finished @brbrr's task in 1m 24s —— View job

---

Re-review after fix: address various feedback (26b1787)

• Diff the new commit vs. prior review feedback
• Verify header-invariant guards (count, toBlock)
• Verify trailing-byte / canonical-length check
• Verify row-length collapse kept bitLen check independent
• Confirm new corruption tests match the guards

The latest commit resolves every open item from the prior reviews. Nice, thorough follow-up.

What got addressed

• Header invariants now validated (core/aggregated_bloom_filter.go:284-289) — count != EventsBloomLength and toBlock != fromBlock+NumBlocksPerFilter-1 both reject before allocation. This closes the DoS gap flagged by @Ehsan-saradar / Copilot: a self-consistent count < EventsBloomLength (e.g. a zero-value filter's 20-byte header) no longer decodes with err == nil and panics index out of range on the next BlocksForKeys query, and a widened toBlock can no longer let Insert grow a row past 8192 bits.
• Canonical length enforced (:331-333) — off != len(data) → io.ErrUnexpectedEOF, so trailing junk no longer decodes successfully.
• Row-length checks collapsed correctly (:312-317) — folded the three redundant conditions into a single blobLen != blobLenWant while keeping the bitLen != NumBlocksPerFilter check separate, exactly as @RafaelGranza noted (bitLen and blobLen are independent fields and can't fold together). The blobLen check also makes the subsequent bitLen read provably in-bounds.
• Doc comment added (:261-263) — per @rodrodros; also documents the "discard f on error" contract.
• Test lint fix — dropped the unnecessary uint64(...) conversion; new sub-tests (wrong row count, toBlock not matching range, trailing bytes) pin each guard, and the alloc comment is updated to the real ~2.

Notes (non-blocking)

• The anti-OOM check at :295 (count > (len(data)-headerSize)/rowSize) is now strictly redundant with count == EventsBloomLength above it — with count pinned, this only ever guards the fixed EventsBloomLength value. Harmless and arguably still nice as defense-in-depth, so no change needed.
• fromBlock+NumBlocksPerFilter-1 can wrap for an absurd fromBlock near MaxUint64, but toBlock is read from the same blob and must match, and matrix dimensions stay fixed by count, so there's no panic path — not worth a guard.

Verdict: all prior feedback addressed; looks good to merge. ✅ (Verified via go build; go test couldn't run in this sandbox.)

[github-actions]

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

---

Re-review after fix: address various feedback

• Read decode path + call sites
• Verify header guards, bounds, canonical-length check
• Build (go build ./core/... ✅); go test/-fuzz blocked in this sandbox
• Post summary

Verdict: clean — no blockers, no new issues. ✅ Every item from the earlier reviews is resolved and I found nothing further worth an inline comment.

What I re-verified

• Header invariants (core/aggregated_bloom_filter.go:284-289) — count != EventsBloomLength and toBlock != fromBlock+NumBlocksPerFilter-1 both reject before allocation. Confirmed EventsBloomLength == 8192 (core/transaction.go:157), so all persisted rows satisfy count == 8192; the guards close the DoS gap (a self-consistent small count no longer decodes then panics index out of range in BlocksForKeys, and a widened toBlock can no longer let Insert grow a row past 8192 bits).
• Bounds / anti-OOM (:295) — every per-row read (off+rowLenSize, off+blobLen, the bitLen/word reads) is provably in-bounds; count is pinned before the make, so the allocation can't be driven oversized. FuzzAggregatedBloomFilterUnmarshal (incl. the 0xFFFFFFFF-count seed) pins this.
• Shared backing safety (:320) — the three-index slice caps len == cap == wordsPerFilterRow (128), and 8192 bits fit exactly in 128 words, so no in-range Set/Union/intersection ever grows a row into a neighbor's memory. FromWithLength shares the subslice deliberately.
• Canonical length (:331) — off != len(data) → io.ErrUnexpectedEOF, so trailing junk no longer decodes.
• Row-length collapse (:312-317) — folded into a single blobLen != blobLenWant while keeping bitLen != NumBlocksPerFilter separate (independent fields), exactly as @RafaelGranza noted; the blobLen check makes the bitLen read in-bounds.
• Tests / bench — corruption sub-tests pin each guard (wrong row count, toBlock not matching range, trailing bytes), the alloc-count assertion guards the O(1) claim, and the committed benchmark is CI-guarded and exercises the full 8192-row decode.

Notes (non-blocking, no action needed)

• The anti-OOM check at :295 is now strictly redundant with count == EventsBloomLength above it, but it's harmless defense-in-depth.
• fromBlock+NumBlocksPerFilter-1 can wrap for an absurd fromBlock near MaxUint64, but toBlock must match the same blob and matrix dims stay fixed by count, so there's no panic path.

Nice, thorough follow-up — this is a well-executed hot-path optimization (49,157 → ~2 allocs/op) with unusually complete corrupt-input hardening.

[github-actions]

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

---

Re-review after fix: address various feedback (80fc145)

• Diff latest commits vs. prior review feedback
• Verify header-invariant guards (count, toBlock)
• Verify bounds / anti-OOM / word-read in-bounds proofs
• Verify canonical-length + row-length checks
• Build (go build ./core/... ✅)

Verdict: clean — no blockers, no new issues. ✅ Every item from the earlier reviews (Copilot, @Ehsan-saradar, @RafaelGranza, @rodrodros) is resolved, and I found nothing further worth an inline comment. Since the diff is a hardened decoder with full test coverage and I have nothing to add on any specific line, I'm skipping inline comments per the review guidelines.

What I re-verified

• Header invariants (core/aggregated_bloom_filter.go:284-289) — count != EventsBloomLength and toBlock != fromBlock+NumBlocksPerFilter-1 both reject before allocation, closing the DoS gap: a self-consistent small count (e.g. a zero-value filter's 20-byte header) can no longer decode with err == nil and then panic index out of range in BlocksForKeys, and a widened toBlock can no longer let Insert grow a row past 8192 bits.
• Bounds / anti-OOM (:295, :303-317) — count is pinned before the make([]uint64, count*wordsPerFilterRow), so the allocation can't be driven oversized. Per-row off+rowLenSize / off+blobLen checks plus the blobLen != blobLenWant gate make every binary.BigEndian.Uint64 read provably in-bounds; the last word read ends at exactly off+blobLen ≤ len(data).
• Row-length collapse (:312-317) — folded into a single blobLen != blobLenWant while keeping bitLen != NumBlocksPerFilter separate, exactly as @RafaelGranza noted (independent fields); the blobLen check makes the bitLen read in-bounds.
• Canonical length (:331) — off != len(data) → io.ErrUnexpectedEOF, so trailing junk no longer decodes.
• Shared-backing safety (:319-325) — the three-index slice caps len == cap == wordsPerFilterRow (128), and 8192 bits fit exactly in 128 words, so no in-range Set/Union ever grows a row into a neighbor's memory.
• Doc comment (:261-263) — added per @rodrodros; documents the "discard f on error" contract.
• Tests / bench / lint — corruption sub-tests pin each guard, the alloc-count assertion guards the O(1) claim, the committed benchmark is CI-guarded, and the unnecessary uint64(...) conversion flagged by @RafaelGranza is gone.

Notes (non-blocking, no action needed)

• The anti-OOM check at :295 is now strictly redundant with count == EventsBloomLength above it — harmless defense-in-depth.
• fromBlock+NumBlocksPerFilter-1 can wrap for an absurd fromBlock near MaxUint64, but toBlock must match the same blob and matrix dims stay fixed by count, so there's no panic path.

Nice, thorough follow-up — a well-executed hot-path optimization (49,157 → ~2 allocs/op) with unusually complete corrupt-input hardening. (Verified via go build ./core/...; go test/-fuzz couldn't run in this sandbox, but @Ehsan-saradar reproduced the benchmark and @RafaelGranza ran the fuzzer for 1M execs with no crashes.)

[brbrr]

@rodrodros I've addressed all the feedback. please give it another look when you'll have some time.

[rodrodros]

I assume this variable represents, an offset, why not call it offset?

[rodrodros]

Solving the issue is not good enough if the code that accompanies is plagued by readability issues – causing issues for future developers.

I would like for you to reinforce on this side of code writing and doing self reviews.

[rodrodros]

Maybe we can make this a constant equal to NumBlocksPerFilter - 1. I believe it might be a bit more legible.

[rodrodros]

Wording like "It never panics on corrup DB bytes, returning an error instead" may have a more direct wording of: "returns an error in case of DB corruption". There is no need to say what it doesn't do, if it doesn't do it. It is enough to say what the function does.

// UnmarshalBinary decodes MarshalBinary's output in place. Returns an error
// in case of DB corruption. On error f may be partially written and must be 
// discarded. 

[rodrodros]

Both blobLenWant and rowSize could be constants, assuming that the previous comment is applied (i.e. making wordsPerFilterRow a constant)

[rodrodros]

Why is off created here when it is used 23 lines below? Why it is not defined right before being used?

[rodrodros]

Why not use a matrix here? [][]uint64?

Have you consider using a static array, since count must be EventsBloomLength and wordsFilterPerRow is fixed as well: [a][b]uint64?

[rodrodros]

It also seems that you don't need all of that space at once. You only need one row that is re-used per loop and passed to the bitmap in i, or am I mistaken?

[thiagodeev]

AFAIU, the real improvement of the PR comes from exactly this. Before, there were thousands of allocations per filter, and now we allocate a big array once and use pieces of it to back all the thousands of slices, avoiding allocating them.
Reusing a single small array is not possible because we need individual backing arrays for all the slices/rows since each one of them stores different filters

[rodrodros]

@RafaelGranza I would've expect for you to catch some the things I catched on my review. As said previously on dailies, reviews are not only to check if an issue is solved, but to also check that is done in the best way possible – readability wise included.

@brbrr please be more careful in follow up PRs and take your time to familiarize with what you wrote and try to give yourself AI-less self reviews.

[thiagodeev]

Nice improvement!
Partial review

[thiagodeev]

Suggested change

	// UnmarshalBinary decodes MarshalBinary's output in place. It never panics on
	// corrupt DB bytes, returning an error instead. On error f may be partially
	// written and must be discarded.
	// UnmarshalBinary decodes MarshalBinary's output in place. It never panics on
	// corrupt DB bytes, returning an error instead. On error, f may be partially
	// written and must be discarded.

[thiagodeev]

Can we rename bitLenSize to bitsetLenSize?

[thiagodeev]

Can we rename bitLen to bitsetLen?

[thiagodeev]

We can make this a const instead of using len(row.Words()).

const wordsPerFilterRow = int((NumBlocksPerFilter + 63) / 64) // 128

This is the formula actually being used by BitSet.WriteTo/ BitSet.MarshalBinary to calculate the used words - ref, ref, ref ((int(i) + wordMask) >> log2WordSize == (i + 63) / 64).

By calling len(row.Words()) we are getting the length of the internal storage slice, while BitSet.WriteTo/ BitSet.MarshalBinary only write the actual used words, obtained by calling wordCount().
This works today because makeBitset() happens to allocate exactly that many words, but that isn’t really an API guarantee (thinking about a future change in the lib). Having it as a fixed const is better IMO since we are creating our own custom decode function with fixed values.