Optimize PMMR segments to only include hashes at pruning boundaries

[mkorovkin2]

Addressing TODO item in code: optimize PMMR segments to only include hashes at pruning boundaries; tests updated as segment size dropped from 521 to 281 bytes (7 hashes -> 1 hash)

---

name: Pull Request
about: Pull Request checklist
title: 'Optimize PMMR segments to only include hashes at pruning boundaries'
labels: ''
assignees: ''

---

• This PR addresses the TODO at core/src/core/pmmr/segment.rs:338: "optimize, no need to send every intermediary hash"
• PROBLEM: During PIBD sync, segments are generated containing leaf data, intermediate hashes, Merkle proofs. Previously, the from_pmmr function included hashes for every position in prunable segments, even when many of these hashes were unnecessary for validation. This resulted in unnecessarily large segment payloads being transmitted over the network
• SOLUTION: identify "pruning boundaries" (positions where a fully pruned subtree meets a non fully pruned sibling). Only hashes at these boundaries are actually needed during segment validation (when the root() function encounters a (None, Some) or (Some, None) case and must look up the missing child's hash)
• CHANGE IMPACT: in the ser_round_trip test case, this optimization reduces the number of hashes from 7 to 1, shrinking the segment from 521 bytes to 281 bytes (46%); real-world savings will vary depending on pruning patterns but any segment with partial pruning will see reduced size
• CONSENUS BREAKING? No. (this change does not affect consensus rules)
• BREAKING CHANGES: API change: The Segment::from_pmmr() function signature changed:
  • ---> Before: from_pmmr(segment_id, pmmr, prunable: bool)
  • ---> After: from_pmmr(segment_id, pmmr, bitmap: Option<&Bitmap>)
  • Callers update:
  • ---> false to None (for non-prunable segments like kernels, bitmaps)
  • ---> true to Some(&bitmap) (for prunable segments like outputs, rangeproofs)
• TESTING:
  • Unit tests: All existing segment tests pass cargo test -p grin_core unprunable_mmr / cargo test -p grin_store --test segment / cargo test -p grin_chain --test bitmap_segment
  • All unit test pass and they're pretty reliable here since they generate segments of varied pruning patterns, immediately validate those segments, and cover a number of edge cases
  • The ser_round_trip test assertion was updated to reflect the reduced hash count, serving as verification that the optimization is working as intended
  • Manual build testing: build verification completed with cargo build. no new warnings introduced by this change.
  • Manual testnet sync: manual testnet sync; fully works and passes; ensured that node runs and syncs.
  • Manual local testing across two nodes: synced node (node1) and fresh node (node2)
  • node1 had synced full chain data, node2 connected to node1 and requested segments
  • Flow tested:
    • node1 calls Segmenter::output_segment() / rangeproof_segment() which uses the modified from_pmmr() to generate optimized segments
    • node1 serializes and sends segments
    • node2 deserializes and calls segment.validate() to verify the segemnt
    • node2 successfully reconstructs the chain state
    • ---> can see that optimized segments (with fewer hashes) survive serialization, network transmission, deserialization, and validation by a real node

[mkorovkin2]

First PR! (People in the Keybase/Telegram chat suggested to tag John/David)

[mkorovkin2]

Hey @DavidBurkett - when you get a chance would appreciate your feedback on this PR!

[ardocrat]

PIBD Sync is restarting every time before validation when this peer exists in the end of txhashsetpibd_download step. (PIBD Reported Failure - Restarting Sync when this peer appeared somewhere in the middle and error validating PIBD state: Invalid Root when we have him from the beginning in the logs). Seems like we need to introduce new capability PIBD_HIST_2 to receive such data only for new nodes and fix the validation.

[wiesche89]

If need_hash is true, silently skipping a missing hash can produce a segment that later fails validation with MissingHash or Invalid Root. Should this return an error instead?

like:

let hash = pmmr
    .get_from_file(pos0)
    .ok_or_else(|| SegmentError::MissingHash(pos0))?;

[wiesche89]

This also matches the failure pattern you observed, @ardocrat.

[ardocrat]

I'll check this out

[wiesche89]

Can we avoid .ok() here? If building the bitmap fails, I think this should return an error

[wiesche89]

Same here, I think bitmap errors should be returned instead of being converted to None.

[wiesche89]

Seems like we need to introduce new capability PIBD_HIST_2 to receive such data only for new nodes

@ardocrat
the segment serialisation itself is still variable sized and already includes the hash count and hash positions, so I am not fully convinced yet that this strictly requires a new capability

[ardocrat]

Seems like we need to introduce new capability PIBD_HIST_2 to receive such data only for new nodes

@ardocrat the segment serialisation itself is still variable sized and already includes the hash count and hash positions, so I am not fully convinced yet that this strictly requires a new capability

Then we'll need additional validation for segments, current validation or root validation after applying new segment is not helping as I tested