routing: Add Validation for Decoding Blinded Paths #8142
Conversation
|
@morehouse opening this up early for a first look, specifically want to make sure that I'm grasping the fuzz test properly:
To do this, I've split fuzzing up into final and non-final tests, but I'm not sure that it's the best-practice way of doing things? I could also pull a byte off the fuzzing data to decide this context? |
5585186 to
a85d723
Compare
9405c80 to
df88cc2
Compare
df88cc2 to
3f01dba
Compare
3f01dba to
8b55b2d
Compare
|
Code looks good and the fuzz test seems happy again (mostly). There's another small change that needs to be made to the fuzz tests to accommodate the route blinding stuff, but it doesn't belong in this PR. I'll send a separate PR once this one is merged. I'll run the fuzz tests overnight to check for any other problems, and do a final review tomorrow if everything passes. There' a couple small unresolved comments, but nothing blocking. |
|
cc: @ziggie1984 for review |
There was a problem hiding this comment.
Great addition of validation logic when creating/parsing the payload data.
PR looks really good 👏, just had some minor nits
For me personally I found the commit splitting a bit too fine granular because all these small testcase commits in the beginning were clear to me after I reviewed the commit where the validation logic was added.
|
@carlaKC, remember to re-request review from reviewers when ready |
8b55b2d to
e1db579
Compare
Add the missing channel field to the final hop in our clear text route test case. Note that this is the channel of the hop. With the addition of stricter validation, we'll need this so that the penultimate hop has a non-zero next channel ID.
Blinding points will always be accompanied by encrypted data, so update the test to more accurately represent reality.
e1db579 to
eec6795
Compare
Update test to include the sphinx action to more closely represent reality. This will be required when we add more validation to the presence of a nextChanID field. A MoreHops action is chose because we're testing the case with a payload that contains forwarding info.
Provide valid hop payloads for tests cases that use TLV onion format.
Previously, we were using nextChanID to determine whether a hop payload is for the final recipient. This is no longer suitable in a route-blinding world where intermediate hops are allowed to have zero nextChanID TLVs (as this information is provided to forwarding nodes in their encrypted data). This commit updates payload reading to use the signal provided by sphinx that we are on the last packet, rather than implying it from the contents of a hop.
Previously, we'd use the value of nextChanID to infer whether a payload was for the final hop in a route. This commit updates our packing logic to explicitly signal to account for blinded routes, which allow zero value nextChanID in intermediate hops. This is a preparatory commit that allows us to more thoroughly validate payloads.
Fix our existing test to have a valid intermediate hop that will pass stricter validation. Previously, we did not specify a next channel for an intermediate hop (which violates bolt4).
eec6795 to
31d4242
Compare
|
Pushed comment change + additional test case for validation. |
Fixes #8137 and expands validation of blinded routes.
As outlined in the original issue, the fuzzer ran into the following problems:
PackHopPayload.NewPayloadFromReaderbecause LND doesn't support forwarding blinded routes.This PR updates validation to be consistent across all validation functions.
A few of the commits in the beginning of the PR are just fixing up test data to be more accurate before stricter validation is implemented. They're included separately to make sure that they pass with the new and old code.