Social graph#98
Conversation
|
@mmalmi when you ran this, what was the total storage size for nostr's social graph (from your perspective, or otherwise)? |
|
@jb55 agent meta-review: Findings
Positive Aspects
Recommended Actions Before merge (Critical/High):
Can defer (Low):
|
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces a complete social graph subsystem to nostrdb, enabling tracking of follow and mute relationships with configurable root-based distance computation. The implementation uses bucketed UID containers for memory efficiency, LMDB for persistent indexing, and integrates seamlessly into the event ingestion pipeline. ChangesSocial Graph Subsystem
Sequence DiagramsequenceDiagram
participant Ingester
participant NDB as Social Graph
participant FollowDB as LMDB (Follow Index)
participant DistanceDB as LMDB (Distance Index)
Ingester->>NDB: ingest contact list event (kind 3)
NDB->>NDB: parse p-tags to new UID list
NDB->>FollowDB: load previous followed list
NDB->>NDB: diff new vs previous follows
NDB->>FollowDB: remove unfollows from follower index
NDB->>FollowDB: decrement follower counts
NDB->>FollowDB: add new follows to follower index
NDB->>FollowDB: increment follower counts
NDB->>DistanceDB: incrementally update follow distances
FollowDB-->>NDB: index updated
DistanceDB-->>NDB: distances computed
Note over NDB: Writer thread (set_root message)
NDB->>NDB: ndb_sg_set_root(new_root_pubkey)
NDB->>NDB: recalculate_distances (BFS from root)
NDB->>DistanceDB: repopulate all distance entries
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Depends how much you crawl, but I think I tested with 100k users or so |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (7)
src/bucketed_u32_list.h (2)
11-11: ⚡ Quick winClarify legacy format detection comment.
The comment states legacy format is "detected when u8_offset > count" but line 14 and the implementation (line 216 in bucketed_u32_list.c) actually detect it by checking
u8_offset == LEGACY_UID_LIST_MARKER (0xFFFF). Update the comment to match the implementation.🤖 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/bucketed_u32_list.h` at line 11, Update the header comment for Format v1 to match the implementation: replace "detected when u8_offset > count" with "detected when u8_offset == LEGACY_UID_LIST_MARKER (0xFFFF)" so it matches the check performed on u8_offset (see LEGACY_UID_LIST_MARKER and the detection logic in bucketed_u32_list.c).
16-16: ⚡ Quick winRemove incorrect "(unused)" annotation.
The comment says
u32_offsetis "unused" but it's actually used inuid_list_size()(line 171 in bucketed_u32_list.c) to calculate the serialized size. The field stores the end offset of the data buffer.🤖 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/bucketed_u32_list.h` at line 16, The comment on the struct field u32_offset is incorrect — it is used (e.g., by uid_list_size()) to compute the serialized size; remove the "(unused)" annotation and update the comment to accurately describe the field as the end offset of the data buffer in bytes (i.e., the byte offset to the end of the u32 data), referencing the u32_offset field and uid_list_size() to validate usage.src/bucketed_u32_list.c (1)
82-90: ⚖️ Poor tradeoffConsider qsort for larger lists.
The bubble sort is O(n²) and runs on every add/remove. For typical social graph sizes (hundreds of follows/mutes per user) this is acceptable, but if lists can grow to thousands, replacing with qsort would improve performance.
🤖 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/bucketed_u32_list.c` around lines 82 - 90, The current O(n^2) bubble-sort over temp_uids (using list->count and ndb_uid_t) should be replaced with qsort to improve performance on large lists: implement a comparator for ndb_uid_t (e.g., compare two const ndb_uid_t* values), call qsort(temp_uids, list->count, sizeof(ndb_uid_t), comparator) in place of the nested loops, and add the necessary `#include` <stdlib.h>; ensure the comparator uses the same type (ndb_uid_t) and that the sorted array semantics remain identical to the original code.src/ndb_socialgraph.c (1)
705-705: 💤 Low valueMagic number 1000 for unknown distance should be a named constant.
Lines 705 and 711 return
1000to indicate that a user is not in the graph. Define a named constant (e.g.,NDB_FOLLOW_DISTANCE_UNKNOWNorNDB_FOLLOW_DISTANCE_MAX) to improve readability and make the sentinel value easy to change.Also applies to: 711-711
🤖 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/ndb_socialgraph.c` at line 705, Replace the magic literal 1000 used to indicate "unknown/not in graph" with a descriptive constant: add a definition like NDB_FOLLOW_DISTANCE_UNKNOWN (or NDB_FOLLOW_DISTANCE_MAX) at the top of src/ndb_socialgraph.c or the appropriate header, then change the two return sites in the functions around the current return statements (the returns at/near the code that currently returns 1000) to return that constant instead; also search the file for any other occurrences of the literal 1000 used as a sentinel and use the new constant so the sentinel is centralized and easy to change.test_socialgraph.c (1)
49-52: ⚖️ Poor tradeoffConsider replacing fixed
usleep()calls with polling or synchronization.Ten test locations rely on fixed-duration
usleep()calls (200ms, 500ms, 100ms) to wait for asynchronous event ingestion before asserting on social graph state. This pattern can cause flaky failures on slow systems or under load when processing takes longer than expected.For more robust tests, consider one of:
- Polling with timeout: loop checking
ndb_uid_exists()or target state with a max wait time- Synchronous flush API: if feasible, add a test-only API to block until the ingester queue is empty
- Tunable wait: environment variable to scale wait duration for CI vs local development
This is a test quality improvement and does not affect production correctness.
Also applies to: 122-125, 178-181, 245-246, 260-261, 328-330, 341-342, 353-354, 395-396, 399-400
🤖 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 `@test_socialgraph.c` around lines 49 - 52, Tests use fixed usleep delays after calling ndb_process_event(ndb, ...) which is flaky; replace each usleep(200/500/100ms) with a polling-with-timeout loop that repeatedly checks the expected ingestion result (e.g. call ndb_uid_exists(ndb, expected_uid) or the specific verification used later in the test) sleeping short intervals (e.g. 10-50ms) and failing if a configurable timeout elapses; alternatively add/support a test-only synchronous flush helper (e.g. ndb_wait_ingester_empty or similar) and call that after ndb_process_event, and make the timeout tunable via an environment variable so CI can increase wait without hardcoded usleep.src/nostrdb.h (1)
793-793: ⚡ Quick winDocument the
ndb_deletefunction.This new function declaration lacks any documentation. Please add a doc comment explaining its purpose, parameters, return value semantics, and any thread-safety or transactional requirements—matching the documentation standard of the social graph APIs above (lines 585-720).
🤖 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.h` at line 793, Add a doc comment above the declaration of ndb_delete that follows the existing social graph API style: describe that ndb_delete removes records from the given ndb matching the provided array of ndb_filter (filters, num_filters) and explain that count is an out-parameter set to the number of items deleted; document the meaning of the int return value (0 on success, negative errno-style error codes on failure), any validation errors (e.g., NULL pointers or empty filter arrays), and whether the operation is atomic/transactional and thread-safe (e.g., note if callers must hold external locks or if the implementation provides internal synchronization). Include parameter descriptions for struct ndb *ndb, struct ndb_filter *filters, int num_filters, and int *count, plus a brief examples/notes line if the surrounding API uses it.Makefile (1)
221-221: ⚡ Quick winAdd
testandtest_socialgraphto.PHONYdeclaration.Both
test(line 199) andtest_socialgraph(line 204) are non-file targets and should be declared phony to ensure Make always executes them regardless of filesystem state.📝 Suggested fix
-.PHONY: tags clean fake update-ccan +.PHONY: tags clean fake update-ccan test test_socialgraph🤖 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 `@Makefile` at line 221, The .PHONY declaration is missing the non-file targets "test" and "test_socialgraph", so update the .PHONY line (current declaration containing "tags clean fake update-ccan") to also include the targets test and test_socialgraph so Make treats them as phony and always runs them; locate the .PHONY line in the Makefile and append test and test_socialgraph to that symbol list.Source: Linters/SAST tools
🤖 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/bucketed_u32_list.c`:
- Around line 199-201: The function uid_list_remove_at silently returns on
malloc failure (temp_uids) which can break caller assumptions; change
uid_list_remove_at to return an int status (0 on success, -1 on allocation
failure), propagate that return value to callers, and on malloc failure
immediately return -1 instead of a void return; update all callers to check the
return and handle failure (or document and add assertion checks), and ensure any
early exits leave the uid list in a consistent state.
In `@src/ndb_socialgraph.c`:
- Line 461: The call to mdb_del(txn, followers_by_user_dbi, &composite_key,
NULL) must have its return value checked; if mdb_del returns non-zero, abort the
transaction and propagate the error rather than continuing. Update the code
around the mdb_del call (reference txn, followers_by_user_dbi, composite_key) to
capture the returned rc, call mdb_txn_abort(txn) (or the existing txn rollback
helper used elsewhere in this file) and return or propagate rc so the follower
count update does not proceed on failure.
- Around line 1017-1051: The loop over new_muted_list is incrementing muter
counts for entries that may already exist; add an "is_new" check like in
handle_contact_list so only truly new mutes get the composite insert and count
bump: before creating struct muter_key mkey and calling mdb_put on
user_muted_by_dbi, compute int is_new = 1; if (old_muted_list &&
uid_list_contains(old_muted_list, muted_uid)) is_new = 0; and if (!is_new)
continue; this ensures only new entries update muter_count_dbi and avoids
duplicate increments.
- Line 476: The mdb_put calls updating follower_count_dbi (using txn,
&count_key, &count_val) are unchecked; capture the return code (rc) for each
mdb_put call, and on non-zero: abort the transaction (mdb_txn_abort(txn)),
log/propagate the error, and return or propagate rc so the caller knows the
write failed; apply the same check-and-abort logic to both occurrences that
touch follower_count_dbi (the mdb_put at the shown line and the other at line
525).
- Line 997: The mdb_del and mdb_put calls in the mute handling (specifically the
mdb_del(txn, user_muted_by_dbi, &composite_key, NULL) and the following mdb_put
calls that update muter counts and reinsert composite keys) currently ignore
return values; wrap each call (mdb_del and each mdb_put that updates
muter_count_dbi or user_muted_by_dbi) to check the returned int against
MDB_SUCCESS, and on any non-success return abort/cleanup the transaction and
propagate an error (e.g., call mdb_txn_abort(txn) or return the error code) so
the transaction does not commit partial state; update the code paths around
txn/mdb_del, mdb_put for muter_count_dbi, mdb_put for composite_key, and the
final mdb_put for count to perform this check and handle failure consistently.
- Around line 94-137: add_follower_incremental (and its counterpart
remove_follower_incremental) only updates the immediate user's distance and
doesn't propagate changes to downstream nodes; replace the single-node update
with a BFS-style propagation: after computing and setting the new distance for
followed_uid (via update_follow_distance), enqueue followed_uid and iteratively
traverse its outgoing adjacency (use the graph's follower/adjacency DB accessed
from the ndb_socialgraph struct), for each neighbor compute candidate_distance =
current_node_distance + 1, compare with stored distance in follow_distance_db
and if improved call update_follow_distance and push that neighbor onto the
queue; similarly ensure remove_follower_incremental performs the same cascade
logic when distances increase, and avoid infinite loops by tracking visited
nodes or only enqueuing when a strict improvement occurs.
In `@src/ndb_uid.c`:
- Line 117: The increment of map->next_id in ndb_uid_get_or_create is not atomic
and can cause duplicate UIDs under concurrent ingester threads; fix by either
routing all UID allocation and social-graph writes through the single writer
thread (so ndb_uid_get_or_create is only called by that thread) or make next_id
updates atomic and handle DB failures: replace non-atomic map->next_id++ with an
atomic increment (e.g., atomic_fetch_add on map->next_id) and, after calling
mdb_put for str_to_id/id_to_str, detect transaction failures and roll back the
increment (decrement or otherwise reclaim the reserved ID) so failed mdb_put
does not permanently consume an ID. Ensure the change consistently updates both
the in-memory map and MDB operations in the ndb_uid_get_or_create code path.
- Around line 117-137: The code in ndb_uid_get_or_create increments map->next_id
into new_uid before performing two mdb_puts (str_to_id_dbi and id_to_str_dbi),
which can leave an orphaned mapping or leak UIDs if one put fails; change the
flow to either perform both puts inside a nested/child LMDB transaction and
commit only on success (roll back on any mdb_put error) using mdb_txn or similar
transaction APIs, or delay advancing map->next_id until after both inserts
succeed by using a temporary new_uid candidate and only assign/increment
map->next_id at the end; ensure error handling for mdb_put failures cleans up
any partial state and does not permanently consume the UID.
In `@src/nostrdb.c`:
- Around line 3326-3343: The current handling of note kinds 3 and 10000 starts
and commits separate LMDB write transactions around
ndb_socialgraph_handle_contact_list and ndb_socialgraph_handle_mute_list, which
can persist social-graph changes even if the main writer fails; instead, remove
the immediate mdb_txn_begin/mdb_txn_commit here and queue the contact/mute
processing to the writer thread (the same path that enqueues to NDB_WRITER_NOTE)
so those handlers run inside the writer's RW transaction; ensure you pass the
note and ingester->socialgraph into the queued work and make the writer abort
the entire transaction if ndb_socialgraph_handle_contact_list or
ndb_socialgraph_handle_mute_list return failure (non-zero), so no social-graph
mutation is committed separately from the note.
- Around line 5611-5614: Currently the code calls ndb_migrate_metadata(&txn) on
every delete which rebuilds the entire metadata table; instead implement and
call a targeted metadata update routine (e.g., ndb_remove_metadata_for_note or
ndb_update_metadata_on_delete) that removes or adjusts only the metadata entries
for the deleted note within the same txn, and call that here; only fall back to
ndb_migrate_metadata(&txn) when the targeted update reports an inconsistency or
full-rebuild-needed condition. Replace the direct ndb_migrate_metadata call with
the new targeted function, keep mdb_txn_abort(txn.mdb_txn) logic for error
paths, and ensure the targeted routine accepts the txn handle and the deleted
note id so the delete scales with affected metadata rather than the whole DB.
- Around line 3335-3342: The mute-list handler is currently incrementing
muter_count for every UID in the incoming kind-10000 note regardless of whether
the mute already existed; update ndb_socialgraph_handle_mute_list so it compares
the incoming mute set from note with the stored mute set (using the same MDB_txn
passed in) and only increment muter_count for UIDs that are newly added, and
similarly decrement muter_count for UIDs that were removed; use the provided txn
(the void* txn passed from the note->kind == 10000 branch) and socialgraph state
(ingester->socialgraph) to look up existing mutes and apply only the delta
updates to muter_count.
In `@test_bucketed_uid_list.c`:
- Around line 1-224: This test file duplicates production definitions
(ndb_uid_t, struct uid_list) and functions (uid_list_create, uid_list_contains,
uid_list_add, uid_list_rebuild_buckets, uid_list_get, uid_list_remove_at), so
remove the duplicated implementations and instead include the production header
and reuse the production implementation; specifically delete the typedef, struct
and all function definitions from the test and add `#include`
"bucketed_u32_list.h" at top, then link against the compiled bucketed_u32_list.o
(or, if unavoidable for the test build, `#include` "bucketed_u32_list.c") so the
test references the existing ndb_uid_t and the functions (uid_list_create,
uid_list_add, uid_list_contains, uid_list_get, uid_list_remove_at) without
duplicating code.
---
Nitpick comments:
In `@Makefile`:
- Line 221: The .PHONY declaration is missing the non-file targets "test" and
"test_socialgraph", so update the .PHONY line (current declaration containing
"tags clean fake update-ccan") to also include the targets test and
test_socialgraph so Make treats them as phony and always runs them; locate the
.PHONY line in the Makefile and append test and test_socialgraph to that symbol
list.
In `@src/bucketed_u32_list.c`:
- Around line 82-90: The current O(n^2) bubble-sort over temp_uids (using
list->count and ndb_uid_t) should be replaced with qsort to improve performance
on large lists: implement a comparator for ndb_uid_t (e.g., compare two const
ndb_uid_t* values), call qsort(temp_uids, list->count, sizeof(ndb_uid_t),
comparator) in place of the nested loops, and add the necessary `#include`
<stdlib.h>; ensure the comparator uses the same type (ndb_uid_t) and that the
sorted array semantics remain identical to the original code.
In `@src/bucketed_u32_list.h`:
- Line 11: Update the header comment for Format v1 to match the implementation:
replace "detected when u8_offset > count" with "detected when u8_offset ==
LEGACY_UID_LIST_MARKER (0xFFFF)" so it matches the check performed on u8_offset
(see LEGACY_UID_LIST_MARKER and the detection logic in bucketed_u32_list.c).
- Line 16: The comment on the struct field u32_offset is incorrect — it is used
(e.g., by uid_list_size()) to compute the serialized size; remove the "(unused)"
annotation and update the comment to accurately describe the field as the end
offset of the data buffer in bytes (i.e., the byte offset to the end of the u32
data), referencing the u32_offset field and uid_list_size() to validate usage.
In `@src/ndb_socialgraph.c`:
- Line 705: Replace the magic literal 1000 used to indicate "unknown/not in
graph" with a descriptive constant: add a definition like
NDB_FOLLOW_DISTANCE_UNKNOWN (or NDB_FOLLOW_DISTANCE_MAX) at the top of
src/ndb_socialgraph.c or the appropriate header, then change the two return
sites in the functions around the current return statements (the returns at/near
the code that currently returns 1000) to return that constant instead; also
search the file for any other occurrences of the literal 1000 used as a sentinel
and use the new constant so the sentinel is centralized and easy to change.
In `@src/nostrdb.h`:
- Line 793: Add a doc comment above the declaration of ndb_delete that follows
the existing social graph API style: describe that ndb_delete removes records
from the given ndb matching the provided array of ndb_filter (filters,
num_filters) and explain that count is an out-parameter set to the number of
items deleted; document the meaning of the int return value (0 on success,
negative errno-style error codes on failure), any validation errors (e.g., NULL
pointers or empty filter arrays), and whether the operation is
atomic/transactional and thread-safe (e.g., note if callers must hold external
locks or if the implementation provides internal synchronization). Include
parameter descriptions for struct ndb *ndb, struct ndb_filter *filters, int
num_filters, and int *count, plus a brief examples/notes line if the surrounding
API uses it.
In `@test_socialgraph.c`:
- Around line 49-52: Tests use fixed usleep delays after calling
ndb_process_event(ndb, ...) which is flaky; replace each usleep(200/500/100ms)
with a polling-with-timeout loop that repeatedly checks the expected ingestion
result (e.g. call ndb_uid_exists(ndb, expected_uid) or the specific verification
used later in the test) sleeping short intervals (e.g. 10-50ms) and failing if a
configurable timeout elapses; alternatively add/support a test-only synchronous
flush helper (e.g. ndb_wait_ingester_empty or similar) and call that after
ndb_process_event, and make the timeout tunable via an environment variable so
CI can increase wait without hardcoded usleep.
🪄 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: 89f7af5e-5a24-46e2-a644-57404eb801f9
📒 Files selected for processing (14)
.gitignoreMakefileSOCIAL_GRAPH.mdsrc/bucketed_u32_list.csrc/bucketed_u32_list.hsrc/ndb_socialgraph.csrc/ndb_socialgraph.hsrc/ndb_uid.csrc/ndb_uid.hsrc/nostrdb.csrc/nostrdb.htest.ctest_bucketed_uid_list.ctest_socialgraph.c
| ndb_uid_t *temp_uids = malloc(list->count * sizeof(ndb_uid_t)); | ||
| if (!temp_uids) | ||
| return; |
There was a problem hiding this comment.
Silent malloc failure may cause inconsistency.
If malloc fails, uid_list_remove_at returns early without modifying the list or notifying the caller. Since the function returns void, callers cannot detect this failure. If a caller expects a UID to be removed (e.g., to maintain index consistency), the silent failure could lead to subtle bugs.
Consider either changing the signature to int uid_list_remove_at(...) to return success/failure, or documenting that removal may silently fail and callers should verify via uid_list_contains afterward.
🤖 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/bucketed_u32_list.c` around lines 199 - 201, The function
uid_list_remove_at silently returns on malloc failure (temp_uids) which can
break caller assumptions; change uid_list_remove_at to return an int status (0
on success, -1 on allocation failure), propagate that return value to callers,
and on malloc failure immediately return -1 instead of a void return; update all
callers to check the return and handle failure (or document and add assertion
checks), and ensure any early exits leave the uid list in a consistent state.
| // Incremental: add follower relationship and update distance | ||
| static int add_follower_incremental(void *txn, struct ndb_socialgraph *graph, | ||
| ndb_uid_t followed_uid, ndb_uid_t follower_uid) | ||
| { | ||
| MDB_dbi distance_dbi = (MDB_dbi)(intptr_t)graph->follow_distance_db; | ||
| MDB_val key, val; | ||
| int rc; | ||
|
|
||
| // Don't process if followed_uid is root | ||
| if (followed_uid == graph->root_uid) | ||
| return 1; | ||
|
|
||
| uint32_t new_distance; | ||
|
|
||
| // If follower is root, set followed to distance 1 | ||
| if (follower_uid == graph->root_uid) { | ||
| new_distance = 1; | ||
| } else { | ||
| // Get follower's distance | ||
| key.mv_data = &follower_uid; | ||
| key.mv_size = sizeof(ndb_uid_t); | ||
| if ((rc = mdb_get(txn, distance_dbi, &key, &val)) != 0) { | ||
| // Follower has no distance, can't establish path | ||
| return 1; | ||
| } | ||
| uint32_t follower_distance; | ||
| memcpy(&follower_distance, val.mv_data, sizeof(uint32_t)); | ||
| new_distance = follower_distance + 1; | ||
| } | ||
|
|
||
| // Get current distance of followed user | ||
| key.mv_data = &followed_uid; | ||
| key.mv_size = sizeof(ndb_uid_t); | ||
| uint32_t old_distance = UINT32_MAX; | ||
| if ((rc = mdb_get(txn, distance_dbi, &key, &val)) == 0) { | ||
| memcpy(&old_distance, val.mv_data, sizeof(uint32_t)); | ||
| // Only update if new distance is better | ||
| if (new_distance >= old_distance) | ||
| return 1; | ||
| } | ||
|
|
||
| // Update distance | ||
| return update_follow_distance(txn, graph, followed_uid, old_distance, new_distance); | ||
| } |
There was a problem hiding this comment.
Incomplete incremental distance updates affect both add and remove follower paths.
Both add_follower_incremental and remove_follower_incremental update only the immediate target user's distance without propagating changes downstream. When a follow/unfollow changes a user's distance, all users transitively reachable through that user should also be recalculated, but the current implementation stops at the first hop.
Shared root cause: Incremental updates lack cascading logic. Consider implementing a BFS-based propagation from the modified node, or document that periodic full recalculations are required to maintain accurate distances.
🤖 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/ndb_socialgraph.c` around lines 94 - 137, add_follower_incremental (and
its counterpart remove_follower_incremental) only updates the immediate user's
distance and doesn't propagate changes to downstream nodes; replace the
single-node update with a BFS-style propagation: after computing and setting the
new distance for followed_uid (via update_follow_distance), enqueue followed_uid
and iteratively traverse its outgoing adjacency (use the graph's
follower/adjacency DB accessed from the ndb_socialgraph struct), for each
neighbor compute candidate_distance = current_node_distance + 1, compare with
stored distance in follow_distance_db and if improved call
update_follow_distance and push that neighbor onto the queue; similarly ensure
remove_follower_incremental performs the same cascade logic when distances
increase, and avoid infinite loops by tracking visited nodes or only enqueuing
when a strict improvement occurs.
| composite_key.mv_data = &fkey; | ||
| composite_key.mv_size = sizeof(fkey); | ||
|
|
||
| mdb_del(txn, followers_by_user_dbi, &composite_key, NULL); |
There was a problem hiding this comment.
Unchecked mdb_del return value.
Line 461 calls mdb_del to remove a follower composite key but doesn't check the return value. If the deletion fails (e.g., due to a transaction conflict or database error), the follower index will remain inconsistent with the followed_by_user list, and the follower count update will proceed incorrectly.
Check the return value and abort the transaction on failure.
🤖 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/ndb_socialgraph.c` at line 461, The call to mdb_del(txn,
followers_by_user_dbi, &composite_key, NULL) must have its return value checked;
if mdb_del returns non-zero, abort the transaction and propagate the error
rather than continuing. Update the code around the mdb_del call (reference txn,
followers_by_user_dbi, composite_key) to capture the returned rc, call
mdb_txn_abort(txn) (or the existing txn rollback helper used elsewhere in this
file) and return or propagate rc so the follower count update does not proceed
on failure.
| } | ||
| count_val.mv_data = &count; | ||
| count_val.mv_size = sizeof(uint32_t); | ||
| mdb_put(txn, follower_count_dbi, &count_key, &count_val, 0); |
There was a problem hiding this comment.
Unchecked mdb_put return values for follower count updates.
Lines 476 and 525 call mdb_put to update follower counts but don't check the return value. If the write fails, the count will be out of sync with the actual follower composite index.
Check return values and abort the transaction on failure to maintain consistency.
Also applies to: 525-525
🤖 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/ndb_socialgraph.c` at line 476, The mdb_put calls updating
follower_count_dbi (using txn, &count_key, &count_val) are unchecked; capture
the return code (rc) for each mdb_put call, and on non-zero: abort the
transaction (mdb_txn_abort(txn)), log/propagate the error, and return or
propagate rc so the caller knows the write failed; apply the same
check-and-abort logic to both occurrences that touch follower_count_dbi (the
mdb_put at the shown line and the other at line 525).
| composite_key.mv_data = &mkey; | ||
| composite_key.mv_size = sizeof(mkey); | ||
|
|
||
| mdb_del(txn, user_muted_by_dbi, &composite_key, NULL); |
There was a problem hiding this comment.
Unchecked mdb_del and mdb_put return values in mute handling.
Lines 997 (mdb_del), 1012 (mdb_put for count), 1032 (mdb_put for composite key), and 1050 (mdb_put for count) don't check return values. If any of these writes fail, the mute index and muter counts will be inconsistent with the muted_by_user list.
Check return values and abort the transaction on failure.
Also applies to: 1012-1012, 1032-1032, 1050-1050
🤖 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/ndb_socialgraph.c` at line 997, The mdb_del and mdb_put calls in the mute
handling (specifically the mdb_del(txn, user_muted_by_dbi, &composite_key, NULL)
and the following mdb_put calls that update muter counts and reinsert composite
keys) currently ignore return values; wrap each call (mdb_del and each mdb_put
that updates muter_count_dbi or user_muted_by_dbi) to check the returned int
against MDB_SUCCESS, and on any non-success return abort/cleanup the transaction
and propagate an error (e.g., call mdb_txn_abort(txn) or return the error code)
so the transaction does not commit partial state; update the code paths around
txn/mdb_del, mdb_put for muter_count_dbi, mdb_put for composite_key, and the
final mdb_put for count to perform this check and handle failure consistently.
| } else if (note->kind == 3) { | ||
| // process contact list for social graph | ||
| if (ingester->socialgraph) { | ||
| MDB_txn *txn; | ||
| if (mdb_txn_begin(ingester->lmdb->env, NULL, 0, &txn) == 0) { | ||
| ndb_socialgraph_handle_contact_list((void*)txn, ingester->socialgraph, note); | ||
| mdb_txn_commit(txn); | ||
| } | ||
| } | ||
| } else if (note->kind == 10000) { | ||
| // process mute list for social graph | ||
| if (ingester->socialgraph) { | ||
| MDB_txn *txn; | ||
| if (mdb_txn_begin(ingester->lmdb->env, NULL, 0, &txn) == 0) { | ||
| ndb_socialgraph_handle_mute_list((void*)txn, ingester->socialgraph, note); | ||
| mdb_txn_commit(txn); | ||
| } | ||
| } |
There was a problem hiding this comment.
Keep social-graph mutations in the writer transaction.
This block commits a separate RW transaction before the note is enqueued to NDB_WRITER_NOTE, and it commits even when ndb_socialgraph_handle_contact_list() / ndb_socialgraph_handle_mute_list() returns 0. That lets follow/mute state persist without the source note, or partially persist after an internal write failure. Queue these updates to the writer thread and abort on handler failure instead of committing unconditionally.
🤖 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 3326 - 3343, The current handling of note kinds 3
and 10000 starts and commits separate LMDB write transactions around
ndb_socialgraph_handle_contact_list and ndb_socialgraph_handle_mute_list, which
can persist social-graph changes even if the main writer fails; instead, remove
the immediate mdb_txn_begin/mdb_txn_commit here and queue the contact/mute
processing to the writer thread (the same path that enqueues to NDB_WRITER_NOTE)
so those handlers run inside the writer's RW transaction; ensure you pass the
note and ingester->socialgraph into the queued work and make the writer abort
the entire transaction if ndb_socialgraph_handle_contact_list or
ndb_socialgraph_handle_mute_list return failure (non-zero), so no social-graph
mutation is committed separately from the note.
| } else if (note->kind == 10000) { | ||
| // process mute list for social graph | ||
| if (ingester->socialgraph) { | ||
| MDB_txn *txn; | ||
| if (mdb_txn_begin(ingester->lmdb->env, NULL, 0, &txn) == 0) { | ||
| ndb_socialgraph_handle_mute_list((void*)txn, ingester->socialgraph, note); | ||
| mdb_txn_commit(txn); | ||
| } |
There was a problem hiding this comment.
Kind-10000 ingestion currently inflates muter_count.
The handler this path calls increments muter_count for every UID in the new mute list, not just newly added mutes. Reprocessing a newer mute list with unchanged entries will keep increasing counts even though no new muter was added.
🤖 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 3335 - 3342, The mute-list handler is currently
incrementing muter_count for every UID in the incoming kind-10000 note
regardless of whether the mute already existed; update
ndb_socialgraph_handle_mute_list so it compares the incoming mute set from note
with the stored mute set (using the same MDB_txn passed in) and only increment
muter_count for UIDs that are newly added, and similarly decrement muter_count
for UIDs that were removed; use the provided txn (the void* txn passed from the
note->kind == 10000 branch) and socialgraph state (ingester->socialgraph) to
look up existing mutes and apply only the delta updates to muter_count.
| static int ndb_delete_note_indexes(struct ndb_txn *txn, struct ndb_note *note, uint64_t note_key) | ||
| { | ||
| MDB_val key, val; | ||
| struct ndb_tsid tsid; | ||
| struct ndb_u64_ts kind_key; | ||
| struct ndb_id_u64_ts pubkey_kind_key; | ||
|
|
||
| val.mv_data = ¬e_key; | ||
| val.mv_size = sizeof(note_key); | ||
|
|
||
| ndb_tsid_init(&tsid, note->id, note->created_at); | ||
| key.mv_data = &tsid; | ||
| key.mv_size = sizeof(tsid); | ||
| mdb_del(txn->mdb_txn, txn->lmdb->dbs[NDB_DB_NOTE_ID], &key, &val); | ||
|
|
||
| ndb_u64_ts_init(&kind_key, note->kind, note->created_at); | ||
| key.mv_data = &kind_key; | ||
| key.mv_size = sizeof(kind_key); | ||
| mdb_del(txn->mdb_txn, txn->lmdb->dbs[NDB_DB_NOTE_KIND], &key, &val); | ||
|
|
||
| ndb_tsid_init(&tsid, note->pubkey, note->created_at); | ||
| key.mv_data = &tsid; | ||
| key.mv_size = sizeof(tsid); | ||
| mdb_del(txn->mdb_txn, txn->lmdb->dbs[NDB_DB_NOTE_PUBKEY], &key, &val); | ||
|
|
||
| ndb_id_u64_ts_init(&pubkey_kind_key, note->pubkey, note->kind, note->created_at); | ||
| key.mv_data = &pubkey_kind_key; | ||
| key.mv_size = sizeof(pubkey_kind_key); | ||
| mdb_del(txn->mdb_txn, txn->lmdb->dbs[NDB_DB_NOTE_PUBKEY_KIND], &key, &val); | ||
|
|
||
| ndb_delete_note_tag_index(txn, note, note_key); | ||
| ndb_delete_note_text_index(txn, note, note_key); | ||
| ndb_delete_note_relay_indexes(txn, note, note_key); | ||
| ndb_delete_profile_for_note(txn, note, note_key); | ||
|
|
||
| key.mv_data = ¬e_key; | ||
| key.mv_size = sizeof(note_key); | ||
| mdb_del(txn->mdb_txn, txn->lmdb->dbs[NDB_DB_NOTE_BLOCKS], &key, NULL); | ||
|
|
||
| key.mv_data = note->id; | ||
| key.mv_size = 32; | ||
| mdb_del(txn->mdb_txn, txn->lmdb->dbs[NDB_DB_META], &key, NULL); | ||
|
|
||
| return 1; |
There was a problem hiding this comment.
Deleting contact/mute notes leaves derived graph state behind.
ndb_delete_note_indexes() removes the note and every note-side index, but it never reverts or rebuilds ndb->socialgraph. Since kind 3 and 10000 notes are applied into the graph during ingest, deleting those notes leaves follower/muter counts and distance state with no backing event.
| if (!ndb_migrate_metadata(&txn)) { | ||
| mdb_txn_abort(txn.mdb_txn); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Avoid rebuilding all metadata on every delete.
Calling ndb_migrate_metadata() here drops and rebuilds the entire metadata table for every delete operation. On a large DB that makes a targeted delete scale with total note count and holds a write transaction for a full metadata rewrite.
🤖 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 5611 - 5614, Currently the code calls
ndb_migrate_metadata(&txn) on every delete which rebuilds the entire metadata
table; instead implement and call a targeted metadata update routine (e.g.,
ndb_remove_metadata_for_note or ndb_update_metadata_on_delete) that removes or
adjusts only the metadata entries for the deleted note within the same txn, and
call that here; only fall back to ndb_migrate_metadata(&txn) when the targeted
update reports an inconsistency or full-rebuild-needed condition. Replace the
direct ndb_migrate_metadata call with the new targeted function, keep
mdb_txn_abort(txn.mdb_txn) logic for error paths, and ensure the targeted
routine accepts the txn handle and the deleted note id so the delete scales with
affected metadata rather than the whole DB.
| #include <stdio.h> | ||
| #include <assert.h> | ||
| #include <stdlib.h> | ||
| #include <stdint.h> | ||
| #include <string.h> | ||
|
|
||
| typedef uint32_t ndb_uid_t; | ||
|
|
||
| struct uid_list { | ||
| uint32_t count; | ||
| uint16_t u8_offset; | ||
| uint16_t u16_offset; | ||
| uint32_t u32_offset; | ||
| unsigned char data[]; | ||
| }; | ||
|
|
||
| static int bsearch_u16(const uint16_t *arr, uint32_t count, uint16_t val) | ||
| { | ||
| uint32_t left = 0, right = count; | ||
| while (left < right) { | ||
| uint32_t mid = (left + right) / 2; | ||
| if (arr[mid] == val) | ||
| return 1; | ||
| if (arr[mid] < val) | ||
| left = mid + 1; | ||
| else | ||
| right = mid; | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| static int bsearch_u32(const uint32_t *arr, uint32_t count, uint32_t val) | ||
| { | ||
| uint32_t left = 0, right = count; | ||
| while (left < right) { | ||
| uint32_t mid = (left + right) / 2; | ||
| if (arr[mid] == val) | ||
| return 1; | ||
| if (arr[mid] < val) | ||
| left = mid + 1; | ||
| else | ||
| right = mid; | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| static int uid_list_contains(struct uid_list *list, ndb_uid_t uid) | ||
| { | ||
| if (list->count == 0) | ||
| return 0; | ||
|
|
||
| if (uid <= 0xFF) { | ||
| const uint8_t *u8_data = (const uint8_t*)list->data; | ||
| for (uint32_t i = 0; i < list->u8_offset; i++) { | ||
| if (u8_data[i] == (uint8_t)uid) | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } else if (uid <= 0xFFFF) { | ||
| const uint16_t *u16_data = (const uint16_t*)(list->data + list->u8_offset); | ||
| uint32_t u16_count = (list->u16_offset - list->u8_offset) / sizeof(uint16_t); | ||
| return bsearch_u16(u16_data, u16_count, (uint16_t)uid); | ||
| } else { | ||
| const uint32_t *u32_data = (const uint32_t*)(list->data + list->u16_offset); | ||
| uint32_t u32_count = (list->u32_offset - list->u16_offset) / sizeof(uint32_t); | ||
| return bsearch_u32(u32_data, u32_count, uid); | ||
| } | ||
| } | ||
|
|
||
| static struct uid_list *uid_list_create(uint32_t capacity) | ||
| { | ||
| size_t size = sizeof(struct uid_list) + capacity * sizeof(uint32_t); | ||
| struct uid_list *list = malloc(size); | ||
| if (list) { | ||
| list->count = 0; | ||
| list->u8_offset = 0; | ||
| list->u16_offset = 0; | ||
| list->u32_offset = 0; | ||
| } | ||
| return list; | ||
| } | ||
|
|
||
| static void uid_list_rebuild_buckets(struct uid_list *list, ndb_uid_t *temp_uids) | ||
| { | ||
| if (list->count == 0) { | ||
| list->u8_offset = 0; | ||
| list->u16_offset = 0; | ||
| list->u32_offset = 0; | ||
| return; | ||
| } | ||
|
|
||
| for (uint32_t i = 0; i < list->count - 1; i++) { | ||
| for (uint32_t j = i + 1; j < list->count; j++) { | ||
| if (temp_uids[i] > temp_uids[j]) { | ||
| ndb_uid_t tmp = temp_uids[i]; | ||
| temp_uids[i] = temp_uids[j]; | ||
| temp_uids[j] = tmp; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| uint32_t u8_count = 0, u16_count = 0, u32_count = 0; | ||
| for (uint32_t i = 0; i < list->count; i++) { | ||
| if (temp_uids[i] <= 0xFF) | ||
| u8_count++; | ||
| else if (temp_uids[i] <= 0xFFFF) | ||
| u16_count++; | ||
| else | ||
| u32_count++; | ||
| } | ||
|
|
||
| uint8_t *u8_ptr = (uint8_t*)list->data; | ||
| uint16_t *u16_ptr = (uint16_t*)(list->data + u8_count); | ||
| uint32_t *u32_ptr = (uint32_t*)(list->data + u8_count + u16_count * 2); | ||
|
|
||
| uint32_t u8_idx = 0, u16_idx = 0, u32_idx = 0; | ||
| for (uint32_t i = 0; i < list->count; i++) { | ||
| if (temp_uids[i] <= 0xFF) | ||
| u8_ptr[u8_idx++] = (uint8_t)temp_uids[i]; | ||
| else if (temp_uids[i] <= 0xFFFF) | ||
| u16_ptr[u16_idx++] = (uint16_t)temp_uids[i]; | ||
| else | ||
| u32_ptr[u32_idx++] = temp_uids[i]; | ||
| } | ||
|
|
||
| list->u8_offset = u8_count; | ||
| list->u16_offset = u8_count + u16_count * 2; | ||
| list->u32_offset = u8_count + u16_count * 2 + u32_count * 4; | ||
| } | ||
|
|
||
| static int uid_list_add(struct uid_list **list_ptr, uint32_t *capacity, ndb_uid_t uid) | ||
| { | ||
| struct uid_list *list = *list_ptr; | ||
|
|
||
| if (uid_list_contains(list, uid)) | ||
| return 1; | ||
|
|
||
| if (list->count >= *capacity) { | ||
| uint32_t new_capacity = *capacity * 2; | ||
| size_t new_size = sizeof(struct uid_list) + new_capacity * sizeof(uint32_t); | ||
| struct uid_list *new_list = realloc(list, new_size); | ||
| if (!new_list) | ||
| return 0; | ||
| *list_ptr = new_list; | ||
| *capacity = new_capacity; | ||
| list = new_list; | ||
| } | ||
|
|
||
| ndb_uid_t *temp_uids = malloc((list->count + 1) * sizeof(ndb_uid_t)); | ||
| if (!temp_uids) | ||
| return 0; | ||
|
|
||
| uint32_t idx = 0; | ||
| const uint8_t *u8_data = (const uint8_t*)list->data; | ||
| for (uint32_t i = 0; i < list->u8_offset; i++) | ||
| temp_uids[idx++] = u8_data[i]; | ||
|
|
||
| const uint16_t *u16_data = (const uint16_t*)(list->data + list->u8_offset); | ||
| uint32_t u16_count = (list->u16_offset - list->u8_offset) / sizeof(uint16_t); | ||
| for (uint32_t i = 0; i < u16_count; i++) | ||
| temp_uids[idx++] = u16_data[i]; | ||
|
|
||
| const uint32_t *u32_data = (const uint32_t*)(list->data + list->u16_offset); | ||
| uint32_t u32_count = (list->u32_offset - list->u16_offset) / sizeof(uint32_t); | ||
| for (uint32_t i = 0; i < u32_count; i++) | ||
| temp_uids[idx++] = u32_data[i]; | ||
|
|
||
| temp_uids[list->count] = uid; | ||
| list->count++; | ||
|
|
||
| uid_list_rebuild_buckets(list, temp_uids); | ||
|
|
||
| free(temp_uids); | ||
| return 1; | ||
| } | ||
|
|
||
| static size_t uid_list_size(struct uid_list *list) | ||
| { | ||
| return sizeof(struct uid_list) + list->u32_offset; | ||
| } | ||
|
|
||
| // Get UID at index (for iteration) | ||
| static ndb_uid_t uid_list_get(struct uid_list *list, uint32_t index) | ||
| { | ||
| if (index >= list->count) | ||
| return 0; | ||
|
|
||
| // Count UIDs in each bucket | ||
| uint32_t u8_count = list->u8_offset; | ||
| uint32_t u16_count = (list->u16_offset - list->u8_offset) / sizeof(uint16_t); | ||
|
|
||
| if (index < u8_count) { | ||
| const uint8_t *u8_data = (const uint8_t*)list->data; | ||
| return u8_data[index]; | ||
| } else if (index < u8_count + u16_count) { | ||
| const uint16_t *u16_data = (const uint16_t*)(list->data + list->u8_offset); | ||
| return u16_data[index - u8_count]; | ||
| } else { | ||
| const uint32_t *u32_data = (const uint32_t*)(list->data + list->u16_offset); | ||
| return u32_data[index - u8_count - u16_count]; | ||
| } | ||
| } | ||
|
|
||
| // Remove UID at index (for iteration + removal) | ||
| static void uid_list_remove_at(struct uid_list *list, uint32_t index) | ||
| { | ||
| if (index >= list->count) | ||
| return; | ||
|
|
||
| // Extract all UIDs except the one at index | ||
| ndb_uid_t *temp_uids = malloc(list->count * sizeof(ndb_uid_t)); | ||
| if (!temp_uids) | ||
| return; | ||
|
|
||
| uint32_t idx = 0; | ||
| for (uint32_t i = 0; i < list->count; i++) { | ||
| if (i != index) | ||
| temp_uids[idx++] = uid_list_get(list, i); | ||
| } | ||
|
|
||
| list->count--; | ||
| uid_list_rebuild_buckets(list, temp_uids); | ||
| free(temp_uids); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Eliminate code duplication by including the production header and implementation.
Lines 7-224 duplicate the ndb_uid_t typedef, struct uid_list, and all function implementations from src/bucketed_u32_list.h and src/bucketed_u32_list.c. This creates a maintenance burden: any bug fix or enhancement in the production code must be manually copied here, and the two can drift out of sync.
Instead, #include "bucketed_u32_list.h" and either link against the compiled object or #include "bucketed_u32_list.c" directly to reuse the production code.
🤖 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 `@test_bucketed_uid_list.c` around lines 1 - 224, This test file duplicates
production definitions (ndb_uid_t, struct uid_list) and functions
(uid_list_create, uid_list_contains, uid_list_add, uid_list_rebuild_buckets,
uid_list_get, uid_list_remove_at), so remove the duplicated implementations and
instead include the production header and reuse the production implementation;
specifically delete the typedef, struct and all function definitions from the
test and add `#include` "bucketed_u32_list.h" at top, then link against the
compiled bucketed_u32_list.o (or, if unavoidable for the test build, `#include`
"bucketed_u32_list.c") so the test references the existing ndb_uid_t and the
functions (uid_list_create, uid_list_add, uid_list_contains, uid_list_get,
uid_list_remove_at) without duplicating code.
See SOCIAL_GRAPH.md for description
Summary by CodeRabbit
Release Notes
New Features
Tests