feat(relay): add atomic complete read-state snapshots - #7572
Conversation
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
🔐 Codex Security Review
|
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 813bbd14121edacc6cb4733301a3af12131aa10e..1fd5a5e0c0d893c1259386885517c0c44ae7630f (exact head 1fd5a5e0c0d893c1259386885517c0c44ae7630f)
Risk: high — this adds an authenticated relay protocol boundary that advertises complete enumeration and hard response limits across tenant, signer, persistence, and HTTP contracts.
Behavior/contracts traced: community-scoped NIP-11 discovery; NIP-98/replay/membership gates; strict opt-in dispatch and self-author filter shape; tenant/signer binding; retained replacement/deletion semantics; writer routing and one-statement MVCC consistency; reconstruction/signature/storage failure; count/byte overflow; snapshot identity; ordinary POST-query/WebSocket compatibility.
Blocking finding — the advertised 8 MiB maximum does not bound the complete HTTP response. crates/buzz-db/src/store/read_state.rs:84-108 counts only the serialized event array (brackets, commas, and event values) against MAX_SNAPSHOT_BYTES. crates/buzz-relay/src/api/bridge/read_state_snapshot.rs:51-58 then wraps an already-approved array with version, completeness, community, pubkey, snapshot ID, and object syntax. An event array at or just under 8,388,608 bytes therefore produces a response over the advertised maximum. A client or intermediary sizing its response ceiling from NIP-11 can reject the supposedly supported boundary, defeating the completeness primitive exactly where discovery says it works.
Author action: preferably enforce max_bytes against the serialized entire success envelope and return 413 unless the complete body is ≤ 8,388,608 bytes, with a boundary regression asserting the emitted body/Content-Length stays within the advertised limit. Keep database preflight conservative so oversized results still fail before unnecessary retrieval. Alternatively, explicitly rename and document the advertised field as an event-array-only budget and stop describing the complete response as 8 MiB-bounded.
Verification owner: author for the fix and causal boundary regression; reviewer/CI for exact replacement-head rerun.
Other findings: no additional concrete defect found. Discovery is host/community scoped; auth, replay, admission, and membership gates precede dispatch; exact one-filter self-author structure is enforced; SQL is tenant+author scoped, excludes soft-deleted rows, includes all retained own-author kind-30078 coordinates, uses the writer, and performs budget preflight plus retrieval at one statement cut. Reconstruction, signature, serialization, storage, count, and event-array overflow fail closed without a partial complete:true envelope. Ordinary queries and WebSockets remain on existing paths.
Validation at matching head: repository just check passed on a clean exact-head tree. Focused source/contract trace passed. cargo test -p buzz-relay --lib reported 1,039 passed and 97 ignored with one unrelated mesh_demo 504. Required just test / full affected-crate execution stopped on the observability-source assertion at buzz-db/tests/observability_source.rs:621; independent base/head reproduction showed the unchanged store/event.rs:270 path fails identically at base and candidate, so it is not a PR-caused gate defect. Exact-head CI has Rust lint/unit/Windows, cross-compiles, security, Docker builds, and three Desktop smoke shards green; Desktop Core, smoke shard 4, and relay artifact producer remained in progress at review time.
Manual/native evidence: not applicable to this relay protocol change; no production traffic or deployment was exercised.
Residual risk: ignored Postgres router/MVCC tests were not independently run locally; hosted CI/reviewer tooling owns that confidence gap. The pre-existing observability-source failure needs separate trunk repair, not author rework in this PR.
— :bot: Jude’s code review agent
jedwards27
left a comment
There was a problem hiding this comment.
Reviewed exact head 1fd5a5e0c0d893c1259386885517c0c44ae7630f against base 813bbd14121edacc6cb4733301a3af12131aa10e.
The authentication, community/signer binding, strict opt-in dispatch, writer routing, retained-coordinate enumeration, MVCC consistency, failure handling, snapshot identity, and ordinary-query compatibility all held up under source trace. I found one boundary-contract defect that needs correction before merge.
[P2] Enforce the advertised 8 MiB limit on the complete response
crates/buzz-db/src/store/read_state.rs:84-108 initializes and validates encoded_bytes as the serialized events array only (array brackets, commas, and event values). crates/buzz-relay/src/api/bridge/read_state_snapshot.rs:51-58 then wraps that accepted array in a larger success envelope containing the version, complete, community ID, pubkey, and snapshot ID.
Consequently, an events array at or just below 8,388,608 bytes produces a wire response larger than 8 MiB. That conflicts with the extension's unqualified NIP-11 max_bytes: 8388608 advertisement and its documented complete-response bound. A client or intermediary that sizes its response ceiling from discovery can reject the response at the exact advertised boundary, making the completeness primitive unavailable despite the relay accepting the snapshot.
Please either:
- Preferably, count/serialize the entire success envelope and return 413 unless the complete wire JSON is at most
max_bytes, while retaining a conservative DB preflight; add a boundary regression asserting the final body length is<= 8_388_608, or - Rename and document the field explicitly as an events-array-only budget and remove the claim that the complete response is bounded to 8 MiB.
Author action: fix the advertised/wire-size contract and add the causal boundary test.
Verification owner: author for the fix and test; reviewer/CI for exact-head rerun.
Validation and confidence
just checkpassed at the clean exact head.- Full local
just test/cargo test -p buzz-db -p buzz-relay --all-targetsencounteredbuzz-db/tests/observability_source.rs:621. The same unchangedcrates/buzz-db/src/store/event.rs:270.fetch_all(pool)failure reproduces at clean base and clean head, so it is pre-existing and is not a reason for this request-changes verdict. - A separate relay-lib run passed 1,039 tests with 97 ignored;
demo_join_forwarded_arm_round_trips_echotimed out outside the changed snapshot path. Hosted exact-head CI remained in progress with no failures reported when this review was submitted. - Ignored PostgreSQL router/MVCC cases were not independently rerun locally; hosted CI owns that confidence gap.
— :bot: Jude’s code review agent
Opened by Brain on behalf of Wes (GitHub: wesbillman).
Summary
Add an opt-in, host-scoped completeness primitive for NIP-RS read-state clients. Ordinary query arrays and WebSocket behavior remain unchanged.
read_state_snapshotcapability in NIP-11 only when the request resolves a community.POST /query, after the existing admission, replay and membership gates.Safety and scope
This is point-in-time enumeration, not a live freshness guarantee, cursor, CAS token, cross-relay transaction, or unread total. NIP-RS now explicitly requires fresh complete enumeration from every write relay before each override action, canonical publication, carry-forward or deletion. It does not enable synchronized manual-unread overrides in any client. No schema migration or deployment is included.
Validation
Executed against the source committed as
1fd5a5e0c0d893c1259386885517c0c44ae7630f:cluster_global_serialization prefix.Hosted CI and maintainer review remain separate gates. No production traffic or deployment was used for these fixture tests.
Originating Buzz channel:
a394ecdb-9c67-4c4e-a3c6-4a95f69e8cc8(unread-but-good), thread2413d0ba7fa5b2ac5c777df6b3754e2099b042ec8fb96ca64302a0919a0550b3.