invite join drift analysis#60
Open
gzuuus wants to merge 3 commits into
Open
Conversation
…gnostics Add ScriptedMockNetwork test harness for deterministic race condition testing. Add comprehensive test suite (R1-R8) reproducing invite-join drift scenarios including concurrent commit races, stale producer depth, and buggy client state divergence. Add diagnostic logging to MarmotGroup and MarmotClient to aid in debugging join and commit operations.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (4)
WalkthroughAdds documentation and tests to reproduce invite-join ratchet-tree drift, introduces deterministic test network, runtime diagnostics for join/commit flows, welcome epoch-authenticator tagging and parsing, grease utility extraction, several example tweaks, and updates two submodule pointers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/__tests__/invite-join-drift-repro.test.ts (1)
561-600: Consider extracting shared tree mutation logic to reduce duplication.The
forceDirectPathUnmergedLeafDivergencehelper at lines 631-675 is reusable, but similar inline mutations exist in:
runR5RealWorldSequence(lines 561-600)- R4 test case (lines 811-850)
While the inline versions predate the helper extraction (or serve as documentation of the exact mutation), consolidating them would reduce maintenance burden.
♻️ Optional: Consolidate tree mutations to use the shared helper
// In runR5RealWorldSequence (around line 558): - const tree = groupC.state.ratchetTree; - let mutated = false; - for (let nodeIndex = 0; nodeIndex < tree.length && !mutated; nodeIndex++) { - // ... 40 lines of mutation logic - } + const mutated = forceDirectPathUnmergedLeafDivergence(groupC.state.ratchetTree as any[]); // In R4 test (around line 811): - const tree = groupC.state.ratchetTree; - let mutated = false; - for (let nodeIndex = 0; nodeIndex < tree.length && !mutated; nodeIndex++) { - // ... 40 lines of mutation logic - } + const mutated = forceDirectPathUnmergedLeafDivergence(groupC.state.ratchetTree as any[]);Also applies to: 631-675, 811-850
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/invite-join-drift-repro.test.ts` around lines 561 - 600, Replace the duplicated inline tree-mutation block in runR5RealWorldSequence (the loop that touches tree, nodeTypes, toLeafIndex, directPath, leafToNodeIndex, leafWidth and sets firstParent.parent.unmergedLeaves/secondParent.parent.unmergedLeaves and mutated) with a call to the existing helper forceDirectPathUnmergedLeafDivergence; update the helper signature if needed so it accepts the tree (and returns a boolean or sets mutated) and performs the same logic (ensure it creates parent nodes at dp[0] and dp[1] with the same hpkePublicKey seeds and sets unmergedLeaves = [leafIndex] / [] respectively), then replace the other duplicated instance in the R4 test with the same helper call so all three places share the exact mutation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/__tests__/invite-join-drift-repro.test.ts`:
- Around line 561-600: Replace the duplicated inline tree-mutation block in
runR5RealWorldSequence (the loop that touches tree, nodeTypes, toLeafIndex,
directPath, leafToNodeIndex, leafWidth and sets
firstParent.parent.unmergedLeaves/secondParent.parent.unmergedLeaves and
mutated) with a call to the existing helper
forceDirectPathUnmergedLeafDivergence; update the helper signature if needed so
it accepts the tree (and returns a boolean or sets mutated) and performs the
same logic (ensure it creates parent nodes at dp[0] and dp[1] with the same
hpkePublicKey seeds and sets unmergedLeaves = [leafIndex] / [] respectively),
then replace the other duplicated instance in the R4 test with the same helper
call so all three places share the exact mutation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aeac9c06-6164-40d2-96e2-72290ae47fd2
📒 Files selected for processing (6)
documentation/invite-join-drift-analysis.mdreferences/ts-mlssrc/__tests__/helpers/scripted-mock-network.tssrc/__tests__/invite-join-drift-repro.test.tssrc/client/group/marmot-group.tssrc/client/marmot-client.ts
…rift prevention Add optional "ea" (epoch authenticator) tag to welcome events (kind 444) to prevent invite-join drift attacks. When a new member receives a welcome, they can now verify the epoch authenticator matches the inviter's current state, ensuring they joined the correct branch and not a competing commit from the same epoch window. Includes tests for tag encoding, validation, and various drift scenarios including branch races and forged authenticator attempts.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/__tests__/epoch-authenticator-drift.test.ts (3)
143-188: Consider adding type annotations toforceDirectPathUnmergedLeafDivergence.The
tree: any[]parameter loses type information. While this is intentionally mutating internal state for test purposes, a brief JSDoc comment explaining the mutation intent would improve clarity.📝 Suggested documentation
+/** + * Mutates the ratchet tree to create an invalid unmerged_leaves configuration. + * This simulates a buggy client that produces an inconsistent tree state, + * used to verify that join validation rejects such malformed welcomes. + * + * `@param` tree - The ratchet tree array (mutated in place) + * `@returns` true if mutation was applied, false otherwise + */ function forceDirectPathUnmergedLeafDivergence(tree: any[]): boolean {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/epoch-authenticator-drift.test.ts` around lines 143 - 188, The function forceDirectPathUnmergedLeafDivergence currently accepts tree: any[] which loses type safety; update its signature to use a proper typed array (e.g., tree: Array<TreeNode> or the existing internal node type used in your tests) and add a brief JSDoc above the function stating that the function intentionally mutates the tree for test setup; keep references to nodeTypes, parent.unmergedLeaves, directPath, leafToNodeIndex, and leafWidth to locate the logic that mutates nodes and ensure the chosen TreeNode type includes nodeType and parent fields used here.
16-26: Consolidate duplicate import from../core/protocol.js.
GROUP_EVENT_KIND(line 24) andKEY_PACKAGE_KIND,WELCOME_EPOCH_AUTHENTICATOR_TAG(lines 18-19) are imported from the same module in separate statements. Consider consolidating them.♻️ Proposed fix
import { MarmotClient } from "../client/marmot-client.js"; import { + GROUP_EVENT_KIND, KEY_PACKAGE_KIND, WELCOME_EPOCH_AUTHENTICATOR_TAG, } from "../core/protocol.js"; import { getWelcomeEpochAuthenticator } from "../core/welcome.js"; import { KeyPackageStore } from "../store/key-package-store.js"; import { KeyValueGroupStateBackend } from "../store/adapters/key-value-group-state-backend.js"; -import { GROUP_EVENT_KIND } from "../core/protocol.js"; import { MemoryBackend } from "./ingest-commit-race.test.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/epoch-authenticator-drift.test.ts` around lines 16 - 26, The test imports KEY_PACKAGE_KIND and WELCOME_EPOCH_AUTHENTICATOR_TAG and separately imports GROUP_EVENT_KIND from the same module; consolidate these into a single import statement from ../core/protocol.js by combining KEY_PACKAGE_KIND, WELCOME_EPOCH_AUTHENTICATOR_TAG, and GROUP_EVENT_KIND into one import to remove the duplicate import and keep imports tidy.
40-49: Consider stronger return type instead ofPromise<any>.The function returns
Promise<any>which loses type safety. While this is test code, a more specific type would improve maintainability.♻️ Proposed fix
+import type { NostrEvent } from "nostr-tools"; + async function getGiftWrapFor( network: ScriptedMockNetwork, recipientPubkey: string, -): Promise<any> { +): Promise<NostrEvent | undefined> { const giftwraps = await network.request(["wss://mock-inbox.test"], { kinds: [1059], "#p": [recipientPubkey], }); return giftwraps[giftwraps.length - 1]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/epoch-authenticator-drift.test.ts` around lines 40 - 49, The getGiftWrapFor function currently returns Promise<any>; replace the any with a specific event type (e.g., a GiftWrapEvent or NostrEvent type) to restore type safety: define or import an interface describing the returned object shape (fields such as kind:number, tags:string[][], content:string, pubkey:string, id:string) and change the signature to Promise<GiftWrapEvent | undefined> (or use the ScriptedMockNetwork.request generic if available) and update usages of getGiftWrapFor and the giftwraps variable accordingly so callers handle the possibly undefined last item.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/welcome.test.ts`:
- Around line 2-13: Update the ESM imports to include .js extensions: change the
import sourcing for createWelcomeRumor, getWelcome, getWelcomeEpochAuthenticator
(currently imported from "../core/welcome") to "../core/welcome.js" and change
the import sourcing for WELCOME_EPOCH_AUTHENTICATOR_TAG and WELCOME_EVENT_KIND
(currently imported from "../core/protocol") to "../core/protocol.js" so the
test uses NodeNext-compatible module paths.
---
Nitpick comments:
In `@src/__tests__/epoch-authenticator-drift.test.ts`:
- Around line 143-188: The function forceDirectPathUnmergedLeafDivergence
currently accepts tree: any[] which loses type safety; update its signature to
use a proper typed array (e.g., tree: Array<TreeNode> or the existing internal
node type used in your tests) and add a brief JSDoc above the function stating
that the function intentionally mutates the tree for test setup; keep references
to nodeTypes, parent.unmergedLeaves, directPath, leafToNodeIndex, and leafWidth
to locate the logic that mutates nodes and ensure the chosen TreeNode type
includes nodeType and parent fields used here.
- Around line 16-26: The test imports KEY_PACKAGE_KIND and
WELCOME_EPOCH_AUTHENTICATOR_TAG and separately imports GROUP_EVENT_KIND from the
same module; consolidate these into a single import statement from
../core/protocol.js by combining KEY_PACKAGE_KIND,
WELCOME_EPOCH_AUTHENTICATOR_TAG, and GROUP_EVENT_KIND into one import to remove
the duplicate import and keep imports tidy.
- Around line 40-49: The getGiftWrapFor function currently returns Promise<any>;
replace the any with a specific event type (e.g., a GiftWrapEvent or NostrEvent
type) to restore type safety: define or import an interface describing the
returned object shape (fields such as kind:number, tags:string[][],
content:string, pubkey:string, id:string) and change the signature to
Promise<GiftWrapEvent | undefined> (or use the ScriptedMockNetwork.request
generic if available) and update usages of getGiftWrapFor and the giftwraps
variable accordingly so callers handle the possibly undefined last item.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24125b72-811e-4a20-96e7-4f44b5c2c0a2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
src/__tests__/epoch-authenticator-drift.test.tssrc/__tests__/welcome.test.tssrc/client/group/marmot-group.tssrc/core/protocol.tssrc/core/welcome.ts
✅ Files skipped from review due to trivial changes (1)
- src/core/protocol.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/group/marmot-group.ts
Comment on lines
+2
to
+13
| import { bytesToHex } from "@noble/hashes/utils.js"; | ||
|
|
||
| import type { NostrEvent } from "nostr-tools"; | ||
| import { getWelcome } from "../core/welcome"; | ||
| import { WELCOME_EVENT_KIND } from "../core/protocol"; | ||
| import { | ||
| createWelcomeRumor, | ||
| getWelcome, | ||
| getWelcomeEpochAuthenticator, | ||
| } from "../core/welcome"; | ||
| import { | ||
| WELCOME_EPOCH_AUTHENTICATOR_TAG, | ||
| WELCOME_EVENT_KIND, | ||
| } from "../core/protocol"; |
Contributor
There was a problem hiding this comment.
Missing .js extensions in imports.
Per the coding guidelines, all imports must include .js extensions (ESM with NodeNext module resolution). The imports on lines 9 and 13 are missing the .js extension.
🔧 Proposed fix
import {
createWelcomeRumor,
getWelcome,
getWelcomeEpochAuthenticator,
-} from "../core/welcome";
+} from "../core/welcome.js";
import {
WELCOME_EPOCH_AUTHENTICATOR_TAG,
WELCOME_EVENT_KIND,
-} from "../core/protocol";
+} from "../core/protocol.js";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { bytesToHex } from "@noble/hashes/utils.js"; | |
| import type { NostrEvent } from "nostr-tools"; | |
| import { getWelcome } from "../core/welcome"; | |
| import { WELCOME_EVENT_KIND } from "../core/protocol"; | |
| import { | |
| createWelcomeRumor, | |
| getWelcome, | |
| getWelcomeEpochAuthenticator, | |
| } from "../core/welcome"; | |
| import { | |
| WELCOME_EPOCH_AUTHENTICATOR_TAG, | |
| WELCOME_EVENT_KIND, | |
| } from "../core/protocol"; | |
| import { bytesToHex } from "@noble/hashes/utils.js"; | |
| import type { NostrEvent } from "nostr-tools"; | |
| import { | |
| createWelcomeRumor, | |
| getWelcome, | |
| getWelcomeEpochAuthenticator, | |
| } from "../core/welcome.js"; | |
| import { | |
| WELCOME_EPOCH_AUTHENTICATOR_TAG, | |
| WELCOME_EVENT_KIND, | |
| } from "../core/protocol.js"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/welcome.test.ts` around lines 2 - 13, Update the ESM imports to
include .js extensions: change the import sourcing for createWelcomeRumor,
getWelcome, getWelcomeEpochAuthenticator (currently imported from
"../core/welcome") to "../core/welcome.js" and change the import sourcing for
WELCOME_EPOCH_AUTHENTICATOR_TAG and WELCOME_EVENT_KIND (currently imported from
"../core/protocol") to "../core/protocol.js" so the test uses
NodeNext-compatible module paths.
gzuuus
force-pushed
the
docs/invite-join-drift-analysis
branch
from
March 23, 2026 15:17
c66f0e0 to
60fefca
Compare
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 by CodeRabbit
New Features
Documentation
Tests
Chores