fix(NodeDB): addFromContact follow-up - DEBUG log level, shorter comment, regression tests - #11443
Draft
h3lix1 wants to merge 3 commits into
Draft
fix(NodeDB): addFromContact follow-up - DEBUG log level, shorter comment, regression tests#11443h3lix1 wants to merge 3 commits into
h3lix1 wants to merge 3 commits into
Conversation
Clients send add_contact before every text-message DM, so a contact with no key is an expected, high-frequency case - LOG_INFO there would emit one record per DM. Drop it to LOG_DEBUG. Also trims the comment to two lines per AGENTS.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…irections The fix in 7425f84 snapshots the stored key before CopyUserToNodeInfoLite() and restores it when the entry held a full 32-byte key and the post-copy key is not 32 bytes. Nothing exercised that guard, and the guard is the kind that is easy to widen by accident, so pin all four corners of it. Added to test_nodedb_blocked rather than test_type_conversions: addFromContact() is a NodeDB method that needs a real hot store, getOrCreateMeshNode(), the protected-node cap and saveNodeDatabaseToDisk(). test_nodedb_blocked already constructs a NodeDB via NodeDBTestShim and already declares nodes.proto/warm.dat in test/state-manifest.tsv, which is exactly the state addFromContact persists - so no manifest or suite-registration change is needed. test_type_conversions deliberately avoids the global nodeDB and covers CopyUserToNodeInfoLite in isolation, where the erasure is correct behaviour, not a bug. What each case pins: 1. emptyKeyKeepsStoredKey - a contact with public_key.size == 0 must not erase a stored 32-byte key. This is the reported bug: public_key is a singular bytes field, so a client that simply has no key for a contact is indistinguishable on the wire from one clearing it, and clients send add_contact before every DM. 2. shortKeyKeepsStoredKey - the same for a 4-byte key. CopyUserToNodeInfoLite memcpy's all 32 bytes and then sets size, so a short key both truncates size and rewrites the byte array; anything short of 32 bytes is unusable for PKI and must not displace a full key either. 3. fullKeyReplacesStoredKey - a well-formed 32-byte contact key still replaces a different stored 32-byte key. This is the case that matters most: the obvious over-correction is to turn "never erase" into updateUser()'s blanket first-key-wins pin, which would silently break QR-code contact sharing and every other documented add_contact key-update flow. Without this test, that regression passes cases 1, 2 and 4 unnoticed. 4. fullKeyStoredWhenNoneHeld - the ordinary add_contact path: a full key lands on an entry that held none, so the guard's storedKey.size == 32 precondition cannot be loosened into refusing new keys. Every case asserts both public_key.size and public_key.bytes, because the two fail independently - restoring size alone would leave the zeroed byte array the memcpy wrote, which reads as a valid key and is not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
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 |
Contributor
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
h3lix1
changed the title
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.
Follow-up to #11432, which merged before these three items landed on the branch. All were raised in review of that PR.
1. Log the keyless-contact path at DEBUG. Clients send
add_contactbefore every text-message DM, so a contact carrying no key is an expected, high-frequency case.LOG_INFOthere emits one record per DM. (CodeRabbit flagged this on #11432.)2. Shorten the comment to two lines, per
AGENTS.md: "Keep code comments minimal - one or two lines, max."3. Add the regression coverage requested on #11432 — four cases in
test/test_nodedb_blocked, which already builds a realNodeDBvia its test shim:0xA50x5A0x110x220x33Each asserts
.sizeand.bytesindependently. That matters:CopyUserToNodeInfoLitememcpy's all 32 bytes and then sets size, so restoring size alone would leave a zeroed byte array that still reads as a valid key. The third case is the guard against over-correcting the original fix into a blanket pin —add_contactmust still be able to update a key.No manifest changes needed:
test_nodedb_blockedalready declaresnodes.protoandwarm.datwrites intest/state-manifest.tsv, and suite discovery is dynamic since #11413.Behaviour of the merged fix is unchanged — this only lowers a log level, trims a comment, and adds tests.
🤖 Generated with Claude Code