diff --git a/.claude/rules/contract-summary-determinism.md b/.claude/rules/contract-summary-determinism.md index 86f95bd2b..d8da9879b 100644 --- a/.claude/rules/contract-summary-determinism.md +++ b/.claude/rules/contract-summary-determinism.md @@ -53,6 +53,47 @@ changes → follow the room-contract + delegate migration ritual (`.claude/rules/delegate-migration.md`) before publishing, and bump the `river-core` / `riverctl` versions if a WASM changed. +## Summary VALUES are a wire-format commitment too + +Determinism is about the collection type; this section is about what goes in it. +A summary value that is only ever compared (never verified, never decoded back +into anything) should be a fixed-width digest, not the thing it fingerprints — +the summary is re-sent to every interested peer on every state change, so a +64-byte signature per entry is paid over and over. + +When a summary carries a digest, four properties become wire format, and none of +them may change without re-keying the contract: + +- which hash function (and it must be cryptographic if an attacker can choose + the input — a base-31 polynomial like `freenet_scaffold::util::fast_hash` is + fine for accidental collisions only); +- how wide, judged against who controls the colliding inputs. If a party can + grind BOTH sides of the comparison, 64 bits is a ~2^32 birthday search, i.e. + hours; use 128. +- which bytes are kept, and in what order; +- how the value serializes — a `[u8; 16]` through the serde derive emits a + 16-element CBOR array (~32 bytes for random content, since each byte >= 24 + costs two), not a byte string (17). Write `Serialize` by hand with + `serialize_bytes`. + +Pin all four with a **golden vector**: ONE fixed input, ONE fixed expected +digest, ONE fixed expected encoding. Oracles that compare digests of randomly +generated keys are NOT sufficient — a byte-order change leaves them agreeing +some of the time, so they detect it only intermittently. Measured twice on this +codebase with the digest reversed: 11 of 30 runs missed it in one sample, 1 of 12 +in an independent reproduction. The exact rate depends on the keys drawn and is +not the point; a non-zero miss rate makes it a coin flip rather than a check. See +`sig_digest_golden_vector` in `common/src/room_state/member_info.rs`. + +Also assert bytes-per-entry for the summary, built by calling the real +`summarize()` with realistic key values — `MemberId(FastHash(i))` for small `i` +encodes in 1-3 bytes against a real key's ~9 and understates the entry by ~30%. +**Measure the OLD shape in the same test, rebuilt from the same records**, rather +than quoting a per-entry figure in prose. A size claim is the whole justification +for a summary change, and a derived byte count is exactly the kind of number that +survives review while being wrong (see the encoding trap in History below). See +`member_info_summary_stays_small_per_entry`. + ## History - **freenet/river** (2026-07): `MemberInfoV1::Summary` was @@ -62,4 +103,24 @@ changes → follow the room-contract + delegate migration ritual `BTreeMap`/`BTreeSet`. `bincode` (the old wire path) doesn't care about key order, so this survived undetected until freenet-core added the summary-byte-compare staleness check. +- **freenet/river#571** (2026-07): the same summary's VALUE then shrank, + `(u32, Signature)` → `(u32, SigDigest)`, where `SigDigest` is a 128-bit BLAKE3 + digest of the signature serialized as a CBOR byte string. Measured 134.08 → + 28.01 bytes/entry at 470 records, a 4.8x reduction. The collection type was + already `BTreeMap` and did not change, so this is the value-side rule above + rather than the determinism rule. + `DirectMessagesSummary.message_signatures: BTreeSet` still + carries raw signatures and has the same fix available — but at 66 bytes each, + not 134, because it uses River's `SignatureBytes` newtype rather than + `ed25519::Signature` (see the next bullet). +- **The same 64 bytes have two very different CBOR encodings, and the wrong one + was quoted for months.** `ed25519::Signature::serialize` calls + `serialize_tuple(64)`; ciborium maps a tuple to a CBOR ARRAY, where each + uniformly random byte costs 2 bytes whenever it is >= 24 — so ~124 bytes. + River's own `SignatureBytes` newtype calls `serialize_bytes`, giving a CBOR + byte string at 66. The 66 figure was carried through issue #571, PR #572's + body, and a review, applied to a summary that used `ed25519::Signature`. It + produced an arithmetic that could not close (470 x 66 exceeded the stated + total) and it understated the win by nearly half. **Measure the encoding in a + test against the real type; do not derive it from the byte count.** - **freenet/freenet-core#4857** — the update-drop divergence this feeds. diff --git a/Cargo.lock b/Cargo.lock index 8810c4fa5..d23fea066 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4703,7 +4703,7 @@ dependencies = [ [[package]] name = "river-core" -version = "0.1.18" +version = "0.1.19" dependencies = [ "aes-gcm", "base64 0.22.1", @@ -4730,7 +4730,7 @@ dependencies = [ [[package]] name = "river-ui" -version = "0.1.18" +version = "0.1.19" dependencies = [ "aes-gcm", "blake3", @@ -4766,7 +4766,7 @@ dependencies = [ [[package]] name = "riverctl" -version = "0.2.9" +version = "0.2.10" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index c9323d683..20eee52c6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -64,7 +64,7 @@ freenet-scaffold-macro = "0.2.2" freenet-stdlib = { version = "0.8.5", features = ["contract"] } [workspace.package] -version = "0.1.18" +version = "0.1.19" edition = "2021" [profile.release] diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 33d249cc1..6ecd933f4 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "riverctl" -version = "0.2.9" +version = "0.2.10" edition = "2021" authors = ["Freenet Project"] description = "Command-line interface for River decentralized chat on Freenet" @@ -50,7 +50,7 @@ dialoguer = "0.11" atty = "0.2" # Internal dependencies -river-core = { version = "=0.1.18", path = "../common", features = ["ecies", "ecies-randomized", "migration", "mentions"] } +river-core = { version = "=0.1.19", path = "../common", features = ["ecies", "ecies-randomized", "migration", "mentions"] } freenet-stdlib = { workspace = true, features = ["net"] } freenet-scaffold = "0.2.2" # Sans-IO backward-probe decision driver (freenet/river#398 phase 2b): drives diff --git a/cli/contracts/room_contract.wasm b/cli/contracts/room_contract.wasm index ecee7e8f7..d1f0f5e3a 100755 Binary files a/cli/contracts/room_contract.wasm and b/cli/contracts/room_contract.wasm differ diff --git a/common/legacy_room_contracts.toml b/common/legacy_room_contracts.toml index 05c7168a0..7b393be8d 100644 --- a/common/legacy_room_contracts.toml +++ b/common/legacy_room_contracts.toml @@ -217,3 +217,9 @@ version = "V30" description = "Before the global direct-message retention cap (freenet/river#519): last generation whose DirectMessagesV1 had no whole-set bound, so every DM participant was pinned as a room member forever" date = "2026-07-27" code_hash = "f8cca7600a63dac16de1974e08211e3eb6e530713a8cfe78caed3a66372a3e50" + +[[entry]] +version = "V31" +description = "Before the member_info summary digest (freenet/river#571): last generation whose MemberInfoV1::Summary carried a raw ed25519 Signature per member and broke equal-version ties on raw signature bytes" +date = "2026-07-30" +code_hash = "dd63bcc974a6e4ab9aed2fa05e8a1085713ff69d0a160f4a487551c51c1a9d0f" diff --git a/common/src/migration.rs b/common/src/migration.rs index fae75924d..bcf9c555e 100644 --- a/common/src/migration.rs +++ b/common/src/migration.rs @@ -106,10 +106,11 @@ mod tests { for hash in LEGACY_ROOM_CONTRACT_CODE_HASHES { hasher.update(hash); } - // V30 registers the pre-global-DM-cap generation (freenet/river#519, - // the whole-set bound on `direct_messages`), which re-keys the contract. - assert_eq!(LEGACY_ROOM_CONTRACT_CODE_HASHES.len(), 30); - assert_eq!(&hasher.finalize().to_hex()[..16], "fc9e2622d9fa7d6f"); + // V31 registers the pre-summary-digest generation (freenet/river#571, + // `MemberInfoV1::Summary` carrying a raw `Signature` per member), which + // re-keys the contract. + assert_eq!(LEGACY_ROOM_CONTRACT_CODE_HASHES.len(), 31); + assert_eq!(&hasher.finalize().to_hex()[..16], "b5f02d45b6370b4d"); } #[test] diff --git a/common/src/room_state/member_info.rs b/common/src/room_state/member_info.rs index 5f82c1733..d889b1912 100644 --- a/common/src/room_state/member_info.rs +++ b/common/src/room_state/member_info.rs @@ -20,8 +20,8 @@ pub struct MemberInfoV1 { impl MemberInfoV1 { /// The CANONICAL `member_info` record for `member_id`: the highest- - /// `member_info_rank` (higher `version`, else lexicographically-greater - /// signature) among ALL records present for that member, or `None` if there + /// `member_info_rank` (higher `version`, else greater signature DIGEST) + /// among ALL records present for that member, or `None` if there /// is none. /// /// LOAD-BEARING (#411 round 8 item A). `verify` deliberately ACCEPTS a state @@ -37,10 +37,34 @@ impl MemberInfoV1 { /// [`Self::dedup_to_canonical`] once cleanup runs, but reads must not depend /// on that having happened yet.) pub fn canonical(&self, member_id: MemberId) -> Option<&AuthorizedMemberInfo> { - self.member_info + // The FIRST maximum wins, matching [`Self::dedup_to_canonical`] and + // `apply_delta` (both replace only on strict `>`). `Iterator::max_by_key` + // is deliberately NOT used here: it returns the LAST maximum, so with + // three selectors of "the canonical record" a tie would resolve one way + // on a freshly-GET'd full state and the other way after the next + // `apply_delta` ran dedup — a silent flip in `deputies_of`, i.e. in ban + // authority. A tie needs two records with the same version AND the same + // 128-bit signature digest, which [`SigDigest`] makes infeasible to mint, + // so this is closing a latent trap rather than a reachable bug. + // + // The rank is computed once per candidate rather than once per + // comparison, because `member_info_rank` hashes (see [`SigDigest`]). + let mut best: Option<(&AuthorizedMemberInfo, (u32, SigDigest))> = None; + for info in self + .member_info .iter() .filter(|info| info.member_info.member_id == member_id) - .max_by_key(|info| member_info_rank(info.member_info.version, &info.signature)) + { + let rank = member_info_rank(info.member_info.version, &info.signature); + let better = match &best { + Some((_, best_rank)) => outranks(rank, *best_rank), + None => true, + }; + if better { + best = Some((info, rank)); + } + } + best.map(|(info, _)| info) } /// The deputies currently listed by `member_id`'s CANONICAL signed @@ -69,53 +93,216 @@ impl MemberInfoV1 { if self.member_info.len() < 2 { return; } - let mut best: HashMap = HashMap::new(); + // Carry each incumbent's rank in the map rather than re-deriving it: a + // rank costs a blake3 hash (see [`SigDigest`]), and recomputing the + // incumbent's on every collision doubles that cost for no benefit. + // Ties keep the INCUMBENT (strict `>`), i.e. the first record in vector + // order, matching [`Self::canonical`] and `apply_delta`. + let mut best: HashMap = HashMap::new(); for info in self.member_info.drain(..) { let id = info.member_info.member_id; + let rank = member_info_rank(info.member_info.version, &info.signature); match best.entry(id) { std::collections::hash_map::Entry::Occupied(mut e) => { - if member_info_rank(info.member_info.version, &info.signature) - > member_info_rank(e.get().member_info.version, &e.get().signature) - { - e.insert(info); + if outranks(rank, e.get().0) { + e.insert((rank, info)); } } std::collections::hash_map::Entry::Vacant(e) => { - e.insert(info); + e.insert((rank, info)); } } } - self.member_info = best.into_values().collect(); + self.member_info = best.into_values().map(|(_, info)| info).collect(); // Deterministic order (HashMap iteration order is not stable). self.member_info .sort_by_key(|info| info.member_info.member_id); } } -/// Total, deterministic ordering used to pick the canonical `MemberInfo` when -/// two signed records for the SAME member collide (#411 round 4 item B). +/// Deterministic ordering used to pick the canonical `MemberInfo` when two +/// signed records for the SAME member collide (#411 round 4 item B). +/// +/// Rule: **higher `version` wins; at equal version, the greater SIGNATURE DIGEST +/// wins.** Two records with the same member and version but different content +/// (e.g. different `deputies`) have different signatures — the signature is over +/// the whole `MemberInfo` — so their digests differ and this breaks the tie +/// deterministically. (Before freenet/river#571 the tiebreak compared raw +/// signature bytes; the winner in a tie therefore changed with that PR, which is +/// safe because every peer applies the same rule and the change re-keys the +/// contract. See [`SigDigest`].) /// -/// Rule: **higher `version` wins; at equal version, the lexicographically-greater -/// SIGNATURE wins.** Two records with the same member and version but different -/// content (e.g. different `deputies`) have different signatures — the signature -/// is over the whole `MemberInfo` — so this breaks the tie deterministically. +/// This is a total ORDER on the returned `(version, digest)` pair, but only a +/// total PREORDER on records: two distinct records rank equal exactly when their +/// signatures collide under [`SigDigest`]. That tie is the dangerous case, not a +/// benign one — see [`SigDigest`] for what a tie does to anti-entropy and for the +/// 128-bit bound that is what actually keeps ties out of reach. "Records never +/// tie" is a cryptographic property here, not a structural guarantee, so the +/// three selectors that consume this order ([`MemberInfoV1::canonical`], +/// [`MemberInfoV1::dedup_to_canonical`], and `apply_delta`) are nevertheless +/// written to break a tie the SAME way (keep the first, i.e. replace only on +/// strict `>`). /// /// It is applied IDENTICALLY in [`ComposableState::apply_delta`] (conflict /// resolution), [`ComposableState::delta`], and [`ComposableState::summarize`] -/// (via the `(version, signature)` summary value), so anti-entropy can DETECT a +/// — the summary value IS this tuple — so anti-entropy can DETECT a /// same-version content difference and both peers converge on the same record. /// Without it, equal-version resolution was order-dependent AND the summary /// carried only the version, so anti-entropy saw "same version", sent no /// correction, and peers disagreed on ban authority permanently. -fn member_info_rank(version: u32, signature: &Signature) -> (u32, [u8; 64]) { - (version, signature.to_bytes()) +fn member_info_rank(version: u32, signature: &Signature) -> (u32, SigDigest) { + (version, sig_digest(signature)) +} + +/// Whether `candidate` beats `incumbent` under [`member_info_rank`]'s order. +/// +/// THE SINGLE IMPLEMENTATION OF THE TIE RULE. Five sites choose between two +/// ranked records — [`MemberInfoV1::canonical`], +/// [`MemberInfoV1::dedup_to_canonical`], `summarize`, `delta`, and `apply_delta` +/// — and every one of them must break a tie the SAME way, keeping the incumbent. +/// They previously each spelled `>` inline, which is one edit away from silent +/// disagreement: relaxing any one of them to `>=` flips that site to last-wins +/// while the others stay first-wins, and `deputies_of` then answers differently +/// depending on which code path last touched the record — i.e. ban authority +/// flips. That mutation left the whole suite green, because reaching the tie +/// branch behaviorally needs a genuine [`SigDigest`] collision. +/// +/// Routing all five through here means the rule has one definition and one test +/// (`outranks_keeps_the_incumbent_on_a_tie`) rather than five prose assertions. +/// Strict `>` is the rule: **ties keep the incumbent.** +fn outranks(candidate: (u32, SigDigest), incumbent: (u32, SigDigest)) -> bool { + candidate > incumbent +} + +/// 16-byte BLAKE3 digest of a signature: the equal-version tiebreak +/// discriminator in [`member_info_rank`], and the value [`MemberInfoV1`]'s +/// summary carries per member. +/// +/// WHY A DIGEST AND NOT THE SIGNATURE (freenet/river#571, landed as PR #572; +/// every "#571" elsewhere in this file is that ISSUE, not the PR): the summary carried +/// the raw ed25519 `Signature` per member — ~124 of ~134 CBOR bytes per entry, +/// about 92% of it. That summary is re-sent on every state change to every +/// interested peer, and `interest_sync_summaries` was measured as the largest +/// single consumer of outbound bytes on the Freenet network (49.8%). The +/// signature is never verified here — it is only ever compared for equality and +/// ordering — so a digest serves the identical purpose. Measured through the real +/// `summarize()` on 470 entries with realistic `MemberId`s: **134.08 → 28.01 CBOR +/// bytes per entry, a 4.8x reduction**. A 64-bit digest would be exactly 8 bytes +/// per entry cheaper; the next paragraph is why those 8 bytes are bought +/// deliberately. Both figures are measured, not derived, by +/// `member_info_summary_stays_small_per_entry`, which rebuilds the old shape from +/// the same records. +/// +/// WHY 124 AND NOT 66, since 66 is the number the issue and the first draft of +/// this change both used: `ed25519::Signature`'s `Serialize` calls +/// `serialize_tuple(64)`, which ciborium encodes as a CBOR ARRAY of 64 integers, +/// and a uniformly random byte costs 2 bytes there whenever it is >= 24. 66 is +/// the CBOR BYTE STRING encoding — what River's own +/// [`crate::room_state::direct_messages::SignatureBytes`] newtype produces via +/// `serialize_bytes`, and what the deferred `DirectMessagesSummary` follow-up +/// will actually be saving. The member_info summary never used that type. Two +/// different encodings of the same 64 bytes; do not reason about both with one +/// number. +/// +/// The 29.1 KB mean `interest_sync_summaries` message that motivated #571 is a +/// FLEET-WIDE mean across all rooms, so it is not this room's own summary size +/// and the two figures must not be multiplied together: at the Official room's +/// ~470 records the member_info term ALONE measures ~63 KB, well above the fleet +/// mean, because that mean also averages in many far smaller rooms. No per-room +/// summary measurement is on record, so no claim is made about what any single +/// room's total summary weighed before this change. (The issue's own arithmetic +/// did not close for the same reason this doc's did not: it used 66 rather than +/// ~124 for the signature.) +/// +/// WHY 128 BITS AND NOT 64: a collision here is not cosmetic, and it is not +/// self-correcting. Two same-version records whose discriminators tie are +/// INDISTINGUISHABLE to anti-entropy — `summarize` advertises an identical +/// `(version, digest)` on both peers, `delta` filters on strict `>` so neither +/// peer ever offers its record to the other, `apply_delta` replaces only on +/// strict `>` so each keeps whichever arrived first, and full-state merge does +/// not rescue it either (freenet-scaffold implements `merge` as summarize → +/// delta → apply_delta). The two halves of the network then disagree +/// permanently and SILENTLY on that member's `deputies`, i.e. on who may ban +/// whom (#411 round 4 B is the bug that added this discriminator in the first +/// place). A member SELF-SIGNS their own record and has unlimited grinding +/// entropy for it — `preferred_nickname` is free-form and `deputies` entries +/// are never validated for membership — so the attacker controls BOTH sides of +/// the comparison: at 64 bits that is a ~2^32 birthday search, which is hours on +/// commodity hardware. 128 bits puts it at ~2^64. +/// +/// This mirrors [`crate::room_state::direct_messages::PurgeToken`], which +/// derives a 16-byte BLAKE3 value from a signature for the same reason under a +/// strictly WEAKER threat model (there the attacker cannot influence the other +/// side of the comparison, and it still chose 128 bits). The two are +/// deliberately NOT factored into a shared helper: each is an independent +/// wire-format commitment — `PurgeToken`'s bytes live in stored state, these +/// live in the summary — and they must stay free to evolve separately. +/// +/// WHY BLAKE3 AND NOT `freenet_scaffold::util::fast_hash`: `fast_hash` is a +/// base-31 polynomial, fine for the accidental collisions `MessageId` and +/// `BanId` care about but trivially collidable by construction, which would +/// price the attack above at roughly nothing regardless of its width. +/// +/// WIRE FORMAT, load-bearing in two ways. freenet-core byte-compares +/// `summarize_state` output for staleness, so this must be a fixed function of +/// the signature bytes; and the digest orders the records, so every peer must +/// derive the same bytes and compare them the same way. Both are pinned by +/// `sig_digest_golden_vector`: +/// +/// - the digest is the FIRST 16 bytes of `blake3(signature.to_bytes())`, kept in +/// their natural order — there is no integer conversion, hence no endianness +/// decision to get wrong (the 64-bit form needed `from_le_bytes` for this); +/// - ordering is plain lexicographic over those bytes (the derived `Ord`); +/// - it serializes as a CBOR byte string (17 bytes), via the hand-written +/// `Serialize` below rather than the derive, which would emit a 16-element +/// CBOR array — ~32 bytes for random digest content, since each byte >= 24 +/// costs two — and undo most of the saving. +/// +/// None of the three may change without re-keying the contract. See +/// `.claude/rules/contract-summary-determinism.md` and freenet/freenet-core#4857. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub struct SigDigest(pub [u8; 16]); + +impl Serialize for SigDigest { + fn serialize(&self, serializer: S) -> Result { + serializer.serialize_bytes(&self.0) + } +} + +impl<'de> Deserialize<'de> for SigDigest { + fn deserialize>(deserializer: D) -> Result { + let bytes = >::deserialize(deserializer)?; + let arr: [u8; 16] = bytes.as_slice().try_into().map_err(|_| { + serde::de::Error::custom(format!( + "expected 16-byte SigDigest, got {} bytes", + bytes.len() + )) + })?; + Ok(SigDigest(arr)) + } +} + +/// The [`SigDigest`] of `signature`. See that type for the threat model, why the +/// width is 128 bits, and the wire-format commitments this function makes. +fn sig_digest(signature: &Signature) -> SigDigest { + let digest = blake3::hash(signature.to_bytes().as_ref()); + let mut out = [0u8; 16]; + out.copy_from_slice(&digest.as_bytes()[..16]); + SigDigest(out) } impl ComposableState for MemberInfoV1 { type ParentState = ChatRoomStateV1; - /// `(version, signature)` per member. The signature is the equal-version - /// tiebreak discriminator (see [`member_info_rank`]); carrying it lets - /// anti-entropy detect a content difference at the SAME version (#411 B). + /// `(version, signature-digest)` per member — i.e. exactly the value + /// [`member_info_rank`] returns. The digest is the equal-version tiebreak + /// discriminator; carrying it lets anti-entropy detect a content difference + /// at the SAME version (#411 B). + /// + /// This was `(u32, Signature)` until freenet/river#571. The raw 64-byte + /// signature was ~124 of ~134 CBOR bytes per entry (~92%), and this summary + /// is re-sent to every interested peer on every state change. See [`SigDigest`] + /// for why a digest is sufficient, why it is 128-bit blake3 rather than 64, + /// and why it serializes as a CBOR byte string. /// /// BTreeMap (not HashMap) so the ciborium-serialized summary bytes are /// deterministic: freenet-core byte-compares `summarize_state` output for @@ -123,7 +310,7 @@ impl ComposableState for MemberInfoV1 { /// two identical member_info sets summarize to different bytes → spurious /// anti-entropy heals. See `.claude/rules/contract-summary-determinism.md` /// and freenet/freenet-core#4857. - type Summary = BTreeMap; + type Summary = BTreeMap; type Delta = Vec; type Parameters = ChatRoomParametersV1; @@ -182,25 +369,26 @@ impl ComposableState for MemberInfoV1 { _parent_state: &Self::ParentState, _parameters: &Self::Parameters, ) -> Self::Summary { - // Carry the signature alongside the version so anti-entropy can detect a - // SAME-version content difference and correct it (#411 round 4 B). + // Carry the signature DIGEST alongside the version so anti-entropy can + // detect a SAME-version content difference and correct it (#411 round + // 4 B). // // Fold keeping the HIGHEST-`member_info_rank` record per member (#411 // round 7 / Codex P1 #3), NOT a plain `.collect()` (which keeps whichever // duplicate was iterated LAST). If a state holds two records for one - // member, the advertised `(version, signature)` MUST match the record + // member, the advertised `(version, digest)` MUST match the record // `deputies_of` enforces on, or a peer with a different duplicate set // would advertise a different summary and anti-entropy would never // reconcile. Migration-safe: `verify` still accepts duplicates. let mut summary: Self::Summary = BTreeMap::new(); for info in &self.member_info { - let candidate = (info.member_info.version, info.signature); + // The summary value IS the rank, so the fold compares tuples + // directly rather than re-deriving a rank from a stored signature. + let candidate = member_info_rank(info.member_info.version, &info.signature); summary .entry(info.member_info.member_id) .and_modify(|existing| { - if member_info_rank(candidate.0, &candidate.1) - > member_info_rank(existing.0, &existing.1) - { + if outranks(candidate, *existing) { *existing = candidate; } }) @@ -221,16 +409,18 @@ impl ComposableState for MemberInfoV1 { .filter(|info| { // Include if the member is absent from the old summary, OR this // record OUTRANKS what the old summary has (higher version, or - // equal version with a greater signature). The equal-version arm + // equal version with a greater signature digest). The equal-version arm // is what lets a same-version content difference propagate (#411 // round 4 B) — without it, anti-entropy would never send the // correction and peers would disagree on deputies forever. match old_state_summary.get(&info.member_info.member_id) { None => true, - Some((old_version, old_signature)) => { - member_info_rank(info.member_info.version, &info.signature) - > member_info_rank(*old_version, old_signature) - } + // The summary value IS the rank (version, signature digest), + // so compare against it directly. + Some(old_rank) => outranks( + member_info_rank(info.member_info.version, &info.signature), + *old_rank, + ), } }) .cloned() @@ -296,7 +486,7 @@ impl ComposableState for MemberInfoV1 { // Update or add the member info. Conflict resolution uses the // total, deterministic `member_info_rank` order (higher version, - // else greater signature) so that two DIFFERENT records for the + // else greater signature digest) so that two DIFFERENT records for the // same member at the SAME version resolve identically regardless // of delta arrival order (#411 round 4 B). Using only // `version >` (as before) left equal-version conflicts @@ -307,12 +497,13 @@ impl ComposableState for MemberInfoV1 { .iter_mut() .find(|info| info.member_info.member_id == *member_id) { - if member_info_rank(member_info.member_info.version, &member_info.signature) - > member_info_rank( + if outranks( + member_info_rank(member_info.member_info.version, &member_info.signature), + member_info_rank( existing_info.member_info.version, &existing_info.signature, - ) - { + ), + ) { *existing_info = member_info.clone(); } } else { @@ -462,6 +653,258 @@ mod tests { MemberInfo::new_public(member_id, 1, "TestUser".to_string()) } + /// GOLDEN VECTOR for [`SigDigest`] — ONE fixed signature, ONE fixed expected + /// digest, ONE fixed expected CBOR encoding. + /// + /// This pins the four things a peer must agree with every other peer on, and + /// which cannot change without re-keying the contract: that the hash is + /// blake3 over `signature.to_bytes()`, that the digest is the FIRST 16 bytes + /// of it, that those bytes are kept in their natural order, and that the + /// value serializes as a 17-byte CBOR byte string rather than a 32-byte CBOR + /// array. + /// + /// WHY A FIXED VECTOR AND NOT ONLY THE EXISTING ORACLES: every other check + /// on the digest — `deputy_ban_test`'s winner oracles and `room_data`'s + /// key-search fixture — compares digests of RANDOMLY-keyed signatures. A + /// change to the byte order leaves those comparisons agreeing about half the + /// time, i.e. an INTERMITTENT detector, which this project treats as a broken + /// one. Measured twice with the digest bytes reversed: those oracles let the + /// change through in 11 of 30 runs in one sample and 1 of 12 in an + /// independent reproduction. The rate depends on the keys a run happens to + /// draw and is not worth pinning down; what matters is that it is not 0, so + /// the detector is a coin flip rather than a check. A fixed input makes the + /// detection deterministic. (Byte order was + /// a live risk while the digest was a `u64` built with `from_le_bytes`; the + /// `[u8; 16]` form removes the conversion, but "which 16 bytes, in what + /// order" still has to be pinned, and the encoding certainly does.) + /// + /// The expected bytes were produced with the `b3sum` 1.8.3 CLI, outside this + /// crate, rather than by running the code under test: + /// `printf '%b' "\x00\x01...\x3f" | b3sum` → + /// `4eed7141ea4a5cd4b788606bd23f46e212af9cacebacdc7d1f4c6dc7f2511b98`. + /// That is a separate binary but the same reference BLAKE3 implementation + /// the `blake3` crate wraps, so this pins OUR choices (which hash, which + /// bytes, what order, what encoding) — it is not an independent check that + /// BLAKE3 itself is correct, and is not claimed to be. + #[test] + fn sig_digest_golden_vector() { + // Bytes 0x00..=0x3f. `Signature::from_bytes` is infallible and does not + // validate the encoded point, and `to_bytes` returns these bytes back + // unchanged, so the digest input is exactly this fixed array. + let mut raw = [0u8; 64]; + for (i, b) in raw.iter_mut().enumerate() { + *b = i as u8; + } + let signature = Signature::from_bytes(&raw); + assert_eq!( + signature.to_bytes(), + raw, + "precondition: the fixture signature must round-trip to the bytes hashed" + ); + + const EXPECTED: [u8; 16] = [ + 0x4e, 0xed, 0x71, 0x41, 0xea, 0x4a, 0x5c, 0xd4, 0xb7, 0x88, 0x60, 0x6b, 0xd2, 0x3f, + 0x46, 0xe2, + ]; + + let digest = sig_digest(&signature); + assert_eq!( + digest.0, EXPECTED, + "sig_digest changed. This is a WIRE-FORMAT and ORDERING change: every \ + peer derives the tiebreak discriminator with this exact function, and \ + freenet-core byte-compares summarize_state output for staleness. If \ + the change is intended it re-keys the room contract — follow the \ + migration ritual and update this vector deliberately." + ); + + // Reversing the digest bytes must be observably different, so the vector + // genuinely constrains ORDER and not merely the multiset of bytes. + let mut reversed = EXPECTED; + reversed.reverse(); + assert_ne!( + digest.0, reversed, + "the golden vector must distinguish byte order" + ); + + // The encoding is the other half of the commitment: a CBOR byte string + // (major type 2, length 16 => header 0x50) at 17 bytes total. The derived + // Serialize would emit a 16-element CBOR array at 32 bytes, undoing most + // of the saving freenet/river#571 exists for. + let mut encoded = Vec::new(); + ciborium::ser::into_writer(&digest, &mut encoded).expect("serialize SigDigest"); + let mut want = vec![0x50u8]; + want.extend_from_slice(&EXPECTED); + assert_eq!( + encoded, want, + "SigDigest must serialize as a 17-byte CBOR byte string" + ); + + // And it must survive the round-trip the contract actually performs + // (summarize_state serializes; get_state_delta deserializes). + let decoded: SigDigest = + ciborium::de::from_reader(encoded.as_slice()).expect("deserialize SigDigest"); + assert_eq!( + decoded, digest, + "SigDigest must round-trip through ciborium" + ); + } + + /// The three selectors of "the canonical record" must break a rank TIE the + /// same way. [`SigDigest`] makes a tie between DISTINCT records infeasible to + /// mint, but two byte-identical duplicates of the SAME record tie trivially, + /// and `Iterator::max_by_key` (which `canonical` used before) returns the + /// LAST maximum while `dedup_to_canonical` and `apply_delta` keep the FIRST. + /// Left disagreeing, a state holding duplicates would answer `deputies_of` + /// one way on a freshly-GET'd full state and the other way after the next + /// `apply_delta` ran dedup. + /// [`outranks`] is the single definition of the tie rule that all five rank + /// consumers share, so this is the one place it is pinned. + /// + /// Relaxing `>` to `>=` at any individual call site used to leave the entire + /// suite green: reaching the tie branch behaviorally needs a genuine + /// [`SigDigest`] collision, since `verify` checks every stored record's + /// signature. That made the tie direction prose-only, which is exactly the + /// state that let `canonical` (last-wins) and `dedup_to_canonical` + /// (first-wins) disagree for as long as they did. + #[test] + fn outranks_keeps_the_incumbent_on_a_tie() { + let lo = SigDigest([0x11; 16]); + let hi = SigDigest([0x22; 16]); + + // The load-bearing case: equal rank must NOT outrank. `>=` fails here. + assert!( + !outranks((1, lo), (1, lo)), + "a tie must keep the INCUMBENT — all five consumers depend on this" + ); + + // Version dominates the digest in both directions. + assert!(outranks((2, lo), (1, hi)), "higher version wins"); + assert!(!outranks((1, hi), (2, lo)), "lower version loses"); + + // At equal version the digest decides, lexicographically over its bytes. + assert!( + outranks((1, hi), (1, lo)), + "greater digest wins at equal version" + ); + assert!( + !outranks((1, lo), (1, hi)), + "lesser digest loses at equal version" + ); + } + + /// `canonical` must select on the DIGEST, not merely on the version. + /// + /// Without this, a mutation that compares only `version` (keeping the first + /// on a tie, matching the real tie direction) passes every river-core test; + /// only a river-ui test catches it. Deterministic by construction: two fixed + /// signatures whose digests are known to differ, so there is no retry loop + /// and no randomness. + #[test] + fn canonical_selects_by_digest_not_just_version() { + let mut csprng = OsRng; + let member_sk = SigningKey::generate(&mut csprng); + let member_id = MemberId::from(&member_sk.verifying_key()); + + // blake3([1; 64]) = 29c04cc4... blake3([2; 64]) = fe969aba... + // so the [2; 64] record has the greater digest and MUST win. (Computed + // with the `b3sum` CLI, not by calling `sig_digest`.) + let low_sig = Signature::from_bytes(&[1u8; 64]); + let high_sig = Signature::from_bytes(&[2u8; 64]); + assert!( + sig_digest(&high_sig) > sig_digest(&low_sig), + "fixture precondition: [2; 64] must digest greater than [1; 64]" + ); + + let mut winner_info = MemberInfo::new_public(member_id, 1, "Same".to_string()); + winner_info.deputies = vec![member_id]; + let winner = AuthorizedMemberInfo::with_signature(winner_info, high_sig); + let loser = AuthorizedMemberInfo::with_signature( + MemberInfo::new_public(member_id, 1, "Same".to_string()), + low_sig, + ); + + // Loser FIRST, so a version-only comparison that keeps the first (the + // real tie direction) returns the wrong record. + let state = MemberInfoV1 { + member_info: vec![loser, winner.clone()], + }; + + assert_eq!( + state.canonical(member_id), + Some(&winner), + "canonical must break the equal-version tie on the digest, not fall \ + back to vector position" + ); + assert_eq!( + state.deputies_of(member_id), + &[member_id], + "deputies_of must follow canonical's choice — this is the ban-authority path" + ); + } + + /// The two records are built with `with_signature` so they share a signature + /// while carrying DIFFERENT `deputies`. That is exactly what a [`SigDigest`] + /// collision would look like to these two functions, which are pure + /// orderings and never verify a signature — and it is the only way to reach + /// the tie branch without actually finding a 128-bit collision. Records built + /// the normal way cannot be used here: ed25519 signing is deterministic and + /// covers the whole `MemberInfo`, so two records that tie on rank are + /// byte-identical, and the assertion would hold under either tie direction. + #[test] + fn canonical_and_dedup_break_rank_ties_identically() { + let mut csprng = OsRng; + let member_sk = SigningKey::generate(&mut csprng); + let member_id = MemberId::from(&member_sk.verifying_key()); + let other_id = MemberId::from(&SigningKey::generate(&mut csprng).verifying_key()); + + let signed = AuthorizedMemberInfo::new_with_member_key( + MemberInfo::new_public(member_id, 1, "Tie".to_string()), + &member_sk, + ); + let shared_signature = signed.signature; + + let mut with_deputy = MemberInfo::new_public(member_id, 1, "Tie".to_string()); + with_deputy.deputies = vec![other_id]; + + let first = AuthorizedMemberInfo::with_signature( + MemberInfo::new_public(member_id, 1, "Tie".to_string()), + shared_signature, + ); + let second = AuthorizedMemberInfo::with_signature(with_deputy, shared_signature); + + assert_ne!(first, second, "precondition: the records must differ"); + assert_eq!( + member_info_rank(first.member_info.version, &first.signature), + member_info_rank(second.member_info.version, &second.signature), + "precondition: the records must nevertheless tie on rank" + ); + + let mut state = MemberInfoV1 { + member_info: vec![first.clone(), second], + }; + let picked = state.canonical(member_id).cloned(); + state.dedup_to_canonical(); + + assert_eq!( + state.member_info.len(), + 1, + "dedup must collapse the tied duplicate" + ); + assert_eq!( + picked.as_ref(), + state.member_info.first(), + "canonical() and dedup_to_canonical() must keep the SAME record on a tie \ + (`max_by_key` keeps the LAST maximum, dedup keeps the FIRST — using both \ + makes deputies_of flip after an unrelated apply_delta)" + ); + // Both keep the FIRST record in vector order (replace only on strict `>`). + assert_eq!( + picked.as_ref(), + Some(&first), + "the tie must keep the first record" + ); + } + /// LOAD-BEARING regression test (issue #410). /// /// `MemberInfo` is individually signed over its ciborium bytes @@ -678,7 +1121,7 @@ mod tests { // Summary says the peer already holds member1 at (version 1, sig1), so // member1 does not outrank it and only member2 appears in the delta. let mut old_summary = BTreeMap::new(); - old_summary.insert(member_id1, (1, sig1)); + old_summary.insert(member_id1, member_info_rank(1, &sig1)); let delta = member_info_v1.delta(&parent_state, ¶meters, &old_summary); @@ -882,14 +1325,14 @@ mod tests { let delta = member_info_v1.delta(&parent_state, ¶meters, &BTreeMap::new()); assert_eq!(delta.unwrap().len(), 5); - // Test when all members are old with the same (version, signature) — + // Test when all members are old with the same (version, digest) — // nothing outranks the summary, so the delta is empty (#411 round 4 B). - let old_summary: BTreeMap = member_infos + let old_summary: BTreeMap = member_infos .iter() .map(|info| { ( info.member_info.member_id, - (info.member_info.version, info.signature), + member_info_rank(info.member_info.version, &info.signature), ) }) .collect(); @@ -900,11 +1343,11 @@ mod tests { let mut old_summary = BTreeMap::new(); old_summary.insert( member_infos[0].member_info.member_id, - (1, member_infos[0].signature), + member_info_rank(1, &member_infos[0].signature), ); old_summary.insert( member_infos[1].member_info.member_id, - (1, member_infos[1].signature), + member_info_rank(1, &member_infos[1].signature), ); let delta = member_info_v1.delta(&parent_state, ¶meters, &old_summary); assert_eq!(delta.unwrap().len(), 3); @@ -1201,7 +1644,7 @@ mod tests { let mut revoke_mi = MemberInfo::new_public(member_id, 2, "nick".to_string()); revoke_mi.deputies = vec![]; let revoke = AuthorizedMemberInfo::new_with_member_key(revoke_mi, &member_signing_key); - let revoke_summary_value = (2u32, revoke.signature); + let revoke_summary_value = member_info_rank(2u32, &revoke.signature); let mut parent_state = ChatRoomStateV1::default(); parent_state.members.members.push(AuthorizedMember::new( @@ -1237,19 +1680,19 @@ mod tests { "deputies_of must return the v2 (revoke) result ({label})" ); - // Anti-entropy advertises the highest-rank (version, signature). + // Anti-entropy advertises the highest-rank (version, digest). let summary = state.summarize(&parent_state, ¶meters); assert_eq!( summary.get(&member_id).copied(), Some(revoke_summary_value), - "summarize must advertise the v2 (revoke) (version, signature) ({label})" + "summarize must advertise the v2 (revoke) (version, digest) ({label})" ); } } /// #411 round 7 / Codex P1 #3: when two records for one member are present, /// `deputies_of` must return the HIGHEST-rank record's deputies (higher - /// version, else greater signature) — the same winner `apply_delta` / + /// version, else greater signature digest) — the same winner `apply_delta` / /// `summarize` converge on — regardless of vector order. A bare `.find()` /// (first) could disagree with the converged state. #[test] diff --git a/common/tests/deputy_ban_test.rs b/common/tests/deputy_ban_test.rs index 056046838..f3f5529fb 100644 --- a/common/tests/deputy_ban_test.rs +++ b/common/tests/deputy_ban_test.rs @@ -115,6 +115,25 @@ fn member_ids(state: &ChatRoomStateV1) -> HashSet { .collect() } +/// The equal-version `member_info` tiebreak discriminator: higher `version` +/// wins, and at equal version the greater signature DIGEST wins +/// (freenet/river#571; before that the tiebreak compared raw signature bytes). +/// +/// Recomputed here from blake3 rather than calling into river-core, +/// deliberately: an oracle that reuses the implementation under test only +/// proves self-consistency. If production changes the digest function or which +/// bytes it keeps, this picks a different winner and the asserting test fails, +/// which is the point. +/// +/// This is a RANDOMLY-keyed oracle, so on its own it only detects a change that +/// alters the winner for the keys a given run happened to draw. The FIXED-input +/// pin is `sig_digest_golden_vector` in `river_core::room_state::member_info`; +/// the two are complementary and neither replaces the other. +fn sig_digest(sig: &ed25519_dalek::Signature) -> [u8; 16] { + let hash = blake3::hash(&sig.to_bytes()); + hash.as_bytes()[..16].try_into().unwrap() +} + /// Deputize A->B (B may be anyone), B bans T where T is in A's subtree -> T /// (and their downstream) are removed. #[test] @@ -1559,8 +1578,10 @@ fn equal_version_member_info_resolves_deterministically_across_apply_order() { peer1.member_info, peer2.member_info, "equal-version conflict must resolve identically regardless of apply order" ); - // Canonical winner: higher version (equal here), else greater signature. - let winner = if ra.signature.to_bytes() > rb.signature.to_bytes() { + // Canonical winner: higher version (equal here), else greater signature + // DIGEST (freenet/river#571; was raw signature bytes). See `sig_digest` + // for why the oracle recomputes the digest instead of calling river-core. + let winner = if sig_digest(&ra.signature) > sig_digest(&rb.signature) { &ra } else { &rb @@ -1571,14 +1592,18 @@ fn equal_version_member_info_resolves_deterministically_across_apply_order() { .iter() .find(|i| i.member_info.member_id == m.id) .unwrap(); - assert_eq!(got, winner, "resolves to the greater-signature record"); + assert_eq!( + got, winner, + "resolves to the greater signature-digest record" + ); peer1.verify(&peer1, &p).expect("peer1 verifies"); } /// #411 round 4 B — a same-version content difference is DETECTED and corrected /// by anti-entropy (the `summarize` discriminator half of the fix). Each peer has /// only ONE of the two equal-version records; because the summary now carries the -/// signature, the merge transfers the canonical winner and both converge. If +/// signature DIGEST (freenet/river#571; originally the raw signature), the merge +/// transfers the canonical winner and both converge. If /// `summarize` regressed to version-only, the delta would be empty and the peers /// would disagree on ban authority forever. #[test] @@ -1635,7 +1660,10 @@ fn equal_version_member_info_diff_detected_by_anti_entropy() { peer1.member_info, peer2.member_info, "peers converge on the SAME MemberInfo record after anti-entropy" ); - let winner = if ra.signature.to_bytes() > rb.signature.to_bytes() { + // Same tiebreak as `equal_version_member_info_resolves_deterministically_across_apply_order`: + // greater signature DIGEST wins at equal version (freenet/river#571; was + // raw signature bytes). See `sig_digest`. + let winner = if sig_digest(&ra.signature) > sig_digest(&rb.signature) { &ra } else { &rb @@ -1646,7 +1674,10 @@ fn equal_version_member_info_diff_detected_by_anti_entropy() { .iter() .find(|i| i.member_info.member_id == m.id) .unwrap(); - assert_eq!(got, winner, "both converge to the greater-signature record"); + assert_eq!( + got, winner, + "both converge to the greater signature-digest record" + ); peer1.verify(&peer1, &p).expect("peer1 verifies"); peer2.verify(&peer2, &p).expect("peer2 verifies"); } diff --git a/common/tests/summary_determinism_test.rs b/common/tests/summary_determinism_test.rs index ddd780439..b6b4fae18 100644 --- a/common/tests/summary_determinism_test.rs +++ b/common/tests/summary_determinism_test.rs @@ -15,7 +15,7 @@ //! //! See `.claude/rules/contract-summary-determinism.md`. -use ed25519_dalek::Signature; +use ed25519_dalek::{Signature, SigningKey}; use freenet_scaffold::util::FastHash; use freenet_scaffold::ComposableState; use river_core::room_state::ban::{BanId, BansV1}; @@ -23,12 +23,15 @@ use river_core::room_state::direct_messages::{ DirectMessagesSummary, DmOrderKey, DmPairHorizon, DmRetentionHorizon, SignatureBytes, }; use river_core::room_state::member::{MemberId, MembersV1}; -use river_core::room_state::member_info::MemberInfoV1; +use river_core::room_state::member_info::{ + AuthorizedMemberInfo, MemberInfo, MemberInfoV1, SigDigest, +}; use river_core::room_state::message::{ MessageId, MessageOrderKey, MessagesSummary, MessagesV1, RetentionHorizon, }; use river_core::room_state::secret::SecretsSummary; -use river_core::room_state::ChatRoomStateV1Summary; +use river_core::room_state::{ChatRoomParametersV1, ChatRoomStateV1, ChatRoomStateV1Summary}; +use std::collections::BTreeMap; use std::time::{Duration, SystemTime}; // Reference the ACTUAL associated `Summary` types (not a hard-coded `BTreeSet`), @@ -56,8 +59,21 @@ fn ban_id(i: i64) -> BanId { fn member_id(i: i64) -> MemberId { MemberId(FastHash(i)) } -fn sig(i: i64) -> Signature { - Signature::from_bytes(&[i as u8; 64]) +/// A distinct signature digest per index, matching the `(u32, SigDigest)` shape +/// the member_info summary carries since freenet/river#571. The tests using this +/// exercise ORDER-independence of the serialized summary, so only distinctness +/// matters here, not that these equal any real `blake3` digest. (The real +/// digest's value and encoding are pinned separately by `sig_digest_golden_vector` +/// in `river_core::room_state::member_info`.) +fn sig_rank(i: i64) -> SigDigest { + // Spread the bits so a byte-order bug in the summary encoding shows up as a + // difference rather than cancelling out across entries. + let a = (i as u64).wrapping_mul(0x9E37_79B9_7F4A_7C15); + let b = (i as u64).wrapping_mul(0xBF58_476D_1CE4_E5B9); + let mut out = [0u8; 16]; + out[..8].copy_from_slice(&a.to_le_bytes()); + out[8..].copy_from_slice(&b.to_le_bytes()); + SigDigest(out) } #[test] @@ -87,10 +103,12 @@ fn members_summary_serialization_is_order_independent() { #[test] fn member_info_summary_serialization_is_order_independent() { - // MemberInfoV1::Summary = BTreeMap. - let fwd: Vec<(MemberId, (u32, Signature))> = - (0..N).map(|i| (member_id(i), (i as u32, sig(i)))).collect(); - let rev: Vec<(MemberId, (u32, Signature))> = fwd.iter().rev().cloned().collect(); + // MemberInfoV1::Summary = BTreeMap — the digest + // replaced a raw 64-byte Signature in freenet/river#571. + let fwd: Vec<(MemberId, (u32, SigDigest))> = (0..N) + .map(|i| (member_id(i), (i as u32, sig_rank(i)))) + .collect(); + let rev: Vec<(MemberId, (u32, SigDigest))> = fwd.iter().rev().cloned().collect(); let s_fwd: MemberInfoSummary = fwd.into_iter().collect(); let s_rev: MemberInfoSummary = rev.into_iter().collect(); @@ -258,7 +276,7 @@ fn top_level_summary_serialization_is_order_independent() { let member_info = (0..N) .map(|i| { let j = order(i); - (member_id(j), (j as u32, sig(j))) + (member_id(j), (j as u32, sig_rank(j))) }) .collect(); let secrets = SecretsSummary { @@ -323,3 +341,138 @@ fn top_level_summary_serialization_is_order_independent() { the order its elements were inserted" ); } + +/// freenet/river#571 — the member_info summary must stay SMALL. +/// +/// This is the whole point of that change and it has no other guard: the summary +/// is re-sent to every interested peer on every state change, and +/// `interest_sync_summaries` was measured as the largest single consumer of +/// outbound bytes on the Freenet network. The entry previously carried a raw +/// ed25519 `Signature`, which is ~92% of it. +/// +/// THE BASELINE IS MEASURED HERE, NOT ASSERTED IN PROSE. Both the issue and the +/// first draft of this change quoted 66 bytes for the signature, which is the +/// encoding of River's OWN `SignatureBytes` newtype (a CBOR byte string). It is +/// NOT what `ed25519::Signature` does: `ed25519`'s `Serialize` calls +/// `serialize_tuple(64)`, ciborium maps that to a CBOR ARRAY, and a random byte +/// costs 2 bytes there whenever it is >= 24 — so the real figure is ~124. The +/// wrong number survived an issue, a PR body, a review, and a round of review +/// fixes, so the old shape is now rebuilt from the SAME records and measured +/// alongside the new one. A derived figure is not evidence. +/// +/// REALISM MATTERS HERE, because the number this test reports is quoted as the +/// production figure: +/// +/// - `MemberId`s come from REAL `VerifyingKey`s, so their `FastHash` values span +/// the `i64` range and CBOR-encode in the ~9 bytes production sees. The obvious +/// shortcut, `MemberId(FastHash(i))` for small `i`, encodes in 1-3 bytes and +/// understates the entry by ~30%. +/// - The summary is produced by calling the REAL `summarize()` on built state, +/// not by hand-constructing `MemberInfoV1::Summary`. Hand-construction can only +/// catch a change to the type's SHAPE, which would be a compile error anyway; +/// it cannot catch `summarize` starting to populate the map differently. +/// - Versions are small (1-3), matching production: a member's `version` +/// increments only when they edit their nickname or deputies, so it is a +/// single CBOR byte for nearly every real record. +/// +/// Keys are derived from fixed seeds, so the measurement is exactly reproducible +/// and the assertion cannot flake. +/// +/// Asserted as bytes-per-entry rather than a total, so the bound does not need +/// revising when the member count changes. +#[test] +fn member_info_summary_stays_small_per_entry() { + const MEMBERS: u64 = 470; // the official room's rough membership + + // Deterministic but REAL keys: blake3 of the index seeds the signing key, so + // the derived MemberId is a full-width FastHash exactly as in production. + let signing_keys: Vec = (0..MEMBERS) + .map(|i| SigningKey::from_bytes(blake3::hash(&i.to_le_bytes()).as_bytes())) + .collect(); + + let member_info: Vec = signing_keys + .iter() + .enumerate() + .map(|(i, sk)| { + let info = MemberInfo::new_public( + MemberId::from(&sk.verifying_key()), + 1 + (i % 3) as u32, + format!("member{i}"), + ); + AuthorizedMemberInfo::new_with_member_key(info, sk) + }) + .collect(); + + let state = MemberInfoV1 { member_info }; + let parent = ChatRoomStateV1::default(); + let parameters = ChatRoomParametersV1 { + owner: signing_keys[0].verifying_key(), + }; + + // The REAL summarize(), so a change in how it populates the map is caught. + let summary: MemberInfoSummary = state.summarize(&parent, ¶meters); + assert_eq!( + summary.len() as u64, + MEMBERS, + "precondition: one summary entry per member" + ); + + let bytes = cbor(&summary).len(); + let per_entry = bytes as f64 / MEMBERS as f64; + + // The PRE-CHANGE shape, rebuilt from the same records: the summary value was + // the raw `Signature` rather than a digest of it. Everything else about the + // entry is identical, so the difference is exactly what this change bought. + let before: BTreeMap = state + .member_info + .iter() + .map(|r| { + ( + r.member_info.member_id, + (r.member_info.version, r.signature), + ) + }) + .collect(); + let before_bytes = cbor(&before).len(); + let before_per_entry = before_bytes as f64 / MEMBERS as f64; + + println!( + "member_info summary over {MEMBERS} members:\n \ + before (u32, Signature): {before_bytes} B = {before_per_entry:.2} B/entry\n \ + after (u32, SigDigest): {bytes} B = {per_entry:.2} B/entry\n \ + reduction: {:.1}x", + before_per_entry / per_entry + ); + + // Measured 134.08 → 28.01 B/entry, a 4.8x reduction. A 64-bit digest would + // be exactly 8 B/entry cheaper than the 128-bit one (a CBOR byte string + // costs 1 header byte either way, and a random u64 always takes the 8-byte + // form) — see `SigDigest` for why those 8 bytes are bought deliberately. + // + // The bound leaves headroom above 28.01 without approaching the 134 it + // exists to catch. The measurement is deterministic, so that headroom is for + // future entry-shape changes, not for run-to-run variance. + assert!( + per_entry < 32.0, + "member_info summary must stay under 32 bytes/entry (got {per_entry:.2}, \ + {bytes} bytes for {MEMBERS} members). This failing likely means the \ + summary regressed to carrying signatures rather than digests \ + (freenet/river#571), or that `SigDigest` lost its hand-written \ + `Serialize` — the serde derive emits a 16-element CBOR array (32 bytes) \ + instead of a byte string (17), which measures 42.53 B/entry." + ); + + // Pin the baseline too. If this ever drops near the bound above, the + // reduction being claimed for this change has stopped being real — most + // likely because someone "fixed" the comparison to use a byte-string + // encoding that `ed25519::Signature` does not actually use. + assert!( + before_per_entry > 100.0, + "the pre-change encoding measured {before_per_entry:.2} B/entry, but \ + `ed25519::Signature` serializes as a 64-element CBOR array (~124 bytes), \ + so this should be ~134. A much lower figure means the baseline is being \ + measured against the wrong encoding — River's `SignatureBytes` newtype \ + uses `serialize_bytes` and costs 66, but the member_info summary never \ + used that type." + ); +} diff --git a/legacy_delegates.toml b/legacy_delegates.toml index 86a1a0a3d..ec9412fad 100644 --- a/legacy_delegates.toml +++ b/legacy_delegates.toml @@ -216,3 +216,10 @@ description = "Before the global direct-message retention cap (freenet/river#519 date = "2026-07-27" delegate_key = "d46b5363858c82ed91f0709d179c620c74c1ab84483b114181594c08a3d4b915" code_hash = "2f8c5f1d5c517e57208538fb2a7ec819e882eafa29bc43047eb6c025b37eba8e" + +[[entry]] +version = "V30" +description = "Before the member_info summary digest (freenet/river#571): last chat-delegate generation built against river-core 0.1.18 and the raw-signature member_info summary" +date = "2026-07-30" +delegate_key = "c3624f29fdfdb1ca3473a3d4b11c83b635cb98bf6d89e1b5114c003e1d1c485a" +code_hash = "6f65e45cd8b903374b4ac7c9c916e4fe9f9403660e7391c9192ea8378933a1b4" diff --git a/ui/public/contracts/chat_delegate.wasm b/ui/public/contracts/chat_delegate.wasm index ab74b909c..f1d5f2749 100755 Binary files a/ui/public/contracts/chat_delegate.wasm and b/ui/public/contracts/chat_delegate.wasm differ diff --git a/ui/public/contracts/room_contract.wasm b/ui/public/contracts/room_contract.wasm index ecee7e8f7..d1f0f5e3a 100755 Binary files a/ui/public/contracts/room_contract.wasm and b/ui/public/contracts/room_contract.wasm differ diff --git a/ui/src/components/app/chat_delegate.rs b/ui/src/components/app/chat_delegate.rs index 2bb689f3f..dc2f6702e 100644 --- a/ui/src/components/app/chat_delegate.rs +++ b/ui/src/components/app/chat_delegate.rs @@ -2468,18 +2468,24 @@ mod tests { /// (freenet/river#398 moved codegen to `freenet-migrate-build`) must /// reproduce it byte-identically, or every user silently re-runs legacy /// migration once. Pinned to the value computed from the current - /// `legacy_delegates.toml` (26 entries spanning V1..V29 — V4–V6 removed — + /// `legacy_delegates.toml` (27 entries spanning V1..V30 — V4–V6 removed — /// in file order). This value /// SHOULD change when a genuinely new legacy entry is added — update the /// constant then — but must NEVER change from a codegen/tooling swap. /// - /// Updated for V29 (freenet/river#519, the global DM cap): the change moves - /// the delegate WASM, so the added entry legitimately re-fingerprints the - /// set and every user re-probes the legacy delegates once. That is the - /// intended behaviour for a real new generation, not a codegen artefact. + /// Updated for V30 (freenet/river#571, the member_info summary digest). + /// Note WHY the delegate moves here, because it is not the obvious reason: + /// the summary change itself is dead-code-eliminated from the delegate + /// (verified — rebuilding with it left chat_delegate.wasm byte-identical). + /// What moves the delegate is the river-core version bump 0.1.18 -> 0.1.19, + /// which is itself mandatory: the V31 room-contract registry is compiled + /// into river-core, riverctl pins it as `=`, and 0.1.18 is already + /// published, so without the bump `cargo install riverctl` would resolve a + /// river-core WITHOUT the new entry. So a version bump alone re-keys the + /// delegate, and that is enough to require an entry. #[test] fn legacy_set_fingerprint_is_stable_across_codegen_changes() { - assert_eq!(legacy_set_fingerprint(), "323a2f640bfd7a7f"); + assert_eq!(legacy_set_fingerprint(), "c43e66ee147e3739"); } /// The "migration in progress" and "migration done" localStorage keys MUST diff --git a/ui/src/components/app/freenet_api/room_synchronizer.rs b/ui/src/components/app/freenet_api/room_synchronizer.rs index 79ee8f496..38b8f792a 100644 --- a/ui/src/components/app/freenet_api/room_synchronizer.rs +++ b/ui/src/components/app/freenet_api/room_synchronizer.rs @@ -183,7 +183,7 @@ fn outbound_summary( // // "Horizon-shaped" means the field makes the SENDER withhold something it // holds and the receiver lacks; every other field is a pure have-statement - // (an id set, a version, a signature map), which is safe — indeed required — + // (an id set, a version, a digest map), which is safe — indeed required — // to feed from the sender's own baseline, because that is what makes the // delta "what changed since I last synced". let MessagesSummary { diff --git a/ui/src/components/members.rs b/ui/src/components/members.rs index 494d9089f..eea425988 100644 --- a/ui/src/components/members.rs +++ b/ui/src/components/members.rs @@ -1704,7 +1704,7 @@ fn ExportIdentityModal(is_active: Signal) -> Element { // Look up member_info from cached or current state. // Routed through `canonical` (highest member_info_rank: - // version, then signature bytes), not a version-only + // version, then signature digest), not a version-only // `max_by_key`, so a same-version duplicate can't export // the losing record (freenet/river#411 round 8). let member_info = room_data.self_member_info.clone().or_else(|| { diff --git a/ui/src/components/members/member_info_modal.rs b/ui/src/components/members/member_info_modal.rs index 229d328b0..cf9c72739 100644 --- a/ui/src/components/members/member_info_modal.rs +++ b/ui/src/components/members/member_info_modal.rs @@ -108,7 +108,7 @@ pub fn MemberInfoModal() -> Element { let modal_content = if let Some(member_id) = MEMBER_INFO_MODAL.read().member { // Find the CANONICAL AuthorizedMemberInfo for the given member_id - // (highest member_info_rank: version, then signature bytes) — not a + // (highest member_info_rank: version, then signature digest) — not a // bare first-match. `verify` accepts duplicate member_info records // per member_id (migration safety), so a first-match `.find()` can // read a losing (e.g. revoked) record (freenet/river#411 round 8). diff --git a/ui/src/components/members/member_info_modal/nickname_field.rs b/ui/src/components/members/member_info_modal/nickname_field.rs index 920c49a2e..7b07b03f6 100644 --- a/ui/src/components/members/member_info_modal/nickname_field.rs +++ b/ui/src/components/members/member_info_modal/nickname_field.rs @@ -190,7 +190,7 @@ pub fn NicknameField(member_info: AuthorizedMemberInfo) -> Element { // `self_member_info` version — not from the canonical // base alone. On a stale/reset client the room_state max // can collide at the SAME version as a still-propagating - // record and lose the signature tiebreak, silently + // record and lose the digest tiebreak, silently // no-op'ing the edit (freenet/river#411 round 8). let cached_version = room_data .self_member_info diff --git a/ui/src/room_data.rs b/ui/src/room_data.rs index 8252fdc1f..df95f2096 100644 --- a/ui/src/room_data.rs +++ b/ui/src/room_data.rs @@ -740,7 +740,7 @@ impl RoomData { // and nickname edit. A stale `self_nickname` can never override a // newer `self_member_info`. // Route through `canonical` (highest member_info_rank: version, then - // signature bytes) rather than a version-only `max_by_key` — `verify` + // signature digest) rather than a version-only `max_by_key` — `verify` // accepts duplicate member_info records per member_id (migration // safety), and a version-only tiebreak can seed this cache from a // LOSING record on a same-version collision (freenet/river#411 @@ -840,7 +840,7 @@ impl RoomData { let self_id = MemberId::from(&self.self_sk.verifying_key()); // The viewer's CANONICAL signed member_info (highest member_info_rank: - // version, then signature bytes) — NOT a bare first-match. `verify` + // version, then signature digest) — NOT a bare first-match. `verify` // accepts duplicate member_info records per member_id (migration // safety), and a client can hold such a duplicate-containing full // state before cleanup runs. A first-match `.find()` can seed this @@ -874,7 +874,7 @@ impl RoomData { // version and the cached `self_member_info` version — not from // room_state alone. On a stale/reset client the room_state max can // collide at the SAME version as a still-propagating grant/revoke - // and lose the signature tiebreak, silently no-op'ing the change + // and lose the digest tiebreak, silently no-op'ing the change // (freenet/river#411 round 8). let cached_version = self .self_member_info @@ -6239,7 +6239,7 @@ mod tests { // ------------------------------------------------------------------ // #411 round 8 (Fix E): `apply_deputy_change` must route through the // CANONICAL member_info record (highest member_info_rank: version, then - // signature bytes), not a first-match `.find()`, and must derive the + // signature digest), not a first-match `.find()`, and must derive the // republished version from the higher of the canonical room_state // version and the cached `self_member_info` version. `verify` accepts // duplicate member_info records per member_id (migration safety), so a @@ -6261,11 +6261,23 @@ mod tests { // "clean" (no deputies) and one "stale_grant" that already lists T // as a deputy. `verify` accepts duplicate member_info records at the // same version (a genuine concurrent-edit collision). Which one is - // CANONICAL is decided by `member_info_rank`'s signature-bytes - // tiebreak, not Vec position — retry with fresh D keys until "clean" - // outranks "stale_grant" by signature, so the test's expectations + // CANONICAL is decided by `member_info_rank`'s signature-DIGEST + // tiebreak (freenet/river#571; before that it compared raw signature + // bytes), not Vec position — retry with fresh D keys until "clean" + // outranks "stale_grant" by that digest, so the test's expectations // don't depend on how ed25519 happens to sign one particular key's // bytes. + // + // The digest is recomputed here from blake3 rather than calling into + // river-core (`member_info_rank`/`sig_digest` are private anyway): + // a fixture that picked its key via the implementation under test + // would make the `canonical` sanity assertion below tautological. + // Computed independently, a change to the digest function or to which + // bytes it keeps makes this pick the wrong key and that assertion fails. + let sig_digest = |sig: &ed25519_dalek::Signature| -> [u8; 16] { + let hash = blake3::hash(&sig.to_bytes()); + hash.as_bytes()[..16].try_into().unwrap() + }; let (d_sk, clean_authorized, stale_grant_authorized) = 'retry: { for _ in 0..500 { let d_sk = SigningKey::generate(&mut rng); @@ -6285,13 +6297,15 @@ mod tests { }; let stale_grant_authorized = AuthorizedMemberInfo::new_with_member_key(stale_grant, &d_sk); - if clean_authorized.signature.to_bytes() - > stale_grant_authorized.signature.to_bytes() + if sig_digest(&clean_authorized.signature) + > sig_digest(&stale_grant_authorized.signature) { break 'retry (d_sk, clean_authorized, stale_grant_authorized); } } - panic!("failed to find a D key where 'clean' outranks 'stale_grant' by signature"); + panic!( + "failed to find a D key where 'clean' outranks 'stale_grant' by signature digest" + ); }; let d_id = MemberId::from(&d_sk.verifying_key()); @@ -6318,20 +6332,28 @@ mod tests { .push(AuthorizedMember::new(member, &owner_sk)); } - // Push the CANONICAL winner (clean) FIRST and the loser (stale_grant) - // LAST. A version-only `max_by_key` ties on version=1 and — per - // `Iterator::max_by_key`'s documented "last element wins" tie-break — - // returns whichever is LAST in the Vec (stale_grant, the wrong one), - // while `canonical` (ranked by `(version, signature)`) returns the - // true winner (clean) regardless of position. + // Push the LOSER (stale_grant) FIRST and the canonical winner (clean) + // LAST, so vector position and rank disagree. Both records tie on + // version=1, so any selector that ignores the signature digest returns + // stale_grant — the wrong one — whether it is a first-match `.find()` or + // a version-only comparison that keeps the first on a tie. Only a + // selector ranking by `(version, signature-digest)` returns clean, which + // the retry loop above guaranteed outranks stale_grant. + // + // The order matters and was flipped deliberately: it used to be + // clean-first, which caught a version-only `max_by_key` (whose documented + // tie-break returns the LAST element). `canonical` now keeps the FIRST + // maximum, to agree with `dedup_to_canonical` and `apply_delta`, so + // clean-first would let a version-only selector return the right answer + // by accident and the test would prove nothing. room_state .member_info .member_info - .push(clean_authorized.clone()); + .push(stale_grant_authorized.clone()); room_state .member_info .member_info - .push(stale_grant_authorized.clone()); + .push(clean_authorized.clone()); assert_eq!( room_state .member_info @@ -6369,11 +6391,12 @@ mod tests { }; // Deputize T. Since the CANONICAL base (clean) does not yet list T, - // this is a genuine change that must publish. A version-only - // `max_by_key` would instead select `stale_grant` (last in the Vec, - // tied on version) — which ALREADY lists T — so the buggy code - // short-circuits on "already a deputy, nothing to publish" and - // returns `false` without publishing anything. + // this is a genuine change that must publish. A selector that ignores + // the signature digest — a first-match `.find()`, or a version-only + // comparison — instead selects `stale_grant` (first in the Vec, tied on + // version), which ALREADY lists T, so the buggy code short-circuits on + // "already a deputy, nothing to publish" and returns `false` without + // publishing anything. assert!( room.apply_deputy_change(t_id, true), "must publish a change: canonical base (clean) does not yet list T" @@ -6399,7 +6422,7 @@ mod tests { // a prior edit was cached locally but the client's own room_state // view has not caught up. Deriving the next version from room_state // alone would collide with a still-propagating record at the SAME - // version, risking losing the signature tiebreak and silently + // version, risking losing the digest tiebreak and silently // no-op'ing the change. The next version must be derived from the // HIGHER of the two sources. let mut rng = rand::thread_rng();