diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 1ddb26573..c98a53568 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{ @@ -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) @@ -1724,22 +1714,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 diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 69d10d1cc..ceb5583e1 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,375 @@ 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) + } +} + +// 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) +}