From 9a76b3ee58fc581f8c4d0bef6f4d0979da3e6ca1 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Mon, 8 Jan 2018 19:45:36 -0800 Subject: [PATCH] lnwallet: only forward freshly locked in HTLC's in ReceiveRevocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes a nasty bug that has been lingering within lnd, and has been noticed due to the added retransmission logic. Before this commit, upon a restart, if we had an active HTLC and received a new commitment update, then we would re-forward ALL active HTLC’s. This could at times lead to a nasty cycle: * We re-forward an HTLC already processed. * We then notice that the time-lock is out of date (retransmitted HTLC), so we go to fail it. * This is detected as a replay attack, so we send an UpdateMalformedHTLC * This second failure ends up creating a nil entry in the log, leading to a panic. * Remote party disconnects. * Upon reconnect we send again as we need to retransmit the changes, this goes on forever. In order to fix this, we now ensure that we only forward HTLC’s that have been newly locked in at this next state. With this, we now avoid the loop described above, and also ensure that we don’t accidentally attempt an HTLC replay attack on our selves. Fixes #528. Fixes #545. --- lnwallet/channel.go | 13 +++- lnwallet/channel_test.go | 141 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 2 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 48cb2c02..2028ce6a 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -3905,18 +3905,27 @@ func (lc *LightningChannel) ReceiveRevocation(revMsg *lnwire.RevokeAndAck) ([]*P continue } + // We'll only forward any new HTLC additions iff, it's "freshly + // locked in". Meaning that the HTLC was only *just* considered + // locked-in at this new state. By doing this we ensure that we + // don't re-forward any already processed HTLC's after a + // restart. if htlc.EntryType == Add && - remoteChainTail >= htlc.addCommitHeightRemote && + remoteChainTail == htlc.addCommitHeightRemote && localChainTail >= htlc.addCommitHeightLocal { htlc.isForwarded = true htlcsToForward = append(htlcsToForward, htlc) - } else if htlc.EntryType != Add && + continue + } + + if htlc.EntryType != Add && remoteChainTail >= htlc.removeCommitHeightRemote && localChainTail >= htlc.removeCommitHeightLocal { htlc.isForwarded = true htlcsToForward = append(htlcsToForward, htlc) + continue } } diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index da3aa1eb..4a234ecb 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io/ioutil" + "math/big" "math/rand" "os" "reflect" @@ -4074,6 +4075,146 @@ func TestSignCommitmentFailNotLockedIn(t *testing.T) { } } +// TestLockedInHtlcForwardingSkipAfterRestart ensures that after a restart, a +// state machine doesn't attempt to re-forward any HTLC's that were already +// locked in, but in a prior state. +func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { + t.Parallel() + + // First, we'll make a channel between Alice and Bob. + aliceChannel, bobChannel, cleanUp, err := createTestChannels(1) + if err != nil { + t.Fatalf("unable to create test channels: %v", err) + } + defer cleanUp() + + // We'll now add two HTLC's from Bob to Alice, then Bob will initiate a + // state transition. + var htlcAmt lnwire.MilliSatoshi = 100000 + htlc, _ := createHTLC(0, htlcAmt) + if _, err := aliceChannel.AddHTLC(htlc); err != nil { + t.Fatalf("unable to add htlc: %v", err) + } + if _, err := bobChannel.ReceiveHTLC(htlc); err != nil { + t.Fatalf("unable to recv htlc: %v", err) + } + htlc2, _ := createHTLC(1, htlcAmt) + if _, err := aliceChannel.AddHTLC(htlc2); err != nil { + t.Fatalf("unable to add htlc2: %v", err) + } + if _, err := bobChannel.ReceiveHTLC(htlc2); err != nil { + t.Fatalf("unable to recv htlc2: %v", err) + } + + // We'll now manually initiate a state transition between Alice and + // bob. + aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + if err != nil { + t.Fatal(err) + } + err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs) + if err != nil { + t.Fatal(err) + } + bobRevocation, err := bobChannel.RevokeCurrentCommitment() + if err != nil { + t.Fatal(err) + } + bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + if err != nil { + t.Fatal(err) + } + + // Alice should detect that she doesn't need to forward any HTLC's. + aliceHtlcsToForward, err := aliceChannel.ReceiveRevocation( + bobRevocation, + ) + if err != nil { + t.Fatal(err) + } + if len(aliceHtlcsToForward) != 0 { + t.Fatalf("alice shouldn't forward any HTLC's, instead wants to "+ + "forward %v htlcs", len(aliceHtlcsToForward)) + } + + err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs) + if err != nil { + t.Fatal(err) + } + aliceRevocation, err := aliceChannel.RevokeCurrentCommitment() + if err != nil { + t.Fatal(err) + } + + // Bob on the other hand, should detect that he now has 2 incoming + // HTLC's that he can forward along. + bobHtlcsToForward, err := bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { + t.Fatal(err) + } + if len(bobHtlcsToForward) != 2 { + t.Fatalf("bob should forward 2 hltcs, instead has %v", + len(bobHtlcsToForward)) + } + + // We'll now restart both Alice and Bob. This emulates a reconnection + // between the two peers. + aliceChannel, err = restartChannel(aliceChannel) + if err != nil { + t.Fatalf("unable to restart alice: %v", err) + } + bobChannel, err = restartChannel(bobChannel) + if err != nil { + t.Fatalf("unable to restart bob: %v", err) + } + + // With both nodes restarted, Bob will now attempt to cancel one of + // Alice's HTLC's. + err = bobChannel.FailHTLC(htlc2.ID, []byte("failreason")) + if err != nil { + t.Fatalf("unable to cancel HTLC: %v", err) + } + err = aliceChannel.ReceiveFailHTLC(htlc2.ID, []byte("bad")) + if err != nil { + t.Fatalf("unable to recv htlc cancel: %v", err) + } + + // We'll now initiate another state transition, but this time Bob will + // lead. + bobSig, bobHtlcSigs, err = bobChannel.SignNextCommitment() + if err != nil { + t.Fatal(err) + } + err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs) + if err != nil { + t.Fatal(err) + } + aliceRevocation, err = aliceChannel.RevokeCurrentCommitment() + if err != nil { + t.Fatal(err) + } + aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + if err != nil { + t.Fatal(err) + } + + // At this point, Bob receives the revocation from Alice, which is now + // his signal to examine all the HTLC's that have been locked in to + // process. + bobHtlcsToForward, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { + t.Fatal(err) + } + + // Bob should detect that he doesn't need to forward *any* HTLC's, as + // he was the one that initiated extending the commitment chain of + // Alice. + if len(bobHtlcsToForward) != 0 { + t.Fatalf("bob shouldn't forward any htlcs, but has: %v", + spew.Sdump(bobHtlcsToForward)) + } +} + // TestInvalidCommitSigError tests that if the remote party sends us an invalid // commitment signature, then we'll reject it and return a special error that // contains information to allow the remote party to debug their issues.