perf(storer): filter excluded batches and rogue chunks early in reserve sample - #5634
gacevicljubisa wants to merge 2 commits into
Conversation
| isExcludedBatch, err := db.batchExclusionFilter(minBatchBalance) | ||
| if err != nil { | ||
| db.logger.Error(err, "get batches below value") | ||
| db.logger.Error(err, "get batch exclusion filter") |
There was a problem hiding this comment.
Not introduced here (master also logs and continues, with a partial map), but since this is being touched: if this fails, the sample includes chunks from below-balance batches and the reveal will disagree with the neighborhood, which gets the node frozen. Returning the error would make the node skip the round instead:
if err != nil {
return Sample{}, fmt.Errorf("batch exclusion filter: %w", err)
}
There was a problem hiding this comment.
It makes sense. Great for noticing this.
| func (db *DB) batchesBelowValue(until *big.Int) (map[string]struct{}, error) { | ||
| res := make(map[string]struct{}) | ||
|
|
||
| func (db *DB) batchExclusionFilter(until *big.Int) (func([]byte) bool, error) { |
There was a problem hiding this comment.
Every ReserveSample test passes nil for minBatchBalance, but production (Agent.minBatchBalance()) never does, so this path has no test. Could you add one: put a low-value batch in the mock batchstore (batchstore.WithBatch), store chunks from that batch and from another one, then sample with a minimum below / equal to / above the batch value, and assert that no sample items come from the low batch and that BelowBalanceIgnored is correct? (The PR checklist says tests were added, but none are in the diff.)
2b1a4ee to
2eadeb8
Compare
3 participants
Checklist
Description
Optimize batch filtering in ReserveSample by replacing
map[string]struct{}with a stack-allocated[32]bytelookup, where string conversions and heap allocations are eliminated. Filtering is moved to Phase 1.Benchmark Results
BenchmarkReserveSample10kWith1000ExcludedBatches(10k chunks in reserve, 1000 excluded batches):master3,294,111 ns/op(~3.29 ms)1,412,442 ns/op(~1.41 ms)67,081 allocs/op66,079 allocs/op4,299,470 B/op4,318,118 B/opOpen API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):
AI Disclosure