Write the path hash size on zero hop packets 🤖🤖 - #3293
Open
TeTeHacko wants to merge 1 commit into
Open
Conversation
`sendZeroHop()` set `packet->path_len = 0` to mean "no path segments". That byte also carries the path hash size in its upper 2 bits (Packet.h), so the assignment cleared the size as well and every zero hop packet went out declaring a 1-byte path hash, whatever the node's `path.hash.mode` was. `sendFlood()` already takes a `path_hash_size` and writes it via `setPathHashSizeAndCount()`; this makes `sendZeroHop()` do the same. Routing is unaffected either way: a zero hop packet carries no path and is never repeated (Mesh.cpp checks `getPathHashCount() > 0`). What it breaks is what the node reports about itself, since observers derive a node's hash size from the packets they hear. A node set to 2 bytes sends its flood adverts as 2-byte and its periodic local adverts as 1-byte, so it shows up inconsistently and analyzers have had to special-case zero hop adverts to work around it. Reported in meshcore-dev#2154, where the mechanism was identified but the behaviour was left as-is; also reproduced independently there across firmware 14.0-15.0 on several boards. Receivers already tolerate this: Dispatcher.cpp reads the size with `pkt->path_len >> 6` and the hop count with `pkt->path_len & 63`, and treats the legacy value 00 as 1 byte, so a zero hop packet that now declares 2 or 3 bytes parses correctly on existing firmware. Nothing in the tree compares the whole `path_len` byte against zero to detect a zero hop packet. The parameter defaults to 1, so any caller that does not pass a size keeps today's exact bits on the wire. Examples that have a `path.hash.mode` setting now pass it, matching their neighbouring `sendFlood()` calls. `BaseChatMesh::shareContactZeroHop()` deliberately keeps the default: it rebroadcasts another node's stored advert, so our own setting does not apply there, and the default reproduces the previous byte exactly. `simple_secure_chat` has no such setting at all and also keeps the default. Co-Authored-By: Claude Opus 5 (1M context) <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
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.
sendZeroHop()setspacket->path_len = 0to mean "no path segments". That byte also carries the path hash size in its upper 2 bits (Packet.h), so the assignment clears the size too, and every zero hop packet goes out declaring a 1-byte path hash regardless of the node'spath.hash.mode.sendFlood()already takes apath_hash_sizeand writes it throughsetPathHashSizeAndCount(); this makessendZeroHop()do the same.Why it matters
Routing is unaffected — a zero hop packet carries no path and is never repeated (
Mesh.cppgates ongetPathHashCount() > 0). What it affects is what a node reports about itself, because observers derive a node's hash size from the packets they hear. A node configured for 2 bytes sends its flood adverts as 2-byte and its periodic local adverts as 1-byte, so it appears inconsistent and downstream tooling has had to special-case zero hop adverts.This was reported in #2154. The mechanism was identified correctly there ("
sendFloodis now correctly using the path hash mode ... howeversendZeroHop... doesn't") but the behaviour was left as is, and it was independently reproduced in that thread across firmware 14.0–15.0 on several boards. The question asked there and not answered was whether changing it would break older firmware — see the next section.Measured on the Czech community mesh, counting only packets that carry at least one hop (where the width is corroborated by the path length rather than just declared): 849 packets from 158 senders at 2 bytes, 107 from 4 senders at 3 bytes, 85 from 32 senders at 1 byte. Zero hop packets have to be excluded from that kind of census entirely, which is exactly the workaround this change removes the need for.
Backwards compatibility
Receivers already tolerate a non-zero size on a zero hop packet.
Dispatcher.cppreadsso the size is masked out of the hop count and the legacy
00is already accounted for. Nothing in the tree compares the wholepath_lenbyte against zero to detect a zero hop packet. A zero hop packet that now declares 2 or 3 bytes parses correctly on existing firmware.Scope
path_hash_sizedefaults to1, so any caller that does not pass a size produces byte-identical output to today. Examples that have apath.hash.modesetting now pass it, mirroring their adjacentsendFlood()calls. Two call sites deliberately keep the default:BaseChatMesh::shareContactZeroHop()rebroadcasts another node's stored advert blob, so this node's setting does not apply.simple_secure_chathas nopath.hash.modein itsNodePrefsat all.Note that the second
sendZeroHop()overload takesuint16_t* transport_codes, so a bare0fordelay_millisbecomes ambiguous once a fourth parameter exists; the affected call sites pass(uint32_t)0.Testing
pio test -e native -e native_kiss_modem— 48/48 pass.Heltec_v3_repeater,Heltec_v3_companion_radio_ble,Heltec_v3_room_server,RAK_4631_repeater,PicoW_repeater,wio-e5-mini_repeater, plusHeltec_t096_sensorandPicoW_terminal_chat— the last two coversimple_sensorandsimple_secure_chat, which the PR build matrix does not include but this change touches.Related to #2154.