Fix searchPeersByHash() silently dropping contacts beyond the 8th hash match - #3243
Open
smellyspice wants to merge 1 commit into
Open
Fix searchPeersByHash() silently dropping contacts beyond the 8th hash match#3243smellyspice wants to merge 1 commit into
smellyspice wants to merge 1 commit into
Conversation
…h match Identity/dest hashes for ANON_REQ (repeater admin login), REQ, and RESPONSE packets are truncated to a fixed PATH_HASH_SIZE of 1 byte on the wire (Identity::isHashMatch()/copyHashTo(), no-length overload), independent of the runtime-configurable path-hash-size setting used for routing. With only 256 possible values, a busy mesh's contact list routinely has multiple contacts sharing the same first public-key byte. searchPeersByHash() stopped scanning the contact list after collecting MAX_SEARCH_RESULTS (8) same-hash matches. Once a 9th contact sharing that byte is added, it can never be reached by the search regardless of position, so any reply from it (including a repeater admin login/telemetry response) permanently fails to decrypt -- a deterministic, silent, per-contact hard failure rather than an intermittent one, since every returned match is tried against its own real shared secret and a genuine mismatch just fails to authenticate (no security reason to cap the search early). Size MAX_SEARCH_RESULTS from MAX_CONTACTS+MAX_ANON_CONTACTS instead of a fixed 8, so the search can never be capped before it's checked every contact that could possibly match. Cost is trivial (a few hundred bytes at most, scaling with the same MAX_CONTACTS the contact list itself already commits to) -- verified building companion firmware at both default (32) and large (350) MAX_CONTACTS, plus a RAM-constrained nRF52 target. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3242
Summary
BaseChatMesh::searchPeersByHash()stops scanning the contact list after collectingMAX_SEARCH_RESULTS(fixed at 8) matches on a contact's 1-byte identity hash prefix.ANON_REQ/REQ/RESPONSE(admin login, telemetry, message delivery) is always exactly 1 byte (PATH_HASH_SIZEinMeshCore.h), independent of the runtimehash_modepath-hash-size setting — a separate mechanism used only for routing path arrays.Fix
Size
MAX_SEARCH_RESULTSfrom the actual contact list bound (MAX_CONTACTS+MAX_ANON_CONTACTS) instead of a fixed 8, so the search can never be capped before every real candidate has been checked. There's no correctness reason for the cap to exist at all — every returned candidate is tried against its own real shared secret, so a genuine collision simply fails to authenticate rather than needing to be excluded from consideration.Not attempting a wire-protocol change here (widening the identity hash itself) — that's a bigger, ecosystem-wide compatibility discussion better left to maintainers.
Testing
Confirmed clean builds (no functional changes beyond the array/macro sizing) on:
Heltec_v3_companion_radio_usb(defaultMAX_CONTACTS=32)Xiao_S3_WIO_companion_radio_usb(MAX_CONTACTS=350) — RAM usage 44.6%, negligible impact from the larger arrayRAK_4631_companion_radio_ble(RAM-constrained nRF52) — RAM usage 63.7%, no meaningful impactNative unit tests currently fail to build on
devindependent of this change (pre-existingatoi/atol/atofmissing-include issue inConfigSerializer.cpp, confirmed viagit stashon a clean checkout) — unrelated to this fix.🤖 Generated with Claude Code