test(blockassembly/subtreeprocessor): rapid property tests for validFromMillis admit logic#848
Merged
liam merged 2 commits intoMay 14, 2026
Conversation
…romMillis admit logic Adds pgregory.net/rapid (test-only dependency) and three property tests that pin the queue filter and dequeueDuringBlockMovement admit behaviour across randomly generated inputs. Test_propertyDequeueBatchAdmitPredicate: asserts the queue filter at queue.go:96 produces decisions matching its spec (validFromMillis <= 0 || batch.time < validFromMillis) for any (batch_time, validFromMillis) pair, including negative cutoff (bypass region), boundary region (batch.time-10 to batch.time+10), and unrelated cutoffs. 100 generated cases. Test_propertyDrainAdmitInvariant: drives the real dequeueDuringBlockMovement with random (window, enqueue_clock, drain_clock) - covering backward clock jumps, same-instant, and arbitrary forward advances - and asserts the integration outcome matches the expected admit predicate. Catches regressions in either the validFromMillis formula at SubtreeProcessor.go:3789-3796 or the queue filter. 100 generated cases. Test_propertyAgingAlwaysAdmits: liveness invariant. For any (window, enqueue_time), advancing the drain clock past (enqueue_time + window + 1ms) must admit the batch. Pins the fully-aged guarantee against future filter changes that might accidentally invert boundaries. 100 generated cases. All properties run sub-millisecond at the queue level. The integration property runs ~400ms total (100 iterations of newTestProcessorNoStart). With -race: ~10s for the three properties combined.
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: No issues found Summary This PR adds three well-structured property-based tests using pgregory.net/rapid to verify the queue admission logic. The tests correctly validate the boundary conditions and integration behavior of dequeueDuringBlockMovement. Key Verification Points: ✅ Documentation accuracy - All referenced line numbers and code snippets match the actual implementation:
✅ Test logic correctness - The property tests correctly implement the documented predicates:
✅ Dependencies - Uses existing test helpers (newTestProcessorNoStart, collectSubtreeHashes, fixedClock) from prior merged PRs ✅ Test hygiene - Properly uses t.Helper() indirection for outerT in rapid callbacks, appropriate cleanup handling The tests are defensive in nature (regression detection) and align with project verification standards in AGENTS.md. |
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-12 17:00 UTC |
This was referenced May 12, 2026
sugh01
approved these changes
May 12, 2026
…id.OneOf draw gofmt/gci flagged the three rapid.Int64Range arguments in Test_propertyDequeueBatchAdmitPredicate for inconsistent comment column alignment. Align all three comments to the same column.
|
ordishs
approved these changes
May 13, 2026
ordishs
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Verified the contract reformulations match source exactly: queue.go:96 and SubtreeProcessor.go:3789-3796 both reproduce verbatim. Three-tier coverage (queue spec → integration → liveness) with deliberate boundary targeting via Int64Range(batchTimeMs-10, batchTimeMs+10) and enqueue + window + 1ms. No mocks, sqlitememory utxo store, deterministic replay via rapid seeds.
Minor nits, none blocking:
t.Cleanupaccumulates on outer*testing.Tacross ~100 rapid iterations (acknowledged in PR). If-rapid.checksever gets raised, the goroutine/channel footprint grows linearly. A short comment at the call site would prevent surprise.- The
drainTime.Add(-window).UnixMilli()formula is duplicated in tests 2 and 3 — a small helper would keep the expected-value computation in one place. fmt.Sprintf(ctx, ...)in test 2 is computed unconditionally; trivial, but inline%vargs inrequire.Equalwould be cheaper.
ordishs
approved these changes
May 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds three property tests that pin the queue filter and
dequeueDuringBlockMovementadmit behaviour across randomly generated inputs, usingpgregory.net/rapidas a new test-only dependency.New dependency
pgregory.net/rapid v1.3.0- ~3kloc actively maintained property-testing library. Test-only (not imported from production code). Adds one line togo.modand two lines togo.sum.Considered the alternatives:
testing/quick(stdlib): no shrinking, manual generators per type, awkward for invariants over multiple correlated values.go test -fuzz: requires oneFuzzXxxfunction per property with[]byteinputs, verbose for the multi-value tuples here.Rapid's composable generators (
OneOf,Int64Range) and automatic shrinking make these invariants much easier to express and debug if they ever fail.Properties
Test_propertyDequeueBatchAdmitPredicate- queue-level. AssertsdequeueBatchdecisions match the spec atqueue.go:96:Generators target three regions: negative cutoff (
-1000..0, the bypass case), boundary (batch_time ± 10, the inclusive-reject case), and unrelated cutoffs. 100 cases, sub-millisecond.Test_propertyDrainAdmitInvariant- integration-level. Drives the realdequeueDuringBlockMovementwith random(window, enqueue_clock, drain_clock), including backward clock jumps (NTP-correction territory), same-instant drains, and arbitrary forward advances. Asserts the queue state and subtree contents match the expected admit predicate. 100 cases, ~400ms.This is the test that would have caught the asymmetry fixed in #846, and now guards against re-introducing it.
Test_propertyAgingAlwaysAdmits- liveness invariant. For any(window, enqueue_time), advancing the drain clock pastenqueue_time + window + 1msmust admit the batch. Pins the fully-aged guarantee against future filter changes that might accidentally invert boundaries.Test plan
go vet ./services/blockassembly/...- cleango test -count=1 -run 'Test_property' ./services/blockassembly/subtreeprocessor/- pass (~1s)go test -race -count=1 -run 'Test_property' ./services/blockassembly/subtreeprocessor/- pass (~10s)go test -race -count=1 ./services/blockassembly/subtreeprocessor/- pass (~170s)go mod tidyclean (only rapid added)Notes
fixedClockand the unified validFromMillis formula at both call sites.windowMs == 0 && deltaMs == 0.rapid.Checklogs the seed on failure) means counter-examples can always be re-run deterministically.