Conversation
|
The test-first approach looks useful, but the implementation should avoid inheriting the first-ingested-route limitation identified in #2085. CoreScope deduplicates transmissions by packet hash, while later observations of the same packet may contain different route evidence. The stored Please consider accumulating route evidence across observations and representing a mixed case when both flood and zero-hop routes have been observed. Otherwise the Relay Airtime Share result can depend on observation arrival order rather than the complete route evidence. It would also be valuable to include a regression test where the same packet hash is observed first through one route class and later through the other, in both ingestion orders. |
|
Review feedback addressed (commit
All 183 frontend suites, full server tests, parent race checks and real-ingestor desktop/mobile validation passed. Three independent reviews are clear. Exact-head Linux CI remains required; the local full ingestor suite needs Windows symlink privilege. |
|
Reviewed against I verified the protocol decoding in Two things should be settled before this merges. 1. It adds a second advert classifier next to the one it was written to fix, in the same response
WHERE from_pubkey = ? AND payload_type = ? AND route_type = ? AND first_seen >= ?That is the first-ingested-route bias. It feeds writeJSON(w, NodeDetailResponse{
Node: node, // carries flood_advert_count_7d
RecentAdverts: recentAdverts, // carries advert_kind
})For any mixed advert those two contradict each other: Either migrate 2. A failed write to an analytics side table now drops the advert packet
if _, err := s.stmtInsertAdvertEvidence.Exec(txID, bit, txID, bit); err != nil {
s.Stats.WriteErrors.Add(1)
return isNew, fmt.Errorf("record advert route evidence: %w", err)
}Sixty lines later the observation insert itself is explicitly non-fatal (
Four smaller thingsThe perf number measures the case least affected. Related, and I have not measured it: The durability claim needs scoping. "Persisted bounded route evidence survives same-observation overwrites in both ingestion orders, restart and retention" is true for traffic ingested after the upgrade, and I confirmed that: the bit is derived from the incoming frame and written on every Backfill wall clock. The backfill scans both Unrelated change bundled in. One questionCan a direct-routed ADVERT reach the air with Minor, no action needed
|
Fixes #2041; addresses the mixed-route review and supplies #2085's authoritative node classification.
An advert hash excludes routing. Observation UPSERTs can erase an intermediate route, so the first frame and surviving rows are insufficient. The ingestor now persists at most two route-evidence rows per retained advert. Repeated evidence does not append rows or advance its sequence; retention removes evidence with its transmission.
A bounded independent feed updates the read-only server even when observation IDs stay unchanged. Cold/chunk/live loads agree on flood, zero-hop, mixed or unknown. The chart keeps existing count/score formulas, including zero-relay rows. Node details expose the same
advert_kind.Backfill runs after ingestion readiness in cancellable, resumable 500-row batches. Live writes preserve unbackfilled observations before overwrite. It recovers surviving evidence only; the chart states the historical limitation. Time windows filter counts/scores, while classification uses known hash history.
Validation
Red
6f8066d: assertion failures. Tests cover both ingestion orders, overwritten middle frames, restart, retention, backfill rollback/resume, loading, cache updates and unchanged totals. Real ingestor/API/desktop/mobile proof, all 183 frontend suites, lint and focused race checks passed.O(n) cached analytics: 30K-packet seven-day median 17.69 → 21.96 ms versus
6f8066d, unchanged ~1.75 MB/165 allocations. Bulk masks run outside the store lock; invalidation is per batch. No speedup claim.Correction red
6a87e0a; green8523576. Full CI passed at8523576(run), including Linux Go, race, browsers, coverage, both container architectures and ARM smoke tests. Local Windows full-ingestor testing required symlink privilege. No configuration changes.