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.