REVIEW-f6c4a17.md (2155B)
1 # Review: IMPL6 Route Blinding (f6c4a17) 2 3 ## Status: Needs fix for key derivation 4 5 ## Critical Issue 6 7 ### 1. Wrong key-type in deriveBlindingRho 8 9 **File:** `Blinding.hs:113` 10 11 Current: 12 ```haskell 13 deriveBlindingRho (SharedSecret !ss) = 14 let SHA256.MAC !result = SHA256.hmac "blinded_node_id" ss 15 in DerivedKey result 16 ``` 17 18 Per BOLT4 spec, the rho key for encrypting blinded hop data should use 19 `"rho"` as the HMAC key, not `"blinded_node_id"`. The `"blinded_node_id"` 20 key-type is only for deriving the blinded node ID scalar. 21 22 **Fix:** 23 ```haskell 24 deriveBlindingRho (SharedSecret !ss) = 25 let SHA256.MAC !result = SHA256.hmac "rho" ss 26 in DerivedKey result 27 ``` 28 29 **Priority:** High - affects interoperability with other implementations 30 31 ## Minor Issues 32 33 ### 2. Duplicate helper functions 34 35 `Blinding.hs` duplicates these from `Codec.hs`: 36 - `word16BE`, `word32BE` 37 - `encodeWord64TU`, `decodeWord64TU` 38 - `encodeWord32TU`, `decodeWord32TU` 39 - `toStrict` 40 41 **Suggestion:** Export from Codec, import in Blinding. 42 43 **Priority:** Low (cosmetic) 44 45 ### 3. Silent empty return on encrypt error 46 47 **File:** `Blinding.hs:173` 48 49 ```haskell 50 case AEAD.encrypt BS.empty rho nonce plaintext of 51 Left _ -> BS.empty -- Should not happen with valid key 52 Right (!ciphertext, !mac) -> ciphertext <> mac 53 ``` 54 55 Returning empty ByteString masks potential bugs. 56 57 **Suggestion:** Either return `Maybe BS.ByteString` or use `error` with 58 clear message for truly impossible cases (if key is always 32 bytes). 59 60 **Priority:** Low (defensive) 61 62 ### 4. Manual modular arithmetic in mulSecKey 63 64 Same pattern as Prim.hs - manual Integer arithmetic for scalar 65 multiplication mod q. Works correctly but could potentially use 66 secp256k1 primitives. 67 68 **Priority:** Low (micro-optimization) 69 70 ## Test Coverage 71 72 Tests are comprehensive: 73 - TLV encoding/decoding roundtrips 74 - Encryption/decryption roundtrips 75 - Path creation with 2-3 hops 76 - Chain processing through multiple hops 77 - Error cases (empty path, invalid seed, wrong key) 78 - `next_path_key_override` handling 79 - Determinism checks 80 81 ## Summary 82 83 Fix the `deriveBlindingRho` key-type from `"blinded_node_id"` to `"rho"`. 84 Other issues are low priority.