Skip to content

Make the sparse PIBD segment optimization (#3820) safe to apply#3915

Open
iho wants to merge 4 commits into
mimblewimble:stagingfrom
iho:fix/pibd-sparse-segment-producer
Open

Make the sparse PIBD segment optimization (#3820) safe to apply#3915
iho wants to merge 4 commits into
mimblewimble:stagingfrom
iho:fix/pibd-sparse-segment-producer

Conversation

@iho

@iho iho commented Jul 18, 2026

Copy link
Copy Markdown

Make the sparse PIBD segment optimization (#3820) safe to apply

This PR carries #3820's segment size optimization together with the fixes it needs to
be deployable. It is intended either to supersede #3820 or to be merged into it -
happy to go whichever way the maintainers and @mkorovkin2 prefer.

Why #3820 needs this

#3820 optimizes Segment::from_pmmr to only ship hashes at pruning boundaries derived
from the unspent bitmap. Testing it against a live node reproduced @ardocrat's report
on the PR thread: PIBD sync from a #3820 peer ends in error validating PIBD state: Invalid Root (peer present from the start) or a PIBD Reported Failure - Restarting Sync loop (peer appears mid-sync), with no peer attribution.

The root cause is that #3820 decides hash inclusion from the bitmap (spent status
at the archive header) while leaf inclusion is decided from on-disk presence -
and on any node between compaction passes those two signals disagree. For a subtree
that is fully spent per the bitmap but only partially compacted on disk (some leaves
spent recently, still in the files; others compacted away), the producer ships the
spent-but-present leaves as data and collapses the whole region into a single
higher boundary hash. The same positions are described twice, in incompatible ways.

Crucially, no receiver-side validation can catch this:

  • Root reconstruction (Segment::root) ignores leaf data at bitmap-spent positions
    and consumes the boundary hash, which is genuinely bound to the proof-validated
    root - the segment is cryptographically consistent. This also holds under Speed up chain compaction, harden PIBD validation, crash-safe compact #3906's
    carried-hash hardening (verify_carried_hashes): every carried hash is bound to
    the root. There is nothing wrong with the segment as a commitment; it is wrong as
    an application plan.
  • Apply (TxHashSet::apply_output_segment) then pushes the spent leaves at the
    frontier, and on reaching the boundary hash (guarded only by pos0 >= size) calls
    push_pruned_subtree for a subtree that overlaps the leaves it just appended.
    prune_list.append registers positions as pruned that are physically present in
    the hash file, desyncing every subsequent shift computation. Apply reports success;
    the corruption only surfaces at full state validation as Invalid Root.

Reproduced deterministically in store/tests/segment.rs:: redundant_leaf_and_boundary_hash_segment_corrupts_apply: the redundant segment shape
passes validate() and produces a divergent root through the exact
sort_pmmr_hashes_and_leaves/apply insertion rules.

This is also why the fix belongs in the producer, not behind a PIBD_HIST_2
capability bit (discussed on the #3820 thread): with the producer emitting only
segments that satisfy the apply invariant, existing deployed receivers handle sparse
segments correctly as-is - the wire format is already self-describing. A capability
bit would only be needed to protect old nodes from the broken producer.

What this PR changes

1. Segment::from_pmmr derives its hash set from the receiver's own reconstruction
walk instead of a bitmap heuristic
(core/src/core/pmmr/segment.rs)

Two passes:

  • Pass 1 selects exactly the leaves receiver-side root() consumes: present on disk
    and required by the bitmap, its sibling, or the final uneven leaf.
  • Pass 2 walks the segment bottom-up in postorder - the same traversal root()
    performs - tracking which positions are derivable from the selected leaves. At each
    parent with exactly one derivable child it emits the other child's hash (mirroring
    the (None, Some)/(Some, None) lookups in root()). Subtree roots still
    underivable at the end of the walk (fully pruned peaks of the final partial
    segment, or the root of a fully pruned segment) are emitted directly, matching
    where root() and first_unpruned_parent() probe.

Producer and receiver can no longer disagree about which positions are covered by
which hash. As a bonus the redundant spent-but-uncompacted leaves fold into the
boundary hash instead of being shipped twice, so segments get smaller than #3820's
in exactly the mixed regions where #3820 broke.

2. Receiver-side guard: reject overlapping pruned-subtree collapses instead of
corrupting state
(chain/src/txhashset/txhashset.rs)

apply_output_segment/apply_rangeproof_segment now check that a carried hash's
subtree lies entirely beyond the local frontier (bintree_leftmost(pos0) >= size)
before push_pruned_subtree, returning Error::InvalidSegment otherwise.
Defense-in-depth: any future producer bug of this class fails fast at apply time with
attribution, instead of surfacing hours later as an unattributable Invalid Root and a
restart loop.

3. Desegmenter fixes for small chains (chain/src/txhashset/desegmenter.rs)

Found while trying to run PIBD end-to-end on test chains; both are unreachable on
mainnet but panic or stall any chain with <= 1024 outputs, including the canned
chains for test_pibd_copy:

  • calc_bitmap_mmr_sizes: unwrap_or evaluates its fallback eagerly, panicking on
    the empty peak set of a single-chunk bitmap MMR. Now evaluated lazily, degrading
    to 0.
  • next_desired_segments: the bitmap branch's strict > size comparison never
    requests the first segment of a single-leaf bitmap MMR, silently stalling sync
    (positions 0..size-1 are present locally, so a segment whose last position
    equals the local size still contains new data).

Testing

  • cargo test -p grin_core -p grin_store -p grin_chain: 221 passed.
  • New: sparse_segment_mixed_compaction_round_trip - a segment from a
    partially-compacted region (spent+compacted sibling pair, spent-but-present pair
    under the same bitmap boundary) validates and round-trips through the desegmenter
    apply rules to the identical root, and asserts the mixed region collapses to the
    single boundary hash with no leaf data beneath it.
  • New: redundant_leaf_and_boundary_hash_segment_corrupts_apply - regression test
    pinning down why the bitmap-only heuristic was unsafe: the redundant segment shape
    validates but must not silently reproduce the source root through apply.
  • All pre-existing segment tests pass unchanged, including Optimize PMMR segments to only include hashes at pruning boundaries #3820's updated
    ser_round_trip (the optimized sizes are preserved) and the
    prunable_mmr/pruned_segment pruning-pattern batteries, which caught two edge
    cases in earlier iterations of the selection walk (fully pruned segment roots and
    pruned peaks of the final partial segment) - both covered by the final version.

Note: test_pibd_copy_sample (ignored, canned-chain end-to-end) currently fails on
master before reaching any segment logic - first run panics in
calc_bitmap_mmr_sizes (fixed here), reruns then fail opening the fixture because
the first open migrates/mutates the checked-in chain data in place. With this PR the
desegmenter panic is fixed; refreshing the canned fixtures is left as a separate
task.

mkorovkin2 and others added 4 commits July 18, 2026 08:12
…t pruning boundaries; tests updated as segment size dropped from 521 to 281 bytes (7 hashes -> 1 hash)
…ions

apply_output_segment / apply_rangeproof_segment: a pruned-subtree hash
must cover a region that is entirely absent locally. If any position
under it was already applied (leftmost < current size), collapsing via
push_pruned_subtree silently discards already-applied leaves/hashes and
desyncs the prune list from the physical hash file. Detect this and
return Error::InvalidSegment instead of corrupting local state.
…tmap

The bitmap-boundary heuristic decided hash inclusion from spent status
alone while leaf inclusion was decided from on-disk presence. For a
subtree fully spent per the bitmap but only partially compacted on disk
(the normal state of a node between compaction passes) those two signals
disagree: spent-but-present leaves were shipped as data AND collapsed
into a higher boundary hash. Such a segment passes validation (the extra
leaves are ignored by root reconstruction and the boundary hash is
genuinely bound to the root), but apply pushes the leaves at the
frontier and then registers the boundary subtree as pruned over them,
desyncing the prune list from the hash file. The corruption only
surfaced later as an unattributable Invalid Root during full state
validation, causing the reported PIBD restart loop.

Select leaves and hashes with the same bottom-up walk the receiver's
root reconstruction performs: ship exactly the leaves root() consumes,
then walk the segment in postorder tracking which positions are
derivable from them, emitting a hash wherever reconstruction will look
one up - the missing sibling at each partially-known parent, plus any
subtree root left unknown at the top (fully pruned peaks of the final
segment, or the root of a fully pruned segment). Producer and receiver
can no longer disagree about which positions are covered by which hash,
and redundant spent leaves fold into the boundary hash, shrinking the
segment further.

Adds store tests covering the mixed-compaction round trip through the
desegmenter apply logic, plus a regression test documenting that the
redundant leaf-plus-boundary-hash shape corrupts apply.
calc_bitmap_mmr_sizes: unwrap_or evaluates its fallback eagerly, so any
output MMR with <= 1024 leaves (a single bitmap chunk) computed
peaks(insertion_to_pmmr_index(0)) and panicked unwrapping the empty peak
set. Evaluate lazily and degrade to 0 when even the reduced set is
empty.

next_desired_segments: the bitmap branch compared a segment's last
position against the local PMMR size with a strict greater-than, so the
first segment of a single-leaf bitmap MMR (last position 0, local size
0) was never requested and sync stalled silently. Positions 0..size-1
are present locally, so a segment whose last position equals the size
still contains new data.

Unreachable on mainnet (far more than 1024 outputs) but panics or
stalls any small test chain, including the canned chains used by
test_pibd_copy.
@iho
iho changed the base branch from master to staging July 18, 2026 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants