diff --git a/REPORT-mst-inversion-prevdata.md b/REPORT-mst-inversion-prevdata.md new file mode 100644 index 0000000..c2117a9 --- /dev/null +++ b/REPORT-mst-inversion-prevdata.md @@ -0,0 +1,115 @@ +# report: MST delete left empty subtree nodes in place, breaking commit-proof inversion + +Status: **reproduced and fixed** on this branch. Found downstream in zlay via the +atmoq relay-conformance corpus; root-caused and fixed here in zat. + +## Symptom + +`verifyCommitDiff` returned `error.PrevDataMismatch` for a **valid** second commit +produced by `@atproto/repo`. The signature verified, the CAR parsed, op inversion +raised no error — the recomputed root simply did not equal `prevData`. + +Downstream effect in zlay: since `0b85d4b` made Sync 1.1 proofs enforcing, a +commit that fails inversion is dropped. Production showed +`relay_validation_failed{reason="sync_1_1"} = 1,522,716`, with +`commit_integrity` at 1,522,787 — i.e. essentially all integrity drops were +inversion/prevData. Those counters cannot distinguish an intended +missing-`prevData` drop from this false rejection, which is why it read as +healthy. Quantifying how much of that 1.5M was this bug still needs doing; see +"open follow-ups". + +## Root cause + +`Mst.deleteFromNode` recursed into a child subtree, marked the parent dirty, and +returned — but never dropped the child when the delete emptied it. The emptied +node stayed pointed-to, so `rootCid()` serialized it as a real block and every +ancestor CID changed. The tree therefore did not equal the tree that never +contained the key, which is exactly the equality inversion depends on. + +`deleteReturn` had this trim logic for the **root** only: + +```zig +while (self.root) |root| { + if (root.entries.items.len == 0) { ... } +} +``` + +Nothing equivalent existed one level down. The TS reference prunes in the +recursive case: + +```ts +const subtree = await prev.deleteRecurse(key) +if ((await subtree.getEntries()).length === 0) { + return this.removeEntry(index - 1) // drop the emptied subtree +} +``` + +Fix: `pruneIfEmpty` after the recursive delete, clearing the child pointer when +the node holds no entries and no left subtree. In TS a node whose only content +is a left subtree has `entries.length === 1`, so `length === 0` there is +equivalent to `entries.len == 0 and left == null` in our representation — a node +holding only a left subtree is still kept, and the existing root-level trim +handles collapsing it. + +## Why the existing tests missed it + +Three separate blind spots, all of which now have coverage: + +1. **`deleteReturn mutates through lazy stubs and matches eager root`** compares + the lazy path against the eager path. Both share `deleteFromNode`, so a wrong + root that is *consistently* wrong passes. It asserts agreement, not + correctness. + +2. **`interop: mst commit proofs`** (`firehose/commit-proof-fixtures.json`) does + exercise `dels`, but every deleted key in those six fixtures is a high key at + or above the root layer — handled by the `height >= layer` branch and its + `mergeSubtrees` path. No fixture deletes a *low* key that is the sole + occupant of its subtree, which is the only shape that triggers this. + +3. **`verifyCommitDiff: build tree, serialize partial CAR, verify inversion`** + and zlay's `Sync 1.1 accepts commit diff with correct prevData` both build the + fixture with our own encoder, invert an **update** rather than a **create**, + and ship the whole tree rather than a partial proof. Real firehose traffic is + the opposite on all three counts. + +The shape that breaks is ordinary: a repo with two records whose keys have +different heights. `app.bsky.feed.post/3lsync00001zz` has height 1, +`...00002zz` height 0, so the second lives alone in a subtree hanging off the +first. Inverting its creation must prune that subtree. + +## Reproduction + +Two tests, both of which fail without the fix (verified by reverting the prune): + +- `src/internal/repo/mst.zig` — `deleting the only key in a subtree prunes the + empty node`. Pure MST, no CAR or signatures: add a low key to a tree, delete + it, assert the root returns to its previous CID. Asserts against an + independently built tree rather than against another code path. + +- `src/internal/repo/repo.zig` — `verifyCommitDiff: interop — real @atproto + second commit inverts to prevData`. Golden fixture in + `src/internal/repo/testdata/atproto-commit-proof.json`, generated by + `@atproto/repo` 0.8.x: CAR is `newBlocks + relevantBlocks` (a genuine partial + proof), one `create` op, `prevData` = the preceding commit's MST root. + +Full suite: 482 tests, `zig fmt --check src/ build.zig` clean. (`zig fmt --check .` +reports a vendored file under the gitignored `zig-pkg/`; pre-existing.) + +## Open follow-ups + +- **Release + downstream bump.** zlay pins zat v0.3.10 by url+hash and needs a + released version carrying this fix before its conformance row flips. Not done + here: releasing pushes tags. +- **Split zlay's counters.** `failed_commit_integrity` / `failed_sync_1_1` are + bumped by both `validatePrevDataPresence` (intended drop) and inversion + failure (this bug). They should be distinguishable before anyone reads a + prevData drop rate as intended behavior. +- **Upstream the fixture.** The atproto interop fixture set has no + low-key-alone-in-subtree delete case. Worth contributing, since any + implementation with this bug passes the current suite. +- **`mergeSubtrees` looks safe but is unproven.** `pruneIfEmpty` runs after the + recursive call regardless of which branch the child took, so a child emptied by + the delete-at-own-level path is pruned too. `mergeSubtrees` itself only + constructs a node when both sides are non-null, and non-empty inputs cannot + merge into an empty node — but that rests on the non-empty invariant this fix + establishes rather than on a test. diff --git a/src/internal/repo/mst.zig b/src/internal/repo/mst.zig index cfb5fc5..dfd5118 100644 --- a/src/internal/repo/mst.zig +++ b/src/internal/repo/mst.zig @@ -378,22 +378,30 @@ pub const Mst = struct { // height < layer: recurse into the appropriate gap if (layer == 0) return null; // can't go deeper - if (node.entries.items.len == 0) { - if (try self.ensureChildNode(&node.left)) |left| { - const prev = try self.deleteFromNode(left, layer - 1, key); - if (prev != null) node.dirty = true; - return prev; - } else return null; - } + const child_ref = if (node.entries.items.len == 0) + &node.left + else + childAtIndex(node, entryLowerBound(node.entries.items, key)); - const child_ref = childAtIndex(node, entryLowerBound(node.entries.items, key)); if (try self.ensureChildNode(child_ref)) |sub| { const prev = try self.deleteFromNode(sub, layer - 1, key); - if (prev != null) node.dirty = true; + if (prev != null) { + node.dirty = true; + pruneIfEmpty(child_ref); + } return prev; } else return null; } + /// Drop a subtree pointer whose node no longer holds anything. MST nodes are + /// content-addressed, so an emptied node left in place serializes as a real + /// block and changes every ancestor CID — the tree would no longer match the + /// one that never contained the deleted key, breaking commit-proof inversion. + fn pruneIfEmpty(child_ref: *?*Node) void { + const child = child_ref.* orelse return; + if (child.entries.items.len == 0 and child.left == null) child_ref.* = null; + } + /// merge two subtrees that were separated by a deleted entry. /// both nodes are at the same layer. concatenate their entries /// and recursively merge if the junction creates adjacent children. @@ -1986,6 +1994,39 @@ test "deleteReturn mutates through lazy stubs and matches eager root" { try std.testing.expect(store.loads > 1); } +test "deleting the only key in a subtree prunes the empty node" { + const alloc = std.testing.allocator; + var arena = std.heap.ArenaAllocator.init(alloc); + defer arena.deinit(); + const a = arena.allocator(); + + // heights 1 and 0, so the second key is the sole occupant of a subtree + // hanging off the first key's entry — the shape a real two-record repo has + const high = "app.bsky.feed.post/3lsync00001zz"; + const low = "app.bsky.feed.post/3lsync00002zz"; + try std.testing.expectEqual(@as(u32, 1), keyHeight(high)); + try std.testing.expectEqual(@as(u32, 0), keyHeight(low)); + + const cid1 = try cbor.Cid.forDagCbor(a, "record-one"); + const cid2 = try cbor.Cid.forDagCbor(a, "record-two"); + + // the tree as it was before `low` was ever added + var before = Mst.init(a); + try before.put(high, cid1); + const before_root = try before.rootCid(); + + // add then remove `low`: the root must return to its previous CID, which + // requires dropping the emptied subtree rather than serializing an empty node + var after = Mst.init(a); + try after.put(high, cid1); + try after.put(low, cid2); + const removed = try after.deleteReturn(low) orelse return error.NotFound; + try std.testing.expectEqualSlices(u8, cid2.raw, removed.raw); + + const after_root = try after.rootCid(); + try std.testing.expectEqualSlices(u8, before_root.raw, after_root.raw); +} + test "inversion: create then invert" { const alloc = std.testing.allocator; var arena = std.heap.ArenaAllocator.init(alloc); diff --git a/src/internal/repo/repo.zig b/src/internal/repo/repo.zig index d598617..8e3dd14 100644 --- a/src/internal/repo/repo.zig +++ b/src/internal/repo/repo.zig @@ -14,6 +14,7 @@ const Allocator = std.mem.Allocator; const Did = @import("../syntax/did.zig").Did; const DidDocument = @import("../identity/did_document.zig").DidDocument; const multicodec = @import("../crypto/multicodec.zig"); +const multibase = @import("../crypto/multibase.zig"); const jwt = @import("../crypto/jwt.zig"); const Keypair = @import("../crypto/keypair.zig").Keypair; const cbor = @import("cbor.zig"); @@ -484,6 +485,54 @@ pub fn verifyCommitDiff( // === tests === +test "verifyCommitDiff: interop — real @atproto second commit inverts to prevData" { + // Golden fixture from @atproto/repo, not from our own encoder: the CAR holds + // only newBlocks + relevantBlocks, so the deleted key's subtree is the sole + // occupant of its node. A self-built fixture that ships the whole tree, or + // that inverts an update instead of a create, does not exercise this. + const fixture_json = @embedFile("testdata/atproto-commit-proof.json"); + + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + const a = arena.allocator(); + + const Fixture = struct { + signingKey: []const u8, + prevDataCid: []const u8, + dataCid: []const u8, + ops: []const struct { + action: []const u8, + path: []const u8, + cid: []const u8, + }, + carHex: []const u8, + }; + const parsed = try std.json.parseFromSlice(Fixture, a, fixture_json, .{ .ignore_unknown_fields = true }); + defer parsed.deinit(); + const fixture = parsed.value; + + const car_bytes = try a.alloc(u8, fixture.carHex.len / 2); + _ = try std.fmt.hexToBytes(car_bytes, fixture.carHex); + + const did_key_prefix = "did:key:"; + try std.testing.expect(std.mem.startsWith(u8, fixture.signingKey, did_key_prefix)); + const key_bytes = try multibase.decode(a, fixture.signingKey[did_key_prefix.len..]); + const public_key = try multicodec.parsePublicKey(key_bytes); + + var ops: std.ArrayListUnmanaged(mst.Operation) = .empty; + for (fixture.ops) |op| { + try std.testing.expectEqualStrings("create", op.action); + const value = try mst.parseCidString(a, op.cid); + try ops.append(a, .{ .path = op.path, .value = value.raw, .prev = null }); + } + + const prev_data = try mst.parseCidString(a, fixture.prevDataCid); + const result = try verifyCommitDiff(a, car_bytes, ops.items, prev_data.raw, public_key, .{}); + + const expected_data = try mst.parseCidString(a, fixture.dataCid); + try std.testing.expectEqualSlices(u8, expected_data.raw, result.data_cid); +} + test "verifyCommitDiff: build tree, serialize partial CAR, verify inversion" { // this test constructs a tree, applies ops, builds a partial CAR // with the commit + changed MST nodes, and verifies the diff diff --git a/src/internal/repo/testdata/atproto-commit-proof.json b/src/internal/repo/testdata/atproto-commit-proof.json new file mode 100644 index 0000000..e72ebe6 --- /dev/null +++ b/src/internal/repo/testdata/atproto-commit-proof.json @@ -0,0 +1,15 @@ +{ + "note": "Real second commit produced by @atproto/repo 0.8.x: CAR = newBlocks + relevantBlocks (a partial MST proof), one create op, prevData = the MST root of the preceding commit. Inverting the op must reproduce prevData exactly.", + "repoDid": "did:plc:conformancesync0000000000", + "signingKey": "did:key:zQ3shgW5CGLAs7w2G2ESgwudPibtkbL3ktr3ehAcowhSUSafY", + "prevDataCid": "bafyreigz4mkstjkvpij3lbdu7nsqlvuz2sffearuwouougb5pkin6ulze4", + "dataCid": "bafyreihso7sdpsqqwqdw6pywd5kanq6zx3lgtercteoy3putxrt6u34bdy", + "ops": [ + { + "action": "create", + "path": "app.bsky.feed.post/3lsync00002zz", + "cid": "bafyreid634o3htw6d7rm7x2e7yduhhcj2y7pnkcffr5qmnolh73c6cty4m" + } + ], + "carHex": "3aa265726f6f747381d82a58250001711220d89f43f3fb590e94c95fc7e1d953dbfb2651da47e7105ae6f013a5e305ed4d0f6776657273696f6e01a90101711220f277e437ca10b4076f3f161f5406c3d9bed6699222991d8dbe93bc67ea6f811ea2616581a4616b58206170702e62736b792e666565642e706f73742f336c73796e6330303030317a7a6170006174d82a582500017112206a7b0d87aeff58c6ff70efe240f0c6cdba67e08ee37826eb1b735adbe4c2f94c6176d82a582500017112203c2cdab2e0c9562cc5bfc7ee96bcf3fffa07e0d2365a0f5e8c4089c9db91a331616cf68101017112206a7b0d87aeff58c6ff70efe240f0c6cdba67e08ee37826eb1b735adbe4c2f94ca2616581a4616b58206170702e62736b792e666565642e706f73742f336c73796e6330303030327a7a6170006174f66176d82a582500017112207edf1db3cede1fe2cfdf44fe07439c49d63ef6a8452c7b0635cb3ff62f0a78e3616cf66e017112207edf1db3cede1fe2cfdf44fe07439c49d63ef6a8452c7b0635cb3ff62f0a78e3a36474657874667365636f6e64652474797065726170702e62736b792e666565642e706f7374696372656174656441747818323032362d30372d31395430303a30303a30302e3030305ae10101711220d89f43f3fb590e94c95fc7e1d953dbfb2651da47e7105ae6f013a5e305ed4d0fa66364696478216469643a706c633a636f6e666f726d616e636573796e6330303030303030303030637265766d336d7268627036326269633235637369675840748a376875630bd2e8915fed67af22e186c4671a998ce372cf3880290bee94bb0dac59f59388e6624618ce43967a8712140adc94b814f41af0c4a57f333414df6464617461d82a58250001711220f277e437ca10b4076f3f161f5406c3d9bed6699222991d8dbe93bc67ea6f811e6470726576f66776657273696f6e03" +} \ No newline at end of file