From cb521511233e72bcae4624472e25244461dea5cf Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Mon, 28 May 2018 22:09:24 +0200 Subject: [PATCH 1/2] lnwallet/channel: set add heights based on the locked in commits This commit fixes a bug which would cause the add heights of the HTLCs in the update log to be set wrongly. At times, an add height could be incorrecly set, leading to the HTLCs not being accounted for correctly during evaluating the HTLC views. This was caused by the assumption that if the HTLC was not on the pending remote commit, then it was locked in on both the local and the remote commit, which is not always true. Instead of making this assumption, we instead now inspect the three commits: the local, remote and pending remote; and set the add heights accordingly. This should ensure that HTLCs are subtracted from the balances only when they are first added. --- lnwallet/channel.go | 86 +++++++++++++++++++++++++++++++-------------- 1 file changed, 60 insertions(+), 26 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index d5b86e30..38e2b8d2 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) } From d4700640e00dc8c174690635763b7368a9c4c042 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Mon, 28 May 2018 22:22:26 +0200 Subject: [PATCH 2/2] lnwallet/channel test: add TestChannelRestoreCommitHeight This commit adds a test which will restore a channel from an OpenChannel struct at various stages of the state transation cycle, ensuring the HTLC local and remote add heights are restored properly. --- lnwallet/channel_test.go | 191 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 191 insertions(+) diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 29fe4c5e..08ceafde 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -5505,3 +5505,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() +}