Replace the RNG criterion that latched healthy co-signers with one that catches partial fills

keep-core/src/entropy.rs

@@ -308,25 +308,65 @@ fn hamming_distance(a: &[u8; 32], b: &[u8; 32]) -> u32 {
     a.iter().zip(b).map(|(x, y)| (x ^ y).count_ones()).sum()
 }
 
-pub(crate) fn check_bit_distribution(samples: &[[u8; 32]]) -> bool {
-    (0..32).all(|pos| {
-        let all_zeros = samples.iter().all(|s| s[pos] == 0x00);
-        let all_ones = samples.iter().all(|s| s[pos] == 0xff);
-        !all_zeros && !all_ones
-    })
-}
-
 const MIN_HAMMING: u32 = 64;
 
+/// The most byte positions that may hold the same value in every sample before
+/// the samples are refused. Only meaningful across three or more samples.
+pub(crate) const MAX_FROZEN_POSITIONS: usize = 3;
+
+/// How many of the 32 byte positions hold the same value, whatever it is, in
+/// every one of `samples`.
+///
+/// This replaces a test that refused any position holding `0x00` or `0xff` in
+/// every sample, which got both its rate and its coverage wrong.
+///
+/// Its rate: that happens to healthy randomness about once in 260,000
+/// three-sample calls, measured at 12 in 3,000,000 checked draws against the
+/// analytic 11.4. Every failure latches a co-signing node, and the periodic
+/// check runs every 30 seconds, so a healthy always-on node took itself out of
+/// service about once a month.
+///
+/// Its coverage: it caught a frozen extreme byte, and any number of them, but
+/// never a frozen byte of any other value. A source that fills part of a buffer
+/// and leaves the rest as it was is the case that matters, because several
+/// draws turn bytes straight into a scalar with no hash between: the dealer's
+/// key, every polynomial coefficient, refresh and repair deltas, and the DKG
+/// proof-of-knowledge nonce. Twelve frozen bytes there leave 160 bits in known
+/// positions, about 2^80 work to recover, and the Hamming floor does not catch
+/// it until roughly half the bytes are frozen.
+///
+/// Counting positions frozen at any value, and refusing above
+/// [`MAX_FROZEN_POSITIONS`], fixes both. A position matches across three
+/// independent samples with probability 1 in 65,536, so four or more of 32
+/// doing so is a binomial tail of about 2e-15, the same order as the Hamming
+/// floor, and no healthy node latches on it. What it still lets through, one to
+/// three frozen positions, leaves at least 232 bits.
+///
+/// Three samples is the floor. Across two, a position matches with probability
+/// 1 in 256, and four of 32 matching becomes about 8e-6 per call, which is the
+/// rate this exists to get away from.
+pub(crate) fn frozen_positions(samples: &[[u8; 32]]) -> usize {
+    debug_assert!(
+        samples.len() >= 3,
+        "frozen_positions needs three samples; below that healthy input trips it"
+    );
+    let Some((first, rest)) = samples.split_first() else {
+        return 0;
+    };
+    (0..32)
+        .filter(|&pos| rest.iter().all(|s| s[pos] == first[pos]))
+        .count()
+}
+
 /// Applies the health criteria to three samples: none all-zero, all distinct,
-/// no byte position constant across all three, and every pair at least
-/// [`MIN_HAMMING`] bits apart.
+/// no more than [`MAX_FROZEN_POSITIONS`] byte positions holding the same value
+/// across all three, and every pair at least [`MIN_HAMMING`] bits apart.
 fn samples_look_random(samples: &[[u8; 32]; 3]) -> bool {
     let all_nonzero = samples.iter().all(|s| s.iter().any(|&b| b != 0));
     let all_unique =
         samples[0] != samples[1] && samples[1] != samples[2] && samples[0] != samples[2];
 
-    if !all_nonzero || !all_unique || !check_bit_distribution(samples) {
+    if !all_nonzero || !all_unique || frozen_positions(samples) > MAX_FROZEN_POSITIONS {
         return false;
     }
 
@@ -335,18 +375,15 @@ fn samples_look_random(samples: &[[u8; 32]; 3]) -> bool {
         .all(|&(i, j)| hamming_distance(&samples[i], &samples[j]) >= MIN_HAMMING)
 }
 
-/// The health criteria that two draws can carry, for a caller judging the bytes
-/// it was handed rather than a sample drawn beside them.
-///
-/// [`check_entropy_health`] takes three samples, which is what lets it also
-/// reject a byte position that is `0x00` or `0xff` across all of them. Two
-/// samples cannot carry that test: a position fails it when both draws happen
-/// to hold the same extreme byte, which for two healthy draws is about one pair
-/// in a thousand across the 32 positions, and a caller that latches on the
-/// verdict would take itself out of service about that often. What is left is
-/// what a stuck or barely-moving source fails anyway: neither draw all-zero,
+/// The health criteria for a pair of draws, for a caller judging the bytes it
+/// was handed rather than a sample drawn beside them: neither draw all-zero,
 /// the two distinct, and at least [`MIN_HAMMING`] bits apart, which a source
 /// repeating itself with a handful of bits flipped still fails.
+///
+/// These are the same criteria [`check_entropy_health`] applies to each of its
+/// pairs. The frozen-position count is not among them, because across two
+/// samples healthy randomness trips it often enough to latch a node; it needs
+/// three, see [`frozen_positions`].
 pub(crate) fn pair_looks_random(a: &[u8; 32], b: &[u8; 32]) -> bool {
     let nonzero = |s: &[u8; 32]| s.iter().any(|&b| b != 0);
     nonzero(a) && nonzero(b) && a != b && hamming_distance(a, b) >= MIN_HAMMING
@@ -391,7 +428,7 @@ fn check_entropy_health_internal() -> Result<(), EntropyHealthError> {
 /// Generates multiple samples and checks:
 /// - Output is not all zeros
 /// - Different calls produce different output
-/// - Byte positions are not all 0x00 or 0xff across samples
+/// - At most three byte positions hold the same value across every sample
 /// - Hamming distance between samples is at least 64 bits
 pub fn check_entropy_health() -> Result<(), EntropyHealthError> {
     check_entropy_health_internal()
@@ -548,6 +585,33 @@ mod tests {
         );
     }
 
+    /// The periodic check refuses a source that fills only part of each
+    /// sample, which the pairwise criteria pass: these are distinct, nonzero
+    /// and far apart, with twelve positions frozen at the same value.
+    #[test]
+    fn health_criteria_reject_a_source_filling_only_part_of_each_sample() {
+        let samples: [[u8; 32]; 3] = std::array::from_fn(|_| {
+            let mut s = [0x5au8; 32];
+            gather_os_entropy(&mut s[..20]).expect("the OS RNG is available");
+            s
+        });
+        assert!(!samples_look_random(&samples));
+
+        // The line itself, on built samples: unfrozen bytes take a different
+        // value in each sample so none can match by chance, which is what made
+        // a random version of this fail on healthy input about once in 2,260.
+        let exactly_frozen = |frozen: usize| -> [[u8; 32]; 3] {
+            std::array::from_fn(|k| {
+                let value = [0x0fu8, 0xf0, 0x33][k];
+                std::array::from_fn(|pos| if pos < frozen { 0x00 } else { value })
+            })
+        };
+        assert_eq!(frozen_positions(&exactly_frozen(3)), 3);
+        assert!(samples_look_random(&exactly_frozen(3)));
+        assert_eq!(frozen_positions(&exactly_frozen(4)), 4);
+        assert!(!samples_look_random(&exactly_frozen(4)));
+    }
+
     #[test]
     fn health_criteria_reject_a_degraded_source() {
         // What the pre-mix check catches, given the above.

keep-core/src/frost/checked_rng.rs

@@ -27,8 +27,7 @@
 //! all-zero fill of any width; two blocks closer than the Hamming floor, which
 //! is a source that moves but barely; a source cycling through a handful of
 //! values, because every block is compared against all the blocks before it
-//! and not just the one immediately before; and a source that is random except
-//! for a byte position it never moves. A call that draws fewer blocks than the
+//! and not just the one immediately before. A call that draws fewer blocks than the
 //! criteria need draws more from the same source until it has them, so a call
 //! that makes a single draw is judged rather than waved through. Bytes drawn
 //! that no whole block could be formed from are refused outright instead of
@@ -51,7 +50,9 @@
 use frost_secp256k1_tr::rand_core::{impls, CryptoRng, Error as RngError, OsRng, RngCore};
 use zeroize::{Zeroize, Zeroizing};
 
-use crate::entropy::{check_bit_distribution, pair_looks_random, EntropyHealthError};
+use crate::entropy::{
+    frozen_positions, pair_looks_random, EntropyHealthError, MAX_FROZEN_POSITIONS,
+};
 
 /// An RNG that holds every fill it hands out to the health criteria.
 ///
@@ -96,10 +97,7 @@ const MAX_RETAINED: usize = 8;
 /// Two allow exactly one comparison, which a source alternating two values
 /// passes: draw A, probe B, A against B looks healthy while the source holds
 /// one bit of entropy. Three is the smallest number that catches it, because
-/// the third block is A again and A against A fails. It also matches what
-/// [`check_entropy_health`] applies to its own samples.
-///
-/// [`check_entropy_health`]: crate::entropy::check_entropy_health
+/// the third block is A again and A against A fails.
 const MIN_JUDGED: usize = 3;
 
 impl<R: RngCore> CheckedRng<R> {
@@ -159,7 +157,8 @@ impl<R: RngCore> CheckedRng<R> {
         }
     }
 
-    /// Draw extra blocks from the same source until two have been judged.
+    /// Draw extra blocks from the same source until [`MIN_JUDGED`] have been
+    /// judged.
     ///
     /// One draw can be checked for nothing beyond being all zero, because the
     /// criteria compare one draw against another. Some calls make exactly one:
@@ -206,14 +205,15 @@ impl<R: RngCore> CheckedRng<R> {
             let mut probe = Zeroizing::new([0u8; 32]);
             self.fill_bytes(probe.as_mut());
         }
-        // The one criterion a pair cannot carry. `pair_looks_random` drops it
-        // because across two draws a byte position holds the same extreme value
-        // about once in a thousand tries, which would refuse healthy draws at
-        // that rate; across three or more it is the test that catches a source
-        // whose output is random except for a position it never moves.
+        // A source that fills only part of what it is asked for leaves the rest
+        // as it was, so the same byte positions hold the same value in every
+        // block. The pairwise checks miss that until about half the bytes are
+        // frozen, and several draws here take bytes straight into a scalar,
+        // so it is counted across every block judged. The threshold and why it
+        // does not latch a healthy node are on `frozen_positions`.
         if self.fault.is_none() && self.judged.len() >= MIN_JUDGED {
             let blocks = Zeroizing::new(self.judged.iter().map(|b| **b).collect::<Vec<_>>());
-            if !check_bit_distribution(&blocks) {
+            if frozen_positions(&blocks) > MAX_FROZEN_POSITIONS {
                 self.fault = Some(EntropyHealthError::Degraded);
             }
         }
@@ -403,12 +403,44 @@ mod tests {
         s.0 = !s.0;
     });
 
-    /// Healthy everywhere except one byte position it never moves, which is
-    /// the one failure a pair of draws cannot carry: the two are distinct, both
-    /// nonzero and far apart, so only a test across three or more blocks sees
-    /// the position that never changes.
+    /// Random except for one byte position that holds the same value in every
+    /// block. A position matching across three blocks happens to healthy
+    /// randomness once in 65,536 per position, so one or a few must not be
+    /// refused; see `frozen_positions` for where the line is.
     struct ConstantPositionRng;
 
+    /// Fills only the first `.0` bytes of every 32-byte block and leaves the
+    /// rest at `.1`, which is a source that does part of what it is asked and
+    /// is the case the frozen-position count exists for.
+    struct PartialFillRng(usize, u8);
+
+    /// Three blocks with exactly `.0` positions frozen and no others, built
+    /// rather than drawn. The unfrozen bytes take a different value in each
+    /// block, so none of them can match by chance and push the count past
+    /// `.0`, which is what made a random head flaky at the boundary. Every pair
+    /// is still nonzero, distinct and at least 112 bits apart, so the count is
+    /// the only criterion in play.
+    struct ExactlyFrozenRng(usize, usize);
+
+    impl_rng!(ExactlyFrozenRng, |s, dest| {
+        const UNFROZEN: [u8; 3] = [0x0f, 0xf0, 0x33];
+        for block in dest.chunks_mut(32) {
+            let value = UNFROZEN[s.1 % UNFROZEN.len()];
+            for (pos, byte) in block.iter_mut().enumerate() {
+                *byte = if pos < s.0 { 0x00 } else { value };
+            }
+            s.1 += 1;
+        }
+    });
+
+    impl_rng!(PartialFillRng, |s, dest| {
+        for block in dest.chunks_mut(32) {
+            let (head, tail) = block.split_at_mut(s.0.min(block.len()));
+            OsRng.fill_bytes(head);
+            tail.fill(s.1);
+        }
+    });
+
     impl_rng!(ConstantPositionRng, |_s, dest| {
         OsRng.fill_bytes(dest);
         for block in dest.chunks_mut(32) {
@@ -740,17 +772,69 @@ mod tests {
         );
     }
 
-    /// A source random except for a byte position it never moves is refused.
-    /// That position is the one criterion a pair cannot carry, so this is what
-    /// the third block buys beyond catching a repeat.
+    /// A byte position that repeats across the blocks of one call is not a
+    /// fault on its own. The test this replaces refused a single position held
+    /// at `0x00` or `0xff`, which healthy randomness does about once in 260,000
+    /// three-block calls, and since a refusal latches a co-signer, a node making
+    /// nonce commitments took itself out of service at that rate.
     #[test]
-    fn a_source_with_a_frozen_byte_position_is_refused() {
+    fn a_byte_position_repeating_across_blocks_is_not_a_fault() {
         let mut rng = CheckedRng::new(ConstantPositionRng);
         let mut block = [0u8; 32];
         for _ in 0..3 {
             rng.fill_bytes(&mut block);
         }
         rng.finish();
-        assert_eq!(rng.fault, Some(EntropyHealthError::Degraded));
+        assert_eq!(rng.fault, None);
+    }
+
+    fn verdict_of(source: impl RngCore + CryptoRng) -> Option<EntropyHealthError> {
+        let mut rng = CheckedRng::new(source);
+        let mut block = [0u8; 32];
+        for _ in 0..3 {
+            rng.fill_bytes(&mut block);
+        }
+        rng.finish();
+        rng.fault
+    }
+
+    /// A source that fills twenty bytes of each block and leaves twelve zero is
+    /// refused. Every pair is still distinct, nonzero and well past the
+    /// Hamming floor, so nothing but the frozen-position count sees it, and
+    /// before that count existed it passed. A draw that turns bytes straight
+    /// into a scalar would then hold 96 known bits.
+    #[test]
+    fn a_source_filling_only_part_of_each_block_is_refused() {
+        assert_eq!(
+            verdict_of(PartialFillRng(20, 0x00)),
+            Some(EntropyHealthError::Degraded)
+        );
+    }
+
+    /// The count is of positions frozen at any value. The test it replaces
+    /// only ever looked for `0x00` and `0xff`, so a source leaving its unfilled
+    /// bytes at anything else passed it.
+    #[test]
+    fn a_frozen_region_of_any_value_is_refused() {
+        assert_eq!(
+            verdict_of(PartialFillRng(20, 0x5a)),
+            Some(EntropyHealthError::Degraded)
+        );
+    }
+
+    /// The line is four frozen positions: three pass and four do not. Three
+    /// leaves at least 232 bits per block, and refusing at fewer would bring
+    /// back the rate the old test latched healthy nodes at.
+    ///
+    /// Built rather than drawn: with random unfrozen bytes, any one of them
+    /// matching across the three blocks by chance tips three into four, which
+    /// made this fail on a healthy source about once in 2,260 runs.
+    #[test]
+    fn three_frozen_positions_pass_and_four_are_refused() {
+        assert_eq!(verdict_of(ExactlyFrozenRng(3, 0)), None);
+        assert_eq!(
+            verdict_of(ExactlyFrozenRng(4, 0)),
+            Some(EntropyHealthError::Degraded)
+        );
     }
 }