From 71228a6b06d95020682f21c47be70e6ea3cc16b9 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 4 May 2018 14:19:17 +0200 Subject: [PATCH 1/3] lnwallet/channel: don't use commit height to determine FullySynced This commit removes a faulty check we did to determine if the channel commitments were fully synced. We assumed that if out local commitment chain had a height higher than the remote, then we would have state updates present in our chain but not in theirs, and owed a commitment. However, there were cases where this wasn't true, and we would send a new commitment even though we had no new updates to sign. This is a protocol violation. Now we don't longer check the heights to determine if we are fully synced. A consequence of this is that we also need to check if we have any pending fee updates that are nopt yet signed, as those are considered non-empty updates. --- lnwallet/channel.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index f249e555..b048d270 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -3924,15 +3924,27 @@ func (lc *LightningChannel) FullySynced() bool { lastLocalCommit := lc.localCommitChain.tip() lastRemoteCommit := lc.remoteCommitChain.tip() - oweCommitment := lastLocalCommit.height > lastRemoteCommit.height - localUpdatesSynced := (lastLocalCommit.ourMessageIndex == lastRemoteCommit.ourMessageIndex) remoteUpdatesSynced := (lastLocalCommit.theirMessageIndex == lastRemoteCommit.theirMessageIndex) - return !oweCommitment && localUpdatesSynced && remoteUpdatesSynced + pendingFeeAck := false + + // If we have received a fee update which we haven't yet ACKed, then + // we owe a commitment. + if !lc.channelState.IsInitiator { + pendingFeeAck = lc.pendingAckFeeUpdate != nil + } + + // If we have sent a fee update which we haven't yet signed, then + // we owe a commitment. + if lc.channelState.IsInitiator { + pendingFeeAck = lc.pendingFeeUpdate != nil + } + + return localUpdatesSynced && remoteUpdatesSynced && !pendingFeeAck } // RevokeCurrentCommitment revokes the next lowest unrevoked commitment From db254c1258d4b30d00342463446b1f4acf731b43 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 4 May 2018 14:24:40 +0200 Subject: [PATCH 2/3] link test: add TestChannelLinkNoMoreUpdates This commit adds a new test that makes sure we don't try to send commitments for states where there are now new updates. Before the recent change to FullySynced we would do this in this test scenario, as the local an remote commitment heights would differ. The test makes the local commitment chain extend by 1 vs the remote, which would earlier trigger another state update, and checks taht no such state update is made. --- htlcswitch/link_test.go | 237 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 237 insertions(+) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 4e5536b6..8e5d596b 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -3912,3 +3912,240 @@ func restartLink(aliceChannel *lnwallet.LightningChannel, aliceSwitch *Switch, return aliceLink, t, cleanUp, nil } + +// gnerateHtlc generates a simple payment from Bob to Alice. +func generateHtlc(t *testing.T, coreLink *channelLink, + bobChannel *lnwallet.LightningChannel, id uint64) *lnwire.UpdateAddHTLC { + htlcAmt := lnwire.NewMSatFromSatoshis(10000) + hops := []ForwardingInfo{ + { + Network: BitcoinHop, + NextHop: exitHop, + AmountToForward: htlcAmt, + OutgoingCTLV: 144, + }, + } + blob, err := generateRoute(hops...) + invoice, htlc, err := generatePayment(htlcAmt, htlcAmt, 144, + blob) + if err != nil { + t.Fatalf("unable to create payment: %v", err) + } + + // We must add the invoice to the registry, such that Alice + // expects this payment. + err = coreLink.cfg.Registry.(*mockInvoiceRegistry).AddInvoice( + *invoice) + if err != nil { + t.Fatalf("unable to add invoice to registry: %v", err) + } + htlc.ID = id + return htlc +} + +// sendHtlcBobToAlice sends an HTLC from Bob to Alice, that pays to a preimage +// already in Alice's registry. +func sendHtlcBobToAlice(t *testing.T, aliceLink ChannelLink, + bobChannel *lnwallet.LightningChannel, htlc *lnwire.UpdateAddHTLC) { + _, err := bobChannel.AddHTLC(htlc, nil) + if err != nil { + t.Fatalf("bob failed adding htlc: %v", err) + } + + aliceLink.HandleChannelUpdate(htlc) +} + +// sendCommitSigBobToAlice makes Bob sign a new commitment and send it to +// Alice, asserting that it signs expHtlcs number of HTLCs. +func sendCommitSigBobToAlice(t *testing.T, aliceLink ChannelLink, + bobChannel *lnwallet.LightningChannel, expHtlcs int) { + sig, htlcSigs, err := bobChannel.SignNextCommitment() + if err != nil { + t.Fatalf("error signing commitment: %v", err) + } + + commitSig := &lnwire.CommitSig{ + CommitSig: sig, + HtlcSigs: htlcSigs, + } + + if len(commitSig.HtlcSigs) != expHtlcs { + t.Fatalf("Expected %d htlc sigs, got %d", expHtlcs, + len(commitSig.HtlcSigs)) + } + + aliceLink.HandleChannelUpdate(commitSig) +} + +// receiveRevAndAckAliceToBob waits for Alice to send a RevAndAck to Bob, then +// hands this to Bob. +func receiveRevAndAckAliceToBob(t *testing.T, aliceMsgs chan lnwire.Message, + aliceLink ChannelLink, + bobChannel *lnwallet.LightningChannel) { + var msg lnwire.Message + select { + case msg = <-aliceMsgs: + case <-time.After(15 * time.Second): + t.Fatalf("did not receive message") + } + + rev, ok := msg.(*lnwire.RevokeAndAck) + if !ok { + t.Fatalf("expected RevokeAndAck, got %T", msg) + } + + _, _, _, err := bobChannel.ReceiveRevocation(rev) + if err != nil { + t.Fatalf("bob failed receiving revocation: %v", err) + } +} + +// receiveCommitSigAliceToBob waits for Alice to send a CommitSig to Bob, +// signing expHtlcs numbers of HTLCs, then hands this to Bob. +func receiveCommitSigAliceToBob(t *testing.T, aliceMsgs chan lnwire.Message, + aliceLink ChannelLink, bobChannel *lnwallet.LightningChannel, + expHtlcs int) { + var msg lnwire.Message + select { + case msg = <-aliceMsgs: + case <-time.After(15 * time.Second): + t.Fatalf("did not receive message") + } + + comSig, ok := msg.(*lnwire.CommitSig) + if !ok { + t.Fatalf("expected CommitSig, got %T", msg) + } + + if len(comSig.HtlcSigs) != expHtlcs { + t.Fatalf("expected %d htlc sigs, got %d", expHtlcs, + len(comSig.HtlcSigs)) + } + err := bobChannel.ReceiveNewCommitment(comSig.CommitSig, + comSig.HtlcSigs) + if err != nil { + t.Fatalf("bob failed receiving commitment: %v", err) + } +} + +// sendRevAndAckBobToAlice make Bob revoke his current commitment, then hand +// the RevokeAndAck to Alice. +func sendRevAndAckBobToAlice(t *testing.T, aliceLink ChannelLink, + bobChannel *lnwallet.LightningChannel) { + rev, _, err := bobChannel.RevokeCurrentCommitment() + if err != nil { + t.Fatalf("unable to revoke commitment: %v", err) + } + + aliceLink.HandleChannelUpdate(rev) +} + +// receiveSettleAliceToBob waits for Alice to send a HTLC settle message to +// Bob, then hands this to Bob. +func receiveSettleAliceToBob(t *testing.T, aliceMsgs chan lnwire.Message, + aliceLink ChannelLink, bobChannel *lnwallet.LightningChannel) { + var msg lnwire.Message + select { + case msg = <-aliceMsgs: + case <-time.After(15 * time.Second): + t.Fatalf("did not receive message") + } + + settleMsg, ok := msg.(*lnwire.UpdateFulfillHTLC) + if !ok { + t.Fatalf("expected UpdateFulfillHTLC, got %T", msg) + } + + err := bobChannel.ReceiveHTLCSettle(settleMsg.PaymentPreimage, + settleMsg.ID) + if err != nil { + t.Fatalf("failed settling htlc: %v", err) + } +} + +// TestChannelLinkNoMoreUpdates tests that we won't send a new commitment +// when there are no new updates to sign. +func TestChannelLinkNoMoreUpdates(t *testing.T) { + t.Parallel() + + const chanAmt = btcutil.SatoshiPerBitcoin * 5 + const chanReserve = btcutil.SatoshiPerBitcoin * 1 + aliceLink, bobChannel, _, cleanUp, _, err := + newSingleLinkTestHarness(chanAmt, chanReserve) + if err != nil { + t.Fatalf("unable to create link: %v", err) + } + defer cleanUp() + + var ( + coreLink = aliceLink.(*channelLink) + aliceMsgs = coreLink.cfg.Peer.(*mockPeer).sentMsgs + ) + + // Add two HTLCs to Alice's registry, that Bob can pay. + htlc1 := generateHtlc(t, coreLink, bobChannel, 0) + htlc2 := generateHtlc(t, coreLink, bobChannel, 1) + + // We now play out the following scanario: + // + // (1) Alice receives htlc1 from Bob. + // (2) Bob sends signature covering htlc1. + // (3) Alice receives htlc2 from Bob. + // (4) Since Bob has sent a new commitment signature, Alice should + // first respond with a revocation. + // (5) Alice should also send a commitment signature for the new state, + // covering htlc1. + // (6) Bob sends a new commitment signature, covering htlc2 that he sent + // earlier. This signature should cover hltc1 + htlc2. + // (7) Alice should revoke the old commitment. This ACKs htlc2. + // (8) Bob can now revoke his old commitment in response to the + // signature Alice sent covering htlc1. + // (9) htlc1 is now locked in on Bob's commitment, and we expect Alice + // to settle it. + // (10) Alice should send a signature covering this settle to Bob. Only + // htlc2 should now be covered by this signature. + // (11) Bob can revoke his last state, which will also ACK the settle + // of htlc1. + // (12) Bob sends a new commitment signature. This signature should + // cover htlc2. + // (13) Alice will send a settle for htlc2. + // (14) Alice will also send a signature covering the settle. + // (15) Alice should send a revocation in response to the signature Bob + // sent earlier. + // (16) Bob will revoke his commitment in response to the commitment + // Alice sent. + // (17) Send a signature for the empty state. No HTLCs are left. + // (18) Alice will revoke her previous state. + // Alice Bob + // | | + // | ... | + // | | <--- idle (no htlc on either side) + // | | + sendHtlcBobToAlice(t, aliceLink, bobChannel, htlc1) // |<----- add-1 ------| (1) + sendCommitSigBobToAlice(t, aliceLink, bobChannel, 1) // |<------ sig -------| (2) + sendHtlcBobToAlice(t, aliceLink, bobChannel, htlc2) // |<----- add-2 ------| (3) + receiveRevAndAckAliceToBob(t, aliceMsgs, aliceLink, bobChannel) // |------- rev ------>| (4) <--- Alice acks add-1 + receiveCommitSigAliceToBob(t, aliceMsgs, aliceLink, bobChannel, 1) // |------- sig ------>| (5) <--- Alice signs add-1 + sendCommitSigBobToAlice(t, aliceLink, bobChannel, 2) // |<------ sig -------| (6) + receiveRevAndAckAliceToBob(t, aliceMsgs, aliceLink, bobChannel) // |------- rev ------>| (7) <--- Alice acks add-2 + sendRevAndAckBobToAlice(t, aliceLink, bobChannel) // |<------ rev -------| (8) + receiveSettleAliceToBob(t, aliceMsgs, aliceLink, bobChannel) // |------ ful-1 ----->| (9) + receiveCommitSigAliceToBob(t, aliceMsgs, aliceLink, bobChannel, 1) // |------- sig ------>| (10) <--- Alice signs add-1 + add-2 + ful-1 = add-2 + sendRevAndAckBobToAlice(t, aliceLink, bobChannel) // |<------ rev -------| (11) + sendCommitSigBobToAlice(t, aliceLink, bobChannel, 1) // |<------ sig -------| (12) + receiveSettleAliceToBob(t, aliceMsgs, aliceLink, bobChannel) // |------ ful-2 ----->| (13) + receiveCommitSigAliceToBob(t, aliceMsgs, aliceLink, bobChannel, 0) // |------- sig ------>| (14) <--- Alice signs add-2 + ful-2 = no htlcs + receiveRevAndAckAliceToBob(t, aliceMsgs, aliceLink, bobChannel) // |------- rev ------>| (15) + sendRevAndAckBobToAlice(t, aliceLink, bobChannel) // |<------ rev -------| (16) <--- Bob acks that there are no more htlcs + sendCommitSigBobToAlice(t, aliceLink, bobChannel, 0) // |<------ sig -------| (17) + receiveRevAndAckAliceToBob(t, aliceMsgs, aliceLink, bobChannel) // |------- rev ------>| (18) <--- Alice acks that there are no htlcs on Alice's side + + // No there are no more changes to ACK or sign, make sure Alice doesn't + // attempt to send any more messages. + var msg lnwire.Message + select { + case msg = <-aliceMsgs: + t.Fatalf("did not expect message %T", msg) + case <-time.After(100 * time.Millisecond): + } +} From 854e2a0581c7ebf030938d04b01ac732ddc0ad43 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 4 May 2018 14:27:46 +0200 Subject: [PATCH 3/3] link test: add TestChannelLinkWaitForRevocation This commit adds a test where we trigger a situation which would previously make the link think it was never in sync, and potentially create a lot of empty state updates. This would happen if we were waiting for a revocation, while still receiving updates from the remote. Since in this case we could not ACK the updates because of the exhausted revocation window, our local commitchain would extend, while the remote chain would stall. When we finally got the revocation the local commitment height would be far larger than the remote, and FullySynced would return false from that point on. --- htlcswitch/link_test.go | 107 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 8e5d596b..ef6136dc 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -4149,3 +4149,110 @@ func TestChannelLinkNoMoreUpdates(t *testing.T) { case <-time.After(100 * time.Millisecond): } } + +// TestChannelLinkWaitForRevocation tests that we will keep accepting updates +// to our commitment transaction, even when we are waiting for a revocation +// from the remote node. +func TestChannelLinkWaitForRevocation(t *testing.T) { + t.Parallel() + + const chanAmt = btcutil.SatoshiPerBitcoin * 5 + const chanReserve = btcutil.SatoshiPerBitcoin * 1 + aliceLink, bobChannel, _, cleanUp, _, err := + newSingleLinkTestHarness(chanAmt, chanReserve) + if err != nil { + t.Fatalf("unable to create link: %v", err) + } + defer cleanUp() + + var ( + coreLink = aliceLink.(*channelLink) + aliceMsgs = coreLink.cfg.Peer.(*mockPeer).sentMsgs + ) + + // We will send 10 HTLCs in total, from Bob to Alice. + numHtlcs := 10 + var htlcs []*lnwire.UpdateAddHTLC + for i := 0; i < numHtlcs; i++ { + htlc := generateHtlc(t, coreLink, bobChannel, uint64(i)) + htlcs = append(htlcs, htlc) + } + + // We play out the following scenario: + // + // (1) Add the first HTLC. + // (2) Bob sends signature covering the htlc. + // (3) Since Bob has sent a new commitment signature, Alice should first + // respond with a revocation. This revocation will ACK the first htlc. + // (4) Alice should also send a commitment signature for the new state, + // locking in the HTLC on Bob's commitment. Note that we don't + // immediately let Bob respond with a revocation in this case. + // (5.i) Now we send the rest of the HTLCs from Bob to Alice. + // (6.i) Bob sends a new commitment signature, covering all HTLCs up + // to this point. + // (7.i) Alice should respond to Bob's state updates with revocations, + // but cannot send any new signatures for Bob's state because her + // revocation window is exhausted. + // (8) Now let Bob finally send his revocation. + // (9) We expect Alice to settle her first HTLC, since it was already + // locked in. + // (10) Now Alice should send a signature covering this settle + lock + // in the rest of the HTLCs on Bob's commitment. + // (11) Bob receives the new signature for his commitment, and can + // revoke his old state, ACKing the settle. + // (12.i) Now Alice can settle all the HTLCs, since they are locked in + // on both parties' commitments. + // (13) Bob can send a signature covering the first settle Alice sent. + // Bob's signature should cover all the remaining HTLCs as well, since + // he hasn't ACKed the last settles yet. Alice receives the signature + // from Bob. Alice's commitment now has the first HTLC settled, and all + // the other HTLCs locked in. + // (14) Alice will send a signature for all the settles she just sent. + // (15) Bob can revoke his previous state, in response to Alice's + // signature. + // (16) In response to the signature Bob sent, Alice can + // revoke her previous state. + // (17) Bob still hasn't sent a commitment covering all settles, so do + // that now. Since Bob ACKed all settles, no HTLCs should be left on + // the commitment. + // (18) Alice will revoke her previous state. + // Alice Bob + // | | + // | ... | + // | | <--- idle (no htlc on either side) + // | | + sendHtlcBobToAlice(t, aliceLink, bobChannel, htlcs[0]) // |<----- add-1 ------| (1) + sendCommitSigBobToAlice(t, aliceLink, bobChannel, 1) // |<------ sig -------| (2) + receiveRevAndAckAliceToBob(t, aliceMsgs, aliceLink, bobChannel) // |------- rev ------>| (3) <--- Alice acks add-1 + receiveCommitSigAliceToBob(t, aliceMsgs, aliceLink, bobChannel, 1) // |------- sig ------>| (4) <--- Alice signs add-1 + for i := 1; i < numHtlcs; i++ { // | | + sendHtlcBobToAlice(t, aliceLink, bobChannel, htlcs[i]) // |<----- add-i ------| (5.i) + sendCommitSigBobToAlice(t, aliceLink, bobChannel, i+1) // |<------ sig -------| (6.i) + receiveRevAndAckAliceToBob(t, aliceMsgs, aliceLink, bobChannel) // |------- rev ------>| (7.i) <--- Alice acks add-i + select { // | | + case <-aliceMsgs: // | | Alice should not send a sig for + t.Fatalf("unexpectedly received msg from Alice") // | | Bob's last state, since she is + default: // | | still waiting for a revocation + } // | | for the previous one. + } // | | + sendRevAndAckBobToAlice(t, aliceLink, bobChannel) // |<------ rev -------| (8) Finally let Bob send rev + receiveSettleAliceToBob(t, aliceMsgs, aliceLink, bobChannel) // |------ ful-1 ----->| (9) + receiveCommitSigAliceToBob(t, aliceMsgs, aliceLink, bobChannel, numHtlcs-1) // |------- sig ------>| (10) <--- Alice signs add-i + sendRevAndAckBobToAlice(t, aliceLink, bobChannel) // |<------ rev -------| (11) + for i := 1; i < numHtlcs; i++ { // | | + receiveSettleAliceToBob(t, aliceMsgs, aliceLink, bobChannel) // |------ ful-1 ----->| (12.i) + } // | | + sendCommitSigBobToAlice(t, aliceLink, bobChannel, numHtlcs-1) // |<------ sig -------| (13) + receiveCommitSigAliceToBob(t, aliceMsgs, aliceLink, bobChannel, 0) // |------- sig ------>| (14) + sendRevAndAckBobToAlice(t, aliceLink, bobChannel) // |<------ rev -------| (15) + receiveRevAndAckAliceToBob(t, aliceMsgs, aliceLink, bobChannel) // |------- rev ------>| (16) + sendCommitSigBobToAlice(t, aliceLink, bobChannel, 0) // |<------ sig -------| (17) + receiveRevAndAckAliceToBob(t, aliceMsgs, aliceLink, bobChannel) // |------- rev ------>| (18) + + // Both side's state is now updated, no more messages should be sent. + select { + case <-aliceMsgs: + t.Fatalf("did not expect message from Alice") + case <-time.After(50 * time.Millisecond): + } +}