docs(ecstore): pin streaming-only bitrot layout invariant (ECA-18) (#4553)

bitrot_shard_file_size only counts per-block checksum bytes for the two
streaming Highway variants, while BitrotWriter::write interleaves a hash
for any hash_algo.size() > 0 and bitrot_verify's read loop assumes an
interleaved hash per block. The three disagree for non-streaming
algorithms (SHA256/HighwayHash256/BLAKE2b512/Md5), but the divergence is
unreachable in production: every write path hardcodes HighwayHash256S and
ErasureInfo::get_checksum_info defaults to HighwayHash256S.

Per the audit decision (backlog#959), do NOT change the size formula:
it is a byte-for-byte port of MinIO's bitrotShardFileSize and its bare
return for non-streaming algorithms is correct for MinIO whole-file
bitrot; changing it would break legacy interop. Instead, document the
per-algorithm layout contract at bitrot_shard_file_size, BitrotWriter,
and bitrot_verify, and add regression tests that pin the invariants:
get_checksum_info defaults to HighwayHash256S, and the size formula
counts per-block hash bytes for streaming variants only while returning
the bare size for non-streaming ones. No disk layout or formula change.

Co-authored-by: heihutu <heihutu@gmail.com>

crates/ecstore/src/erasure/coding/bitrot.rs

@@ -201,6 +201,16 @@ where
 
         let hash_algo = &self.hash_algo;
 
+        // Interleaved per-block bitrot: prepend the block's hash so the on-disk
+        // block is `[hash][data]`. This `size() > 0` condition is broader than
+        // the streaming-only condition in `bitrot_shard_file_size`, so it is
+        // only self-consistent for the two streaming Highway variants
+        // (`HighwayHash256S` / `HighwayHash256SLegacy`) — the only algorithms
+        // production ever uses here (backlog#959 / ECA-18). For non-streaming
+        // algorithms MinIO uses whole-file bitrot with no interleaved hash, so
+        // driving one through this writer would produce a file whose length
+        // disagrees with `bitrot_shard_file_size` and fail `bitrot_verify`; do
+        // not feed a non-streaming algorithm here without a separate path.
         if hash_algo.size() > 0 {
             let hash = hash_algo.hash_encode(buf);
             if hash.as_ref().is_empty() {
@@ -272,13 +282,52 @@ where
     Ok(())
 }
 
+/// On-disk size of a shard file for a part of `size` data bytes.
+///
+/// This is a byte-for-byte port of MinIO's `bitrotShardFileSize` and encodes
+/// MinIO's per-algorithm bitrot layout, NOT a uniform "one hash per block" rule:
+///
+/// - `HighwayHash256S` / `HighwayHash256SLegacy` are the *streaming* variants.
+///   They use interleaved per-block bitrot: every `shard_size` block on disk is
+///   `[hash][data]`, so the file carries `ceil(size / shard_size)` extra hashes.
+/// - Every other algorithm (`SHA256`, `HighwayHash256`, `BLAKE2b512`, `Md5`,
+///   `None`) maps to MinIO's *whole-file* bitrot for legacy V1 objects: the hash
+///   lives in xl.meta, not interleaved on disk, so the on-disk file is exactly
+///   `size` bytes. Returning the bare `size` here is therefore correct, not a
+///   bug — adding per-block hash bytes would make this guard reject genuine
+///   legacy whole-file-bitrot parts and break MinIO interop.
+///
+/// INVARIANT (backlog#959 / ECA-18): this crate only ever writes and verifies
+/// the *streaming* per-block layout. `BitrotWriter::write` interleaves a hash on
+/// any `hash_algo.size() > 0`, and `bitrot_verify`'s read loop assumes an
+/// interleaved hash per block; both are only consistent with THIS function for
+/// the two streaming Highway variants. That is safe because every production
+/// write path hardcodes `HighwayHash256S` and `ErasureInfo::get_checksum_info`
+/// defaults to `HighwayHash256S` (see the regression tests below and in
+/// rustfs-filemeta). The non-streaming branches of this function exist purely to
+/// preserve the MinIO formula's whole-file semantics; feeding a non-streaming
+/// algorithm through `BitrotWriter` + `bitrot_verify` is unsupported and would
+/// mismatch this size — do not wire one in without a dedicated whole-file path.
 pub fn bitrot_shard_file_size(size: usize, shard_size: usize, algo: HashAlgorithm) -> usize {
     if algo != HashAlgorithm::HighwayHash256S && algo != HashAlgorithm::HighwayHash256SLegacy {
+        // Non-streaming (whole-file bitrot) algorithms carry no interleaved
+        // per-block hashes on disk; the on-disk file is exactly `size` bytes.
         return size;
     }
+    // Streaming Highway variants: one hash is interleaved before every block.
     size.div_ceil(shard_size) * algo.size() + size
 }
 
+/// Verify an interleaved per-block bitrot shard file.
+///
+/// The read loop below assumes every block on disk is `[hash][data]` (streaming
+/// bitrot). It is therefore only valid for the streaming Highway variants, whose
+/// on-disk length matches `bitrot_shard_file_size` — production always uses
+/// `HighwayHash256S` (backlog#959 / ECA-18). Passing a non-streaming algorithm
+/// (`SHA256` / `HighwayHash256` / `BLAKE2b512` / `Md5`) is unsupported: MinIO
+/// stores those as whole-file bitrot with no interleaved hash, so the size guard
+/// on the next line would reject a genuinely healthy part. Reading legacy V1
+/// whole-file-bitrot objects would need a separate verification path.
 #[cfg_attr(feature = "hotpath", hotpath::measure)]
 pub async fn bitrot_verify<R: AsyncRead + Unpin + Send>(
     mut r: R,
@@ -487,6 +536,7 @@ mod tests {
 
     use super::BitrotReader;
     use super::BitrotWriter;
+    use super::bitrot_shard_file_size;
     use rustfs_utils::HashAlgorithm;
     use std::io::{Cursor, IoSlice};
     use std::sync::{
@@ -832,6 +882,101 @@ mod tests {
         assert!(reads.load(Ordering::SeqCst) > 1, "partial reads should loop");
     }
 
+    // --- backlog#959 / ECA-18 invariant guards -------------------------------
+    //
+    // These tests pin the *intended* per-algorithm bitrot layout so a future
+    // change can't silently drift `bitrot_shard_file_size`, `BitrotWriter`, and
+    // `bitrot_verify` out of agreement. The design contract (see the doc
+    // comments above): only the two streaming Highway variants use interleaved
+    // per-block `[hash][data]` layout; every other algorithm maps to MinIO
+    // whole-file bitrot and its shard file is exactly `size` bytes on disk.
+
+    #[test]
+    fn bitrot_shard_file_size_counts_hash_only_for_streaming_variants() {
+        // For a range of sizes (including exact multiples and non-multiples of
+        // shard_size), streaming variants add one hash per block; all others
+        // return the bare `size`.
+        let shard_size = 16usize;
+        let hash = 32usize; // both streaming Highway variants are 32 bytes
+        for &size in &[0usize, 1, 15, 16, 17, 31, 32, 33, 100, 160] {
+            let blocks = size.div_ceil(shard_size);
+
+            for algo in [HashAlgorithm::HighwayHash256S, HashAlgorithm::HighwayHash256SLegacy] {
+                assert_eq!(
+                    bitrot_shard_file_size(size, shard_size, algo.clone()),
+                    size + blocks * hash,
+                    "streaming variant {algo:?} must count per-block hash bytes (size={size})"
+                );
+            }
+
+            for algo in [
+                HashAlgorithm::SHA256,
+                HashAlgorithm::HighwayHash256,
+                HashAlgorithm::BLAKE2b512,
+                HashAlgorithm::Md5,
+                HashAlgorithm::None,
+            ] {
+                assert_eq!(
+                    bitrot_shard_file_size(size, shard_size, algo.clone()),
+                    size,
+                    "non-streaming variant {algo:?} must return the bare on-disk size (size={size})"
+                );
+            }
+        }
+    }
+
+    #[tokio::test]
+    async fn bitrot_shard_file_size_matches_streaming_writer_output() {
+        // The size formula for streaming variants must equal the byte count that
+        // BitrotWriter actually writes to disk, across integral and partial
+        // final blocks — this is the invariant `bitrot_verify`'s size guard
+        // relies on.
+        let shard_size = 16usize;
+        for algo in [HashAlgorithm::HighwayHash256S, HashAlgorithm::HighwayHash256SLegacy] {
+            for &size in &[1usize, 16, 17, 32, 40, 48] {
+                let payload: Vec<u8> = (0..size).map(|i| i as u8).collect();
+                let mut w = BitrotWriter::new(Cursor::new(Vec::<u8>::new()), shard_size, algo.clone());
+                for chunk in payload.chunks(shard_size) {
+                    w.write(chunk).await.unwrap();
+                }
+                let on_disk = w.into_inner().into_inner().len();
+                assert_eq!(
+                    on_disk,
+                    bitrot_shard_file_size(size, shard_size, algo.clone()),
+                    "writer output must match size formula ({algo:?}, size={size})"
+                );
+            }
+        }
+    }
+
+    #[test]
+    fn streaming_variants_are_the_only_per_block_bitrot_algorithms() {
+        // Documents the known, intentional divergence at the heart of ECA-18:
+        // BitrotWriter interleaves a hash whenever `size() > 0`, but the shard
+        // size formula only counts hash bytes for the streaming variants. The
+        // two conditions agree ONLY for the streaming variants; this test locks
+        // that boundary so nobody "fixes" one side without the other. If a new
+        // algorithm is added, this test forces an explicit decision here.
+        let shard_size = 16usize;
+        let size = 40usize; // spans multiple blocks
+        for algo in [
+            HashAlgorithm::SHA256,
+            HashAlgorithm::HighwayHash256,
+            HashAlgorithm::HighwayHash256S,
+            HashAlgorithm::HighwayHash256SLegacy,
+            HashAlgorithm::BLAKE2b512,
+            HashAlgorithm::Md5,
+            HashAlgorithm::None,
+        ] {
+            let streaming = matches!(algo, HashAlgorithm::HighwayHash256S | HashAlgorithm::HighwayHash256SLegacy);
+            let formula_counts_hash = bitrot_shard_file_size(size, shard_size, algo.clone()) > size;
+            assert_eq!(
+                formula_counts_hash, streaming,
+                "only streaming Highway variants may count per-block hash bytes ({algo:?})"
+            );
+        }
+    }
+
     #[tokio::test]
     async fn hashed_read_truncated_within_hash_is_unexpected_eof() {
         // Truncation that lands inside the leading hash (not the data) must still

crates/filemeta/src/fileinfo.rs

@@ -715,6 +715,38 @@ mod tests {
     use proptest::collection::{hash_map, vec};
     use proptest::prelude::*;
 
+    // backlog#959 / ECA-18: the interleaved per-block bitrot subsystem in
+    // rustfs-ecstore (BitrotWriter / bitrot_verify / bitrot_shard_file_size) is
+    // only self-consistent for the streaming Highway variants. That safety rests
+    // on production always resolving a streaming checksum algorithm, so pin the
+    // default here: a part with no explicit ChecksumInfo must resolve to
+    // HighwayHash256S. If this default ever changes, the ecstore bitrot layout
+    // assumptions must be revisited in lockstep.
+    #[test]
+    fn get_checksum_info_defaults_to_highwayhash256s() {
+        let ei = ErasureInfo::default();
+        assert!(ei.checksums.is_empty(), "default ErasureInfo carries no checksums");
+        let info = ei.get_checksum_info(1);
+        assert_eq!(
+            info.algorithm,
+            HashAlgorithm::HighwayHash256S,
+            "missing ChecksumInfo must default to the streaming HighwayHash256S algorithm"
+        );
+
+        // A present entry is returned as-is; the default only applies on miss.
+        let ei = ErasureInfo {
+            checksums: vec![ChecksumInfo {
+                part_number: 2,
+                algorithm: HashAlgorithm::SHA256,
+                hash: Bytes::new(),
+            }],
+            ..Default::default()
+        };
+        assert_eq!(ei.get_checksum_info(2).algorithm, HashAlgorithm::SHA256);
+        // A part_number with no entry still falls back to the streaming default.
+        assert_eq!(ei.get_checksum_info(99).algorithm, HashAlgorithm::HighwayHash256S);
+    }
+
     // backlog#949: distribution range/permutation validation.
     #[test]
     fn is_valid_distribution_accepts_permutation() {