diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 1974e6e04..a3e4e537e 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -714,8 +714,8 @@ func (c *commitment) toDiskCommit(ourCommit bool) *channeldb.ChannelCommitment { // restore commitment state written do disk back into memory once we need to // restart a channel session. func (lc *LightningChannel) diskHtlcToPayDesc(feeRate SatPerKWeight, - commitHeight uint64, isPendingCommit bool, htlc *channeldb.HTLC, - localCommitKeys, remoteCommitKeys *CommitmentKeyRing) (PaymentDescriptor, error) { + commitHeight uint64, htlc *channeldb.HTLC, localCommitKeys, + remoteCommitKeys *CommitmentKeyRing) (PaymentDescriptor, error) { // The proper pkScripts for this PaymentDescriptor must be // generated so we can easily locate them within the commitment @@ -770,18 +770,6 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate SatPerKWeight, theirWitnessScript: theirWitnessScript, } - // If this is a pending commit, then the HTLC was only included in the - // commitment of the remote party, so we only set that commit height. - // Otherwise, we'll set the commit height for both chains as the HTLC - // was written to disk after it was fully locked in. - if isPendingCommit { - pd.addCommitHeightRemote = commitHeight - } else { - pd.addCommitHeightRemote = commitHeight - pd.addCommitHeightLocal = commitHeight - - } - return pd, nil } @@ -790,8 +778,7 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate SatPerKWeight, // these payment descriptors can be re-inserted into the in-memory updateLog // for each side. func (lc *LightningChannel) extractPayDescs(commitHeight uint64, - isPendingCommit bool, feeRate SatPerKWeight, - htlcs []channeldb.HTLC, localCommitKeys *CommitmentKeyRing, + feeRate SatPerKWeight, htlcs []channeldb.HTLC, localCommitKeys, remoteCommitKeys *CommitmentKeyRing) ([]PaymentDescriptor, []PaymentDescriptor, error) { var ( @@ -808,7 +795,7 @@ func (lc *LightningChannel) extractPayDescs(commitHeight uint64, // inadvertently trigger replays payDesc, err := lc.diskHtlcToPayDesc( - feeRate, commitHeight, isPendingCommit, &htlc, + feeRate, commitHeight, &htlc, localCommitKeys, remoteCommitKeys, ) if err != nil { @@ -828,9 +815,9 @@ func (lc *LightningChannel) extractPayDescs(commitHeight uint64, // diskCommitToMemCommit converts the on-disk commitment format to our // in-memory commitment format which is needed in order to properly resume // channel operations after a restart. -func (lc *LightningChannel) diskCommitToMemCommit(isLocal, isPendingCommit bool, - diskCommit *channeldb.ChannelCommitment, - localCommitPoint, remoteCommitPoint *btcec.PublicKey) (*commitment, error) { +func (lc *LightningChannel) diskCommitToMemCommit(isLocal bool, + diskCommit *channeldb.ChannelCommitment, localCommitPoint, + remoteCommitPoint *btcec.PublicKey) (*commitment, error) { // First, we'll need to re-derive the commitment key ring for each // party used within this particular state. If this is a pending commit @@ -851,9 +838,8 @@ func (lc *LightningChannel) diskCommitToMemCommit(isLocal, isPendingCommit bool, // HTLC"s into PaymentDescriptor's so we can re-insert them into our // update log. incomingHtlcs, outgoingHtlcs, err := lc.extractPayDescs( - diskCommit.CommitHeight, isPendingCommit, - SatPerKWeight(diskCommit.FeePerKw), diskCommit.Htlcs, - localCommitKeys, remoteCommitKeys, + diskCommit.CommitHeight, SatPerKWeight(diskCommit.FeePerKw), + diskCommit.Htlcs, localCommitKeys, remoteCommitKeys, ) if err != nil { return nil, err @@ -1620,7 +1606,7 @@ func (lc *LightningChannel) restoreCommitState( // commitment into our in-memory commitment format, inserting it into // the local commitment chain. localCommit, err := lc.diskCommitToMemCommit( - true, false, localCommitState, localCommitPoint, + true, localCommitState, localCommitPoint, remoteCommitPoint, ) if err != nil { @@ -1636,7 +1622,7 @@ func (lc *LightningChannel) restoreCommitState( // We'll also do the same for the remote commitment chain. remoteCommit, err := lc.diskCommitToMemCommit( - false, false, remoteCommitState, localCommitPoint, + false, remoteCommitState, localCommitPoint, remoteCommitPoint, ) if err != nil { @@ -1671,7 +1657,7 @@ func (lc *LightningChannel) restoreCommitState( // corresponding state for the local commitment chain. pendingCommitPoint := lc.channelState.RemoteNextRevocation pendingRemoteCommit, err = lc.diskCommitToMemCommit( - false, true, &pendingRemoteCommitDiff.Commitment, + false, &pendingRemoteCommitDiff.Commitment, nil, pendingCommitPoint, ) if err != nil { @@ -1716,12 +1702,52 @@ func (lc *LightningChannel) restoreStateLogs( pendingRemoteCommitDiff *channeldb.CommitDiff, pendingRemoteKeys *CommitmentKeyRing) error { + // We make a map of incoming HTLCs to the height of the remote + // commitment they were first added, and outgoing HTLCs to the height + // of the local commit they were first added. This will be used when we + // restore the update logs below. + incomingRemoteAddHeights := make(map[uint64]uint64) + outgoingLocalAddHeights := make(map[uint64]uint64) + + // We start by setting the height of the incoming HTLCs on the pending + // remote commitment. We set these heights first since if there are + // duplicates, these will be overwritten by the lower height of the + // remoteCommitment below. + if pendingRemoteCommit != nil { + for _, r := range pendingRemoteCommit.incomingHTLCs { + incomingRemoteAddHeights[r.HtlcIndex] = + pendingRemoteCommit.height + } + } + + // Now set the remote commit height of all incoming HTLCs found on the + // remote commitment. + for _, r := range remoteCommitment.incomingHTLCs { + incomingRemoteAddHeights[r.HtlcIndex] = remoteCommitment.height + } + + // And finally we can do the same for the outgoing HTLCs. + for _, l := range localCommitment.outgoingHTLCs { + outgoingLocalAddHeights[l.HtlcIndex] = localCommitment.height + } + // 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] + + // We'll need to set the add height of the HTLC. Since it is on + // this local commit, we can use its height as local add + // height. As remote add height we consult the incoming HTLC + // map we created earlier. Note that if this HTLC is not in + // incomingRemoteAddHeights, the remote add height will be set + // to zero, which indicates that it is not added yet. + htlc.addCommitHeightLocal = localCommitment.height + htlc.addCommitHeightRemote = incomingRemoteAddHeights[htlc.HtlcIndex] + + // Restore the htlc back to the remote log. lc.remoteUpdateLog.restoreHtlc(&htlc) } @@ -1729,6 +1755,14 @@ func (lc *LightningChannel) restoreStateLogs( // remote commitment, adding them to the local update log. for i := range remoteCommitment.outgoingHTLCs { htlc := remoteCommitment.outgoingHTLCs[i] + + // As for the incoming HTLCs, we'll use the current remote + // commit height as remote add height, and consult the map + // created above for the local add height. + htlc.addCommitHeightRemote = remoteCommitment.height + htlc.addCommitHeightLocal = outgoingLocalAddHeights[htlc.HtlcIndex] + + // Restore the htlc back to the local log. lc.localUpdateLog.restoreHtlc(&htlc) } diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 9668ad380..d0beffa60 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -5683,3 +5683,194 @@ func TestDuplicateSettleRejection(t *testing.T) { t.Fatalf("unable to recv htlc cancel: %v", err) } } + +// TestChannelRestoreCommitHeight tests that the local and remote commit +// heights of HTLCs are set correctly across restores. +func TestChannelRestoreCommitHeight(t *testing.T) { + t.Parallel() + + aliceChannel, bobChannel, cleanUp, err := CreateTestChannels() + if err != nil { + t.Fatalf("unable to create test channels: %v", err) + } + defer cleanUp() + + // helper method to check add heights of the htlcs found in the given + // log after a restore. + restoreAndAssertCommitHeights := func(t *testing.T, + channel *LightningChannel, remoteLog bool, htlcIndex uint64, + expLocal, expRemote uint64) *LightningChannel { + + channel.Stop() + newChannel, err := NewLightningChannel( + channel.Signer, nil, channel.channelState, + ) + if err != nil { + t.Fatalf("unable to create new channel: %v", err) + } + + var pd *PaymentDescriptor + if remoteLog { + if newChannel.localUpdateLog.lookupHtlc(htlcIndex) != nil { + t.Fatalf("htlc found in wrong log") + } + pd = newChannel.remoteUpdateLog.lookupHtlc(htlcIndex) + + } else { + if newChannel.remoteUpdateLog.lookupHtlc(htlcIndex) != nil { + t.Fatalf("htlc found in wrong log") + } + pd = newChannel.localUpdateLog.lookupHtlc(htlcIndex) + } + if pd == nil { + t.Fatalf("htlc not found in log") + } + + if pd.addCommitHeightLocal != expLocal { + t.Fatalf("expected local add height to be %d, was %d", + expLocal, pd.addCommitHeightLocal) + } + if pd.addCommitHeightRemote != expRemote { + t.Fatalf("expected remote add height to be %d, was %d", + expRemote, pd.addCommitHeightRemote) + } + return newChannel + } + + // We'll send an HtLC from Alice to Bob. + htlcAmount := lnwire.NewMSatFromSatoshis(100000000) + 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) + } + + // The HTLC should only be on the pending remote commitment, so the + // only the remote add height should be set during a restore. + aliceChannel = restoreAndAssertCommitHeights(t, aliceChannel, false, + 0, 0, 1) + + // 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) + } + + // Now the HTLC is locked into Bob's commitment, a restoration should + // set only the local commit height, as it is not locked into Alice's + // yet. + bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 0, 1, 0) + + // Alice receives the revocation, ACKing her pending commitment. + _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + if err != nil { + t.Fatalf("unable to recive revocation: %v", err) + } + + // However, the HTLC is still not locked into her local commitment, so + // the local add height should still be 0 after a restoration. + aliceChannel = restoreAndAssertCommitHeights(t, aliceChannel, false, + 0, 0, 1) + + // Now let Bob send the commitment signature making the HTLC lock in on + // Alice's commitment. + bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + if err != nil { + t.Fatalf("unable to sign commitment: %v", err) + } + + // At this stage Bob has a pending remote commitment. Make sure + // restoring at this stage correcly restores the HTLC add commit + // heights. + bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 0, 1, 1) + + err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs) + if err != nil { + t.Fatalf("unable to receive commitment: %v", err) + } + aliceRevocation, _, err := aliceChannel.RevokeCurrentCommitment() + if err != nil { + t.Fatalf("unable to revoke commitment: %v", err) + } + + // Now both the local and remote add heights should be properly set. + aliceChannel = restoreAndAssertCommitHeights(t, aliceChannel, false, + 0, 1, 1) + + _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { + t.Fatalf("unable to recive revocation: %v", err) + } + + // Alice ACKing Bob's pending commitment shouldn't change the heights + // restored. + bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 0, 1, 1) + + // Send andother HTLC from Alice to Bob, to test whether already + // existing HTLCs (the HTLC with index 0) keep getting the add heights + // restored properly. + htlcAlice, _ = createHTLC(1, 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) + } + + // Send a new signature from Alice to Bob, making Alice have a pending + // remote commitment. + aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + if err != nil { + t.Fatalf("unable to sign commitment: %v", err) + } + + // A restoration should keep the add heights iof the first HTLC, and + // the new HTLC should have a remote add height 2. + aliceChannel = restoreAndAssertCommitHeights(t, aliceChannel, false, + 0, 1, 1) + aliceChannel = restoreAndAssertCommitHeights(t, aliceChannel, false, + 1, 0, 2) + + 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) + } + + // Since Bob just revoked another commitment, a restoration should + // increase the add height of the firt HTLC to 2, as we only keep the + // last unrevoked commitment. The new HTLC will also have a local add + // height of 2. + bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 0, 2, 1) + bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 1, 2, 0) + + // Sign a new state for Alice, making Bob have a pending remote + // commitment. + bobSig, bobHtlcSigs, err = bobChannel.SignNextCommitment() + if err != nil { + t.Fatalf("unable to sign commitment: %v", err) + } + + // The signing of a new commitment for Alice should have given the new + // HTLC an add height. + bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 0, 2, 1) + bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 1, 2, 2) + + aliceChannel.Stop() + bobChannel.Stop() +}