fix: BLE architecture review fixes and documentation#37
Merged
Conversation
Expand _compute_identity_hash docstring to explain: - Uses truncated 64-bit keys for spawned_interfaces and identity_to_address - Birthday collision risk at ~2^32 (~4 billion) identities - Astronomically safe for BLE mesh networks with <100 peers - Note that fragmenter keys use full 32-char hex for packet reassembly Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When a peer connects with an identity already connected at a different
MAC address (Android MAC rotation), the connection is correctly rejected.
However, the error message format "Connection failed to {address}" was
matching the blacklist regex, causing the new MAC to be blacklisted.
After 3 duplicate rejections, the new MAC would be blacklisted for 60s+,
creating connectivity gaps when the old MAC finally disconnected.
Fix:
- Detect "Duplicate identity" in exception message
- Use severity "info" instead of "error" (doesn't trigger blacklist)
- Use safe message format "Duplicate identity rejected for {address}"
which doesn't match the blacklist regex pattern
Also adds comprehensive tests for MAC rotation blacklist behavior.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Previously, _handle_identity_handshake (peripheral mode) did not check for duplicate identities. If a peer connected via two MACs simultaneously, both connections could be accepted. Now, _handle_identity_handshake calls _check_duplicate_identity before accepting the handshake. If the identity is already connected at a different MAC, the new connection is rejected and disconnected. This makes both central and peripheral modes consistent in rejecting duplicate connections during MAC rotation overlap. Also adds tests for peripheral mode duplicate rejection. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Document the narrow race window where data could arrive from an old MAC address before onAddressChanged callback invalidates the cache entry. The window is very small since onAddressChanged fires synchronously during Kotlin deduplication, and _address_changed_callback() cleans up the stale cache entry. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryFixes critical bugs in MAC rotation handling and adds comprehensive documentation. Bug Fixes:
Documentation Improvements:
Test Coverage:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Central as Central Device<br/>(MAC_NEW)
participant Peripheral as Peripheral Device
participant Check as _check_duplicate_identity
participant Driver as linux_bluetooth_driver
participant ErrorCB as _error_callback
participant Blacklist as Blacklist Mechanism
Note over Central,Peripheral: Scenario: MAC Rotation During Active Connection
Note over Peripheral: MAC_OLD already connected<br/>with identity X
Central->>Driver: connect(MAC_NEW)
Driver->>Peripheral: GATT connect
Driver->>Peripheral: Read identity characteristic
Peripheral-->>Driver: identity X (16 bytes)
Driver->>Check: on_duplicate_identity_detected(MAC_NEW, identity X)
Check->>Check: identity_to_address[hash(X)] == MAC_OLD
Check-->>Driver: True (duplicate detected)
Note over Driver: Central mode: Raise exception
Driver->>Driver: disconnect client
Driver->>Driver: raise RuntimeError("Duplicate identity...")
Driver->>Driver: except Exception as e:<br/>is_duplicate = "Duplicate identity" in str(e)
alt Duplicate Identity (AFTER FIX)
Driver->>ErrorCB: on_error("info", "Duplicate identity rejected for MAC_NEW", e)
Note over ErrorCB: severity="info" → should_blacklist=False
Note over Blacklist: NO blacklist triggered ✓
else Real Connection Failure
Driver->>ErrorCB: on_error("error", "Connection failed to MAC_X", e)
Note over ErrorCB: severity="error" → should_blacklist=True
ErrorCB->>Blacklist: _record_connection_failure(MAC_X)
Note over Blacklist: Blacklist after 3 failures ✓
end
Note over Central,Peripheral: Scenario: Peripheral Mode Duplicate Identity
Central->>Peripheral: GATT connect (MAC_NEW)
Central->>Peripheral: Write identity X to handshake characteristic
Peripheral->>Peripheral: _handle_identity_handshake(MAC_NEW, identity X)
Peripheral->>Check: _check_duplicate_identity(MAC_NEW, identity X)
Check->>Check: identity_to_address[hash(X)] == MAC_OLD
Check-->>Peripheral: True (duplicate detected)
Note over Peripheral: Peripheral mode (AFTER FIX):<br/>Check before accepting connection
Peripheral->>Peripheral: driver.disconnect(MAC_NEW)
Peripheral->>Peripheral: return True (consumed, rejected)
Note over Peripheral: Connection rejected,<br/>no blacklist triggered ✓
|
…4 compatibility - Fix BLEInterface.handle_peripheral_data to use _compute_identity_hash instead of RNS.Identity.full_hash for consistent identity hash computation - Update MockBLEDriver.on_device_connected callback to match the (address, peer_identity) signature in bluetooth_driver.py - Fix test_v2_2_identity_handshake.py and test_v2_2_race_conditions.py to properly mock ble_reticulum.Interface without breaking the namespace - Use BLEFragmenter/BLEReassembler directly in tests instead of non-existent _create_fragmenter/_create_reassembler methods - Fix asyncio.get_event_loop() deprecation in test_ble_peer_interface.py for Python 3.10+ compatibility - Update MAC address test fixtures to account for v2.2 MAC sorting logic - Fix test_peer_address_mac_rotation to properly simulate MAC rotation where old connection is dropped before new one arrives Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add tests that exercise the actual code paths for: - _handle_identity_handshake rejecting duplicate identity and calling disconnect - _handle_identity_handshake gracefully handling disconnect exceptions - linux_bluetooth_driver duplicate identity error handling (log levels, callbacks) These tests cover the 15 lines that were missing coverage: - BLEInterface.py lines 1137-1149 (duplicate identity check in peripheral mode) - linux_bluetooth_driver.py lines 1207-1216, 1234-1240 (error handling) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…andling Add tests that exercise the actual code path in linux_bluetooth_driver.py for duplicate identity exception handling. These tests patch BleakClient to verify that: - Duplicate identity exceptions are logged as WARNING, not ERROR - on_error callback uses 'info' severity for duplicate identity errors - Normal connection failures still use 'error' severity This improves patch coverage for the duplicate identity handling fix by testing the driver code directly rather than just the logic in isolation. Co-Authored-By: Claude Opus 4.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
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.
Summary
Fixes and documentation improvements discovered during meticulous review of BLE architecture diagrams against source code.
Bug Fixes
_handle_identity_handshakenow calls_check_duplicate_identitybefore accepting connections, preventing the same identity from connecting via two MACs simultaneouslyDocumentation
onAddressChangedcallback invalidates the cache_compute_identity_hashdocstring to explain truncated 64-bit keys and birthday collision risk (astronomically safe for BLE mesh networks)Test plan
🤖 Generated with Claude Code