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.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) } } diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 50931ef8..18b2d760 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 @@ -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) +}