diff --git a/crates/didbot-pds/src/provision.rs b/crates/didbot-pds/src/provision.rs index 8509dcaf..1c646ae1 100644 --- a/crates/didbot-pds/src/provision.rs +++ b/crates/didbot-pds/src/provision.rs @@ -352,6 +352,15 @@ struct Committed { repository: didbot_repo::Repository, /// The blocks this commit has that its predecessor did not. created: BTreeSet, + /// The tree keys this commit wrote or removed. + /// + /// `/` for every operation the commit is made of — + /// one for [`Registry::put_record`] and [`Registry::delete_record`], + /// one per write for [`Registry::apply_writes`]. This is what + /// [`Self::blocks`] proves: the covering proof is taken over these keys + /// and nothing else, because they are the only keys whose walk could + /// have moved. + touched: BTreeSet, /// The revision of the commit this one replaced. since: Option, /// The tree root of the commit this one replaced. @@ -361,18 +370,22 @@ struct Committed { impl Committed { /// The CAR a `com.atproto.sync.subscribeRepos#commit` frame carries. /// - /// Rooted at the new commit and holding only what the commit created, so - /// a consumer that applied the previous one can walk the whole tree out - /// of what it already has plus this. The blocks the previous commit - /// already had are what `to_car_without` is told to leave out, and - /// `created` is the history's own answer for which those are — see - /// [`crate::history`]. + /// A relay holds no previous state — it is not a consumer that already + /// applied the last commit — so a created-only diff is not enough: a + /// split or merge moves a node the commit did not create. What it needs + /// is a covering proof, taken over the tree as this commit leaves it: the + /// union of [`didbot_repo::Repository::covering_proof`] over every key + /// `touched` names, plus every block `created` — a created block need not + /// fall on a covering proof's walk, and the proof alone does not + /// guarantee every new record is present. See `docs/conformance.md`. fn blocks(&self) -> Vec { + let mut keep: BTreeSet = self.created.clone(); + keep.extend(self.repository.covering_proof(&self.touched).into_keys()); let held: BTreeSet = self .repository .block_cids() .into_iter() - .filter(|cid| !self.created.contains(cid)) + .filter(|cid| !keep.contains(cid)) .collect(); self.repository.to_car_without(&held) } @@ -1448,9 +1461,14 @@ where // frame names the commit the write landed in, and there was no key to // build one with until now. let writing = self.writing(); + let touched: BTreeSet = std::iter::once(didbot_repo::tree_key( + registration::COLLECTION, + registration::RKEY, + )) + .collect(); let at = match self .lookup(did.as_str()) - .and_then(|account| self.commit_write(&account)) + .and_then(|account| self.commit_write(&account, touched)) { Ok(at) => at, Err(error) => { @@ -1790,7 +1808,11 @@ where /// the trade `plan/repo-scale.md` is about, moved rather than made: this /// server already rebuilt on every write to name the commit a firehose /// frame carries. - fn commit_write(&self, account: &AgentAccount) -> Result { + fn commit_write( + &self, + account: &AgentAccount, + touched: BTreeSet, + ) -> Result { let did = account.did.as_str(); let key = self.signing_key(account)?; let before = self.history.head(did); @@ -1815,6 +1837,7 @@ where Ok(Committed { repository, created, + touched, since: before.as_ref().map(|head| head.rev.clone()), prev_data: before.map(|head| head.data), }) @@ -3116,7 +3139,9 @@ where if !new_refs.is_empty() { self.blobs.mark_referenced(account.did.as_str(), &new_refs); } - let at = self.commit_write(&account)?; + let touched: BTreeSet = + std::iter::once(didbot_repo::tree_key(collection, &written.rkey)).collect(); + let at = self.commit_write(&account, touched)?; drop(writing); tracing::Span::current().record("rkey", tracing::field::display(&written.rkey)); // The record's own fields are never logged. A scrobble is a sentence @@ -3207,7 +3232,27 @@ where } } } - let at = self.commit_write(&account)?; + // Every key this batch touched, whichever operation touched it. A + // `Create` with no requested `rkey` only learns the key it landed on + // from its own outcome, which is why this zips them rather than + // reading `writes` alone. + let touched: BTreeSet = writes + .iter() + .zip(&outcomes) + .map(|(op, outcome)| match (op, outcome) { + ( + BatchOp::Create { collection, .. } | BatchOp::Update { collection, .. }, + BatchOutcome::Written(written), + ) => didbot_repo::tree_key(collection, &written.rkey), + (BatchOp::Delete { collection, rkey }, _) => { + didbot_repo::tree_key(collection, rkey) + } + (BatchOp::Create { .. } | BatchOp::Update { .. }, BatchOutcome::Deleted) => { + unreachable!("a create or update never produces a delete outcome") + } + }) + .collect(); + let at = self.commit_write(&account, touched)?; drop(writing); tracing::info!( ops = outcomes.len(), @@ -3283,7 +3328,9 @@ where // changed no record, and a commit over an unchanged repository would // move the head for a caller that merely made sure of something. if removed { - self.commit_write(&account)?; + let touched: BTreeSet = + std::iter::once(didbot_repo::tree_key(collection, rkey)).collect(); + self.commit_write(&account, touched)?; } tracing::info!(removed, "record deletion applied"); Ok(removed) diff --git a/crates/didbot-repo/src/repository.rs b/crates/didbot-repo/src/repository.rs index 99a8d41a..5c0c395b 100644 --- a/crates/didbot-repo/src/repository.rs +++ b/crates/didbot-repo/src/repository.rs @@ -290,6 +290,30 @@ impl Repository { } } + /// The Merkle search tree blocks that cover every key in `keys`, over the + /// tree as this repository now stands. + /// + /// The union of [`Tree::covering_proof`] over each key — not just its + /// own path, but the paths to its neighbours either side, because a + /// split or merge moves nodes on both. A caller that touched several + /// keys in one commit (`com.atproto.repo.applyWrites`) unions their + /// proofs here rather than calling the tree once per key and merging + /// itself. + /// + /// This is what a `com.atproto.sync.subscribeRepos#commit` frame's + /// blocks are built from: a relay holds no previous state to walk a + /// created-only diff against, so it needs enough of the tree to invert + /// the operations back to `prevData` starting from nothing. See + /// `docs/conformance.md`. + #[must_use] + pub fn covering_proof(&self, keys: &BTreeSet) -> BTreeMap> { + let mut proof = BTreeMap::new(); + for key in keys { + proof.extend(self.tree.covering_proof(key)); + } + proof + } + /// How many Merkle search tree nodes it took to key them. /// /// An operator-facing number, and the cheapest signal that the tree is