fix(clock-skew): restrict per-node skew to self-originated adverts (#1816, #1818)

[Saarlandpower]

Closes #1816. Closes #1818 (confirmed duplicate of #1816 by the triage bot).

Root cause

byNode is an involvement index (indexResolvedPathHops, store.go:1696-1705, #1558/#1352): a transmission is indexed under every relay-hop pubkey found in an observation's resolved_path, not just its originator. getNodeClockSkewLocked (clock_skew.go:489) iterated every ADVERT transaction under a pubkey without checking who actually signed it, so a relay inherited the clock skew of every broken-clock node it forwarded as if it were its own.

This produced:

• Fleet-wide false no_clock/bimodal_clock classifications on healthy relays whose only "bad" samples were adverts they merely relayed.
• Bit-identical RecentMedianSkewSec "clusters" across unrelated relays that all forwarded the same broken-clock originator.
• Single relays showing a multi-day skew even though their own self-adverts are healthy, because 1-2 relayed adverts from a broken originator landed in the tail of their small recent-window sample (the Bug: Observer node in an island of "me, myself, and I" reports clock is off by nearly ten days. #1818 "island" repro from @cwichura).

Fix

Add txOriginatedBy(tx, pubkey): ADVERTs are self-signed, so decoded["pubKey"] is the originator per protocol (case-insensitive compare as a defensive measure). Apply it as a guard in both the main skew-aggregation loop and the per-hash evidence loop in getNodeClockSkewLocked. byNode itself is untouched — #1558/#1352 still rely on the broader involvement index for other consumers.

Tests

• Existing clock_skew_test.go / clock_skew_issue1094_test.go / clock_skew_issue1285_test.go fixtures built synthetic ADVERT transactions without a pubKey field and seeded s.byNode directly, bypassing the normal indexByNode path where every real ADVERT carries pubKey. Added pubKey to each fixture so it reflects a self-originated advert, which is what these tests already intended to represent. All pre-existing tests pass unchanged in behavior.
• New clock_skew_issue1816_test.go:
  • TestTxOriginatedBy — unit coverage of the new guard (self, foreign, missing pubKey, case-insensitivity).
  • TestIssue1816_RelayDoesNotInheritOriginatorSkew — a relay with healthy self-adverts plus relayed adverts from a broken-clock originator (matching the report's +100.5k s band) must report ok severity based only on its own adverts.
  • TestIssue1816_PureRelaysReportNoSkew_NoBitIdenticalCluster — five relay pubkeys that only ever forward a broken originator's advert (never self-advert) must report nil, not a bit-identical copy of the originator's skew.
  • TestIssue1818_TwoForeignAdvertsDoNotPoisonIslandNode — reproduces the cwichura island scenario: 8 healthy self-adverts + 2 foreign adverts at ~10 days skew must not flip severity or pollute RecentMedianSkewSec.

Full suite: go test ./... passes (one pre-existing, unrelated flaky test — TestHandleNodePaths_PrefixCollision_1352, an index-loading race — reproduces intermittently on unmodified master too).

Operator context: running CoreScope for SaarMesh (SaarLorLux, DE/FR/LU, 800+ nodes); this bug was surfacing as fleet-wide clock-skew false positives on our infra nodes.

Co-Authored-By: Claude noreply@anthropic.com

[Kpa-clawbot]

Automated polish review — parallel personas (carmack / munger / kent-beck). No blocking issues; minor nits only.

Verdict

🟢 Merge-worthy on code + tests. CI shows action_required (first-time-contributor gate) — needs an approver to release the run before merge is possible. No BLOCKER, no MAJOR.

Findings

• kent-beck (tests): PASS. Regression file lands with the fix: TestTxOriginatedBy (unit, 4 cases incl. case-fold + missing pubKey), TestIssue1816_RelayDoesNotInheritOriginatorSkew (matches +100.5k s band from report), TestIssue1816_PureRelaysReportNoSkew_NoBitIdenticalCluster (nails the bit-identical cluster symptom), TestIssue1818_TwoForeignAdvertsDoNotPoisonIslandNode (cwichura island repro, exact 8+2 volume). Pre-existing 1094/1285 fixtures updated to add pubKey — necessary because they seeded byNode directly, bypassing the real indexByNode path where every ADVERT carries pubKey.

• munger (protocol/logic): PASS. Guard is placed in both loops (clock_skew.go:540, clock_skew.go:710) — main aggregation and the per-hash evidence stream stay consistent. byNode itself untouched, so #1558/#1352 involvement consumers are unaffected. Restricting to decoded["pubKey"] is protocol-correct: ADVERTs are self-signed, so the decoded pubkey is authoritative for originator identity.

• carmack (backend): PASS. Change is ~30 LOC of production code, no allocations in the hot path beyond a map lookup that was already happening (ParsedDecoded()), no locking change. strings.EqualFold is the right defensive call and is documented.

Nits (non-blocking, optional)

• clock_skew_issue1816_test.go uses pt := 4 literal rather than the PayloadADVERT constant that production code uses. Existing test files in the tree do the same, so consistent — but the constant would be marginally clearer.
• txOriginatedBy comment references store.go:1696-1705 for indexResolvedPathHops — worth verifying that line range survives future refactors; consider anchoring on function name only.