mirror of
https://github.com/simplex-chat/simplex-chat.git
synced 2026-09-28 19:59:37 +00:00
plans: correct directory name-reapproval fix (honest root cause + gate)
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).
This commit is contained in:
Narasimha-sc
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user