smite: add node_announcement codec#78
Conversation
ca869b5 to
256cc0d
Compare
| assert_eq!(original, decoded); | ||
| } |
There was a problem hiding this comment.
nit: just for the e2e test of the actual usage of the message
| assert_eq!(original, decoded); | |
| } | |
| assert_eq!(original, decoded); | |
| assert!(decoded.verify()); | |
| } |
There was a problem hiding this comment.
Currently roundtrip tests that original == decoded. The verify_succeeds_after_sign test below ensures that original.verify() is true. So if both pass, we already know decoded.verify() is true.
I mildly prefer to have the tests separated like they currently are.
|
Added another commit 25acf4e that includes trailing message bytes in the signature, as required by the spec: This is important in case an implementation eventually adds an extension to the |
ekzyis
left a comment
There was a problem hiding this comment.
LGTM, just one question: What about requirements for addresses like:
[The origin node] MUST place address descriptors in ascending order.
[The receiving node] SHOULD ignore the first
address descriptorthat does NOT match the types defined above.
Are they not relevant for fuzzing (yet)?
On the receiving side, we currently do not parse the address descriptors at all, we just save the bytes. For the forseeable future, I don't think we can gain much from extracting those addresses anyway. On the sending side, we currently just let the fuzzer pick arbitrary bytes. In the future we could add a new IR type if the fuzzer needs some help to exercise the address parsing logic, or if our announcements are all getting rejected due to invalid descriptors. |
NishantBansal2003
left a comment
There was a problem hiding this comment.
LGTM 👍
Added another commit 25acf4e that includes trailing message bytes in the signature, as required by the spec:
This is important in case an implementation eventually adds an extension to the
node_announcementmessage.
In the future, any added fields will likely be optional for forward compatibility, so outbound signature verification should still pass, though inbound messages could cause issues. I was thinking of adding an assert that this extra field is empty, so future additions are caught explicitly, but I don’t think it adds much value. The current approach achieves the desired behavior well enough for now
These methods will be needed to correctly sign node_announcement messages we create and to verify signatures provided by a target.
25acf4e to
a0bd0d6
Compare
The first commit implements the standard codec, and the second commit adds the
signandverifymethods we'll want for fuzzing.Ref: #71