logs.gokuls.in

2 pull requests merged across 1 repo

Offline-Protocol/offline-protocol-sdk

What this is

A post-merge review of #369, fixed. Nine findings: four that change

behaviour, five that were wrong in a way only a reader would notice.

Two of the four leave a human-readable name pointing at a live address in a

public directory, which is the single outcome

the chapter exists to prevent.

The two that matter

A retraction was queued and then forgotten

The engine cleared the persisted claim before anything acknowledged the

tombstone. Its comment argued that was the recoverable direction, because

"the claim expires when its second hop fails".

It does not. The two most likely reasons to retract are a profile rename

and switching discovery off with cold contact left on, and in both the

install keeps publishing key packages for the same address. Hop 2 keeps

succeeding, so nothing expires the claim, ever.

Three ways to lose it, and the third has no failure to report at all:

1. the tombstone's send fails, and the failure drain only resets a flag;

2. the process exits before it lands;

3. there is no Nostr transport installed, where queueing a retraction is a

silent no-op the engine records as done.

By the time any of that is observable the profile has moved on, so the name

it named exists nowhere. A name now moves from claim to retracting and

stays there, persisted, until a relay acknowledges the tombstone. The

transport grew the acknowledgement half to make that observable, mirroring

the failure channel it already had. Re-claiming a name cancels the tombstone

still owed for it, since the two are contradictory statements about one

addressable slot.

A resolution ended on the first relay's answer

A query is broadcast and each relay sends its own end-of-stored-events. Both

bridges closed the subscription on every relay at the first one, and the

FFI flushed username_resolved at that moment. The answer was therefore

whatever the fastest relay happened to hold.

A relay holding nothing wins that race by having nothing to send. That

inverts the property event_is_authentic was added to protect: a claim is

supposed to need *one honest relay* to survive, and this let one fast relay

decide the whole result without forging anything.

Each relay's subscription now closes as that relay finishes, so no filter is

left standing on a public tag, and the resolution completes when every relay

it asked has answered. A silent relay is bounded at 10s, well under the

engine's 30s sweep; a disconnecting relay stops being waited on.

The test that proved nothing

Both forged-tombstone negative controls built their forgery with a **stale

event id**, so both died at the id recomputation and never reached the

signature check that actually stops the attack. An id is a hash of public

fields and free for an attacker to recompute.

Confirmed empirically before fixing: with verify_prehash replaced by

true, the entire suite stayed green.

The forgery is now stamped with a correct id and dressed in a **well-formed

signature taken from a genuinely signed event**, because an unparseable

signature is refused one step early too. nostr_crypto gains direct tests on

event_is_authentic, including a real signature by another key. All of them

fail under that mutation.

The NIP-01 id computation is now one function shared by signing and

verification. They were separate copies of one format, and a divergence would

have been silent in the worst direction: this build's own records failing

this build's own check, indistinguishable from squatting.

The bridges flattened three errors into one

resolveUsername deliberately separates "discovery is off" from "retry

shortly" from "not a claimable name". Both bridges reported all three as

ERROR_INVALID_ARGUMENT, so an app either retries forever or gives up on a

queue that was one drain away from working. Both now route through the

existing typed mapper, which gained InvalidConfiguration, pinned in both

languages.

Also

  • A resolution reports how many verified claims it dropped at its

ceiling. Folding them into rejected describes a squatted name as a broken

one; reporting neither renders it as a clean set, which is exactly what a

name squatted at volume looks like.

  • A failed publication for a name this install no longer claims no longer

marks the *current* claim unpublished, which republished a healthy claim on

every such failure while the backoff ladder, keyed by the failing tag, did

not apply.

  • The NIP-09 deletion is built under the same keypair guard as its tombstone.

Re-reading the keypair let an identity swap in between name a coordinate

this install never published at, and deletion is per-author, so the request

would have deleted nothing silently.

  • A record is verified against its own resolution's username rather than one

passed in beside the query id.

  • A query-id collision flushes the resolution it displaces instead of leaving

that caller waiting on an event with no trigger.

  • The backoff ladder is pruned; its keys are tags, and a rename left one

behind per failure for the life of the process.

  • The deletion carries NIP-09's k tag; the delivered-budget comment now says

what the code does; the FFI config docs are unmangled (a field inserted

mid-comment had given the discovery switch cold contact's description).

Docs

The spec gains the all-relay completion requirement, the retraction

durability rule, truncation reporting, and one residual the review found

undocumented: a lost publishing key is a claim nobody can withdraw, since

replacement, deletion and the nostr_author binding are all author-keyed.

docs/bridges C5 counts five mirrored constant sets, not four.

Verification

  • every Rust crate's suite green locally (engine 1367, transport 297,

uniffi 121, and the rest); the full-workspace aggregate with doctests is

left to CI

  • clippy -D warnings and cargo fmt --all -- --check clean
  • RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps clean
  • 233 Swift tests, 420 Android tests, tsc clean, JS harness 15/15
  • the iOS bridge typecheck that covers NostrManager.swift (run locally,

exit 0) — OfflineProtocolModule.swift remains outside it, same gap as

