From 082098419dcd4ddfd77f72af2756f532d3351fe9 Mon Sep 17 00:00:00 2001 From: Narasimha-sc <166327228+Narasimha-sc@users.noreply.github.com> Date: Mon, 6 Jul 2026 14:36:31 +0000 Subject: [PATCH] plans: correct directory name-reapproval fix (honest root cause + gate) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the first draft found the stated mechanism wrong: the domain proof is NOT a volatile per-presentation nonce (that's the separate badge feature; a SimplexDomainProof presHeader is signed, hence static), and relay-strip converges after one cycle — so neither documented mechanism explains the 30-min repeat. Downgrade "root cause confirmed" to "trigger confirmed, delta unverified"; make capturing the actual per-cycle delta a required gate before the fix; redact only the proof reusing the existing clearProofs/redactedDomain idiom instead of new helpers; state the directory-only blast radius as a fact (client GroupLinkPlan.Known omits the field). --- ...026-07-06-fix-directory-name-reapproval.md | 289 +++++++++--------- 1 file changed, 145 insertions(+), 144 deletions(-) diff --git a/plans/2026-07-06-fix-directory-name-reapproval.md b/plans/2026-07-06-fix-directory-name-reapproval.md index a3e58673c1..40bd552f85 100644 --- a/plans/2026-07-06-fix-directory-name-reapproval.md +++ b/plans/2026-07-06-fix-directory-name-reapproval.md @@ -1,184 +1,185 @@ # Fix: SimpleX directory re-approves a channel every 30 min after a blockchain name is added -Status: proposed +Status: proposed — **fix gated on a diagnostic step (see §5); root-cause delta not yet observed** Branch: `nd/fix-directory-names` Base: `origin/master` (`b38015c7b`) ## 1. Problem After an owner adds a SimpleX name (blockchain domain claim) to a public -channel, the SimpleX Directory service repeatedly asks the owner to re-approve -the channel **every 30 minutes, indefinitely**, with no changes on the owner's -side. The channel keeps flipping to "hidden until approved." +channel, the SimpleX Directory service asks the owner to re-approve the channel +**every 30 minutes, indefinitely**, with no changes on the owner's side. The +channel keeps flipping to "hidden until approved." -The 30-minute cadence is not a coincidence — it is the directory's own periodic -link-check interval. +## 2. What is confirmed (facts, file:line) -## 2. Grounded diagnosis (facts, file:line) +1. **The 30-min cadence is the directory's link-check timer.** `linkCheckInterval` + defaults to **1800 s** (`apps/simplex-directory-service/src/Directory/Options.hs:181`); + `linkCheckThread_` sleeps that long and enqueues a `DEGroupLinkCheck` for every + non-removed registered group (`.../Directory/Service.hs:199-211`). -1. **The 30-min repeat is the directory's link-check timer.** - `linkCheckInterval` defaults to **1800 s** (`apps/simplex-directory-service/src/Directory/Options.hs:181`). - `linkCheckThread_` sleeps that long and enqueues a `DEGroupLinkCheck` event for - every non-removed registered group - (`apps/simplex-directory-service/src/Directory/Service.hs:199-211`). +2. **`DEGroupLinkCheck` re-approves iff `groupUpdated` is true.** `deGroupLinkCheck` + runs `APIConnectPlan` against the channel link and, on a `GLPKnown` plan, does + `when groupUpdated $ reapprove …` (`Service.hs:815-854`, trigger at `:824`), + which sets `GRSPendingApproval` and re-sends for admin approval. -2. **`DEGroupLinkCheck` re-approves whenever `groupUpdated` is true.** - `deGroupLinkCheck` runs `APIConnectPlan` against the channel's link and, on a - `GLPKnown` plan, does `when groupUpdated $ reapprove …` - (`Service.hs:815-854`, trigger at `:824`), which sets `GRSPendingApproval` and - re-sends the channel for admin approval. +3. **`groupUpdated` is a whole-profile byte inequality.** It is the `Bool` returned + by `updateGroupFromLinkData`: `profileChanged = p /= groupProfile` + (`src/Simplex/Chat/Library/Internal.hs:1466-1482`), where `p` is the directory's + **stored** `GroupProfile` and `groupProfile` is the profile in the **fetched + link data**. -3. **`groupUpdated` is a whole-profile byte inequality.** - It is the `Bool` returned by `updateGroupFromLinkData`, defined as - `profileChanged = p /= groupProfile` - (`src/Simplex/Chat/Library/Internal.hs:1466-1482`), where `p` is the - directory's **stored** `GroupProfile` and `groupProfile` is the profile inside - the **fetched link data**. This equality includes - `publicGroup.publicGroupAccess.groupDomainClaim` and its `proof`. +4. **`groupUpdated` is behaviorally directory-only.** The mobile/desktop clients do + not even deserialize it: the client `GroupLinkPlan.Known` variant carries only + `groupInfo` — `class Known(val groupInfo: GroupInfo)` + (`apps/multiplatform/.../model/SimpleXAPI.kt:7158`; iOS + `apps/ios/Shared/Model/AppAPITypes.swift`, `enum GroupLinkPlan`). The only + consumers of the flag are in the directory service: re-approval (`Service.hs:824`), + listing refresh (`:825`), and owner re-registration (`:988` → + `deReregistration:1043,1046`). (All other `groupUpdated` matches in the tree are + an unrelated chat event, `RcvGroupEvent`/`CR.GroupUpdated`.) -4. **A normal profile change self-corrects after one cycle — so this is a - non-convergence bug specific to the domain claim.** - When `profileChanged` is true, `updateGroupFromLinkData` also writes the link - profile to the DB (`Internal.hs:1471`), and the store persists the domain claim - *and its proof* (`group_domain_proof` column, - `src/Simplex/Chat/Store/Groups.hs:2724`; write/read are symmetric — - `publicGroupAccessRow` / `toPublicGroupAccess`, - `src/Simplex/Chat/Store/Shared.hs:721` and `:727`). So an ordinary field change - (name/description/image) re-approves **once** and then converges. For the loop - to persist, the directory's stored profile must be **permanently unequal** to - the fetched link profile — and the only field that behaves this way after a - name is added is the domain claim/proof. +## 3. What is NOT yet established -5. **The domain claim/proof legitimately differs between "stored" and "link" - representations.** Two documented mechanisms, either of which reproduces the - symptom (we do not need to distinguish them — see §4): - - A relay can **drop the domain claim** from the served link data — the code - explicitly handles this case (`src/Simplex/Chat/Library/Commands.hs:4326-4328`, - "an un-upgraded relay dropped the claim"). - - The proof carries a `ProofPresHeader` that supports a **fresh - per-presentation nonce** (`PHTest nonce`, `src/Simplex/Chat/Badges.hs:197-221`), - so a re-presented proof is not byte-identical to the stored one. +**Why the two profiles fail to converge every cycle is unproven.** An ordinary +profile change self-corrects: when `profileChanged` is true, +`updateGroupFromLinkData` writes the link profile to the DB (`Internal.hs:1471`), +and the store round-trips the domain claim + proof faithfully +(`group_domain_proof` column; symmetric `publicGroupAccessRow` / +`toPublicGroupAccess`, `src/Simplex/Chat/Store/Shared.hs:721`/`:727`). So a +one-time change re-approves **once** and then converges. A perpetual 30-min +re-approval requires the directory's stored profile to be **permanently unequal** +to the fetched link profile — and which field differs has **not been observed**. -6. **The domain claim is verified independently and is not admin-moderated - content.** Ownership is proven cryptographically and on-chain by - `verifyEntityDomain` (`Commands.hs:4845`: the on-chain link must match the - connection link, and the proof must be signed by the address owner's key), and - verification state is tracked in a **separate** field, `group_domain_verified` - (`Groups.hs:2710-2713`, `setGroupDomainVerified`). Nothing about the name - requires directory-admin re-approval. +Two hypotheses were considered and both are insufficient as stated: -## 3. Root cause +- **Relay drops the domain claim from the served link data** + (`src/Simplex/Chat/Library/Commands.hs:4326-4328` handles this case). But this + **converges after one cycle**: the directory stores the claim-less link profile, + and with no further owner change nothing restores the claim, so the next check + matches. It explains a single reapproval, not a repeat. +- **A re-presented proof with a fresh nonce.** This does **not** apply to domain + claims. `PHTest`/`badgeProof` (fresh nonce per presentation) is the separate + *badge* feature, attached to `Profile.badge` (`Internal.hs:2085-2086`), not to + the domain claim. A `SimplexDomainProof`'s `presHeader` is **inside the signed + payload** (`Commands.hs:4869`, verified at `:4867`), so it cannot be re-presented + with a new nonce without re-signing by the owner — it is static. The group claim + is also frequently created with `proof = Nothing` (`mkDomainClaim`, + `src/Simplex/Chat/Names.hs:56`; used at `Commands.hs:5693`, `:1510`). -The directory's re-approval trigger (`groupUpdated`) is a byte-comparison of the -**entire** group profile, including the independently-verified domain claim and -its proof. Adding a name introduces a claim/proof whose link-side representation -never byte-matches the directory's stored copy (relay-stripped claim and/or -re-presented proof nonce), so `profileChanged` is true on **every** 30-minute -link check → re-approval on repeat. +So the correlation ("started right after adding a name") strongly implicates the +domain claim as the differing field, but the **exact field and the reason it +never converges are not yet demonstrated.** This plan therefore treats the fix as +gated on a diagnostic. -The conceptual error: `groupUpdated` conflates two different questions — -"did the profile change at all?" (used to decide whether to *store* the fresh -claim/proof) and "did the profile change in a way that requires admin -*re-moderation*?" (used to decide re-approval). These must be separated for the -domain claim. +## 4. Likely mechanism (hypothesis, to be confirmed) -## 4. The fix (minimal, idiomatic) +The differing field is the group profile's `publicGroup.publicGroupAccess.groupDomainClaim` +(or its `proof`): the directory's stored copy and the fetched link copy carry +different claim representations on every fetch, in a way that storing the link +copy does not reconcile. Because the domain claim is verified **independently** — +cryptographically + on-chain by `verifyEntityDomain` (`Commands.hs:4840`), with +verification tracked in a *separate* field `group_domain_verified` +(`Store/Groups.hs:2710-2713`) — it is not admin-moderated content and should never +gate re-approval. The bug is that `groupUpdated` conflates "profile changed at +all" (used to decide whether to *store* the fresh claim) with "profile changed in +a way needing admin *re-moderation*" (used to decide re-approval). -In `src/Simplex/Chat/Library/Internal.hs`, `updateGroupFromLinkData`: -keep storing on the full `profileChanged` (so the claim/proof and verification -stay current), but **report `groupUpdated` with the domain claim excluded**. +## 5. Required diagnostic (gate before the fix) + +Add a temporary log at the one comparison that flips everything, +`Internal.hs:1482`, dumping the two profiles' domain claims when `profileChanged` +fires, and run it against the affected channel across ≥2 link-check cycles: + +```haskell +-- temporary: +when (p /= groupProfile) $ logInfo $ "linkcheck delta: stored=" <> tshow (claimOf p) + <> " link=" <> tshow (claimOf groupProfile) +``` + +Decision: +- **Delta is the domain claim, identical each cycle** → structural asymmetry; + apply §6 (proof redaction covers it if the delta is the proof; if the delta is + the whole claim, widen the redaction to the claim — decide from the log). +- **Delta changes each cycle** → identify the varying field from the log; if it is + not the claim, this plan's fix does not apply and the diagnosis restarts here. + +The fix in §6 is only justified once the log shows the per-cycle delta is confined +to the domain claim/proof. + +## 6. The fix (conditional on §5) + +In `updateGroupFromLinkData` (`Internal.hs`), keep storing on the full +`profileChanged` (so the claim/proof and verification stay current), but report +`groupUpdated` with the domain-claim **proof** redacted — reusing the codebase's +existing proof-redaction idiom (`Types.hs:750` `clearProofs`; `Internal.hs:1265` +`redactedDomain`), rather than new helpers: ```haskell - -- store branch unchanged; only the returned flag changes: pure (g'', moderationChanged) -- was: profileChanged ... where - profileChanged = p /= groupProfile -- keep: still gates storing fresh claim/proof - -- The domain claim is verified independently (on-chain + owner signature) and - -- is not admin-moderated; its link vs stored representation can differ (a relay - -- dropping the claim, or a re-presented proof nonce), which otherwise re-triggers - -- directory approval on every link check. Exclude it from the reported change. - moderationChanged = withoutClaim p /= withoutClaim groupProfile - withoutClaim gp@GroupProfile {publicGroup = pgm} = gp {publicGroup = clearClaim <$> pgm} - clearClaim pg@PublicGroupProfile {publicGroupAccess = a} = - pg {publicGroupAccess = (\acc -> acc {groupDomainClaim = Nothing}) <$> a} + profileChanged = p /= groupProfile -- keep: still gates storing the fresh claim + -- A domain-claim change is verified independently (on-chain + owner signature) + -- and is not admin-moderated, so it must not trigger directory re-approval. + -- Redact the proof before comparing (same idiom as Types.clearProofs). + moderationChanged = redactProof p /= redactProof groupProfile + redactProof gp@GroupProfile {publicGroup} = gp {publicGroup = redactPg <$> publicGroup} + redactPg pg@PublicGroupProfile {publicGroupAccess} = pg {publicGroupAccess = redactAccess <$> publicGroupAccess} + redactAccess a@PublicGroupAccess {groupDomainClaim} = + a {groupDomainClaim = (\d -> d {proof = Nothing} :: SimplexDomainClaim) <$> groupDomainClaim} ``` -This is a semantics fix, not a convergence hack: a domain-claim change should -**never** trigger re-approval, independently of *why* the two representations -differ. It is therefore robust to both mechanisms in §2.5 — we do not need to -resolve which one occurs in production. +Redacting **only the proof** (not the whole claim) is the surgical choice: it kills +any proof-representation delta while still letting a genuine domain add/change +register as a one-time change (which then converges). If §5 shows the delta is the +whole claim (e.g. claim present vs absent every cycle), replace `redactAccess` with +`groupDomainClaim = Nothing`; note this also suppresses the legitimate one-time +reapproval on a name add. -## 5. Why this is correct, sufficient, and safe +Storage keys off the full `profileChanged`, so the claim/proof and +`setGroupDomainVerified` still update — by-name lookups and the verified badge stay +correct. -**Blast radius is contained to the directory.** `GLPKnown.groupUpdated` -(`src/Simplex/Chat/Controller.hs:1131`) is consumed **only** by the directory -service. (The many other `groupUpdated` matches in the tree are an unrelated -chat event — `RcvGroupEvent.GroupUpdated` / `CR.GroupUpdated` — not this plan -flag.) The three directory consumers all behave correctly with the claim -excluded: +## 7. Why this is safe (given §5 confirms the delta) -- `Service.hs:824` — periodic re-approval → **fixed** (the 30-min loop stops). -- `Service.hs:988` → `deReregistration` (`:1018`, uses the flag at `:1043` and - `:1046`) — an owner re-submitting a **name-only** change now correctly gets - "already listed" instead of being forced back into approval. -- `Service.hs:825` — `listingsUpdated` on `groupUpdated || summary changed`. The - only trade-off: a name-only change no longer *immediately* refreshes the web - listing files (it still refreshes on any real profile change or member-count - change). See §6 for the variant that preserves this. +- **Directory-only blast radius** (fact, §2.4). All three consumers behave + correctly with the proof/claim excluded: + - `Service.hs:824` periodic re-approval → **fixed**. + - `Service.hs:988` → `deReregistration` (`:1043`/`:1046`) — an owner re-submitting a + name-only change gets "already listed" instead of forced re-approval. + - `Service.hs:825` `listingsUpdated` — minor trade-off: a name-only change no longer + *immediately* refreshes web-listing files (still refreshes on any real change or + member-count change). If prompt refresh matters, keep `:825` on the raw + `profileChanged` while `:824` uses `moderationChanged`. +- **Moderation not weakened.** Re-approval still fires on any moderatable field + (name, description, image); only proof/claim deltas are excluded, and those are + independently verified. -**Storage and verification stay correct.** Because the store branch still keys -off the full `profileChanged`, the directory continues to persist a genuinely -new claim/proof, and `verifyChanged`/`setGroupDomainVerified` still run — so -by-name lookups and the verified badge remain accurate. +## 8. Residual behaviour (accepted) -**Moderation is not weakened.** Re-approval exists to re-check *moderatable* -fields (display name, description, image) that an owner might change after -approval. The SimpleX name is not such a field: it is cryptographically verified -and, if name-content moderation is ever desired, that is a separate feature -operating on the resolved name value — not on byte-equality of a proof. +If the claim/proof genuinely differs each cycle, the store branch re-writes the +`group_profiles` row every 30 min. This is idempotent and does not thrash +verification (`claimChanged` in `updateGroupProfile` compares the *domain*, not the +proof — `Groups.hs:2708-2709`). A follow-up could skip the store on proof-only +deltas; out of scope. -## 6. Alternatives considered +## 9. Verification plan -- **Fix in the directory (`deGroupLinkCheck`) instead of core.** Rejected: the - directory only receives the `Bool`, not the profile diff, so it cannot tell a - claim-only change from a real one without the core change anyway. The core is - the correct single locus. -- **Make the profiles converge (stop the relay stripping / freeze the proof - nonce).** Larger, mechanism-specific, and touches link-data serving and proof - presentation. The semantics fix in §4 is smaller and correct regardless. -- **Preserve prompt listing refresh on name changes.** If desired, keep the - `listingsUpdated` check (`Service.hs:825`) driven by the raw `profileChanged` - while re-approval (`:824`) uses the new `moderationChanged`. This needs the - plan to carry both signals (or the directory to recompute); recommended only - if the web listing renders the verified name and staleness is a concern. +1. Land the §5 diagnostic, capture the delta on the affected channel, confirm it is + the domain claim/proof, and choose proof-vs-whole-claim redaction from the log. +2. Apply §6. +3. **Regression test** in `tests/Bots/DirectoryTests.hs`: register a channel → + approve → add a SimpleX name → trigger ≥2 link checks → assert the registration + stays `GRSActive` (no `GRSPendingApproval`, no owner re-approval message). +4. Build the directory service; run the directory test suite. -## 7. Residual behaviour (accepted) - -If the claim/proof is genuinely volatile (per-presentation nonce), the store -branch will re-write the group_profiles row on each 30-min check (same domain → -`claimChanged` is false, so verification is **not** thrashed; -`Groups.hs:2708-2713` compares domain, not proof). This is a harmless idempotent -write and is vastly preferable to the owner-facing re-approval spam. A follow-up -could gate the store to skip proof-only deltas; out of scope here. - -## 8. Verification plan - -1. **Regression test** in `tests/Bots/DirectoryTests.hs` (the member-review - branch already added directory concurrency coverage there): register a - channel → approve → add a SimpleX name (domain claim) → trigger a link check → - assert the registration status **stays `GRSActive`** (no `GRSPendingApproval`, - no re-approval message to the owner). -2. **Optional diagnostic** before/after: a one-line log at `Internal.hs:1482` - dumping the two `groupDomainClaim` values confirms which §2.5 mechanism occurs - in a live setup — informative, not required for the fix. -3. Build the directory service and run the directory test suite. - -## 9. Files touched +## 10. Files touched - `src/Simplex/Chat/Library/Internal.hs` — `updateGroupFromLinkData`: return a - claim-insensitive change flag (store logic unchanged). + claim/proof-insensitive change flag (store logic unchanged). Temporary diagnostic + log removed before merge. - `tests/Bots/DirectoryTests.hs` — add the name-added-no-reapproval regression test. -No schema, wire-format, or API changes. `GLPKnown.groupUpdated`'s type is -unchanged; only its value semantics (directory-only consumer) are refined. +No schema, wire-format, or API changes. `GLPKnown.groupUpdated`'s type is unchanged; +only its value semantics (directory-only consumer) are refined.