Add NIP-09 soft-deletion support - #132
Open
jb55 wants to merge 2 commits into
Open
Conversation
The enum values were bit positions (0, 2, 4) but were used as bitmasks elsewhere (flags |= NDB_NOTE_META_FLAG_ZAP_VERIFIED). DELETED at 0 was a no-op; SEEN and ZAP_VERIFIED accidentally hit bits 1 and 2 instead of the intended 2 and 4. Pre-shift the values so direct |= and & work as intended. Even bits remain reserved for system flags, odd bits for user-defined flags. ZAP_VERIFIED's on-disk bit moves from 2 to 4; records written with the old buggy bitmask appear unverified and re-verify on next pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kind-5 events mark their referenced notes' metadata with NDB_NOTE_META_FLAG_DELETED, and ndb_filter_matches_with hides deleted notes from queries (kind-5 events themselves remain queryable). Out-of-order kind-5s are applied to the metadata regardless of whether the target has arrived, since metadata is keyed on note id. When the target exists, the deleter pubkey must match the note's author or the deletion is dropped. When the target is absent, the flag is set in advance and trusted on arrival. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR implements NIP-09 "soft delete" support end-to-end. Deletion events (kind 5) are ingested and converted to writer messages that mark target notes as deleted in metadata. Query-time filtering suppresses deleted notes from results across all query plans. Three tests validate soft delete semantics including authorization and out-of-order ingestion. ChangesNIP-09 Soft Delete Implementation
Sequence DiagramsequenceDiagram
participant Ingester
participant Writer
participant MetadataDB
participant QueryEngine
participant FilterMatcher
Ingester->>Ingester: Receive kind-5 event
Ingester->>Ingester: Extract e-tagged target IDs
Ingester->>Writer: Queue NDB_WRITER_DELETE_NOTE<br/>(target_id, deleter_pubkey)
Writer->>MetadataDB: Look up target note metadata
alt Target exists
Writer->>MetadataDB: Set NDB_NOTE_META_FLAG_DELETED
else Target not yet arrived
Writer->>MetadataDB: Write minimal deleted metadata header
end
Writer->>Writer: Commit transaction
QueryEngine->>FilterMatcher: Match notes with txn context
FilterMatcher->>MetadataDB: Check NDB_NOTE_META_FLAG_DELETED
FilterMatcher-->>QueryEngine: Suppress deleted notes<br/>(except kind-5)
QueryEngine-->>QueryEngine: Return non-deleted results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nostrdb.c (1)
1325-1343:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis still misses some note-return paths.
The deleted-bit check only works for callers that route through
ndb_filter_matches_with()with a live txn.ndb_text_search_with()still falls back tondb_filter_matches(),NDB_PLAN_ALL_NOTESnever filters at all, and subscription delivery still goes throughndb_filter_group_matches(), so soft-deleted notes can still leak through search, empty-filter scans, and live notifications.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nostrdb.c` around lines 1325 - 1343, The deleted-bit check is only in ndb_filter_matches_with so soft-deleted notes still appear via ndb_text_search_with (which uses ndb_filter_matches), through NDB_PLAN_ALL_NOTES, and during live delivery via ndb_filter_group_matches; add a single reusable visibility check (e.g., ndb_note_is_visible or ndb_note_is_deleted) that looks up ndb_get_note_meta(txn, ndb_note_id(note)) and checks NDB_NOTE_META_FLAG_DELETED, then call that helper from ndb_filter_matches_with, ndb_filter_matches, ndb_text_search_with (or its fallback), and ndb_filter_group_matches and ensure NDB_PLAN_ALL_NOTES paths consult it so all search/scan/subscribe code uniformly hides soft-deleted notes when txn is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/nostrdb.c`:
- Around line 4081-4089: The code aborts silently when an existing note meta
exceeds the fixed scratch buffer (4 KiB); replace the static scratch usage with
a dynamic allocation sized to sz: call ndb_note_meta_total_size(existing) into
sz, malloc(sz) (or use caller-provided buffer), check allocation, memcpy into
the allocated buffer, cast to struct ndb_note_meta *existing, set the
NDB_NOTE_META_FLAG_DELETED via ndb_note_meta_flags(existing), call
ndb_writer_set_note_meta(txn, target_id, existing), and free the allocation on
error or after use; update the code around the ndb_get_note_meta /
ndb_note_meta_total_size / ndb_note_meta_flags / ndb_writer_set_note_meta
sequence accordingly.
- Around line 4092-4103: The tombstone write currently sets only the DELETED
flag and loses the deleter identity; modify the apply_deletion path that calls
ndb_note_meta_header_init(&hdr) so it copies the deleter's public key into the
header (e.g., hdr.deleter_pubkey = <deleter_pubkey source> or memcpy into
hdr.deleter_pubkey) before the mdb_put, keep v.mv_size = sizeof(hdr), and ensure
the stored header field (hdr.deleter_pubkey) is later consulted by the
note-write/auth check logic (the code paths that validate incoming notes against
existing meta) so a pending-delete preserves the deleter's identity for future
verification.
- Around line 3419-3444: ndb_process_deletion_event currently ignores
prot_queue_push return values causing partial enqueue on writer_inbox; modify
ndb_process_deletion_event to treat any failed prot_queue_push(&msg) as a fatal
ingest failure: check the return value inside the tag loop, stop iterating on
first failure, avoid proceeding to the later kind-5 write, and propagate the
failure to callers (e.g. change ndb_process_deletion_event signature from void
to an int/bool return and return non-zero on failure, or set a clear ingester
error flag) so the caller can abort/rollback; ensure references to
prot_queue_push, writer_inbox, msg, and NDB_WRITER_DELETE_NOTE are updated
accordingly.
---
Outside diff comments:
In `@src/nostrdb.c`:
- Around line 1325-1343: The deleted-bit check is only in
ndb_filter_matches_with so soft-deleted notes still appear via
ndb_text_search_with (which uses ndb_filter_matches), through
NDB_PLAN_ALL_NOTES, and during live delivery via ndb_filter_group_matches; add a
single reusable visibility check (e.g., ndb_note_is_visible or
ndb_note_is_deleted) that looks up ndb_get_note_meta(txn, ndb_note_id(note)) and
checks NDB_NOTE_META_FLAG_DELETED, then call that helper from
ndb_filter_matches_with, ndb_filter_matches, ndb_text_search_with (or its
fallback), and ndb_filter_group_matches and ensure NDB_PLAN_ALL_NOTES paths
consult it so all search/scan/subscribe code uniformly hides soft-deleted notes
when txn is provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dba848a2-013b-4060-b4d7-8753eee13ca5
📒 Files selected for processing (3)
src/metadata.hsrc/nostrdb.ctest.c
Comment on lines
+3419
to
+3444
| static void ndb_process_deletion_event(struct ndb_ingester *ingester, | ||
| struct ndb_note *note) | ||
| { | ||
| struct ndb_iterator iter; | ||
| struct ndb_str name, value; | ||
| struct ndb_writer_msg msg; | ||
|
|
||
| msg.type = NDB_WRITER_DELETE_NOTE; | ||
| memcpy(msg.delete_note.deleter_pubkey, ndb_note_pubkey(note), 32); | ||
|
|
||
| ndb_tags_iterate_start(note, &iter); | ||
| while (ndb_tags_iterate_next(&iter)) { | ||
| if (iter.tag->count < 2) | ||
| continue; | ||
|
|
||
| name = ndb_tag_str(note, iter.tag, 0); | ||
| if (name.flag != NDB_PACKED_STR || name.str[0] != 'e' || name.str[1] != '\0') | ||
| continue; | ||
|
|
||
| value = ndb_tag_str(note, iter.tag, 1); | ||
| if (value.flag != NDB_PACKED_ID) | ||
| continue; | ||
|
|
||
| memcpy(msg.delete_note.target_id, value.id, 32); | ||
| prot_queue_push(ingester->writer_inbox, &msg); | ||
| } |
There was a problem hiding this comment.
Handle partial enqueue failure for kind-5 fan-out.
Each referenced e tag is pushed independently, but the return value is ignored. If the writer queue fills mid-loop, the kind-5 event can still be written below while only some delete work items were accepted, leaving tombstone storage and metadata state out of sync. Treat any failed push as an ingest failure for the whole deletion event.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/nostrdb.c` around lines 3419 - 3444, ndb_process_deletion_event currently
ignores prot_queue_push return values causing partial enqueue on writer_inbox;
modify ndb_process_deletion_event to treat any failed prot_queue_push(&msg) as a
fatal ingest failure: check the return value inside the tag loop, stop iterating
on first failure, avoid proceeding to the later kind-5 write, and propagate the
failure to callers (e.g. change ndb_process_deletion_event signature from void
to an int/bool return and return non-zero on failure, or set a clear ingester
error flag) so the caller can abort/rollback; ensure references to
prot_queue_push, writer_inbox, msg, and NDB_WRITER_DELETE_NOTE are updated
accordingly.
Comment on lines
+4081
to
+4089
| existing = ndb_get_note_meta(txn, target_id); | ||
| if (existing) { | ||
| sz = ndb_note_meta_total_size(existing); | ||
| if (sz > sizeof(scratch)) | ||
| return 0; | ||
| memcpy(scratch, existing, sz); | ||
| existing = (struct ndb_note_meta *)scratch; | ||
| *ndb_note_meta_flags(existing) |= NDB_NOTE_META_FLAG_DELETED; | ||
| return ndb_writer_set_note_meta(txn, target_id, existing); |
There was a problem hiding this comment.
Don't hard-cap metadata cloning at 4 KiB.
If an existing metadata record grows past scratch, this returns 0 and the delete silently stops applying for that note. High-activity notes are exactly the ones most likely to accumulate larger metadata, so this turns soft-delete into a size-dependent failure. Allocate sz dynamically here, or reuse a caller-provided scratch buffer sized for the record.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/nostrdb.c` around lines 4081 - 4089, The code aborts silently when an
existing note meta exceeds the fixed scratch buffer (4 KiB); replace the static
scratch usage with a dynamic allocation sized to sz: call
ndb_note_meta_total_size(existing) into sz, malloc(sz) (or use caller-provided
buffer), check allocation, memcpy into the allocated buffer, cast to struct
ndb_note_meta *existing, set the NDB_NOTE_META_FLAG_DELETED via
ndb_note_meta_flags(existing), call ndb_writer_set_note_meta(txn, target_id,
existing), and free the allocation on error or after use; update the code around
the ndb_get_note_meta / ndb_note_meta_total_size / ndb_note_meta_flags /
ndb_writer_set_note_meta sequence accordingly.
Comment on lines
+4092
to
+4103
| // no existing metadata: write a minimal header with DELETED set | ||
| ndb_note_meta_header_init(&hdr); | ||
| hdr.flags |= NDB_NOTE_META_FLAG_DELETED; | ||
| k.mv_data = (unsigned char *)target_id; | ||
| k.mv_size = 32; | ||
| v.mv_data = &hdr; | ||
| v.mv_size = sizeof(hdr); | ||
| if ((rc = mdb_put(txn->mdb_txn, txn->lmdb->dbs[NDB_DB_META], &k, &v, 0))) { | ||
| ndb_debug("apply_deletion: mdb_put failed: %s\n", mdb_strerror(rc)); | ||
| return 0; | ||
| } | ||
| return 1; |
There was a problem hiding this comment.
Preemptive tombstones lose the data needed for later auth checks.
When the target note is missing, this persists only the deleted bit and drops deleter_pubkey. If the real note arrives later, there is no remaining information to verify that the tombstone author matches the note author, so a forged kind-5 received first can permanently hide another user's note. This needs a pending-delete representation that preserves the deleter identity until the target is written and validated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/nostrdb.c` around lines 4092 - 4103, The tombstone write currently sets
only the DELETED flag and loses the deleter identity; modify the apply_deletion
path that calls ndb_note_meta_header_init(&hdr) so it copies the deleter's
public key into the header (e.g., hdr.deleter_pubkey = <deleter_pubkey source>
or memcpy into hdr.deleter_pubkey) before the mdb_put, keep v.mv_size =
sizeof(hdr), and ensure the stored header field (hdr.deleter_pubkey) is later
consulted by the note-write/auth check logic (the code paths that validate
incoming notes against existing meta) so a pending-delete preserves the
deleter's identity for future verification.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NDB_NOTE_META_FLAG_DELETEDon the referenced notes' metadata;ndb_filter_matches_withhides deleted notes from queries while the kind-5 event itself remains queryable.enum ndb_note_meta_flagsdiscovered along the way.Closes #129.
Changes
src/metadata.h—NDB_NOTE_META_FLAG_*values were bit positions (0, 2, 4) but were used as bitmasks atnostrdb.c:8384,8465. Pre-shifted to(1<<0), (1<<2), (1<<4)so direct|=/&work. Even bits reserved for system flags; odd bits for user-defined.src/nostrdb.cndb_filter_matches_withnow takes astruct ndb_txn *. When non-NULL, it short-circuits to 0 for notes flagged DELETED (kind-5s still pass). Publicndb_filter_matches/ndb_filter_matches_with_relaypreserve their signatures and pass NULL.NDB_WRITER_DELETE_NOTEwriter message;ndb_writer_apply_deletionlooks up the target, verifies author when present, and ORs inNDB_NOTE_META_FLAG_DELETED(or writes a minimal header with the flag).ndb_ingester_process_notegains a kind-5 branch that emits oneDELETE_NOTEperetag and still queues the kind-5 itself asNDB_WRITER_NOTE.test.c—test_delete_soft,test_delete_wrong_author,test_delete_out_of_order.Trade-offs
data_tablemechanism is fleshed out. Filed as follow-up work if needed.ZAP_VERIFIEDon-disk bit moves 2 → 4 as part of the bitmask fix. Records written under the old buggy|= 4(which actually set bit 2) appear unverified and will re-verify on next pass. No migration needed.a-tag deletions (NIP-09 addressable events) are not implemented in this PR.Test plan
make && ./test— full suite passes, including newtest_delete_*cases🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests