mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-11 16:57:39 +00:00
fix(quartz): audit fixes — foreign-OK confirmation bug, normalizer hot-path allocation
Audit findings across the branch, each verified with a failing test or
measurement before the fix:
- publishAndCollectResults counted an OK from a relay OUTSIDE relayList
(same event id — a probe-wave straggler, or any republish of the same
event to a different relay set) toward its confirmation window, ending
the wait loop early and misreporting still-pending listed relays as
NO_RESPONSE. The OK branch now carries the same relayList guard the
onCannotConnect/onDisconnected branches always had. Regression test
proves the failure without the guard. readWriteCheck additionally
varies the probe event content per wave so wave N's confirmation window
can never match wave N-1's event id at all.
- RelayUrlNormalizer.fix() called trimEnd('%','2','0') unconditionally,
allocating a full string copy for ANY url merely ending in '%', '2' or
'0' — which includes every relay port ending in zero (wss://host:3030).
Now gated on endsWith("%20"), keeping the hot path allocation-free;
semantics unchanged (test pins both the trim and the untouched-port
cases).
- amy relay probe --file: unreadable file is now a clean bad_args error
instead of a stack trace, and skipped onion urls are counted and
reported (file_onion_skipped) instead of vanishing from the tally.
- probeFlow KDoc now states that a slow collector eats into the current
wave's absolute deadline (answers are still recorded; silent relays get
less listening time), not just that it delays the next wave.
Verified non-issues: androidx.collection LruCache is internally locked
(safe for CachedNip11Fetcher/normalizer concurrency); probeWave's
per-terminal emission cannot lose or double-emit verdicts (remaining-set
guard, data maps read at emission time); existing publish callers all
benefit from the OK guard rather than depending on the old behavior.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A9sSyh1QLJD3PZ18tVPPVK
This commit is contained in:
@@ -349,21 +349,26 @@ object RelayCommands {
|
||||
|
||||
var fileRaw = 0
|
||||
var fileRejected = 0
|
||||
var fileOnion = 0
|
||||
val fileRelays = HashSet<NormalizedRelayUrl>()
|
||||
if (fromFile != null) {
|
||||
File(fromFile).forEachLine { line ->
|
||||
val candidates = File(fromFile)
|
||||
if (!candidates.canRead()) return Output.error("bad_args", "cannot read --file $fromFile")
|
||||
candidates.forEachLine { line ->
|
||||
if (line.isBlank()) return@forEachLine
|
||||
fileRaw++
|
||||
val normalized = line.normalizeRelayUrlOrNull()
|
||||
if (normalized == null) {
|
||||
fileRejected++
|
||||
} else if (!RelayUrlNormalizer.isOnion(normalized.url)) {
|
||||
} else if (RelayUrlNormalizer.isOnion(normalized.url)) {
|
||||
fileOnion++
|
||||
} else {
|
||||
fileRelays.add(normalized)
|
||||
}
|
||||
}
|
||||
System.err.println(
|
||||
"[relay-probe] $fromFile: $fileRaw urls → ${fileRelays.size} unique clearnet relays " +
|
||||
"($fileRejected rejected by the normalizer)",
|
||||
"($fileRejected rejected by the normalizer, $fileOnion onion skipped)",
|
||||
)
|
||||
}
|
||||
|
||||
@@ -412,6 +417,7 @@ object RelayCommands {
|
||||
"file_urls" to (if (fromFile != null) fileRaw else null),
|
||||
"file_normalized" to (if (fromFile != null) fileRelays.size else null),
|
||||
"file_rejected" to (if (fromFile != null) fileRejected else null),
|
||||
"file_onion_skipped" to (if (fromFile != null) fileOnion else null),
|
||||
"reachable" to result.reachable.size,
|
||||
"dead" to result.dead.size,
|
||||
"closed_by_policy" to authWalled,
|
||||
|
||||
+6
-1
@@ -141,7 +141,12 @@ suspend fun INostrClient.publishAndCollectResults(
|
||||
|
||||
when (msg) {
|
||||
is OkMessage -> {
|
||||
if (msg.eventId == event.id) {
|
||||
// The relayList guard matters, not just the id: the same event may
|
||||
// have been published to OTHER relays by an earlier call (probe
|
||||
// waves, republish), and counting their late OKs here would inflate
|
||||
// receivedResults and end the wait loop before every listed relay
|
||||
// answered — misreporting the missing ones as NO_RESPONSE.
|
||||
if (msg.eventId == event.id && relay.url in relayList) {
|
||||
resultChannel.trySend(DetailedResult(relay.url, msg.success, msg.message, mark.elapsedNow().inWholeMilliseconds))
|
||||
Log.d("publishAndConfirm") { "onSendResponse Received response for ${msg.eventId} from relay ${relay.url} message ${msg.message} success ${msg.success}" }
|
||||
}
|
||||
|
||||
+5
-6
@@ -180,12 +180,11 @@ class RelayUrlNormalizer {
|
||||
if (rawUrl.length < 4) return null
|
||||
if (rawUrl.contains("%00")) return null
|
||||
|
||||
// Trim trailing %20 (percent-encoded spaces from malformed event data)
|
||||
val url =
|
||||
rawUrl.trimEnd('%', '2', '0').let { trimmed ->
|
||||
// Only accept if we actually removed a trailing %20 pattern
|
||||
if (trimmed.length < rawUrl.length && rawUrl.endsWith("%20")) trimmed else rawUrl
|
||||
}
|
||||
// Trim trailing %20 (percent-encoded spaces from malformed event data).
|
||||
// The endsWith gate keeps the hot path allocation-free: trimEnd would
|
||||
// copy the string for ANY url merely ending in '%', '2' or '0' — which
|
||||
// includes every port ending in zero ("wss://host:3030").
|
||||
val url = if (rawUrl.endsWith("%20")) rawUrl.trimEnd('%', '2', '0') else rawUrl
|
||||
if (url.length < 4) return null
|
||||
|
||||
// Reject URLs with %20 in the middle — these are garbage
|
||||
|
||||
+10
-4
@@ -152,8 +152,11 @@ class RelayProber(
|
||||
* [filters] picks the check, as in [probe]: [LIVENESS_FILTERS] (default) or
|
||||
* [readTestFilter].
|
||||
*
|
||||
* Probing starts when the flow is collected and pauses between waves while the
|
||||
* collector is busy (emission is sequential). Pair each verdict with
|
||||
* Probing starts when the flow is collected, and emission is sequential — a slow
|
||||
* collector delays the next wave AND eats into the current wave's [timeoutMs]
|
||||
* window (the deadline is absolute; answers keep being recorded while the
|
||||
* collector runs, but silent relays get less listening time). Keep per-verdict
|
||||
* work light, or buffer, when precise deadlines matter. Pair each verdict with
|
||||
* [toDiscoveryEventTemplate] to turn the stream into signable NIP-66 kind:30166
|
||||
* records for another process to sign and publish.
|
||||
*/
|
||||
@@ -194,11 +197,14 @@ class RelayProber(
|
||||
): Map<NormalizedRelayUrl, ReadWriteVerdict> {
|
||||
val out = HashMap<NormalizedRelayUrl, ReadWriteVerdict>()
|
||||
val distinct = relays.toSet()
|
||||
for (wave in distinct.chunked(waveSize.coerceAtLeast(1))) {
|
||||
for ((waveIndex, wave) in distinct.chunked(waveSize.coerceAtLeast(1)).withIndex()) {
|
||||
val reads = HashMap<NormalizedRelayUrl, Long>()
|
||||
probeWave(wave, timeoutMs, readTestFilter(readLimit)) { reads[it.relay] = it.rttEoseMs }
|
||||
|
||||
val event = signer.sign(RelayProbeWriteTest.build())
|
||||
// A distinct event id per wave (createdAt has second granularity, so the
|
||||
// content must vary) keeps a straggler OK from an earlier wave's relays
|
||||
// from ever matching this wave's confirmation window.
|
||||
val event = signer.sign(RelayProbeWriteTest.build(content = "NIP-66 write probe $waveIndex"))
|
||||
val writes = client.publishAndCollectResults(event, wave.toSet(), (timeoutMs / 1000).coerceAtLeast(1))
|
||||
|
||||
for (relay in wave) {
|
||||
|
||||
+8
@@ -53,6 +53,14 @@ class RelayUrlFormatterTest {
|
||||
assertNull(RelayUrlNormalizer.normalizeOrNull("wss://relay%20list%20to%20discover%20the%20user's%20content"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun trailingPercentTwentyIsTrimmedButBareTrailingZeroIsNot() {
|
||||
assertEquals("wss://nostr.mom/", RelayUrlNormalizer.normalizeOrNull("wss://nostr.mom%20")?.url)
|
||||
// urls merely ending in '%', '2' or '0' (every port ending in zero) must pass untouched
|
||||
assertEquals("wss://nostr.mom:3030/", RelayUrlNormalizer.normalizeOrNull("wss://nostr.mom:3030")?.url)
|
||||
assertEquals("wss://nostr.mom:8020/", RelayUrlNormalizer.normalizeOrNull("wss://nostr.mom:8020")?.url)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun httpWithPathIsNotARelay() {
|
||||
// Mastodon/bridge actor urls from `proxy` tags: web resources, not relays
|
||||
|
||||
+32
@@ -298,6 +298,38 @@ class RelayProberFlowTest {
|
||||
assertTrue(verdict.rttWriteMs >= 0, "a rejection is still a round trip")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun foreignRelayOkDoesNotEndTheWriteConfirmationEarly() =
|
||||
runTest {
|
||||
// A relay OUTSIDE the checked set answering with the same event id (a
|
||||
// straggler from an earlier wave that got the same probe event) must not
|
||||
// count toward the confirmation window — before the relayList guard in
|
||||
// publishAndCollectResults, it ended the wait early and misreported the
|
||||
// real relay as silent.
|
||||
val client = ScriptedClient()
|
||||
val signer = NostrSignerInternal(KeyPair())
|
||||
val foreign = RelayUrlNormalizer.normalize("wss://foreign.example.com")
|
||||
var result: Map<NormalizedRelayUrl, RelayProber.ReadWriteVerdict>? = null
|
||||
|
||||
val check =
|
||||
launch {
|
||||
result = RelayProber(client).readWriteCheck(listOf(fast), signer, timeoutMs = 5_000)
|
||||
}
|
||||
launch {
|
||||
delay(50)
|
||||
client.listener!!.onEose(fast, null)
|
||||
while (client.published == null) delay(10)
|
||||
client.answerOk(foreign, true, "")
|
||||
delay(100)
|
||||
client.answerOk(fast, true, "")
|
||||
}
|
||||
check.join()
|
||||
|
||||
val verdict = result!![fast]!!
|
||||
assertEquals(true, verdict.writeAccepted, "the listed relay's OK must still be awaited and recorded")
|
||||
assertNull(result!![foreign], "the foreign relay must not appear in the result")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun silentWriteLeavesTheWriteSideUnobserved() =
|
||||
runTest {
|
||||
|
||||
Reference in New Issue
Block a user