From 902e1e77cbeddeab0096c4a7fc1bf515b1f5a345 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 16 May 2018 13:40:43 +0200 Subject: [PATCH 1/5] lnwallet/channel: init localUpdateLog with index from remoteCommit, remoteUpdateLog from localCommit This commit fixes a bug within channel.go that would lead to the content of the update logs and their indexes getting out of sync during restores. The scenario that could occur was that the localUpdateLog was initiated with a log index taken from the localCommitment. Updates we send (which are added to the localUpdateLog) will be added to the remote commitment first. The problem happened when an update was sent and added to the remote commitment, but not ACKed. Since it was not ACKed, we would not add it to our local commitment. During a restart/restore we would init the localUpdateLog with a height too low, such that when going through the outgoing HTLCs on the remote commitment, we would restore an HTLC at an index higher than our local log HTLC counter. The symmetric change is done to the remoteUpdateLog. --- lnwallet/channel.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 1ddb26573..16c90af4b 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1340,10 +1340,10 @@ func NewLightningChannel(signer Signer, pCache PreimageCache, // First, initialize the update logs with their current counter values // from the local and remote commitments. localUpdateLog := newUpdateLog( - localCommit.LocalLogIndex, localCommit.LocalHtlcIndex, + remoteCommit.LocalLogIndex, remoteCommit.LocalHtlcIndex, ) remoteUpdateLog := newUpdateLog( - remoteCommit.RemoteLogIndex, remoteCommit.RemoteHtlcIndex, + localCommit.RemoteLogIndex, localCommit.RemoteHtlcIndex, ) lc := &LightningChannel{ From d69c358dc6c994eee308cd5ad2db096c60ada82d Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 16 May 2018 13:50:57 +0200 Subject: [PATCH 2/5] lnwallet/channel_test: add TestChannelRestoreUpdateLogs This commit adds a test ensuring that the fix applied in the previous commit works as expected. The test exercises the scenario where the HTLCs on the local, remote and pending remote commitment differ, and we attempt to restore the update logs. We now check that in this case the logs before and after restart are equivalent. --- lnwallet/channel_test.go | 202 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 202 insertions(+) diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 69d10d1cc..dcf96d49d 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -2,7 +2,9 @@ package lnwallet import ( "bytes" + "container/list" "crypto/sha256" + "fmt" "reflect" "runtime" @@ -4906,3 +4908,203 @@ func TestNewBreachRetributionSkipsDustHtlcs(t *testing.T) { "instead %v were", len(breachRet.HtlcRetributions)) } } + +// compareHtlcs compares two PaymentDescriptors. +func compareHtlcs(htlc1, htlc2 *PaymentDescriptor) error { + if htlc1.LogIndex != htlc2.LogIndex { + return fmt.Errorf("htlc log index did not match") + } + if htlc1.HtlcIndex != htlc2.HtlcIndex { + return fmt.Errorf("htlc index did not match") + } + if htlc1.ParentIndex != htlc2.ParentIndex { + return fmt.Errorf("htlc parent index did not match") + } + + if htlc1.RHash != htlc2.RHash { + return fmt.Errorf("htlc rhash did not match") + } + return nil +} + +// compareIndexes is a helper method to compare two index maps. +func compareIndexes(a, b map[uint64]*list.Element) error { + for k1, e1 := range a { + e2, ok := b[k1] + if !ok { + return fmt.Errorf("element with key %d "+ + "not found in b", k1) + } + htlc1, htlc2 := e1.Value.(*PaymentDescriptor), e2.Value.(*PaymentDescriptor) + if err := compareHtlcs(htlc1, htlc2); err != nil { + return err + } + } + + for k1, e1 := range b { + e2, ok := a[k1] + if !ok { + return fmt.Errorf("element with key %d not "+ + "found in a", k1) + } + htlc1, htlc2 := e1.Value.(*PaymentDescriptor), e2.Value.(*PaymentDescriptor) + if err := compareHtlcs(htlc1, htlc2); err != nil { + return err + } + } + + return nil +} + +// compareLogs is a helper method to compare two updateLogs. +func compareLogs(a, b *updateLog) error { + if a.logIndex != b.logIndex { + return fmt.Errorf("log indexes don't match: %d vs %d", + a.logIndex, b.logIndex) + } + + if a.htlcCounter != b.htlcCounter { + return fmt.Errorf("htlc counters don't match: %d vs %d", + a.htlcCounter, b.htlcCounter) + } + + if err := compareIndexes(a.updateIndex, b.updateIndex); err != nil { + return fmt.Errorf("update indexes don't match: %v", err) + } + if err := compareIndexes(a.htlcIndex, b.htlcIndex); err != nil { + return fmt.Errorf("htlc indexes don't match: %v", err) + } + + if a.Len() != b.Len() { + return fmt.Errorf("list lengths not equal: %d vs %d", + a.Len(), b.Len()) + } + + e1, e2 := a.Front(), b.Front() + for ; e1 != nil; e1, e2 = e1.Next(), e2.Next() { + htlc1, htlc2 := e1.Value.(*PaymentDescriptor), e2.Value.(*PaymentDescriptor) + if err := compareHtlcs(htlc1, htlc2); err != nil { + return err + } + } + + return nil +} + +// TestChannelRestoreUpdateLogs makes sure we are able to properly restore the +// update logs in the case where a different number of HTLCs are locked in on +// the local, remote and pending remote commitment. +func TestChannelRestoreUpdateLogs(t *testing.T) { + t.Parallel() + + aliceChannel, bobChannel, cleanUp, err := CreateTestChannels() + if err != nil { + t.Fatalf("unable to create test channels: %v", err) + } + defer cleanUp() + + // First, we'll add an HTLC from Alice to Bob, which we will lock in on + // Bob's commit, but not on Alice's. + htlcAmount := lnwire.NewMSatFromSatoshis(20000) + htlcAlice, _ := createHTLC(0, htlcAmount) + if _, err := aliceChannel.AddHTLC(htlcAlice, nil); err != nil { + t.Fatalf("alice unable to add htlc: %v", err) + } + if _, err := bobChannel.ReceiveHTLC(htlcAlice); err != nil { + t.Fatalf("bob unable to recv add htlc: %v", err) + } + + // Let Alice sign a new state, which will include the HTLC just sent. + aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + if err != nil { + t.Fatalf("unable to sign commitment: %v", err) + } + + // Bob receives this commitment signature, and revokes his old state. + err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs) + if err != nil { + t.Fatalf("unable to receive commitment: %v", err) + } + bobRevocation, _, err := bobChannel.RevokeCurrentCommitment() + if err != nil { + t.Fatalf("unable to revoke commitment: %v", err) + } + + // When Alice now receives this revocation, she will advance her remote + // commitment chain to the commitment which includes the HTLC just + // sent. However her local commitment chain still won't include the + // state with the HTLC, since she hasn't received a new commitment + // signature from Bob yet. + _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + if err != nil { + t.Fatalf("unable to recive revocation: %v", err) + } + + // Now make Alice send and sign an additional HTLC. We don't let Bob + // receive it. We do this since we want to check that update logs are + // restored properly below, and we'll only restore updates that have + // been ACKed. + htlcAlice, _ = createHTLC(1, htlcAmount) + if _, err := aliceChannel.AddHTLC(htlcAlice, nil); err != nil { + t.Fatalf("alice unable to add htlc: %v", err) + } + + // Send the signature covering the HTLC. This is okay, since the local + // and remote commit chains are updated in an async fashion. Since the + // remote chain was updated with the latest state (since Bob sent the + // revocation earlier) we can keep advancing the remote commit chain. + aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + if err != nil { + t.Fatalf("unable to sign commitment: %v", err) + } + + // After Alice has signed this commitment, her local commitment will + // contain no HTLCs, her remote commitment will contain an HTLC with + // index 0, and the pending remote commitment (a signed remote + // commitment which is not AKCed yet) will contain an additional HTLC + // with index 1. + + // We now re-create the channels, mimicking a restart. This should sync + // the update logs up to the correct state set up above. + newAliceChannel, err := NewLightningChannel( + aliceChannel.Signer, nil, aliceChannel.channelState, + ) + if err != nil { + t.Fatalf("unable to create new channel: %v", err) + } + defer newAliceChannel.Stop() + + newBobChannel, err := NewLightningChannel( + bobChannel.Signer, nil, bobChannel.channelState, + ) + if err != nil { + t.Fatalf("unable to create new channel: %v", err) + } + defer newBobChannel.Stop() + + // compare all the logs between the old and new channels, to make sure + // they all got restored properly. + err = compareLogs(aliceChannel.localUpdateLog, + newAliceChannel.localUpdateLog) + if err != nil { + t.Fatalf("alice local log not restored: %v", err) + } + + err = compareLogs(aliceChannel.remoteUpdateLog, + newAliceChannel.remoteUpdateLog) + if err != nil { + t.Fatalf("alice remote log not restored: %v", err) + } + + err = compareLogs(bobChannel.localUpdateLog, + newBobChannel.localUpdateLog) + if err != nil { + t.Fatalf("bob local log not restored: %v", err) + } + + err = compareLogs(bobChannel.remoteUpdateLog, + newBobChannel.remoteUpdateLog) + if err != nil { + t.Fatalf("bob remote log not restored: %v", err) + } +} From fad7821e455889f6c10b1a619cec9b9ac734fb0d Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 16 May 2018 20:51:09 +0200 Subject: [PATCH 3/5] lnwallet/channel test: add TestChannelRestoreUpdateLogsFailedHTLC This commit adds a test that runs through a scenario where an HTLC is added then failed, making sure the update logs are properly restored at any point during the process. --- lnwallet/channel_test.go | 172 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 172 insertions(+) diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index dcf96d49d..ceb5583e1 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -5108,3 +5108,175 @@ func TestChannelRestoreUpdateLogs(t *testing.T) { t.Fatalf("bob remote log not restored: %v", err) } } + +// fetchNumUpdates counts the number of updateType in the log. +func fetchNumUpdates(t updateType, log *updateLog) int { + num := 0 + for e := log.Front(); e != nil; e = e.Next() { + htlc := e.Value.(*PaymentDescriptor) + if htlc.EntryType == t { + num++ + } + } + return num +} + +// assertInLog checks that the given log contains the expected number of Adds +// and Fails. +func assertInLog(t *testing.T, log *updateLog, numAdds, numFails int) { + adds := fetchNumUpdates(Add, log) + if adds != numAdds { + t.Fatalf("expected %d adds, found %d", numAdds, adds) + } + fails := fetchNumUpdates(Fail, log) + if fails != numFails { + t.Fatalf("expected %d fails, found %d", numFails, fails) + } +} + +// assertInLogs asserts that the expected number of Adds and Fails occurs in +// the local and remote update log of the given channel. +func assertInLogs(t *testing.T, channel *LightningChannel, numAddsLocal, + numFailsLocal, numAddsRemote, numFailsRemote int) { + assertInLog(t, channel.localUpdateLog, numAddsLocal, numFailsLocal) + assertInLog(t, channel.remoteUpdateLog, numAddsRemote, numFailsRemote) +} + +// restoreAndAssert creates a new LightningChannel from the given channel's +// state, and asserts that the new channel has had its logs restored to the +// expected state. +func restoreAndAssert(t *testing.T, channel *LightningChannel, numAddsLocal, + numFailsLocal, numAddsRemote, numFailsRemote int) { + newChannel, err := NewLightningChannel( + channel.Signer, nil, channel.channelState, + ) + if err != nil { + t.Fatalf("unable to create new channel: %v", err) + } + defer newChannel.Stop() + + assertInLog(t, newChannel.localUpdateLog, numAddsLocal, numFailsLocal) + assertInLog(t, newChannel.remoteUpdateLog, numAddsRemote, numFailsRemote) +} + +// TesstChannelRestoreUpdateLogsFailedHTLC runs through a scenario where an +// HTLC is added and failed, and asserts along the way that we would restore +// the update logs of the channel to the expected state at any point. +func TestChannelRestoreUpdateLogsFailedHTLC(t *testing.T) { + t.Parallel() + + aliceChannel, bobChannel, cleanUp, err := CreateTestChannels() + if err != nil { + t.Fatalf("unable to create test channels: %v", err) + } + defer cleanUp() + + // First, we'll add an HTLC from Alice to Bob, and lock it in for both. + htlcAmount := lnwire.NewMSatFromSatoshis(20000) + htlcAlice, _ := createHTLC(0, htlcAmount) + if _, err := aliceChannel.AddHTLC(htlcAlice, nil); err != nil { + t.Fatalf("alice unable to add htlc: %v", err) + } + // The htlc Alice sent should be in her local update log. + assertInLogs(t, aliceChannel, 1, 0, 0, 0) + + // A restore at this point should NOT restore this update, as it is not + // locked in anywhere yet. + restoreAndAssert(t, aliceChannel, 0, 0, 0, 0) + + if _, err := bobChannel.ReceiveHTLC(htlcAlice); err != nil { + t.Fatalf("bob unable to recv add htlc: %v", err) + } + + // Lock in the Add on both sides. + if err := forceStateTransition(aliceChannel, bobChannel); err != nil { + t.Fatalf("unable to complete state update: %v", err) + } + + // Since it is locked in, Alice should have the Add in the local log, + // and it should be restored during restoration. + assertInLogs(t, aliceChannel, 1, 0, 0, 0) + restoreAndAssert(t, aliceChannel, 1, 0, 0, 0) + + // Now we make Bob fail this HTLC. + err = bobChannel.FailHTLC(0, []byte("failreason"), nil, nil, nil) + if err != nil { + t.Fatalf("unable to cancel HTLC: %v", err) + } + + err = aliceChannel.ReceiveFailHTLC(0, []byte("failreason")) + if err != nil { + t.Fatalf("unable to recv htlc cancel: %v", err) + } + + // This Fail update should have been added to Alice's remote update log. + assertInLogs(t, aliceChannel, 1, 0, 0, 1) + + // Restoring should restore the HTLC added to Alice's local log, but + // NOT the Fail sent by Bob, since it is not locked in. + restoreAndAssert(t, aliceChannel, 1, 0, 0, 0) + + // Bob sends a signature. + bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + if err != nil { + t.Fatalf("unable to sign commitment: %v", err) + } + err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs) + if err != nil { + t.Fatalf("unable to receive commitment: %v", err) + } + + // When Alice receives Bob's new commitment, the logs will stay the + // same until she revokes her old state. The Fail will still not be + // restored during a restoration. + assertInLogs(t, aliceChannel, 1, 0, 0, 1) + restoreAndAssert(t, aliceChannel, 1, 0, 0, 0) + + aliceRevocation, _, err := aliceChannel.RevokeCurrentCommitment() + if err != nil { + t.Fatalf("unable to revoke commitment: %v", err) + } + _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { + t.Fatalf("bob unable to process alice's revocation: %v", err) + } + + // At this point Alice has advanced her local commitment chain to a + // commitment with no HTLCs left. The current state on her remote + // commitment chain, however, still has the HTLC active, as she hasn't + // sent a new signature yet. + assertInLogs(t, aliceChannel, 1, 0, 0, 1) + restoreAndAssert(t, aliceChannel, 1, 0, 0, 0) + + // Now send a signature from Alice. This will give Bob a new commitment + // where the HTLC is removed. + aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + if err != nil { + t.Fatalf("unable to sign commitment: %v", err) + } + err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs) + if err != nil { + t.Fatalf("unable to receive commitment: %v", err) + } + + // When sending a new commitment, Alice will add a pending commit to + // here remote chain. In this case it doesn't contain any new updates, + // so it won't affect the restoration. + assertInLogs(t, aliceChannel, 1, 0, 0, 1) + restoreAndAssert(t, aliceChannel, 1, 0, 0, 0) + + // When Alice receives Bob's revocation, the Fail is irrovacably locked + // in on both sides. She should compact the logs, removing the HTLC + // and the corresponding Fail from the local update log. + bobRevocation, _, err := bobChannel.RevokeCurrentCommitment() + if err != nil { + t.Fatalf("unable to revoke commitment: %v", err) + } + _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + if err != nil { + t.Fatalf("unable to recive revocation: %v", err) + } + + assertInLogs(t, aliceChannel, 0, 0, 0, 0) + restoreAndAssert(t, aliceChannel, 0, 0, 0, 0) +} From c1e96e01e620340ee80d57d6c3cb3ed575ed01fe Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 16 May 2018 13:34:35 +0200 Subject: [PATCH 4/5] lnwallet/channel: remove restoreHtlc from pendingRemoteCommit This commit removes the stage during updateLog restoration where we would attempt to restore incoming HLTCs from the pendingRemoteCommit, in addition to update our log and htlc counter to reflect this state. The reason we can safely remove this is to observe that a pending remote commit is always created from a commitDiff which only contains updates made by _us_, and thus only taken from the localUpdateLog. The same can be said for the counters, when creating a commitDiff we'll always use the remoteACKedIndex as the index into the remoteUpdateLog, meaning that all potential updates will already be included in the remote commit that has been ACKed. --- lnwallet/channel.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 16c90af4b..f041d472d 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1724,22 +1724,6 @@ func (lc *LightningChannel) restoreStateLogs( return nil } - // If we do have a dangling commitment for the remote party, then we'll - // also restore into the log any incoming HTLC's offered by them. Any - // outgoing HTLC's that were initially committed in this new state will - // be restored below. - for i := range pendingRemoteCommit.incomingHTLCs { - htlc := pendingRemoteCommit.incomingHTLCs[i] - lc.remoteUpdateLog.restoreHtlc(&htlc) - } - - // We'll also update the log counters to match the latest known - // counters in this dangling commitment. Otherwise, our updateLog would - // have dated counters as it was initially created using their lowest - // unrevoked commitment. - lc.remoteUpdateLog.logIndex = pendingRemoteCommit.theirMessageIndex - lc.remoteUpdateLog.htlcCounter = pendingRemoteCommit.theirHtlcIndex - pendingCommit := pendingRemoteCommitDiff.Commitment pendingHeight := pendingCommit.CommitHeight From 9c7163187b4b32c87e8f12621076bc21cc0ef60f Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 16 May 2018 14:39:22 +0200 Subject: [PATCH 5/5] lnwallet/channel: don't restore htlcs uneccessary This commit removes redundant HTLC restoring. We don't have to restore outgoing HTLCs from the local commitment, as we _know_ they will always be added to the remote commitment first. Also, when receiving Settles/Fails, they will be removed from the local commitment first. This way we can be sure that outgoing HTLCs found on the local commitment always will be found on the remote commitment Similarly we don't have to restore incoming HTLCs from the remote commitment, as they will be added to the local commitment first. --- lnwallet/channel.go | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index f041d472d..c98a53568 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1692,27 +1692,17 @@ func (lc *LightningChannel) restoreStateLogs( pendingRemoteCommitDiff *channeldb.CommitDiff, pendingRemoteKeys *CommitmentKeyRing) error { - // For each HTLC within the local commitment, we add it to the relevant - // update logc based on if it's incoming vs outgoing. For any incoming - // HTLC's, we also re-add it to the rHashMap so we can quickly look it - // up. + // For each incoming HTLC within the local commitment, we add it to the + // remote update log. Since HTLCs are added first to the receiver's + // commitment, we don't have to restore outgoing HTLCs, as they will be + // restored from the remote commitment below. for i := range localCommitment.incomingHTLCs { htlc := localCommitment.incomingHTLCs[i] lc.remoteUpdateLog.restoreHtlc(&htlc) } - for i := range localCommitment.outgoingHTLCs { - htlc := localCommitment.outgoingHTLCs[i] - lc.localUpdateLog.restoreHtlc(&htlc) - } - // We'll also do the same for the HTLC"s within the remote commitment - // party. We also insert these HTLC's as it's possible our state has - // diverged slightly in the case of a congruent update from both sides. - // The restoreHtlc method will de-dup the HTLC's to handle this case. - for i := range remoteCommitment.incomingHTLCs { - htlc := remoteCommitment.incomingHTLCs[i] - lc.remoteUpdateLog.restoreHtlc(&htlc) - } + // Similarly, we'll do the same for the outgoing HTLCs within the + // remote commitment, adding them to the local update log. for i := range remoteCommitment.outgoingHTLCs { htlc := remoteCommitment.outgoingHTLCs[i] lc.localUpdateLog.restoreHtlc(&htlc)