From 2149157d4901af04415fb7fdaeb9c8740e3b9125 Mon Sep 17 00:00:00 2001 From: nsa Date: Mon, 13 Jul 2020 15:24:36 -0400 Subject: [PATCH 1/3] channeldb: filter out unsigned acked updates in AdvanceCommitChainTail This commit moves the deletion of all updates under the unsigned acked updates key from AppendRemoteCommitChain to AdvanceCommitChainTail. This is done because if we went down after signing for these updates but before receiving a revocation, we would incorrectly reject their commitment signature: Alice Bob -----add-----> -----sig-----> <----rev------ <----sig------ -----rev-----> <----fail----- <----sig------ -----rev-----> -----sig-----> *reconnect* <----rev------ <----add------ x----sig------ It is also important to note that filtering is required when we receive a revocation to ensure that we aren't erroneously deleting remote updates. Take the following state transitions: Alice Bob -----add-----> -----sig-----> <----rev------ <----sig------ -----rev-----> -----add-----> -----sig-----> <----fail----- <----sig------ -----rev-----> (alice stores updates here) <----rev------ In the above case, if Alice deleted all updates rather than filtering when receiving the final revocation from Bob, then Alice would have to force close the channel due to missing updates. Since Alice hasn't signed for any of the unsigned acked updates, she should not filter any of them out. --- channeldb/channel.go | 46 +++++++++++++++++++++++++++++++++------- lnwallet/channel_test.go | 8 +++---- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/channeldb/channel.go b/channeldb/channel.go index 4f5cd447..5bec7a47 100644 --- a/channeldb/channel.go +++ b/channeldb/channel.go @@ -1980,14 +1980,6 @@ func (c *OpenChannel) AppendRemoteCommitChain(diff *CommitDiff) error { return err } - // Clear unsigned acked remote updates. We are signing now for - // all that we've got. - err = chanBucket.Delete(unsignedAckedUpdatesKey) - if err != nil { - return fmt.Errorf("unable to clear dangling remote "+ - "updates: %v", err) - } - // TODO(roasbeef): use seqno to derive key for later LCP // With the bucket retrieved, we'll now serialize the commit @@ -2196,6 +2188,44 @@ func (c *OpenChannel) AdvanceCommitChainTail(fwdPkg *FwdPkg) error { return err } + // Persist the unsigned acked updates that are not included + // in their new commitment. + updateBytes := chanBucket.Get(unsignedAckedUpdatesKey) + if updateBytes == nil { + // If there are no updates to sign, we don't need to + // filter out any updates. + newRemoteCommit = &newCommit.Commitment + return nil + } + + r := bytes.NewReader(updateBytes) + unsignedUpdates, err := deserializeLogUpdates(r) + if err != nil { + return err + } + + var validUpdates []LogUpdate + for _, upd := range unsignedUpdates { + lIdx := upd.LogIndex + + // Filter for updates that are not on the remote + // commitment. + if lIdx >= newCommit.Commitment.RemoteLogIndex { + validUpdates = append(validUpdates, upd) + } + } + + var b bytes.Buffer + err = serializeLogUpdates(&b, validUpdates) + if err != nil { + return fmt.Errorf("unable to serialize log updates: %v", err) + } + + err = chanBucket.Put(unsignedAckedUpdatesKey, b.Bytes()) + if err != nil { + return fmt.Errorf("unable to store under unsignedAckedUpdatesKey: %v", err) + } + newRemoteCommit = &newCommit.Commitment return nil diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 50931ef8..5307d867 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -7119,7 +7119,7 @@ func restoreAndAssert(t *testing.T, channel *LightningChannel, numAddsLocal, assertInLog(t, newChannel.remoteUpdateLog, numAddsRemote, numFailsRemote) } -// TesstChannelRestoreUpdateLogsFailedHTLC runs through a scenario where an +// TestChannelRestoreUpdateLogsFailedHTLC 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) { @@ -7224,10 +7224,10 @@ func TestChannelRestoreUpdateLogsFailedHTLC(t *testing.T) { } // 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. + // her remote chain. Since the unsigned acked updates aren't deleted + // until we receive a revocation, the fail should still be present. assertInLogs(t, aliceChannel, 1, 0, 0, 1) - restoreAndAssert(t, aliceChannel, 1, 0, 0, 0) + restoreAndAssert(t, aliceChannel, 1, 0, 0, 1) // When Alice receives Bob's revocation, the Fail is irrevocably locked // in on both sides. She should compact the logs, removing the HTLC and From 8c002a08a7191a7fef5307663aa397f143724cb0 Mon Sep 17 00:00:00 2001 From: nsa Date: Mon, 13 Jul 2020 15:34:47 -0400 Subject: [PATCH 2/3] lnwallet: properly restore removeCommitHeightRemote Previously, we could sign a pending commitment for the remote party, disconnect, and not restore these signed remote updates as having been removed at the pending commitment height. This commit fixes that to look up whether the update under the unsigned acked updates key is present on the pending commitment or not and appropriately set the remove commit heights. --- lnwallet/channel.go | 53 +++++++++++++++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 7c6a861c..a0ce102a 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1867,6 +1867,7 @@ func (lc *LightningChannel) restoreStateLogs( // in our next signature. err := lc.restorePendingRemoteUpdates( unsignedAckedUpdates, localCommitment.height, + pendingRemoteCommit, ) if err != nil { return err @@ -1879,7 +1880,8 @@ func (lc *LightningChannel) restoreStateLogs( // haven't yet signed for. func (lc *LightningChannel) restorePendingRemoteUpdates( unsignedAckedUpdates []channeldb.LogUpdate, - localCommitmentHeight uint64) error { + localCommitmentHeight uint64, + pendingRemoteCommit *commitment) error { lc.log.Debugf("Restoring %v dangling remote updates", len(unsignedAckedUpdates)) @@ -1894,12 +1896,38 @@ func (lc *LightningChannel) restorePendingRemoteUpdates( return err } + logIdx := payDesc.LogIndex + // Sanity check that we are not restoring a remote log update // that we haven't received a sig for. - if payDesc.LogIndex >= lc.remoteUpdateLog.logIndex { + if logIdx >= lc.remoteUpdateLog.logIndex { return fmt.Errorf("attempted to restore an "+ "unsigned remote update: log_index=%v", - payDesc.LogIndex) + logIdx) + } + + // We previously restored Adds along with all the other upates, + // but this Add restoration was a no-op as every single one of + // these Adds was already restored since they're all incoming + // htlcs on the local commitment. + if payDesc.EntryType == Add { + continue + } + + var ( + height uint64 + heightSet bool + ) + + // If we have a pending commitment for them, and this update + // is included in that commit, then we'll use this commitment + // height as this commitment will include these updates for + // their new remote commitment. + if pendingRemoteCommit != nil { + if logIdx < pendingRemoteCommit.theirMessageIndex { + height = pendingRemoteCommit.height + heightSet = true + } } // Insert the update into the log. The log update index doesn't @@ -1907,23 +1935,20 @@ func (lc *LightningChannel) restorePendingRemoteUpdates( // final value was properly persisted with the last local // commitment update. switch payDesc.EntryType { - case Add: - lc.remoteUpdateLog.restoreHtlc(payDesc) - - // Sanity check to be sure that we are not restoring an - // add update that the remote hasn't signed for yet. - if payDesc.HtlcIndex >= lc.remoteUpdateLog.htlcCounter { - return fmt.Errorf("attempted to restore an "+ - "unsigned remote htlc: htlc_index=%v", - payDesc.HtlcIndex) + case FeeUpdate: + if heightSet { + payDesc.addCommitHeightRemote = height + payDesc.removeCommitHeightRemote = height } - case FeeUpdate: lc.remoteUpdateLog.restoreUpdate(payDesc) default: - lc.remoteUpdateLog.restoreUpdate(payDesc) + if heightSet { + payDesc.removeCommitHeightRemote = height + } + lc.remoteUpdateLog.restoreUpdate(payDesc) lc.localUpdateLog.markHtlcModified(payDesc.ParentIndex) } } From b6eff5b0ecab2a26e44174a8cb7579634ceea14e Mon Sep 17 00:00:00 2001 From: nsa Date: Mon, 13 Jul 2020 15:37:14 -0400 Subject: [PATCH 3/3] lnwallet: add regression test TestChannelUnsignedAckedFailure This commit includes a regression test that checks that a force close won't occur and that unsigned acked updates are properly restored. --- lnwallet/channel_test.go | 116 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 5307d867..18b2d760 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -9096,3 +9096,119 @@ func TestProcessAddRemoveEntry(t *testing.T) { }) } } + +// TestChannelUnsignedAckedFailure tests that unsigned acked updates are +// properly restored after signing for them and disconnecting. +// +// The full state transition of this test is: +// +// Alice Bob +// -----add-----> +// -----sig-----> +// <----rev------ +// <----sig------ +// -----rev-----> +// <----fail----- +// <----sig------ +// -----rev-----> +// -----sig-----X (does not reach Bob! Alice dies!) +// +// -----sig-----> +// <----rev------ +// <----add------ +// <----sig------ +// +// The last sig was rejected with the old behavior of deleting unsigned +// acked updates from the database after signing for them. The current +// behavior of filtering them for deletion upon receiving a revocation +// causes Alice to accept the signature as valid. +func TestChannelUnsignedAckedFailure(t *testing.T) { + t.Parallel() + + // Create a test channel so that we can test the buggy behavior. + aliceChannel, bobChannel, cleanUp, err := CreateTestChannels( + channeldb.SingleFunderTweaklessBit, + ) + require.NoError(t, err) + defer cleanUp() + + // First we create an HTLC that Alice sends to Bob. + htlc, _ := createHTLC(0, lnwire.MilliSatoshi(500000)) + + // -----add-----> + _, err = aliceChannel.AddHTLC(htlc, nil) + require.NoError(t, err) + _, err = bobChannel.ReceiveHTLC(htlc) + require.NoError(t, err) + + // Force a state transition to lock in this add on both commitments. + // -----sig-----> + // <----rev------ + // <----sig------ + // -----rev-----> + err = ForceStateTransition(aliceChannel, bobChannel) + require.NoError(t, err) + + // Now Bob should fail the htlc back to Alice. + // <----fail----- + err = bobChannel.FailHTLC(0, []byte("failreason"), nil, nil, nil) + require.NoError(t, err) + err = aliceChannel.ReceiveFailHTLC(0, []byte("bad")) + require.NoError(t, err) + + // Bob should send a commitment signature to Alice. + // <----sig------ + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() + require.NoError(t, err) + err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs) + require.NoError(t, err) + + // Alice should reply with a revocation. + // -----rev-----> + aliceRevocation, _, err := aliceChannel.RevokeCurrentCommitment() + require.NoError(t, err) + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + require.NoError(t, err) + + // Alice should sign the next commitment and go down before + // sending it. + // -----sig-----X + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() + require.NoError(t, err) + + newAliceChannel, err := NewLightningChannel( + aliceChannel.Signer, aliceChannel.channelState, + aliceChannel.sigPool, + ) + require.NoError(t, err) + + // Bob receives Alice's signature. + // -----sig-----> + err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs) + require.NoError(t, err) + + // Bob revokes his current commitment and sends a revocation + // to Alice. + // <----rev------ + bobRevocation, _, err := bobChannel.RevokeCurrentCommitment() + require.NoError(t, err) + _, _, _, _, err = newAliceChannel.ReceiveRevocation(bobRevocation) + require.NoError(t, err) + + // Now Bob sends an HTLC to Alice. + htlc2, _ := createHTLC(0, lnwire.MilliSatoshi(500000)) + + // <----add------ + _, err = bobChannel.AddHTLC(htlc2, nil) + require.NoError(t, err) + _, err = newAliceChannel.ReceiveHTLC(htlc2) + require.NoError(t, err) + + // Bob sends the final signature to Alice and Alice should not + // reject it, given that we properly restore the unsigned acked + // updates and therefore our update log is structured correctly. + bobSig, bobHtlcSigs, _, err = bobChannel.SignNextCommitment() + require.NoError(t, err) + err = newAliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs) + require.NoError(t, err) +}