Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

router+build: update to the latest version of lightning-onion #3027

Merged
merged 9 commits into from May 3, 2019

Conversation

Projects
None yet
3 participants
@Roasbeef
Copy link
Member

commented Apr 30, 2019

In this commit, as a prep for the latest iteration of the EOB design within the spec, we update the lightning-onion repo 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.

@cfromknecht
Copy link
Collaborator

left a comment

LGTM 🌈

@Roasbeef Roasbeef added this to the 0.6.1 milestone Apr 30, 2019

@Roasbeef Roasbeef force-pushed the Roasbeef:new-onion-structs branch from 07e730d to 86dbb3f Apr 30, 2019

@Roasbeef Roasbeef requested a review from cfromknecht May 1, 2019

@Roasbeef Roasbeef requested a review from joostjager May 1, 2019

@cfromknecht
Copy link
Collaborator

left a comment

good catch @Roasbeef and @joostjager, two small nits. otherwise appears in good shape

Show resolved Hide resolved htlcswitch/failure.go Outdated
Show resolved Hide resolved htlcswitch/failure.go Outdated

Roasbeef added some commits Apr 30, 2019

router: convert Route.ToHopPayloads() to Route.ToSphinxPath()
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.
htlcswitch: for UpdateFailMalformedHTLC packets mark fail as needing …
…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.
htlcswitch: add new EncryptMalformedError method to ErrorEncrypter
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.
htlcswitch: properly handle direct link malformed HTLC failures
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.
htlcswitch: properly convert multi-hop malformed HTLC failures
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).
htlcswitch: add new TestUpdateFailMalformedHTLCErrorConversion test
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.
test: update itest to no longer expect incorrect message for sphinx r…
…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 Roasbeef force-pushed the Roasbeef:new-onion-structs branch from 418cf71 to 2f2a907 May 1, 2019

@joostjager

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

With regards to the converted error in case we receive an unknown code in UpdateFailMalformedHTLC: shouldn't we return a PermanentChannelFailure and force close the channel to the peer?

If we keep it open, we may be forced to send out more of these failures, pulling down our reputation with senders.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

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.

@joostjager

This comment has been minimized.

Copy link
Collaborator

commented May 2, 2019

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 UpdateFailMalformedHTLC message that has a failure code other than invalid onion version, hmac or key. That isn't something that can be triggered by the sender.

}
}

t.Run("multi-hop error conversion", func(t *testing.T) {

This comment has been minimized.

Copy link
@joostjager

joostjager May 2, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef May 3, 2019

Author Member

Since they use Fatalf and don't run concurrently, if one of them fails the other one doesn't execute.

This comment has been minimized.

Copy link
@joostjager

joostjager May 3, 2019

Collaborator

I mean if the first one succeeds, but modifies state in an unexpected way and makes the second one fail.

Show resolved Hide resolved htlcswitch/link.go

@Roasbeef Roasbeef merged commit a8fa409 into lightningnetwork:master May 3, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 60.217%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.