calabro.io
atproto utils for zig
mst: prune subtree nodes emptied by delete
deleteFromNode recursed into a child subtree and marked the parent dirty
but never dropped the child when the delete emptied it. MST nodes are
content-addressed, so the emptied node serialized as a real block and
changed every ancestor CID — the tree no longer equalled the tree that
never contained the key, which is the equality commit-proof inversion
depends on. The TS reference prunes here (deleteRecurse: remove the entry
when the returned subtree has no entries); we only trimmed at the root.
Effect: verifyCommitDiff returned PrevDataMismatch for valid second
commits from a real PDS. Any repo with two records whose keys differ in
height hits it — the lower key lives alone in a subtree, and inverting
its creation must prune that subtree.
Found downstream in zlay through the atmoq relay-conformance corpus,
where the sync11/commit2-valid control was being dropped.
Existing coverage missed it three ways: the lazy-vs-eager delete test
asserts the two paths agree rather than that either is right; the
interop commit-proof fixtures only delete high keys at or above the root
layer; and the verifyCommitDiff test builds its own fixture, inverts an
update rather than a create, and ships the whole tree instead of a
partial proof. Both new tests fail without the prune: one pure-MST, and
one golden fixture generated by @atproto/repo carrying a real partial
proof.
See REPORT-mst-inversion-prevdata.md.
author
zzstoatzz
date
(Jul 25, 2026, 1:56 AM -0500)
commit
f7d08169
f7d08169b9dd5afd34dbb8c6016d42ebafad763e
parent
8db0560c
8db0560c2cb9357a16c9ee65205e3d247b0f7f63
+229
-9
+115
REPORT-mst-inversion-prevdata.md
+115
REPORT-mst-inversion-prevdata.md
···
1
+
# report: MST delete left empty subtree nodes in place, breaking commit-proof inversion
2
+
3
+
Status: **reproduced and fixed** on this branch. Found downstream in zlay via the
4
+
atmoq relay-conformance corpus; root-caused and fixed here in zat.
5
+
6
+
## Symptom
7
+
8
+
`verifyCommitDiff` returned `error.PrevDataMismatch` for a **valid** second commit
9
+
produced by `@atproto/repo`. The signature verified, the CAR parsed, op inversion
10
+
raised no error — the recomputed root simply did not equal `prevData`.
11
+
12
+
Downstream effect in zlay: since `0b85d4b` made Sync 1.1 proofs enforcing, a
13
+
commit that fails inversion is dropped. Production showed
14
+
`relay_validation_failed{reason="sync_1_1"} = 1,522,716`, with
15
+
`commit_integrity` at 1,522,787 — i.e. essentially all integrity drops were
16
+
inversion/prevData. Those counters cannot distinguish an intended
17
+
missing-`prevData` drop from this false rejection, which is why it read as
18
+
healthy. Quantifying how much of that 1.5M was this bug still needs doing; see
19
+
"open follow-ups".
20
+
21
+
## Root cause
22
+
23
+
`Mst.deleteFromNode` recursed into a child subtree, marked the parent dirty, and
24
+
returned — but never dropped the child when the delete emptied it. The emptied
25
+
node stayed pointed-to, so `rootCid()` serialized it as a real block and every
26
+
ancestor CID changed. The tree therefore did not equal the tree that never
27
+
contained the key, which is exactly the equality inversion depends on.
28
+
29
+
`deleteReturn` had this trim logic for the **root** only:
30
+
31
+
```zig
32
+
while (self.root) |root| {
33
+
if (root.entries.items.len == 0) { ... }
34
+
}
35
+
```
36
+
37
+
Nothing equivalent existed one level down. The TS reference prunes in the
38
+
recursive case:
39
+
40
+
```ts
41
+
const subtree = await prev.deleteRecurse(key)
42
+
if ((await subtree.getEntries()).length === 0) {
43
+
return this.removeEntry(index - 1) // drop the emptied subtree
44
+
}
45
+
```
46
+
47
+
Fix: `pruneIfEmpty` after the recursive delete, clearing the child pointer when
48
+
the node holds no entries and no left subtree. In TS a node whose only content
49
+
is a left subtree has `entries.length === 1`, so `length === 0` there is
50
+
equivalent to `entries.len == 0 and left == null` in our representation — a node
51
+
holding only a left subtree is still kept, and the existing root-level trim
52
+
handles collapsing it.
53
+
54
+
## Why the existing tests missed it
55
+
56
+
Three separate blind spots, all of which now have coverage:
57
+
58
+
1. **`deleteReturn mutates through lazy stubs and matches eager root`** compares
59
+
the lazy path against the eager path. Both share `deleteFromNode`, so a wrong
60
+
root that is *consistently* wrong passes. It asserts agreement, not
61
+
correctness.
62
+
63
+
2. **`interop: mst commit proofs`** (`firehose/commit-proof-fixtures.json`) does
64
+
exercise `dels`, but every deleted key in those six fixtures is a high key at
65
+
or above the root layer — handled by the `height >= layer` branch and its
66
+
`mergeSubtrees` path. No fixture deletes a *low* key that is the sole
67
+
occupant of its subtree, which is the only shape that triggers this.
68
+
69
+
3. **`verifyCommitDiff: build tree, serialize partial CAR, verify inversion`**
70
+
and zlay's `Sync 1.1 accepts commit diff with correct prevData` both build the
71
+
fixture with our own encoder, invert an **update** rather than a **create**,
72
+
and ship the whole tree rather than a partial proof. Real firehose traffic is
73
+
the opposite on all three counts.
74
+
75
+
The shape that breaks is ordinary: a repo with two records whose keys have
76
+
different heights. `app.bsky.feed.post/3lsync00001zz` has height 1,
77
+
`...00002zz` height 0, so the second lives alone in a subtree hanging off the
78
+
first. Inverting its creation must prune that subtree.
79
+
80
+
## Reproduction
81
+
82
+
Two tests, both of which fail without the fix (verified by reverting the prune):
83
+
84
+
- `src/internal/repo/mst.zig` — `deleting the only key in a subtree prunes the
85
+
empty node`. Pure MST, no CAR or signatures: add a low key to a tree, delete
86
+
it, assert the root returns to its previous CID. Asserts against an
87
+
independently built tree rather than against another code path.
88
+
89
+
- `src/internal/repo/repo.zig` — `verifyCommitDiff: interop — real @atproto
90
+
second commit inverts to prevData`. Golden fixture in
91
+
`src/internal/repo/testdata/atproto-commit-proof.json`, generated by
92
+
`@atproto/repo` 0.8.x: CAR is `newBlocks + relevantBlocks` (a genuine partial
93
+
proof), one `create` op, `prevData` = the preceding commit's MST root.
94
+
95
+
Full suite: 482 tests, `zig fmt --check src/ build.zig` clean. (`zig fmt --check .`
96
+
reports a vendored file under the gitignored `zig-pkg/`; pre-existing.)
97
+
98
+
## Open follow-ups
99
+
100
+
- **Release + downstream bump.** zlay pins zat v0.3.10 by url+hash and needs a
101
+
released version carrying this fix before its conformance row flips. Not done
102
+
here: releasing pushes tags.
103
+
- **Split zlay's counters.** `failed_commit_integrity` / `failed_sync_1_1` are
104
+
bumped by both `validatePrevDataPresence` (intended drop) and inversion
105
+
failure (this bug). They should be distinguishable before anyone reads a
106
+
prevData drop rate as intended behavior.
107
+
- **Upstream the fixture.** The atproto interop fixture set has no
108
+
low-key-alone-in-subtree delete case. Worth contributing, since any
109
+
implementation with this bug passes the current suite.
110
+
- **`mergeSubtrees` looks safe but is unproven.** `pruneIfEmpty` runs after the
111
+
recursive call regardless of which branch the child took, so a child emptied by
112
+
the delete-at-own-level path is pruned too. `mergeSubtrees` itself only
113
+
constructs a node when both sides are non-null, and non-empty inputs cannot
114
+
merge into an empty node — but that rests on the non-empty invariant this fix
115
+
establishes rather than on a test.
+50
-9
src/internal/repo/mst.zig
+50
-9
src/internal/repo/mst.zig
···
378
378
379
379
// height < layer: recurse into the appropriate gap
380
380
if (layer == 0) return null; // can't go deeper
381
-
if (node.entries.items.len == 0) {
382
-
if (try self.ensureChildNode(&node.left)) |left| {
383
-
const prev = try self.deleteFromNode(left, layer - 1, key);
384
-
if (prev != null) node.dirty = true;
385
-
return prev;
386
-
} else return null;
387
-
}
381
+
const child_ref = if (node.entries.items.len == 0)
382
+
&node.left
383
+
else
384
+
childAtIndex(node, entryLowerBound(node.entries.items, key));
388
385
389
-
const child_ref = childAtIndex(node, entryLowerBound(node.entries.items, key));
390
386
if (try self.ensureChildNode(child_ref)) |sub| {
391
387
const prev = try self.deleteFromNode(sub, layer - 1, key);
392
-
if (prev != null) node.dirty = true;
388
+
if (prev != null) {
389
+
node.dirty = true;
390
+
pruneIfEmpty(child_ref);
391
+
}
393
392
return prev;
394
393
} else return null;
394
+
}
395
+
396
+
/// Drop a subtree pointer whose node no longer holds anything. MST nodes are
397
+
/// content-addressed, so an emptied node left in place serializes as a real
398
+
/// block and changes every ancestor CID — the tree would no longer match the
399
+
/// one that never contained the deleted key, breaking commit-proof inversion.
400
+
fn pruneIfEmpty(child_ref: *?*Node) void {
401
+
const child = child_ref.* orelse return;
402
+
if (child.entries.items.len == 0 and child.left == null) child_ref.* = null;
395
403
}
396
404
397
405
/// merge two subtrees that were separated by a deleted entry.
···
1984
1992
const lazy_root = try lazy.rootCid();
1985
1993
try std.testing.expectEqualSlices(u8, expected_root.raw, lazy_root.raw);
1986
1994
try std.testing.expect(store.loads > 1);
1995
+
}
1996
+
1997
+
test "deleting the only key in a subtree prunes the empty node" {
1998
+
const alloc = std.testing.allocator;
1999
+
var arena = std.heap.ArenaAllocator.init(alloc);
2000
+
defer arena.deinit();
2001
+
const a = arena.allocator();
2002
+
2003
+
// heights 1 and 0, so the second key is the sole occupant of a subtree
2004
+
// hanging off the first key's entry — the shape a real two-record repo has
2005
+
const high = "app.bsky.feed.post/3lsync00001zz";
2006
+
const low = "app.bsky.feed.post/3lsync00002zz";
2007
+
try std.testing.expectEqual(@as(u32, 1), keyHeight(high));
2008
+
try std.testing.expectEqual(@as(u32, 0), keyHeight(low));
2009
+
2010
+
const cid1 = try cbor.Cid.forDagCbor(a, "record-one");
2011
+
const cid2 = try cbor.Cid.forDagCbor(a, "record-two");
2012
+
2013
+
// the tree as it was before `low` was ever added
2014
+
var before = Mst.init(a);
2015
+
try before.put(high, cid1);
2016
+
const before_root = try before.rootCid();
2017
+
2018
+
// add then remove `low`: the root must return to its previous CID, which
2019
+
// requires dropping the emptied subtree rather than serializing an empty node
2020
+
var after = Mst.init(a);
2021
+
try after.put(high, cid1);
2022
+
try after.put(low, cid2);
2023
+
const removed = try after.deleteReturn(low) orelse return error.NotFound;
2024
+
try std.testing.expectEqualSlices(u8, cid2.raw, removed.raw);
2025
+
2026
+
const after_root = try after.rootCid();
2027
+
try std.testing.expectEqualSlices(u8, before_root.raw, after_root.raw);
1987
2028
}
1988
2029
1989
2030
test "inversion: create then invert" {
+49
src/internal/repo/repo.zig
+49
src/internal/repo/repo.zig
···
14
14
const Did = @import("../syntax/did.zig").Did;
15
15
const DidDocument = @import("../identity/did_document.zig").DidDocument;
16
16
const multicodec = @import("../crypto/multicodec.zig");
17
+
const multibase = @import("../crypto/multibase.zig");
17
18
const jwt = @import("../crypto/jwt.zig");
18
19
const Keypair = @import("../crypto/keypair.zig").Keypair;
19
20
const cbor = @import("cbor.zig");
···
483
484
}
484
485
485
486
// === tests ===
487
+
488
+
test "verifyCommitDiff: interop — real @atproto second commit inverts to prevData" {
489
+
// Golden fixture from @atproto/repo, not from our own encoder: the CAR holds
490
+
// only newBlocks + relevantBlocks, so the deleted key's subtree is the sole
491
+
// occupant of its node. A self-built fixture that ships the whole tree, or
492
+
// that inverts an update instead of a create, does not exercise this.
493
+
const fixture_json = @embedFile("testdata/atproto-commit-proof.json");
494
+
495
+
var arena = std.heap.ArenaAllocator.init(std.testing.allocator);
496
+
defer arena.deinit();
497
+
const a = arena.allocator();
498
+
499
+
const Fixture = struct {
500
+
signingKey: []const u8,
501
+
prevDataCid: []const u8,
502
+
dataCid: []const u8,
503
+
ops: []const struct {
504
+
action: []const u8,
505
+
path: []const u8,
506
+
cid: []const u8,
507
+
},
508
+
carHex: []const u8,
509
+
};
510
+
const parsed = try std.json.parseFromSlice(Fixture, a, fixture_json, .{ .ignore_unknown_fields = true });
511
+
defer parsed.deinit();
512
+
const fixture = parsed.value;
513
+
514
+
const car_bytes = try a.alloc(u8, fixture.carHex.len / 2);
515
+
_ = try std.fmt.hexToBytes(car_bytes, fixture.carHex);
516
+
517
+
const did_key_prefix = "did:key:";
518
+
try std.testing.expect(std.mem.startsWith(u8, fixture.signingKey, did_key_prefix));
519
+
const key_bytes = try multibase.decode(a, fixture.signingKey[did_key_prefix.len..]);
520
+
const public_key = try multicodec.parsePublicKey(key_bytes);
521
+
522
+
var ops: std.ArrayListUnmanaged(mst.Operation) = .empty;
523
+
for (fixture.ops) |op| {
524
+
try std.testing.expectEqualStrings("create", op.action);
525
+
const value = try mst.parseCidString(a, op.cid);
526
+
try ops.append(a, .{ .path = op.path, .value = value.raw, .prev = null });
527
+
}
528
+
529
+
const prev_data = try mst.parseCidString(a, fixture.prevDataCid);
530
+
const result = try verifyCommitDiff(a, car_bytes, ops.items, prev_data.raw, public_key, .{});
531
+
532
+
const expected_data = try mst.parseCidString(a, fixture.dataCid);
533
+
try std.testing.expectEqualSlices(u8, expected_data.raw, result.data_cid);
534
+
}
486
535
487
536
test "verifyCommitDiff: build tree, serialize partial CAR, verify inversion" {
488
537
// this test constructs a tree, applies ops, builds a partial CAR
+15
src/internal/repo/testdata/atproto-commit-proof.json
+15
src/internal/repo/testdata/atproto-commit-proof.json
···
1
+
{
2
+
"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.",
3
+
"repoDid": "did:plc:conformancesync0000000000",
4
+
"signingKey": "did:key:zQ3shgW5CGLAs7w2G2ESgwudPibtkbL3ktr3ehAcowhSUSafY",
5
+
"prevDataCid": "bafyreigz4mkstjkvpij3lbdu7nsqlvuz2sffearuwouougb5pkin6ulze4",
6
+
"dataCid": "bafyreihso7sdpsqqwqdw6pywd5kanq6zx3lgtercteoy3putxrt6u34bdy",
7
+
"ops": [
8
+
{
9
+
"action": "create",
10
+
"path": "app.bsky.feed.post/3lsync00002zz",
11
+
"cid": "bafyreid634o3htw6d7rm7x2e7yduhhcj2y7pnkcffr5qmnolh73c6cty4m"
12
+
}
13
+
],
14
+
"carHex": "3aa265726f6f747381d82a58250001711220d89f43f3fb590e94c95fc7e1d953dbfb2651da47e7105ae6f013a5e305ed4d0f6776657273696f6e01a90101711220f277e437ca10b4076f3f161f5406c3d9bed6699222991d8dbe93bc67ea6f811ea2616581a4616b58206170702e62736b792e666565642e706f73742f336c73796e6330303030317a7a6170006174d82a582500017112206a7b0d87aeff58c6ff70efe240f0c6cdba67e08ee37826eb1b735adbe4c2f94c6176d82a582500017112203c2cdab2e0c9562cc5bfc7ee96bcf3fffa07e0d2365a0f5e8c4089c9db91a331616cf68101017112206a7b0d87aeff58c6ff70efe240f0c6cdba67e08ee37826eb1b735adbe4c2f94ca2616581a4616b58206170702e62736b792e666565642e706f73742f336c73796e6330303030327a7a6170006174f66176d82a582500017112207edf1db3cede1fe2cfdf44fe07439c49d63ef6a8452c7b0635cb3ff62f0a78e3616cf66e017112207edf1db3cede1fe2cfdf44fe07439c49d63ef6a8452c7b0635cb3ff62f0a78e3a36474657874667365636f6e64652474797065726170702e62736b792e666565642e706f7374696372656174656441747818323032362d30372d31395430303a30303a30302e3030305ae10101711220d89f43f3fb590e94c95fc7e1d953dbfb2651da47e7105ae6f013a5e305ed4d0fa66364696478216469643a706c633a636f6e666f726d616e636573796e6330303030303030303030637265766d336d7268627036326269633235637369675840748a376875630bd2e8915fed67af22e186c4671a998ce372cf3880290bee94bb0dac59f59388e6624618ce43967a8712140adc94b814f41af0c4a57f333414df6464617461d82a58250001711220f277e437ca10b4076f3f161f5406c3d9bed6699222991d8dbe93bc67ea6f811e6470726576f66776657273696f6e03"
15
+
}