Speed up chain compaction, harden PIBD validation, crash-safe compact#3906
Speed up chain compaction, harden PIBD validation, crash-safe compact#3906iho wants to merge 6 commits into
Conversation
- write_tmp_pruned: prune_pos is ordered, so compare against the head of the remaining slice instead of scanning it for every element. Rewriting the PMMR hash/data files was O(elements x prune_count), which could take minutes on low-end hardware after a large prune set accumulated. - init_output_pos_index: binary search for the first header that can contain the first missing output and stop once the last missing entry is indexed, instead of walking every header from genesis to head on each compaction. Helps with mimblewimble#3872.
Segment root validation only checked hashes that participate in reconstructing the segment root. Apply, however, writes every hash in the segment into the local PMMR. A peer could therefore pass a segment with a valid root proof while poisoning intermediate hashes; the corruption only showed up at full state validation as a bad root, with no peer attribution. After the merkle proof succeeds, verify every carried hash is bound to the proof-validated root: either consumed during reconstruction, or a child of an already bound parent that hashes to that parent. Legitimate dense segments from uncompacted peers (pre-mimblewimble#3820 style) still pass when their extra hashes are consistent; only inconsistent or unbound hashes are rejected early as UnverifiableHash. Adds store segment tests covering tampered intermediate hashes and the uncompacted-peer dense format.
A hard kill between replacing hash/data files and flushing the prune list left compacted files paired with a stale prune list, producing wrong shifts and a bad root on restart. Compaction now: 1. Writes pruned hash/data temps and staged prune/leaf bitmaps 2. Fsyncs those temps, then writes a compact journal marker 3. Only then renames staging files into place and clears the journal On PMMRBackend::new, a present journal rolls renames forward; temps without a journal are discarded as abandoned prep. Variable-size size files are rebuilt on re-open when inconsistent with data, matching the existing open path. Adds tests for successful cleanup, roll-forward recovery, and orphan temp discard.
Reproduce slow compaction and hard-kill mid-compact (issue mimblewimble#3872): - compact_crash_stress example: prepare large pruned PMMR, compact with timing, verify root, kill-test that SIGKILLs a child paused at a phase - Test hooks in check_compact (env-gated, no-op in production): ready file + pause at hash_tmp / data_tmp / journal / applied - etc/qemu-compact-stress/run.sh: native, qemu-user (TCG), or Docker linux/amd64 with 0.5 CPU / 512MB (QEMU on arm hosts) - Manual CI workflow for optional long runs
Testnet is usually the easier real-world repro (and often heavier than mainnet even on a desktop): high spend churn and first compact after PIBD rewrites almost the entire hash/data files. Add prune_pct to prepare/kill-test, --testnet-like runner preset (large leaf count + ~92% pruned), and first vs second compact timing so the post-PIBD vs incremental gap is visible.
- Docker --weak: use host platform (not forced amd64 under QEMU), export /usr/local/cargo/bin, source cargo env, fail clearly if cargo missing - Progress logs during prepare/compact so --testnet-like is not silent - Clarify that harness modes never need a manual kill; only real grin --testnet does for optional full-node recovery experiments
| /// which only surfaces (without attribution) at full state validation. | ||
| /// A hash is accepted if it was consumed during reconstruction, or if it | ||
| /// hashes into an already accepted parent together with its sibling. | ||
| fn verify_carried_hashes(&self, mut known: HashMap<u64, Hash>) -> Result<(), SegmentError> { |
There was a problem hiding this comment.
Pruned hashes are still accepted when we are syncing against a node with #3820, so in the end we get Invalid Root before validation
| /// If a journal is present with staged files, open rolls renames forward and | ||
| /// yields a consistent PMMR (simulates hard kill after journal, before apply). | ||
| #[test] | ||
| fn compact_journal_recovery_roll_forward() { |
There was a problem hiding this comment.
For some reason test is crashing on Windows
| /// Staging files without a journal are abandoned prep — discarded on open, | ||
| /// leaving the pre-compact live files intact. | ||
| #[test] | ||
| fn compact_orphan_temps_discarded() { |
There was a problem hiding this comment.
Test is crashing on Windows
wiesche89
left a comment
There was a problem hiding this comment.
This contains three independent core changes: compaction performance, PIBD validation, and crash recovery with its test harness. Could we split them into three PRs? Each touches a different critical path and would be much easier to review, test, and revert separately.
Could you add before/after timings for both write_tmp_pruned and init_output_pos_index to the PR description? The stress harness is useful, but the expected improvement for #3872 is not recorded yet.
| self.leaf_set.write_to(&leaf_staging)?; | ||
|
|
||
| // --- Phase 2: journal commit point (temps are durable) --- | ||
| write_compact_journal(&self.data_dir)?; |
There was a problem hiding this comment.
The journal is the commit point, but the staging directory entries are not synced before it, and the replacements are not synced before the marker is removed. Could we make these directory syncs mandatory and ordered? Otherwise a power loss can leave mixed files with no journal.
| // --- Phase 3: replace live files, then clear journal --- | ||
| debug!("compact: applying journaled file replacements..."); | ||
| // Release mmaps/handles so renames succeed on all platforms. | ||
| self.hash_file.release(); |
There was a problem hiding this comment.
If apply or reinit fails after these releases, the backend stays without file handles and callers only log the error or restart the sync loop. Could we complete recovery and reopen it before returning, or propagate this as a fatal node restart condition?
| /// Apply any durable compact temps into place and clear the journal. | ||
| /// Safe to call when some temps are already gone (partial prior apply). | ||
| fn apply_compact_temps(data_dir: &Path) -> io::Result<()> { | ||
| replace_if_present( |
There was a problem hiding this comment.
The kill phases stop before any replacement or after all of them, but never in between. Could we add kill points after each hash/data/prune/leaf replacement? That is the main partial apply recovery guarantee here.
| let left = known | ||
| .get(&left_pos0) | ||
| .copied() | ||
| .or_else(|| self.get_hash(left_pos0).ok()); |
There was a problem hiding this comment.
get_hash() scans hash_pos linearly and is called for many bound nodes here, making dense peer segments O(n^2). Since this is peer controlled PIBD input, could we index the carried hashes once or use binary search?
| /// phase so an external watchdog can SIGKILL the process. | ||
| /// - `GRIN_COMPACT_CRASH_PAUSE_MS`: sleep duration in ms (default 5000 when | ||
| /// pause phase is set). | ||
| fn compact_test_hook(phase: &str) { |
There was a problem hiding this comment.
Could we gate these test hooks behind a Cargo feature? That keeps env reads and sleeps out of the production compaction path and prevents a stray GRIN_COMPACT_PAUSE_PHASE from pausing a real node.
| ); | ||
| } | ||
|
|
||
| // Hard kill: no graceful shutdown (simulates OOM killer / kill -9 / power loss). |
There was a problem hiding this comment.
SIGKILL covers process death, but not power loss: the page cache and directory metadata remain. Could we narrow this claim and keep power loss durability separate?
| /// Write the current bitmap to an arbitrary path (used for journaled compact). | ||
| /// Does not update the in-memory backup; call `mark_flushed` after the write | ||
| /// is committed via rename. | ||
| pub fn write_to<P: AsRef<Path>>(&self, path: P) -> io::Result<()> { |
There was a problem hiding this comment.
This duplicates PruneList::write_to() and both methods are only used by compaction. Could we use one shared crate private bitmap writer instead of expanding both public APIs?
| /// file is replaced. Presence means an interrupted compact must roll forward. | ||
| const PMMR_COMPACT_JOURNAL: &str = "pmmr_compact.journal"; | ||
| /// Suffix appended to leaf/prune paths for pre-commit compact temps | ||
| /// (`pmmr_prun.bin.compact`). Distinct from `.tmp` used by single-file flushes. |
There was a problem hiding this comment.
Hash/data compaction still uses .tmp, while prune/leaf uses .compact, so this comment is a little misleading. Could we either use one convention or explain why both suffixes belong to the same journal transaction?
|
|
||
| // Keep every Nth leaf so (prune_pct)% are removed. N = 100 / (100 - pct). | ||
| // e.g. prune_pct=50 → keep every 2nd; prune_pct=90 → keep every 10th. | ||
| let keep_every = (100u32 / (100 - prune_pct)).max(1); |
There was a problem hiding this comment.
This only approximates the requested percentage. For example, 60 selects keep_every = 2 and actually prunes about 50%, while the output still reports 60%. Could we calculate or report the actual percentage?
| let total_outputs = outputs_pos.len(); | ||
| let max_height = batch.head()?.height; | ||
|
|
||
| // outputs_pos is sorted by pos, so binary search for the first header |
There was a problem hiding this comment.
Could we add an equivalence test for missing entries near genesis and at the head, including consecutive headers with the same output_mmr_size? That would cover the boundaries of the new binary search.
Three related fixes for low-end hardware reliability and post-PIBD stability, plus a stress harness for kill-during-compact.
Compaction speed (#3872)
Avoid quadratic scan when rewriting PMMR files.
write_tmp_prunedcheckedprune_pos.containsfor every element against the whole remaining prune list. Both are sorted, so this was O(elements × prune_count). Now only the head of the remaining slice is compared: O(elements + prune_count).Bound the header walk in
init_output_pos_index. When any output was missing from the index, the rebuild walked every height from genesis to head and kept walking after all missing outputs were placed. It now binary-searches for the first relevant header and breaks once the last missing entry is indexed.PIBD segment hash validation (PR #3820 fallout)
Segment root validation only checked hashes used to rebuild the segment root, while apply writes every carried hash into the local PMMR. A peer could pass a valid root proof with poisoned intermediate hashes; failure only appeared at full state validation as a bad root (PIBD restart loop).
After the merkle proof succeeds, every carried hash must be bound to that root (consumed during reconstruction, or a child of a bound parent that hashes correctly). Dense segments from uncompacted peers still pass when consistent; unbound/wrong hashes fail early as
UnverifiableHash.Crash-safe compaction
A hard kill between replacing hash/data and flushing the prune list left compacted files with a stale prune list → wrong shifts → bad root on restart. Compaction now stages all new files, fsyncs, writes a journal marker, then renames. On open, a present journal rolls renames forward; temps without a journal are discarded as abandoned prep.
QEMU / weak-VPS stress harness
etc/qemu-compact-stress/+store/examples/compact_crash_stress.rs:SIGKILLthe compact process athash_tmp/data_tmp/journaland reopenqemu-user(TCG), Dockerlinux/amd64with 0.5 CPU / 512MB (QEMU on arm hosts)check_compact(no-op unless set)Manual workflow:
.github/workflows/compact-stress.yaml(workflow_dispatch).Testing
cargo test -p grin_store -p grin_core -p grin_chainsegment_carried_hash_verificationcompact_clears_journal,compact_journal_recovery_roll_forward,compact_orphan_temps_discardedspend_in_fork_and_compact./etc/qemu-compact-stress/run.sh --all-phases(native smoke)