diff --git a/src/batch.rs b/src/batch.rs index 4403821d..1c8ea370 100644 --- a/src/batch.rs +++ b/src/batch.rs @@ -75,9 +75,10 @@ impl Drop for Batch<'_, T> { debug_assert!(false, "no party to tick in a `Batch` commit"); return false; }; - let hash_before = inner.tree.hash(); - inner.tree.act(party, actions); - inner.tree.hash() != hash_before + // Notify observers iff the batch changed the tree, straight from + // `act`'s changed flag: no root hash is read inside this critical + // section (`Tree::act` states the flag's contract). + inner.tree.act(party, actions) }); } } diff --git a/src/peer/gossip.rs b/src/peer/gossip.rs index 1f77ae2e..646b9c6d 100644 --- a/src/peer/gossip.rs +++ b/src/peer/gossip.rs @@ -882,11 +882,15 @@ impl Peer { // Join the tree we got via gossip: a synchronous, in-memory // merge, run directly inside the critical section, as in `send` // and `redact`. - let prior_hash = inner.tree.hash(); - inner.tree.join(merged); - - // We've modified the watch if the peer retired or the tree changed - peer_retiring || prior_hash != inner.tree.hash() + // + // We've modified the watch if the peer retired or the tree + // changed, straight from `join`'s changed flag: no root hash is + // read inside this critical section (`Tree::join` states the + // flag's contract). The join runs unconditionally — it must + // commit the merge even when the retirement alone decides the + // notification. + let tree_changed = inner.tree.join(merged); + peer_retiring || tree_changed }); if party_overlap { return (Intent::Remain, Err(Error::PartyOverlap)); diff --git a/src/tests.rs b/src/tests.rs index 5cb04985..052203fc 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -559,9 +559,9 @@ fn uncontained_supply_fails_gossip_and_poisons_the_link() { let base = poisoned.inner.borrow().tree.latest().clone(); let (escaped_root, _, escaped) = crate::tree::arb::poisoned_root(&party_of(&poisoned), &base, Message::new(0u64)); - poisoned - .inner - .send_modify(|inner| inner.tree.join(Tree { root: escaped_root })); + poisoned.inner.send_modify(|inner| { + inner.tree.join(Tree { root: escaped_root }); + }); assert!( !crate::tree::mirror::contained(&escaped, poisoned.inner.borrow().tree.latest()), "the planted leaf's version escapes the declared ceiling", @@ -609,3 +609,76 @@ fn uncontained_supply_fails_gossip_and_poisons_the_link() { "a poisoned link must fail the next gossip fast, got {retry:?}", ); } + +/// The root-hash meter is alive: a root-hash read through the public +/// snapshot surface moves the per-thread counter by exactly one. +/// +/// The liveness leg for the two commit-path pins below — a ceiling asserted +/// over a counter that stopped counting would pass vacuously. +#[test] +fn root_hash_read_meter_is_live() { + let peer = with_messages(Peer::::seed(), &[1]); + let before = crate::tree::meter::root_hash_reads(); + let _ = peer.snapshot().hash(); + assert_eq!( + crate::tree::meter::root_hash_reads() - before, + 1, + "one snapshot hash is exactly one root-hash read", + ); +} + +/// Pins the root-hash reads a batch commit performs: zero. +/// +/// The commit decides "did the tree change?" from the changed flag +/// [`Tree::act`] returns, so no root hash is read — and none *forced* over +/// the freshly rebuilt, memo-less copy-on-write spine — inside the watch +/// critical section. Both the batch build and the commit run synchronously +/// on this thread, so the bracketed count is exact. +#[test] +fn batch_commit_root_hash_reads() { + let peer = with_messages(Peer::::seed(), &[1, 2]); + let before = crate::tree::meter::root_hash_reads(); + let mut batch = peer.batch(); + batch.send(3); + drop(batch); + assert_eq!( + crate::tree::meter::root_hash_reads() - before, + 0, + "a batch commit reads no root hash in its critical section", + ); +} + +/// Pins the root-hash reads a plain gossip session performs, across both +/// sides: zero. +/// +/// Each side's write-back commit decides "did the tree change?" from the +/// changed flag [`Tree::join`] returns, so no root hash is read — and none +/// *forced* over the freshly merged, memo-less spine — inside the watch +/// critical section; the mirror exchange itself hashes nodes, never the +/// root through [`Tree::hash`]. Both peers run on this thread (`pollster` +/// drives the joined futures with no spawns), so the bracketed count is +/// exact. +#[test] +fn gossip_session_root_hash_reads() { + let provider = with_messages(Peer::::seed(), &[1, 2, 3]); + let (provider, joiner) = bootstrap_from(provider); + + // Honest divergence on both sides, so the session has real work: each + // side both provides and absorbs content. + let provider = with_messages(provider, &[10]); + let joiner = with_messages(joiner, &[20]); + + let before = crate::tree::meter::root_hash_reads(); + pollster::block_on(async { + let (mut a_link, mut b_link) = memory(); + let (provider_out, joiner_out) = + tokio::join!(provider.gossip(&mut a_link), joiner.gossip(&mut b_link)); + provider_out.expect("provider gossip"); + joiner_out.expect("joiner gossip"); + }); + assert_eq!( + crate::tree::meter::root_hash_reads() - before, + 0, + "a gossip session reads no root hash in either side's commit", + ); +} diff --git a/src/tree.rs b/src/tree.rs index e64849b1..ac01f674 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -264,6 +264,8 @@ impl Tree { /// Returns the root hash for the tree. pub fn hash(&self) -> [u8; MERKLE_HASH_LEN] { + #[cfg(test)] + meter::record_root_hash_read(); Node::root_hash(&self.root.clone().into()).into() } @@ -376,7 +378,27 @@ impl Tree { /// version when several actions address the same key. In that case the /// version is incremented once per changed key, regardless of how many /// actions pertain to it. - pub fn act(&mut self, party: &before::Party, actions: I) + /// + /// # The changed flag + /// + /// Returns whether the batch changed the tree, so the caller can answer + /// "did anything happen?" without reading the root hash — the answer the + /// traversal's effectual-action observer already produced. The two + /// directions carry different promises: + /// + /// - **`false` is exact**: the root hash is byte-identical to what it was + /// before the call, and the causal ceiling did not move. Nothing about + /// the tree changed. A watcher skipped on `false` misses nothing. + /// - **`true` is conservative**: the tree changed *or* an action was + /// silently skipped as causally prior to the leaf it targeted. The skip + /// is unreachable when every leaf's version is bounded by the tree's + /// ceiling — which `act` and `join` both maintain, so every honestly + /// built tree qualifies — because each action ticks strictly above the + /// ceiling. Only a store poisoned by nonconforming gossip (a leaf + /// *above* the ceiling; session ingestion rejects the shape) can + /// produce `true` without a hash change, and then the cost is one + /// spurious watch wakeup, never a missed one. + pub fn act(&mut self, party: &before::Party, actions: I) -> bool where T: Send + Sync, I: IntoIterator>, @@ -418,7 +440,7 @@ impl Tree { } }; (key, version, value) - })); + })) } /// Applies the specified *versioned* actions as a batch to the tree @@ -436,7 +458,12 @@ impl Tree { /// As with [`act`](Self::act), a batch is applied in a single traversal, /// which is more efficient than applying its actions one at a time but /// semantically equivalent. - fn react(&mut self, reactions: I) + /// + /// Returns whether the effectual-action observer fired at all — the + /// changed flag [`act`](Self::act) hands out, with the contract stated + /// there. `false` means no observation and therefore no ceiling + /// movement either: the tree is untouched. + fn react(&mut self, reactions: I) -> bool where T: Send + Sync, M: Into>>, @@ -458,11 +485,17 @@ impl Tree { // Traverse the tree from the root, batch-applying the actions. // The version join is deferred to the effectual-action observer so // that zero-effect actions (e.g. forgetting a nonexistent key) do not - // bump the root version. + // bump the root version. The changed flag rides the same observer: + // no observation means no leaf was inserted, replaced, or removed + // and no version was joined, so the tree — hash and ceiling both — + // is exactly what it was. + let mut changed = false; let root_version = &mut self.root.ceiling; self.root.root = traverse::act(self.root.root.take(), actions, |v: &Version| { *root_version |= v; + changed = true; }); + changed } /// Merges `other` into `self` by a single simultaneous recursion over @@ -473,7 +506,22 @@ impl Tree { /// Deletions are honored by version dominance: a leaf one side lacks /// while its version is `<=` that side's version vector was deleted /// there and is dropped. - pub fn join(&mut self, other: Tree) + /// + /// # The changed flag + /// + /// Returns whether the merge changed this tree's *content* — exactly + /// whether the root hash moved, decided by the traversal itself (each + /// leaf gained is a gain the recursion sees; each leaf dropped by + /// deletion honoring moves a node's exact leaf count) rather than by + /// hashing. `false` means the root hash is byte-identical to what it was + /// before the call; `true` means it differs. + /// + /// The flag deliberately does *not* cover the causal ceiling, which can + /// advance without any content change (absorbing the frontier of a peer + /// whose every message we already hold or honor as deleted): the flag + /// answers for what observers of the *set* can see, and a ceiling-only + /// join leaves the set untouched. + pub fn join(&mut self, other: Tree) -> bool where T: Send + Sync, { @@ -487,10 +535,49 @@ impl Tree { // root is written straight back below. Our version stays in place to be // read as the deletion filter, then joined with theirs. let our_root = std::mem::take(&mut self.root.root); - let merged = traverse::join(our_root, their_root, &self.root.ceiling, &their_version); + let mut changed = false; + let merged = traverse::join( + our_root, + their_root, + &self.root.ceiling, + &their_version, + &mut changed, + ); self.root.ceiling |= their_version; self.root.root = merged; + changed + } +} + +/// Test-only meter for root-hash reads through [`Tree::hash`]. +/// +/// A read may be answered from the node memos, but a *fresh* tree spine (the +/// copy-on-write path every commit rebuilds) has no memo, so a read inside a +/// commit's critical section re-hashes that spine while the watch lock is +/// held. The pinned tests over this counter (`root_hash_read_meter_is_live` +/// and the commit-path pins beside it in `crate::tests`) enforce how many +/// such reads each commit path performs. +/// +/// Thread-local, because every commit critical section runs synchronously on +/// its caller's thread: a test brackets the operation on its own thread and +/// reads a count no concurrent test can perturb. +#[cfg(test)] +pub(crate) mod meter { + use std::cell::Cell; + + thread_local! { + static ROOT_HASH_READS: Cell = const { Cell::new(0) }; + } + + /// How many root hashes [`Tree::hash`](super::Tree::hash) has served on + /// this thread. + pub(crate) fn root_hash_reads() -> u64 { + ROOT_HASH_READS.with(Cell::get) + } + + pub(super) fn record_root_hash_read() { + ROOT_HASH_READS.with(|c| c.set(c.get() + 1)); } } diff --git a/src/tree/tests.rs b/src/tree/tests.rs index 23ebdd6c..51aaa223 100644 --- a/src/tree/tests.rs +++ b/src/tree/tests.rs @@ -1084,6 +1084,160 @@ proptest! { } } +proptest! { + /// The changed flag [`Tree::act`] returns tracks the root hash exactly + /// on honestly built trees: `false` iff the root hash is byte-identical + /// across the call. + /// + /// The batches mix inserts, forgets of live keys, and forgets of keys + /// nothing holds — including the all-no-op and empty batches, which + /// must read `false`. + /// + /// The `false ⇒ hash-equal` direction is the flag's contract — a + /// watcher skipped on `false` must miss nothing. The converse direction + /// pins that honest commits never pay a spurious wakeup: every action + /// ticks strictly above the tree's ceiling, which bounds every leaf, so + /// the causally-prior skip (the flag's one conservative case, see + /// `act_changed_flag_is_conservative_only_in_a_poisoned_store`) is + /// unreachable here. + #[test] + fn act_changed_flag_tracks_the_root_hash( + base_values in distinct_bytes(6), + batch_values in distinct_bytes(4), + forget_live in proptest::collection::vec(any::(), 0..4), + forget_missing in proptest::collection::vec(any::(), 0..3), + ) { + let mut tree: Tree = Tree::new(); + tree.act( + &party_of("A"), + base_values.iter().cloned().map(insert_action), + ); + let live: Vec = tree.iter().map(|(k, ..)| k).collect(); + + let mut actions: Vec> = + batch_values.iter().cloned().map(insert_action).collect(); + for index in forget_live { + if !live.is_empty() { + actions.push(Action::Forget(live[index.index(live.len())])); + } + } + // A drawn key matching a live one is astronomically unlikely but + // harmless: the forget would then be effectual and both sides of + // the equality move together. + actions.extend(forget_missing.into_iter().map(Action::Forget)); + + let before = tree.hash(); + let changed = tree.act(&party_of("A"), actions); + prop_assert_eq!(changed, tree.hash() != before); + } + + /// The changed flag [`Tree::join`] returns tracks the root hash + /// exactly: `false` iff the merge left this tree's root hash + /// byte-identical. + /// + /// The divergent pairs cover one-sided novelty, shared subtrees, and + /// deletion honoring in both directions. + /// + /// The `false ⇒ hash-equal` direction is the flag's contract — a + /// watcher skipped on `false` must miss nothing. The converse pins + /// that a merge which nets nothing — the counterparty's novelty all + /// dropped by deletion honoring, or no novelty at all — never pays a + /// spurious wakeup, even while the causal ceiling advances. + #[test] + fn join_changed_flag_tracks_the_root_hash( + (a, b) in crate::tree::arb::arb_divergent_pair(), + ) { + let mut tree = Tree { root: a }; + let before = tree.hash(); + let changed = tree.join(Tree { root: b }); + prop_assert_eq!(changed, tree.hash() != before); + } +} + +/// A ceiling-only join reports unchanged. +/// +/// Absorbing a counterparty whose every message we already hold or honor +/// as deleted advances our causal ceiling but leaves the content — and the +/// root hash — untouched, and the changed flag stays `false`, so watchers +/// of the *set* are not woken for a merge that taught the set nothing. +#[test] +fn ceiling_only_join_reports_unchanged() { + let mut tree: Tree = Tree::new(); + tree.act(&party_of("A"), [insert_action(Bytes::from_static(b"kept"))]); + + // The counterparty: a tree that sent one message on its own disjoint + // party and then redacted it, leaving no content but an advanced + // ceiling. Its frontier is news to us; its (empty) content is not. + let mut other: Tree = Tree::new(); + other.act(&party_of("B"), [insert_action(Bytes::from_static(b"gone"))]); + let key = other + .iter() + .map(|(k, ..)| k) + .next() + .expect("one live message"); + other.act(&party_of("B"), [Action::Forget(key)]); + assert!( + other.is_empty(), + "the counterparty redacted its only message" + ); + + let before = tree.hash(); + let ceiling_before = tree.latest().clone(); + let changed = tree.join(other); + assert!( + !changed, + "a merge that teaches the set nothing reports unchanged", + ); + assert_eq!(tree.hash(), before, "the root hash is byte-identical"); + assert_ne!( + tree.latest(), + &ceiling_before, + "the causal ceiling did advance: the flag answers for content, not the frontier", + ); +} + +/// The changed flag's one conservative case, constructed: `Tree::act` +/// reporting `true` while the root hash is byte-identical. +/// +/// In a store poisoned by an escaped version (a leaf *above* the tree's +/// ceiling — the shape only nonconforming gossip can plant, and which +/// session ingestion rejects as `UncontainedSupply`), a forget of the +/// escaped key ticks from the ceiling, lands causally *prior* to the leaf +/// it targets, and is silently skipped — yet the traversal's observer +/// fires, so the flag reads `true` with the hash untouched. The cost is +/// one spurious watch wakeup, never a missed one: the flag's `false` stays +/// exact even here. In an honestly built tree the ceiling bounds every +/// leaf, so the skip is unreachable and the flag is exact both ways +/// (pinned by `act_changed_flag_tracks_the_root_hash`). +#[test] +fn act_changed_flag_is_conservative_only_in_a_poisoned_store() { + let (receiver, poisoned, path, _escaped) = super::arb::uncontained_supply_pair(); + let receiver_party = super::arb::nth_party(0); + let key = Key::from(path); + + let mut tree = Tree { root: receiver }; + assert!( + tree.join(Tree { root: poisoned }), + "planting the escaped leaf is a real change", + ); + + let before = tree.hash(); + let changed = tree.act(&receiver_party, [Action::Forget(key)]); + assert!( + changed, + "the skipped forget reports changed: the conservative direction", + ); + assert_eq!( + tree.hash(), + before, + "the skipped forget left the root hash byte-identical", + ); + assert!( + tree.get(&key).is_some(), + "the escaped leaf survives the skipped forget", + ); +} + /// An escaped version already resident in a store defeats redaction and /// survives every merge: the mechanism that session ingestion enforcement /// exists to keep out. diff --git a/src/tree/traverse/join.rs b/src/tree/traverse/join.rs index 01d911aa..1ae6b448 100644 --- a/src/tree/traverse/join.rs +++ b/src/tree/traverse/join.rs @@ -45,26 +45,41 @@ use height::{Height, Root, S, Z}; /// `a_version` / `b_version` are the two roots' version vectors, used to honor /// deletions (a node one side lacks while its version is `<=` that side's vector /// was deleted there, and is dropped). +/// +/// `changed` is set — never cleared — iff the merged result's content differs +/// from `a`'s: some leaf was gained from `b`, or some leaf of `a` was dropped +/// by deletion honoring. The recursion decides this exactly, with no hashing: +/// a gain is a subtree of `b` surviving the deletion filter where `a` held +/// nothing, and a drop moves a node's exact memoized leaf count. Gains and +/// drops live at distinct content-addressed paths and each is monotone at its +/// path, so they cannot cancel: an untouched flag really means the merged +/// tree is `a`, content-identical, equal root hash. pub fn join( a: Option>, b: Option>, a_version: &Version, b_version: &Version, + changed: &mut bool, ) -> Option> where T: Send + Sync, { - Join::join(a, b, a_version, b_version) + Join::join(a, b, a_version, b_version, changed) } /// The inductive step of the merge, implemented per [`Height`]; see the /// module docs for the four-case analysis each level performs. +/// +/// Each step upholds the [`join`] free function's `changed` contract: set +/// on any gain from `b` or any deletion-honoring drop from `a`, left +/// alone when the result is content-identical to `a`. pub trait Join: Unknown { fn join( a: Option>, b: Option>, a_version: &Version, b_version: &Version, + changed: &mut bool, ) -> Option> where T: Send + Sync; @@ -79,6 +94,7 @@ where b: Option>>, a_version: &Version, b_version: &Version, + changed: &mut bool, ) -> Option>> where T: Send + Sync, @@ -89,8 +105,22 @@ where // Filter it against the *other* side's version vector to honor // deletions: causally-known subtrees the other side lacks were // deleted there, and drop out. - (Some(ours), None) => Unknown::unknown(Some(ours), b_version), - (None, Some(theirs)) => Unknown::unknown(Some(theirs), a_version), + // + // On our side, the filter only ever *removes* leaves, so its + // memoized leaf count is an exact change detector: the count + // moved iff some leaf of ours was dropped. On their side, any + // survivor at all is a gain (we held nothing here). + (Some(ours), None) => { + let leaves = ours.len(); + let kept = Unknown::unknown(Some(ours), b_version); + *changed |= kept.as_ref().map_or(0, Node::len) != leaves; + kept + } + (None, Some(theirs)) => { + let gained = Unknown::unknown(Some(theirs), a_version); + *changed |= gained.is_some(); + gained + } (Some(ours), Some(theirs)) => { // Identical subtrees: keep one. Equality short-circuits on // shared backing (the common case for forked trees, hash-free) @@ -147,7 +177,7 @@ where continue; } - match Join::join(our_child, their_child, a_version, b_version) { + match Join::join(our_child, their_child, a_version, b_version, changed) { Some(child) => { merged.insert(radix, child); } @@ -169,14 +199,26 @@ impl Join for Z { b: Option>, a_version: &Version, b_version: &Version, + changed: &mut bool, ) -> Option> where T: Send + Sync, { match (a, b) { (None, None) => None, - (Some(ours), None) => Unknown::unknown(Some(ours), b_version), - (None, Some(theirs)) => Unknown::unknown(Some(theirs), a_version), + // The leaf-level base of the asymmetric arms' change detection: + // our leaf dropped by deletion honoring is a change, and their + // leaf surviving the filter is a gain. + (Some(ours), None) => { + let kept = Unknown::unknown(Some(ours), b_version); + *changed |= kept.is_none(); + kept + } + (None, Some(theirs)) => { + let gained = Unknown::unknown(Some(theirs), a_version); + *changed |= gained.is_some(); + gained + } // Two leaves at the same path are the same leaf: the path is the // content-addressed hash of (version, value) (see // `Path::for_leaf`), so identical paths carry identical contents.