fix(node): reconcile stale identity replays after renumbering#6259
fix(node): reconcile stale identity replays after renumbering#6259jeremiah-k wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
The problem this targets is real — stale identity replays after 2.8 renumbering absolutely need handling, and the reducer + CAS structure with this level of test coverage is a solid foundation. But I don't think this can merge in its current shape: the durable side of the design crosses a trust boundary we deliberately drew, and the identity keying is on the wrong identifier. Concretely:
1. Durable deletes are triggered by unauthenticated mesh packets. On main, NodeInfoDao restricts identity migration to installConfig specifically because those entries "come from the connected device's own vetted node DB over the local link" — anyone can broadcast a NodeInfo. This PR's applyReceivedUserEffects dispatches nodeRepository.reconcileIdentity(deleteNums, upsert) — durable row deletion — from any received User packet. Since public keys are broadcast, an attacker can send User(victim_pubkey) from num = crc32(victim_pubkey): the app durably deletes the victim's real row and accepts attacker-chosen names into the canonical slot (the placeholder base means hasPKC is false, so the incoming key is accepted). Durable identity mutation needs to stay gated on the trusted local link.
2. reconcileIdentity drops app-local state that migrateNodeIdentity exists to preserve. The new DAO method is a bare delete + upsert: notes, isFavorite/isMuted/isIgnored, manuallyVerified, and powerChannelLabels are lost, and remapDmContactKey never runs — so the DM thread stays orphaned under the old !id. A favorited contact upgrading to 2.8 loses your notes and detaches the conversation on their first canonical packet. Any durable path here should go through (or replicate) migrateNodeIdentity semantics.
3. Public key is the wrong canonical identity for durable decisions. Pubkeys change on every firmware flash, erase, and re-key — that's the very event that causes renumbering. device_id (see DeviceIdentity.kt) is the factory-burned identity we treat as canonical, which is why #6228 keyed the install migration on it. Keying durable deletes on crc32(pubkey) means (a) re-keys can never be correlated (the old entry lingers as a ghost and fires a new-node notification anyway), and (b) the machinery entrenches pubkey-as-identity in exactly the layer where we decided it isn't. As a wire-level correlation hint for in-memory dedup, pubkey is fine; as the key for row deletion, it isn't.
4. Two behavior regressions vs main:
- When the same key sits at two non-canonical nums (which
installNodeInfocan create — it writes the device nodedb verbatim without reconciliation), the reducer returns an empty plan and drops the packet: every subsequent User packet from either num is ignored indefinitely, so renames and key updates never apply.mainalways applied the update in-memory. isGeneratedPlaceholdermatches any real node known only from position/telemetry (no key, UNSET hw, default names), so the stale-replay path can durably delete such a node's row — position history included — when a same-key replay arrives at its num.
5. Hot-path cost. NodeIndex.put/remove now full-scan byNum.entries per affected user-id, and put runs on every position/telemetry/status packet via updateNode — O(N) per packet on a 1000-node DB where main was O(1). The reducer also scans all entries per keyed User packet with no fast-path exit when fromNum already holds the key canonically. This needs a reverse index or a fast path before it sits on the packet pipeline.
Smaller notes: the comment on the local-authoritative path says "no repository delete calls," but the branch returns PersistencePlan(deleteNums = otherSameKeyNums, ...) which does exactly that — one of the two needs to change (and given #1, I'd hope it's the code); the myNodeNum guard in the CAS loop is check-then-act (it can change between the read and the CAS, which only covers nodeIndex); and when isNodeDbReady is false, in-memory removals commit while persistence is skipped, so a restart resurrects the rows for packet-by-packet re-reconciliation.
Suggested shape: keep the reducer and CAS for in-memory dedup and notification suppression (that part is good and well-tested), but route all durable identity mutation through the existing installConfig/migrateNodeIdentity path on the trusted local link, keyed on device_id where available. Happy to talk through the design — the in-memory half of this is genuinely useful as-is.
|
@jamesarich Thanks — I agree that the durable part currently crosses the wrong trust boundary. I’ll remove the packet-triggered delete-before-upsert reconciliation API and keep public keys only as an in-memory correlation hint. Remote Durable identity migration will remain exclusively in the trusted I’ll also replace the per-update full scans with incremental reverse indexes, add a canonical fast path, preserve updates when duplicate same-key entries are both noncanonical, and stop treating a telemetry-only placeholder as safe to remove because of an incoming remote key. The local-node number will be included in the same atomic reconciliation state, rather than relying on the current check-then-act guard. The tests will be rewritten around in-memory packet reconciliation and trusted durable migration, with the new packet-driven repository deletion API removed entirely. |
c6e1827 to
8dc7ad6
Compare
Reconcile node identities in memory when firmware 2.8 renumbering or delayed packets present the same validated public key under multiple node numbers. Use one atomic reducer state for the node map, local-node number, reverse ID index, public-key candidates, and session-retired identities so readers never observe an index that disagrees with the published nodes. Treat public keys learned from remote User packets as correlation hints, not as authority to delete or migrate persistent history. Durable number migration remains owned by trusted configuration installation and the existing migrateNodeIdentity repository path. Apply those trusted removals to the in-memory reducer before cancelling old-number notifications, closing both the already-posted and post-selection notification windows without running logging or notification side effects inside a retryable CAS transform. Retain validated old-key hints when trusted migration removes a known identity. Suppress delayed field packets, keyless or invalid User packets, same-key replays, and keys already represented elsewhere for the active database session. Conservatively keep a retired slot closed when trusted migration had no old-key evidence; permit different-key reuse only when a trusted old hint exists and the incoming key is otherwise unrepresented. When legitimate reuse is accepted, clear both the retired-number marker and its old-key hint in the same successful state transition. Select canonical representatives deterministically, preserve placeholders and conflicting different-key identities, and reset session retirement when the node database is cleared or reloaded. Keep diagnostics privacy-safe by logging only a short SHA-256 fingerprint of a validated public key. Never expose raw key prefixes, user identity fields, device identifiers, or transport addresses while explaining reconciliation decisions.
Exercise the identity reducer and trusted-migration boundary across canonical, noncanonical, placeholder, conflict, and local-node presentations. Verify deterministic representative selection and reverse-index consistency as node IDs, numbers, and public keys arrive in different orders. Cover delayed replay suppression for every mutable packet field handled by the manager, including telemetry, position, paxcounter, status, NodeInfo, metadata, keyless User, invalid-key User, and User packets carrying an already represented key. Confirm that remote correlation remains in memory and does not invoke a durable packet-triggered delete-before-upsert repository operation. Use suspending title-format barriers to pause genuine notification candidates after reducer commit but before dispatch. Change retirement, local-node, and identity state while paused, then prove exact stale notifications are skipped. Observe notification cancellation directly to verify trusted retirement is already committed before the cancellation side effect runs. Verify keyless trusted retirements remain closed even for an unrepresented valid key, same-key replay stays suppressed without a canonical candidate, and different-key reuse requires a captured trusted hint. Confirm accepted reuse clears both retirement collections and emits one replacement notification, while complete 2.7-to-2.8 migration suppresses early old-number replay. Retain genuine unrelated first-sighting coverage, clear/reload session isolation, trusted installConfig ordering, and a direct assertion that diagnostic key values are one-way fingerprints rather than raw prefixes. Run all coroutine fixtures through their existing TestScope so JVM and Android-host test execution share a valid scheduler and exception handler.
8dc7ad6 to
d7d7a52
Compare
2 participants
Overview
This makes live node-identity reconciliation deterministic after firmware 2.8 node renumbering while preserving the trusted boundary for durable identity migration.
Firmware 2.8 derives the canonical node number from the public key. A restored node database or delayed
Userpacket can still present the same identity under an older node number, causing stale entries to compete with the canonical node, reappear after reload, or generate duplicate new-node notifications.This separates two responsibilities:
The in-memory reducer reconciles those packets without allowing an unauthenticated remote key to delete, renumber, or rewrite durable identity data.
Builds on: #6228
Depends on: #6256, #6255
Changes
Userpackets as in-memory correlation hints only.clear()cannot republish stale state afterward.Validation
Added coverage for:
Hardware validation completed configuration against firmware 2.8 over TCP and firmware 2.7 over BLE. After trusted migration of a stale identity, normal packet and telemetry processing continued without repeated new-node notifications, durable local-state loss, or a stale-replay loop.