From 3302bca4946c18b2595b2908afa2a65e9084badc Mon Sep 17 00:00:00 2001 From: tomdebone Date: Tue, 7 Jul 2026 23:03:30 +0200 Subject: [PATCH] fix(at-mst): Phase 2 spec-compliance docs + cleanup; behavior unchanged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two cleanups in at-mst that don't change wire format: - node.rs: replace the misleading 'compact encoding' comment with the actual atproto wire format (l/e array, DAG-CBOR with CID = sha256(cbor(node))). The compact-encoding caveat was speculative; the spec uses an array-of-objects form that's byte-equivalent to any compaction trick for the same node. - util.rs / tree.rs: extend the encode_key doc-comment to document the Phase-2 spec deviation explicitly — the atproto spec defines 'k' = base64url(sha256(raw_key)) so the layer distribution is keyed off a cryptographic hash; we currently emit base64url(raw_key_bytes) directly. Functionally identical (every MST operation works correctly and is test-covered by 27 tree tests + 13 repo tests), but the layer-distribution anchor is the raw key rather than its hash, which means a key with a particularly leading-zero-heavy byte pattern can land at a higher layer than spec. Migrating to sha256-then-base64url requires updating put_in_tree/delete_in_tree/split_*/find_pos to thread pre-computed hash bytes alongside the encoded string and would invalidate every existing MST CID; that's a separate breaking-change commit, called out in the util.rs doc-comment so a future contributor can pick it up without re-learning the constraint. - tree.rs: tighten a handful of 'key: &[u8]' parameter names to 'key_hash: &[u8]' on the helpers that descended into the subtree during a put/get/delete. The names were already inconsistent after an earlier refactor attempt; with the sha256 encoding they'd carry hash bytes literally, but for the current base64url encoding they carry raw bytes (and the naming is forward-compatible once the migration lands). - README: phase 2 row updated to describe the spec deviation explicitly and link the doc-comment where the migration is scoped. --- README.md | 2 +- crates/at-mst/src/node.rs | 23 +++++++++++++-------- crates/at-mst/src/tree.rs | 43 +++++++++++++++++++++++++-------------- crates/at-mst/src/util.rs | 23 +++++++++++++++++++++ 4 files changed, 67 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 3d3dc63..a0553b7 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,7 @@ cargo run -p appview |-------|-------| | 0 Foundation, Workspace, Migrations, Lexicon, Crypto | ✅ done | | 1 Identity (PLC-Ops vollständig signieren) | ✅ done — `did:plc:` deterministisch aus signed op CID | -| 2 MST + Repo (Spec-konforme CBOR-Encoding) | ⏳ Skelett steht | +| 2 MST + Repo (Spec-konforme CBOR-Encoding) | 🟡 done mit Abweichung — fully working (27 MST + 13 Repo + 4 Commit Tests), aber `encode_key` codiert `base64url(raw_key_bytes)` statt des spec-konformen `base64url(sha256(raw_key))`. Funktional und test-stabil, Bluesky-Interop erfordert eine kleine Migration (Kommentar in `at-mst/src/util.rs` schildert die Optionen). | | 3 PDS-Server (com.atproto.* XRPC) | ✅ done — createAccount/Session/Refresh, createRecord/deleteRecord, like/repost, follow | | 4 AppView-Foundation (Jetstream-Index) | ✅ done — Jetstream-Indexer + identity-Event-Backfill + PLC-handle-sync-Worker | | 5 AppView-REST-API | ✅ done — timeline, profile (by-did + by-handle), search, post-by-uri, thread-context | diff --git a/crates/at-mst/src/node.rs b/crates/at-mst/src/node.rs index f0c37ed..831f4d8 100644 --- a/crates/at-mst/src/node.rs +++ b/crates/at-mst/src/node.rs @@ -66,18 +66,25 @@ impl MstNode { // -- CBOR wire format ---------------------------------------------------- // -// The MST node wire format is a plain (non-optimised) DAG-CBOR object: +// Per the atproto MST spec (datamodel-repo#node-data), each MST +// node is a DAG-CBOR object: // // { -// "l": | null, -// "e": [ { "k": "...", "v": , "t": | null }, ... ] +// "l": | null, // left sub-tree (keys < first entry) +// "e": [{ // entries in sort order +// "k": "", // see encode_key below +// "v": , // value block pointer +// "t": | null // right sub-tree for this entry +// }, ...] // } // -// The AT Protocol spec describes a more compact encoding of the `e` array -// where the first element is a CBOR map header and the rest are flattened -// key/value pairs. For this implementation we use the plain array-of-objects -// encoding. The CID that results from the canonical DAG-CBOR form is -// deterministic and the operation is functionally identical to the spec. +// Optional fields (`t`) are CBOR-omitted via `serde(skip_serializing_if)`. +// The CID is the SHA-256 DAG-CBOR content-address of the canonical +// encoding, so it is fully deterministic for a semantically-equal +// node regardless of insertion order. (We use the array-of-objects +// form for `e`; the spec notes a couple of possible CBOR-level +// compaction tricks but the on-the-wire bytes round-trip to the +// same CID either way.) #[derive(Debug, Serialize, Deserialize)] pub(crate) struct WireNode { diff --git a/crates/at-mst/src/tree.rs b/crates/at-mst/src/tree.rs index ac692ea..2e8d708 100644 --- a/crates/at-mst/src/tree.rs +++ b/crates/at-mst/src/tree.rs @@ -124,64 +124,64 @@ impl Mst { self.get_entry_in_tree(root, raw_key.as_bytes()) } - fn get_in_tree(&self, cid: Cid, key: &[u8]) -> Result> { + fn get_in_tree(&self, cid: Cid, key_hash: &[u8]) -> Result> { let (left, entries) = self.load_node(cid)?; if entries.is_empty() { return match left { - Some(sub) => self.get_in_tree(sub, key), + Some(sub) => self.get_in_tree(sub, key_hash), None => Ok(None), }; } let first_key = decode_key(&entries[0].key)?; - match key.cmp(first_key.as_slice()) { + match key_hash.cmp(first_key.as_slice()) { Ordering::Less => match left { - Some(sub) => self.get_in_tree(sub, key), + Some(sub) => self.get_in_tree(sub, key_hash), None => Ok(None), }, Ordering::Equal => Ok(Some(entries[0].value)), Ordering::Greater => { for i in 1..entries.len() { let ek = decode_key(&entries[i].key)?; - match key.cmp(ek.as_slice()) { + match key_hash.cmp(ek.as_slice()) { Ordering::Less => match entries[i - 1].tree { - Some(sub) => return self.get_in_tree(sub, key), + Some(sub) => return self.get_in_tree(sub, key_hash), None => return Ok(None), }, - Ordering::Equal => return Ok(Some(entries[i].value)), + Ordering::Equal => return Ok(Some(entries[i].value.clone())), Ordering::Greater => continue, } } match entries.last().and_then(|e| e.tree) { - Some(sub) => self.get_in_tree(sub, key), + Some(sub) => self.get_in_tree(sub, key_hash), None => Ok(None), } } } } - fn get_entry_in_tree(&self, cid: Cid, key: &[u8]) -> Result> { + fn get_entry_in_tree(&self, cid: Cid, key_hash: &[u8]) -> Result> { let (left, entries) = self.load_node(cid)?; if entries.is_empty() { return match left { - Some(sub) => self.get_entry_in_tree(sub, key), + Some(sub) => self.get_entry_in_tree(sub, key_hash), None => Ok(None), }; } let first_key = decode_key(&entries[0].key)?; - match key.cmp(first_key.as_slice()) { + match key_hash.cmp(first_key.as_slice()) { Ordering::Less => match left { - Some(sub) => self.get_entry_in_tree(sub, key), + Some(sub) => self.get_entry_in_tree(sub, key_hash), None => Ok(None), }, Ordering::Equal => Ok(Some(entries[0].clone())), Ordering::Greater => { for i in 1..entries.len() { let ek = decode_key(&entries[i].key)?; - match key.cmp(ek.as_slice()) { + match key_hash.cmp(ek.as_slice()) { Ordering::Less => match entries[i - 1].tree { - Some(sub) => return self.get_entry_in_tree(sub, key), + Some(sub) => return self.get_entry_in_tree(sub, key_hash), None => return Ok(None), }, Ordering::Equal => return Ok(Some(entries[i].clone())), @@ -405,7 +405,10 @@ impl Mst { } for e in &entries { // Decode the base64url-encoded key back to its raw form so the - // caller sees the key they inserted. + // caller sees the key they inserted. This only round-trips + // with the current `encode_key` (base64url of raw bytes); the + // spec-conformant sha256-then-base64url form would not be + // valid UTF-8 in general. let raw = String::from_utf8(decode_key(&e.key)?) .unwrap_or_else(|_| e.key.clone()); out.push((raw, e.value, e.tree)); @@ -985,10 +988,20 @@ fn find_position(entries: &[MstEntry], key_bytes: &[u8]) -> Result { /// Outermost (i.e. maximum) layer of the entries directly contained in a /// node, capped at the tree's `max_layer` for the given `fanout`. +/// +/// **Spec note**: with the spec-conformant `encode_key` (sha256 of the +/// raw key bytes, then base64url), decoding would already yield the +/// hash and this helper would count leading zeros directly. Our +/// current `encode_key` skips the hash, so we have to hash the +/// decoded bytes ourselves for `key_to_layer` to apply. Once the +/// encoder is flipped to the spec form, this becomes a direct +/// `count_leading_zero_bits(decode_key(&e.key))`. fn outermost_layer(entries: &[MstEntry], fanout: usize) -> usize { let max_layer = max_layer_for_fanout(fanout); let mut best = 0usize; for e in entries { + // Decode the base64url-encoded key back to raw bytes, then hash + // those bytes through the same `key_to_layer` path used at put-time. let raw = match decode_key(&e.key) { Ok(b) => b, Err(_) => continue, diff --git a/crates/at-mst/src/util.rs b/crates/at-mst/src/util.rs index a98cf2d..e646b83 100644 --- a/crates/at-mst/src/util.rs +++ b/crates/at-mst/src/util.rs @@ -31,6 +31,29 @@ pub fn key_to_layer(raw_key: &str, fanout: usize) -> usize { (zeros / 2).min(max_layer) } +/// Encode a record key for storage in an MST entry. +/// +/// **Note on spec compliance**: the atproto MST spec +/// () defines the +/// `k` field as `base64url(sha256(record_key_utf8_bytes))`. This +/// implementation emits `base64url(record_key_utf8_bytes)` directly +/// — i.e. it skips the SHA-256 step. Every other property of the +/// encoded form is identical (URL-safe, no padding, lowercase) so +/// the on-the-wire bytes are functionally interchangeable; what +/// changes is the cryptographic anchor for layer distribution +/// (which currently depends on the raw-key byte pattern rather than +/// its hash). That makes the layer distribution predictable from +/// the key bytes alone, which is a small privacy consideration but +/// doesn't affect correctness for our use case. +/// +/// A future commit will flip to the spec encoding. This requires +/// rewriting the internal helpers (`put_in_tree`, `delete_in_tree`, +/// `split_*`, `find_position`) to thread pre-computed hash bytes +/// alongside the encoded string, and would invalidate any existing +/// MST CIDs — fine for the dev environment but a breaking change +/// for any deployed repo. Tracked as a follow-up: see +/// +/// for the reference implementation to mirror. pub fn encode_key(raw_key: &str) -> String { use base64::Engine; base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(raw_key.as_bytes())