before

  • all four binding sets regenerated: zero drift, since only UDL comments

changed

The three fixes with a behavioural claim were negative-controlled by

mutation: reverting each fails the test written for it. The signature test's

mutation was run against the *old* tests first, which is how the vacuity was

confirmed rather than assumed.

What this is

PR #331 re-keyed the Nostr routing tag from SHA-256(username) to

SHA-256(address), correctly: the old preimage was guessable, so anyone who

could guess a name could watch that inbox. The cost was cold contact, because

the tag's preimage became a value you can only learn *from* the peer, which is

exactly what "we have never spoken" means you do not have.

The machinery was never broken. Given an address, every hop already worked.

Only the first one was missing, and this adds it.

Two hops. A signed DiscoveryRecordV1 sits at a username-derived tag and

hands back {address, pubkey}; from there it is the existing

published-key-package path, untouched. The record cannot lie about a key,

because derive(pubkey) == address is checked the same way every control

frame is. It can only lie about a name.

Two paths, and invites are the primary one. createInvite() /

parseInvite() produce and verify a self-certifying blob, checkable offline

before create(). That is not a stopgap for discovery: the out-of-band

confirmation a scanned code represents is the *only* trust anchor the

directory has, so removing invites would remove its security model.

The API shape is the security control

Anyone may claim any name, so a username resolves to a set, always, even

for a single-device user (a phone and a laptop are two genuine claims). The

whole set arrives as one username_resolved event: no ranking, no "best"

claim, no per-claim event to race.

That shape is deliberate. A per-claim stream would make "take the first" the

easiest thing an app could write, and an app that auto-picks has quietly

turned a non-authoritative directory into an authoritative-looking one. Its

user then believes the *name* was verified when only a *key* ever was.

Discovery is off by default and additionally requires cold contact

(a claim pointing at an address with no published key packages resolves and

then dead-ends one hop later, so the two are coupled in the transport rather

than merely documented).

The bug class this closes structurally

publication_slot_id became a SyntheticPublication enum

(KeyPackage / Discovery / Deletion). Adding a second record type to the

send queue is exactly the change that half-wires: consult the discriminator at

three of four sites and publication outcomes start poisoning DORS reliability

scoring, silently. That bug has shipped here once already (PR #289).

Now all four sites match on a variant, so a third record kind fails to

compile at each of them rather than scoring itself as delivery.

Deviations from the design record, both deliberate

1. A third Deletion discriminator exists, because a NIP-09 deletion also

rides the send queue and must stay out of the delivery metrics.

2. Resolution accumulates engine-side and emits one event at

end-of-stored-events, with a tick-driven timeout sweep as backstop, rather

than emitting per claim. See the API-shape reasoning above; the record

names auto-selection as risk 1.

Verification

  • 2197 tests green (cargo test --workspace, including doctests)
  • cargo clippy --workspace -- -D warnings clean
  • cargo fmt --all -- --check clean
  • RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps clean
  • tsc clean; JS harness 15/15
  • All four binding sets regenerated together and verified idempotent
  • THIRD-PARTY-NOTICES.md regenerated; license/README/NIP-44 guards pass

**All six test obligations the design record names are covered and

negative-controlled by mutation**: the four-domain non-prefix pin, the

re-authored-record refusal (nostr_author binding), address-shaped usernames

refused, discovery tag != address tag, resolution returning every claimant

with two devices of one user in the fixture, and the DORS metrics exclusion.

One of my own tests was found weak *by* its mutation: the original set test

passed when claims were keyed by address instead of by publisher, because the

fixture did not distinguish the two. test_username_resolution_keys_claims_by_publisher_not_by_address

was added and fails under that mutation.

Golden vectors were computed by an independent Python implementation

(BIP-350 reference bech32m, cryptography Ed25519, secp256k1 from the curve

equation, RFC 5869 HKDF), not by pasting encoder output. That script was

itself validated by reproducing the three already-shipped

test_routing_tag_golden_values literals.

Reviewer notes

  • New runtime dependency: unicode-normalization (plus tinyvec,

tinyvec_macros) for the NFC the record specifies. It is the only part of

this with a binary-size cost, and this repo has a minisize profile that

cares. An ASCII-only username policy would remove it entirely and close the

homograph hole the record calls its least-defended surface, at the cost of

narrowing the spec. Worth measuring before release.

  • Kind 30777 is unregistered. Nothing in the NIPs registry is assigned

anywhere in 30700-30800 (re-checked 2026-08-17). Recorded as an open

question in the spec chapter.

  • No iOS test covers the new config flag, because

OfflineProtocolModule.swift is excluded from the Swift test target. Same

gap nostrColdContactEnabled already has; not a regression.

  • Nothing has run against a live relay. That is item 1's territory, and

this adds a fifth thing for its relay leg to watch.

Wire format, verification order and threat model:

docs/spec/username-discovery.md.