router+build: update to the latest version of lightning-onion #3027
Conversation
Roasbeef
force-pushed
the
new-onion-structs
branch
from
07e730d to
86dbb3f
Compare
cfromknecht
left a comment
cfromknecht
left a comment
There was a problem hiding this comment.
good catch @Roasbeef and @joostjager, two small nits. otherwise appears in good shape
In this commit, we update the process that we use to generate a sphinx packet to send our onion routed HTLC. Due to recent changes in the `sphinx` package we use, we now need to use a new PaymentPath struct. As a result, it no longer makes sense to split up the nodes in a route and their per hop paylods as they're now in the same struct. All tests have been updated accordingly.
…conversion In this commit, we start the first phase of fixing an existing bug within the switch. As is, we don't properly convert `UpdateFailMalformedHTLC` to regular `UpdateFailHTLC` messages that are fully encrypted. When we receive a `UpdateFailMalformedHTLC` today, we'll convert it into a regular fail message by simply encoding the failure message raw. This failure message doesn't have a MAC yet, so if we sent it backwards, then the destination wouldn't be able to decrypt it. We can recognize this type of failure as it'll be the same size as the raw failure message max size, but it has 4 extra bytes for the encoding. When we come across this message, we'll mark is as needing conversion so the switch can take care of it.
In this commit, we add a new method to the ErrorEncrypter interface: `EncryptMalformedError`. This takes a raw error (no encryption or MAC), and encrypts it as if we were the originator of this error. This will be used by the switch to convert malformed fail errors to regular fully encrypted errors.
In this commit, we fix a bug that caused us to be unable to properly handle malformed HTLC failures from our direct link. Before this commit, we would attempt to decrypt it and fail since it wasn't well formed. In this commit, if its an error for a local payment, and it needed to be converted, then we'll decode it w/o decrypting since it's already plaintext.
In this commit, we now properly convert multi-hop malformed HTLC failures. Before this commit, we wouldn't properly add a layer of encryption to these errors meaning that the destination would fail to decrypt the error as it was actually plaintext. To remedy this, we'll now check if we need to convert an error, and if so we'll encrypt it as if it we were the source of the error (the true source is our direct channel peer).
In this commit, we add a new test to ensure that we're able to properly convert malformed HTLC errors that are sourced from multiple hops away, or our direct channel peers. In order to test this effectively, we force the onion decryptors of various peers to always fail which will trigger the malformed HTLC logic.
…eplay test In this commit, we update the itests to expect the correct message for the sphinx replay test. Before the fixes in the prior commits, we expected the wrong error since we were actually unable to decrypt these converted malformed HTLC errors. Now, we'll properly return a parse able error, so we assert against that error instead of the failure to decode an error.
Roasbeef
force-pushed
the
new-onion-structs
branch
from
418cf71 to
2f2a907
Compare
|
With regards to the converted error in case we receive an unknown code in If we keep it open, we may be forced to send out more of these failures, pulling down our reputation with senders. |
|
Right now the returned error when decoding hop iterators doesn't distinguish between a replay and an actual error when decrypting the packet. In both the case of modified packet and an attempted replay, it may actually be the fault of the sender (re-using a packet or messing up with the sphinx packet construction) and not actually the direct peer. Even in the case of a direct peer, force closing inconveniences us more than the peer if it is indeed nefarious. |
I mean just in the case the we receive a |
| } | ||
| } | ||
|
|
||
| t.Run("multi-hop error conversion", func(t *testing.T) { |
There was a problem hiding this comment.
I prefer test cases to not share any state, keep them fully independent to prevent one test from forcing the second one to fail and making it hard to find out what is happening.
There was a problem hiding this comment.
Since they use Fatalf and don't run concurrently, if one of them fails the other one doesn't execute.
There was a problem hiding this comment.
I mean if the first one succeeds, but modifies state in an unexpected way and makes the second one fail.
3 participants
In this commit, as a prep for the latest iteration of the EOB design within the spec, we update the
lightning-onionrepo to the latest version. The second commit in this PR updates any relevant callers to use the new API. This commit was broken off of #2455. The ultimate spontaneous payment implementation will still proceed within #2455 once the re-worked code to encode/decode EOBs from the onion is finished.