fix: throttle chain compaction with a 1h wall-clock gap by iho · Pull Request #3892 · mimblewimble/grin

iho · 2026-07-09T07:56:46Z

Summary

Compaction is allowed during sync (to avoid one huge post-sync compact), but the only rate limit was probabilistic: ~1/DAY_HEIGHT (1440) blocks. On mainnet that is ~daily; during fast/archive sync blocks arrive much faster, so compact runs too often and slows sync.

Change

Add global::MIN_COMPACTION_INTERVAL_SECS = 3600 (1 hour).
NetToChainAdapter::check_compact refuses to trigger if less than 1 hour has elapsed since the last trigger.
Still applies the existing 1/COMPACTION_CHECK dice roll when the wall-clock window allows.
Record trigger time before spawning the compact thread to avoid compact storms under concurrent process_block.

Post-sync mainnet behavior is essentially unchanged (dice already averages ~1 day).

Test plan

cargo test -p grin_servers --lib common::adapters::tests -- --test-threads=1 (4 passed)
Sync a node and confirm compact logs at most about once per hour during body/archive sync

During fast sync, blocks arrive much faster than mainnet's 1/min, so the 1/DAY_HEIGHT probabilistic compaction check fires far too often and slows sync. Gate compaction triggers with MIN_COMPACTION_INTERVAL_SECS (1 hour) in addition to the existing height-based dice roll. Addresses mimblewimble#3594.

- Extract should_trigger_compaction and unit-test 50k same-instant process_block-equivalent checks (dice always hits) → exactly one compact per wall-clock window. - Log compact starts at info for ops verification. - Add scripts/compaction_sync_observe.sh for live mainnet observe runs.

iho · 2026-07-09T08:07:47Z

Verification: compact at most ~once per hour during sync

1. Worst-case rapid body-sync simulation (unit test) — PASS

rapid_sync_compacts_at_most_once_per_wall_clock_window:

50,000 consecutive process_block-equivalent checks
dice always hits (worst case; real dice is 1/1440)
wall clock frozen in a single instant → exactly 1 compact trigger
half-hour later still frozen → still 1
after full 1h window → 2nd trigger allowed

cargo test -p grin_servers --lib common::adapters::tests::rapid_sync_compacts_at_most_once_per_wall_clock_window
# ok

This is the property that matters for archive/body sync (many blocks/sec).

2. Live mainnet node observe — PASS (≤1 in window)

DURATION_SECS=180 ./scripts/compaction_sync_observe.sh

Metric	Result
Duration	180s (≪ 1h)
`check_compact: starting compaction`	0
Bound for <1h run	≤1
Outcome	OK

Notes from the run:

Node was in header sync (many Received 32 block headers lines).
check_compact only runs after successful full block process_block, so header-only phase produces 0 compact starts (expected).
Still satisfies “at most once per hour” for the observe window.

Script: scripts/compaction_sync_observe.sh (preserves logs under /tmp/grin-compact-observe-result).

Production constant

MIN_COMPACTION_INTERVAL_SECS = 3600 (1 hour), enforced under a write lock with the dice roll so concurrent block processing cannot double-trigger.

wiesche89

Thanks for addressing this. The diff feels much larger than the fix requires, mostly due to the manual script, overlapping tests, and review specific instrumentation. Could we keep the production change small and cover it with one focused deterministic test and one concurrent Rust test?

- Read `now` under the lock (not before) so a slower thread's stale timestamp can no longer be judged against a newer trigger and move last_compact_trigger backwards. - Only stamp last_compact_trigger after the compactor thread actually spawns, so a spawn failure doesn't suppress compaction for an hour. - Switch last_compact_trigger from RwLock to Mutex since it's never read-locked. - Add a multi-threaded test exercising the real lock/check/stamp gate, not just the pure helper. - Drop the ad-hoc compaction_sync_observe.sh script in favor of the deterministic unit tests.

wiesche89

Thanks, the functional issues are resolved. The remaining diff still feels larger than necessary, mainly due to overlapping tests and duplicated gate logic. Could we reduce it to one small gate with one boundary test and one concurrency test, and update the PR description now that the observe script has been removed?

wiesche89 · 2026-07-12T07:12:10Z

 	}
 }
+
+#[cfg(test)]


Could we reduce this to one focused interval test and one concurrency test? Six tests and xxx lines feel excessive for this small gate.

Two tests is much cleaner. Could we also replace the 50k loop and zero-duration stand in with a small boundary setup using timestamps just inside and outside the interval? That would test the actual comparison more directly.

…ests Fold the wall-clock helper and dice wrapper into a single try_trigger_compaction() used by both production code and tests, reduce the test module to one interval test and one concurrency test, and drop the issue references from comments.

wiesche89 · 2026-07-19T16:43:20Z

 	header_segment_requests: RwLock<HashMap<SocketAddr, (DateTime<Utc>, usize)>>,
+	/// Wall-clock time of last successful compaction *trigger* (not completion).
+	/// Used with `MIN_COMPACTION_INTERVAL_SECS` to avoid compact storms during sync.
+	last_compact_trigger: Mutex<Option<Instant>>,


This state is local to the adapter, the sync loop’s direct Chain::compact() call bypasses it, and a restart resets it. Could we move the interval check to the shared compaction boundary so all trigger paths use the same policy, and define the restart behavior here too?

wiesche89 · 2026-07-19T16:44:02Z

+		Some(t) => now
+			.checked_duration_since(t)
+			.map(|d| d >= min_interval)
+			.unwrap_or(true),


t was recorded from an earlier Instant::now() under the same mutex, so this None case should not be reachable. Could we simplify this to now.duration_since(t) >= min_interval?

wiesche89 · 2026-07-19T16:44:45Z

 	}
 }
+
+#[cfg(test)]


Two tests is much cleaner. Could we also replace the 50k loop and zero-duration stand in with a small boundary setup using timestamps just inside and outside the interval? That would test the actual comparison more directly.

wiesche89 · 2026-07-19T16:47:23Z

 const HEADER_SEGMENT_REQUEST_WINDOW_SECS: i64 = 60;
 const MAX_HEADER_SEGMENT_REQUESTS_PER_WINDOW: usize = 120;

+/// Wall-clock + probabilistic gate for chain compaction. If the dice hit and


The same rationale is repeated around the constant, helper, field, production call, and tests. Could we keep the motivation once near the constant and trim the other comments to just the non obvious locking invariant?

iho added 2 commits July 9, 2026 10:56

wiesche89 reviewed Jul 11, 2026

View reviewed changes

iho requested a review from wiesche89 July 11, 2026 15:39

wiesche89 reviewed Jul 12, 2026

View reviewed changes

iho force-pushed the fix/compaction-wall-clock-throttle branch from db8ea53 to b1923c7 Compare July 12, 2026 08:10

iho requested a review from wiesche89 July 12, 2026 08:14

wiesche89 reviewed Jul 19, 2026

View reviewed changes

Uh oh!

Conversation

iho commented Jul 9, 2026

Summary

Change

Test plan

Uh oh!

iho commented Jul 9, 2026

Verification: compact at most ~once per hour during sync

1. Worst-case rapid body-sync simulation (unit test) — PASS

2. Live mainnet node observe — PASS (≤1 in window)

Production constant

Uh oh!

wiesche89 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wiesche89 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wiesche89 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wiesche89 Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

wiesche89 Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants