freenet
diff --git a/‎Cargo.lock‎
Lines changed: 1 addition & 0 deletions b/‎Cargo.lock‎
Lines changed: 1 addition & 0 deletions
diff --git a/‎common/src/mailbox.rs‎
Lines changed: 52 additions & 2 deletions b/‎common/src/mailbox.rs‎
Lines changed: 52 additions & 2 deletions
diff --git a/‎common/src/reputation.rs‎
Lines changed: 169 additions & 3 deletions b/‎common/src/reputation.rs‎
Lines changed: 169 additions & 3 deletions
diff --git a/‎contracts/reputation-contract/Cargo.toml‎
Lines changed: 12 additions & 0 deletions b/‎contracts/reputation-contract/Cargo.toml‎
Lines changed: 12 additions & 0 deletions
diff --git a/‎contracts/reputation-contract/src/lib.rs‎
Lines changed: 94 additions & 0 deletions b/‎contracts/reputation-contract/src/lib.rs‎
Lines changed: 94 additions & 0 deletions
@@ -1,7 +1,7 @@
 use chrono::{DateTime, Utc};
 use ed25519_dalek::VerifyingKey;
 use serde::{Deserialize, Serialize};
-use std::collections::HashSet;
+use std::collections::{BTreeSet, HashSet};
 
 /// How many messages one mailbox contract will hold.
 ///
@@ -627,7 +627,17 @@ pub fn entry_digest(message: &EncryptedMessage) -> [u8; 32] {
 /// delegate store on receipt AND paying only after the write is confirmed,
 /// which is recorded in `docs/buyer-conversation-persistence.md` and not built
 /// here.
-pub type MailboxSummaryV2 = HashSet<[u8; 32]>;
+/// ---
+///
+/// **`BTreeSet`, not `HashSet`.** A summary is encoded and sent, so its bytes
+/// must be a function of its contents alone. `RandomState::new` bumps a
+/// per-thread counter on every construction, and `summarize` below rebuilds
+/// the set with `collect()` on every call, so under a `HashSet` this encoded
+/// differently each time for the same messages. Pinned by
+/// `mailbox_summary_encoding_is_deterministic`. See
+/// [`crate::reputation::ReputationStateV1::used_nonces`] for why the mechanism
+/// is a counter rather than randomness, which is what it is on wasm32.
+pub type MailboxSummaryV2 = BTreeSet<[u8; 32]>;
 
 /// Delta: new messages to add. Unchanged in shape -- it always carried whole
 /// messages, and only what counts as "already held" moved.
@@ -1217,6 +1227,46 @@ mod determinism_tests {
         );
     }
 
+    /// **A summary must encode as a function of its contents alone.**
+    ///
+    /// The two tests above pin the STATE bytes and were the only determinism
+    /// guards here; the SUMMARY had none, and [`MailboxSummaryV2`] was a
+    /// `HashSet` -- which draws a fresh random seed per instance, so it
+    /// encoded differently on every call for the same contents.
+    ///
+    /// [`MailboxStateV1::summarize`] rebuilds the set with `collect()` each
+    /// time, so under the defect even summarising ONE state twice differed.
+    /// This still uses two independently-built states, to match the shape the
+    /// reputation contract is forced into -- there `summarize` returns a
+    /// `clone()`, and cloning a `HashSet` copies its hasher, so the
+    /// same-state form is green under the defect and pins nothing. Keeping
+    /// one shape across both files means the weaker form cannot be copied
+    /// here by someone reading this as the template.
+    #[test]
+    fn mailbox_summary_encoding_is_deterministic() {
+        let base = 1_700_000_000;
+        let forward: Vec<_> = (0..12u32).map(|i| indexed(i, base + i as i64)).collect();
+        let backward: Vec<_> = forward.iter().rev().cloned().collect();
+
+        let mut a = MailboxStateV1::default();
+        a.apply_delta(&Some(forward)).unwrap();
+
+        let mut b = MailboxStateV1::default();
+        b.apply_delta(&Some(backward)).unwrap();
+
+        assert_eq!(
+            a.summarize(),
+            b.summarize(),
+            "the two mailboxes must hold the same digests, or this test is \
+             measuring the wrong thing"
+        );
+        assert_eq!(
+            crate::to_cbor(&a.summarize()).unwrap(),
+            crate::to_cbor(&b.summarize()).unwrap(),
+            "two peers holding the same messages must send the same summary bytes"
+        );
+    }
+
     /// Age is not a reason to drop anything any more. The message here is from
     /// the epoch and the mailbox's other traffic is from 2023; under the TTL
     /// rule this replaced, the old one was discarded.
 
@@ -1,7 +1,7 @@
 use chrono::{DateTime, Utc};
 use ed25519_dalek::VerifyingKey;
 use serde::{Deserialize, Serialize};
-use std::collections::HashSet;
+use std::collections::{BTreeSet, HashSet};
 
 use crate::feedback::{FeedbackCategory, FeedbackToken};
 
@@ -57,11 +57,42 @@ pub struct ReputationStateV1 {
     /// Append-only list of negative feedback entries.
     pub feedback: Vec<FeedbackEntry>,
     /// Used nonces for replay prevention (mirrors nonces from feedback entries).
-    pub used_nonces: HashSet<[u8; 32]>,
+    ///
+    /// **`BTreeSet`, not `HashSet`, and that is load-bearing.** This field is
+    /// part of the contract STATE, so its CBOR encoding is bytes peers compare,
+    /// and it has to be a function of the contents alone.
+    ///
+    /// The mechanism is a COUNTER, not randomness, and the distinction matters
+    /// because this contract runs on `wasm32-unknown-unknown`. `RandomState::
+    /// new` caches two keys per thread and does `keys.set((k0.wrapping_add(1),
+    /// k1))` on every construction, precisely so each `HashMap` gets a
+    /// different iteration order. That bump is target-independent. What IS
+    /// target-specific is the base: on wasm32 `std` routes to
+    /// `sys::random::unsupported`, whose `hashmap_random_keys` returns a stack
+    /// address and a heap address with the comment "this isn't particularly
+    /// secure, but there isn't really an alternative". So there is no entropy
+    /// on the target at all.
+    ///
+    /// Do not read that as "so it was deterministic anyway and this change was
+    /// unnecessary". Two peers differ in how many sets they have built, which
+    /// is their operation history, so they still encode differently. The
+    /// counter is the whole defect and it survives having no entropy.
+    ///
+    /// Note what is NOT true: two `to_cbor` calls on ONE `HashSet` value give
+    /// identical bytes. Only independently-built instances diverge, which is
+    /// the shape `used_nonces_encoding_is_deterministic` uses.
+    ///
+    /// `feedback` below is sorted for exactly this reason; a `HashSet` beside
+    /// it silently spent that sort.
+    pub used_nonces: BTreeSet<[u8; 32]>,
 }
 
 /// Summary for delta computation: the set of known nonces.
-pub type ReputationSummary = HashSet<[u8; 32]>;
+///
+/// `BTreeSet` for the same reason as [`ReputationStateV1::used_nonces`]: a
+/// summary is encoded and sent, so its bytes must be a function of its
+/// contents alone.
+pub type ReputationSummary = BTreeSet<[u8; 32]>;
 
 /// Delta: new feedback entries to add.
 pub type ReputationDelta = Vec<FeedbackEntry>;
@@ -410,4 +441,139 @@ mod tests {
              correct the commutativity claim on `ReputationStateV1`"
         );
     }
+
+    /// **Two peers given the same feedback in different orders must hold
+    /// byte-identical state.**
+    ///
+    /// This is the property `mailbox::determinism_tests::
+    /// merging_is_order_independent` pins for the mailbox. Reputation had no
+    /// equivalent -- and reputation is the contract whose STATE actually held
+    /// a nondeterministically-encoded collection, so the guard existed on the
+    /// one of the two that did not need it.
+    ///
+    /// Red before `used_nonces` became a `BTreeSet`. `RandomState::new` bumps
+    /// a per-thread counter on EVERY construction, so two independently-built
+    /// `HashSet`s iterate differently and therefore CBOR-encode differently.
+    /// `apply_delta` already sorts `feedback` "deterministically by nonce for
+    /// CRDT convergence"; the set beside it silently spent that sort.
+    ///
+    /// 32 nonces, not a handful: with only a few members two `HashSet`s can
+    /// coincidentally agree on an order and the guard passes by luck. RSA
+    /// signing dominates the runtime either way.
+    #[test]
+    fn used_nonces_encoding_is_deterministic() {
+        let (private, params) = key_pair();
+        let entries: Vec<_> = (1u8..33).map(|n| signed_entry(&private, n)).collect();
+        let reversed: Vec<_> = entries.iter().rev().cloned().collect();
+
+        let mut forward = ReputationStateV1::default();
+        forward.apply_delta(&params, &Some(entries)).expect("apply");
+
+        let mut backward = ReputationStateV1::default();
+        backward
+            .apply_delta(&params, &Some(reversed))
+            .expect("apply");
+
+        assert_eq!(
+            forward.used_nonces, backward.used_nonces,
+            "the two states must hold the same nonces, or this test is \
+             measuring the wrong thing"
+        );
+        assert_eq!(
+            crate::to_cbor(&forward).expect("encode"),
+            crate::to_cbor(&backward).expect("encode"),
+            "the same feedback in a different order must produce identical bytes"
+        );
+    }
+
+    /// **State written by a predecessor generation still decodes, and
+    /// normalises on the way in.**
+    ///
+    /// `legacy/reputation_contract.toml`'s V10 row asserts this, and a
+    /// registry claim that is only reasoned about is how a migration seals
+    /// over data.
+    ///
+    /// **The decode target is the real type, and that is the whole test.** An
+    /// earlier version of this test decoded into a `BTreeSet` it named in its
+    /// own body, so no change to the production types could ever move it: it
+    /// tested `ciborium` and `std`. It was green under every revert, including
+    /// the full one. Review caught it. If you edit this test, keep
+    /// [`ReputationStateV1`] as the thing being decoded INTO.
+    ///
+    /// V9 wrote `used_nonces` as a `HashSet`, whose CBOR is a plain array
+    /// (major type 4) in whatever order that instance chose, so both orders
+    /// below are valid V9 bytes for the same state. It re-encodes rather than
+    /// only comparing the decoded values, because `PartialEq` on the struct
+    /// compares sets and passes under the defect; the bytes are the claim.
+    #[test]
+    fn predecessor_state_decodes_through_the_real_type_and_normalises() {
+        /// V9's on-the-wire shape: `used_nonces` as a bare CBOR array.
+        #[derive(serde::Serialize)]
+        struct V9State {
+            owner_certificate_pem: String,
+            feedback: Vec<FeedbackEntry>,
+            used_nonces: Vec<[u8; 32]>,
+        }
+
+        let ascending: Vec<[u8; 32]> = (1u8..33).map(|n| [n; 32]).collect();
+        let descending: Vec<[u8; 32]> = ascending.iter().rev().copied().collect();
+
+        let v9 = |used_nonces: Vec<[u8; 32]>| V9State {
+            owner_certificate_pem: String::new(),
+            feedback: Vec::new(),
+            used_nonces,
+        };
+
+        let a: ReputationStateV1 =
+            crate::from_cbor(&crate::to_cbor(&v9(ascending)).expect("encode")).expect("decode");
+        let b: ReputationStateV1 =
+            crate::from_cbor(&crate::to_cbor(&v9(descending)).expect("encode")).expect("decode");
+
+        assert_eq!(
+            a, b,
+            "the same members in a different order are the same state"
+        );
+        assert_eq!(
+            crate::to_cbor(&a).expect("encode"),
+            crate::to_cbor(&b).expect("encode"),
+            "predecessor bytes in any order must fold to one canonical encoding"
+        );
+    }
+
+    /// **Documentary, not a guard, and labelled so deliberately.**
+    ///
+    /// [`ReputationStateV1::summarize`] returns `used_nonces.clone()`, so the
+    /// field type and the summary alias must agree or the crate does not
+    /// compile. There is therefore NO revert under which
+    /// `used_nonces_encoding_is_deterministic` passes and this one fails, and
+    /// it should not be counted as separate coverage.
+    ///
+    /// It is kept for the trap it records. Cloning a `HashSet` copies its
+    /// hasher along with its contents, so summarising ONE state twice yields
+    /// identical bytes even under the defect: the obvious form of this test is
+    /// vacuous here. Measured, not assumed. The mailbox is not like this --
+    /// its `summarize` rebuilds with `collect()`, drawing a fresh key each
+    /// call -- so the same shape is a real guard there and a note here. Both
+    /// files use the two-independent-states form so the weaker one cannot be
+    /// copied from either.
+    #[test]
+    fn the_summary_encodes_the_same_for_two_independently_built_states() {
+        let (private, params) = key_pair();
+        let entries: Vec<_> = (1u8..33).map(|n| signed_entry(&private, n)).collect();
+        let reversed: Vec<_> = entries.iter().rev().cloned().collect();
+
+        let mut forward = ReputationStateV1::default();
+        forward.apply_delta(&params, &Some(entries)).expect("apply");
+
+        let mut backward = ReputationStateV1::default();
+        backward
+            .apply_delta(&params, &Some(reversed))
+            .expect("apply");
+
+        assert_eq!(
+            crate::to_cbor(&forward.summarize()).expect("encode"),
+            crate::to_cbor(&backward.summarize()).expect("encode"),
+            "two peers holding the same nonces must send the same summary bytes"
+        );
+    }
 }
@@ -15,6 +15,18 @@ rsa = { workspace = true }
 sha2 = { workspace = true }
 getrandom = { workspace = true }
 
+[dev-dependencies]
+# The contract's entry points take `ReputationParameters`, which can only be
+# built through `ReputationParameters::new` from a real `VerifyingKey` -- that
+# is the single construction site on purpose, because the field set is hashed
+# into the reputation contract's address. A test therefore needs a key, and
+# only a test does. Same reasoning, and same dependency, as
+# `contracts/mailbox-contract`.
+#
+# A dev-dependency is not linked into the `cdylib`, so this does not move the
+# code hash. Verified with `cargo make code-hashes` rather than assumed.
+ed25519-dalek = { workspace = true }
+
 [lib]
 crate-type = ["cdylib", "rlib"]
 
 
@@ -145,3 +145,97 @@ impl ContractInterface for Contract {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// `summarize_state` DECODES the state before summarizing it, which is why
+    /// the tests in `harvest-common` do not reach the sharpest form of the
+    /// encoding defect: they build state in-process, one layer below here.
+    ///
+    /// Every call through this entry point decodes the same bytes into a fresh
+    /// collection. Under a `HashSet` that meant one node, holding one
+    /// unchanging state, answered two `summarize_state` calls with DIFFERENT
+    /// summary bytes -- because `RandomState::new` bumps a per-thread key on
+    /// every construction, so each decode produced a differently-ordered set.
+    ///
+    /// This crate had no tests at all before this one, which is why the gap
+    /// existed. Found by review of #54, not by CI.
+    fn parameters() -> Parameters<'static> {
+        let owner = ed25519_dalek::SigningKey::from_bytes(&[4u8; 32]).verifying_key();
+        // `summarize_state` discards the decoded parameters, so only the SHAPE
+        // has to decode; this DER is never parsed.
+        let params = ReputationParameters::new(vec![9u8; 32], owner);
+        let mut bytes = vec![];
+        into_writer(&params, &mut bytes).expect("encode parameters");
+        Parameters::from(bytes)
+    }
+
+    /// 32 nonces, not a handful: two small collections can agree on an order
+    /// by luck, which would make the guard below pass without meaning to.
+    fn encoded_state() -> State<'static> {
+        let mut state = ReputationStateV1::default();
+        for i in 1u8..33 {
+            state.used_nonces.insert([i; 32]);
+        }
+        let mut bytes = vec![];
+        into_writer(&state, &mut bytes).expect("encode state");
+        State::from(bytes)
+    }
+
+    /// **One node, one unchanging state, two calls: the same summary bytes.**
+    ///
+    /// This is the form the network actually exercises, and it is the one that
+    /// produced the freenet-core incident this class of defect is known for --
+    /// a contract whose `summarize_state` serialized a hash collection emitted
+    /// different summary bytes for the same logical state, so anti-entropy
+    /// could never agree with itself.
+    #[test]
+    fn summarize_state_is_byte_stable_across_calls_on_one_state() {
+        let state = encoded_state();
+
+        let first = <Contract as ContractInterface>::summarize_state(parameters(), state.clone())
+            .expect("summarize");
+        let second = <Contract as ContractInterface>::summarize_state(parameters(), state)
+            .expect("summarize");
+
+        assert_eq!(
+            first.as_ref(),
+            second.as_ref(),
+            "two summaries of one unchanged state must be byte-identical"
+        );
+    }
+
+    /// The same property across two peers: the same members reached by
+    /// different insertion orders must summarize to the same bytes through the
+    /// entry point, not merely through the in-process helper.
+    #[test]
+    fn two_peers_holding_the_same_nonces_summarize_identically() {
+        let mut ascending = ReputationStateV1::default();
+        for i in 1u8..33 {
+            ascending.used_nonces.insert([i; 32]);
+        }
+        let mut descending = ReputationStateV1::default();
+        for i in (1u8..33).rev() {
+            descending.used_nonces.insert([i; 32]);
+        }
+
+        let encode = |s: &ReputationStateV1| {
+            let mut bytes = vec![];
+            into_writer(s, &mut bytes).expect("encode state");
+            State::from(bytes)
+        };
+
+        let a = <Contract as ContractInterface>::summarize_state(parameters(), encode(&ascending))
+            .expect("summarize");
+        let b = <Contract as ContractInterface>::summarize_state(parameters(), encode(&descending))
+            .expect("summarize");
+
+        assert_eq!(
+            a.as_ref(),
+            b.as_ref(),
+            "two peers holding the same nonces must send the same summary bytes"
+        );
+    }
+}