From 1d172428c491070a33d04246c98c3ed64d2e55ba Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 4 May 2018 13:18:31 +0200 Subject: [PATCH 1/6] lnwallet/channel: return missing errors This commit corrects a few cases where we would encounter errors, but return nil by mistake. --- lnwallet/channel.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 2cf85c1e6..049132a6f 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -138,8 +138,7 @@ const ( // from the log. Adding a MalformedFail entry to ones log will modify // the _remote_ parties update log once a new commitment view has been // evaluated which contains the MalformedFail entry. The difference - // from Fail type lie in - // the different data we have to store. + // from Fail type lie in the different data we have to store. MalformedFail // Settle is an update type which settles a prior HTLC crediting the @@ -623,13 +622,13 @@ func (c *commitment) populateHtlcIndexes() error { for i := 0; i < len(c.outgoingHTLCs); i++ { htlc := &c.outgoingHTLCs[i] if err := populateIndex(htlc, false); err != nil { - return nil + return err } } for i := 0; i < len(c.incomingHTLCs); i++ { htlc := &c.incomingHTLCs[i] if err := populateIndex(htlc, true); err != nil { - return nil + return err } } @@ -1640,7 +1639,7 @@ func (lc *LightningChannel) restoreCommitState( // extended to the remote party but which was never ACK'd. pendingRemoteCommitDiff, err = lc.channelState.RemoteCommitChainTip() if err != nil && err != channeldb.ErrNoPendingCommit { - return nil + return err } if pendingRemoteCommitDiff != nil { From 1d676b77e1ab01b13a1919c19009544acf60de83 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 4 May 2018 13:20:11 +0200 Subject: [PATCH 2/6] lnwallet/channel: log pending remote commit during restore --- lnwallet/channel.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 049132a6f..a50a29560 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1658,6 +1658,13 @@ func (lc *LightningChannel) restoreCommitState( } lc.remoteCommitChain.addCommitment(pendingRemoteCommit) + walletLog.Debugf("ChannelPoint(%v), pending remote "+ + "commitment: %v", lc.channelState.FundingOutpoint, + newLogClosure(func() string { + return spew.Sdump(lc.remoteCommitChain.tip()) + }), + ) + // We'll also re-create the set of commitment keys needed to // fully re-derive the state. pendingRemoteKeyChain = deriveCommitmentKeys( From aa1c2cdf81f066120d8c60a284dc2d3c146edd42 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 4 May 2018 13:25:10 +0200 Subject: [PATCH 3/6] lnwallet/channel: add more info in case of crash This commit adds a call to panic in case the HTLC we are looking for is not found in the update log. It _should_ always be there, but we have seen crashes resulting from it not being found. Since it will crash with a nil pointer dereference below, we instead call panic giving us a bit more information to work with. --- lnwallet/channel.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index a50a29560..5bb8add6b 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -2407,6 +2407,21 @@ func (lc *LightningChannel) evaluateHTLCView(view *htlcView, ourBalance, addEntry := lc.remoteUpdateLog.lookupHtlc(entry.ParentIndex) + // We check if the parent entry is not found at this point. We + // have seen this happening a few times and panic with some + // addtitional info to figure out why. + // TODO(halseth): remove when bug is fixed. + if addEntry == nil { + panic(fmt.Sprintf("unable to find parent entry %d "+ + "in remote update log: %v\nUpdatelog: %v", + entry.ParentIndex, newLogClosure(func() string { + return spew.Sdump(entry) + }), newLogClosure(func() string { + return spew.Sdump(lc.remoteUpdateLog) + }), + )) + } + skipThem[addEntry.HtlcIndex] = struct{}{} processRemoveEntry(entry, ourBalance, theirBalance, nextHeight, remoteChain, true, mutateState) @@ -2427,6 +2442,21 @@ func (lc *LightningChannel) evaluateHTLCView(view *htlcView, ourBalance, addEntry := lc.localUpdateLog.lookupHtlc(entry.ParentIndex) + // We check if the parent entry is not found at this point. We + // have seen this happening a few times and panic with some + // addtitional info to figure out why. + // TODO(halseth): remove when bug is fixed. + if addEntry == nil { + panic(fmt.Sprintf("unable to find parent entry %d "+ + "in local update log: %v\nUpdatelog: %v", + entry.ParentIndex, newLogClosure(func() string { + return spew.Sdump(entry) + }), newLogClosure(func() string { + return spew.Sdump(lc.localUpdateLog) + }), + )) + } + skipUs[addEntry.HtlcIndex] = struct{}{} processRemoveEntry(entry, ourBalance, theirBalance, nextHeight, remoteChain, false, mutateState) From 7e1f2a7dc3d16a31893818ffbd8367c3eb617425 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 4 May 2018 13:42:15 +0200 Subject: [PATCH 4/6] lnwallet/channel: remove unused arguments --- lnwallet/channel.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 5bb8add6b..6ab16c9ac 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1368,9 +1368,7 @@ func NewLightningChannel(signer Signer, pCache PreimageCache, // With the main channel struct reconstructed, we'll now restore the // commitment state in memory and also the update logs themselves. - err := lc.restoreCommitState( - &localCommit, &remoteCommit, localUpdateLog, remoteUpdateLog, - ) + err := lc.restoreCommitState(&localCommit, &remoteCommit) if err != nil { return nil, err } @@ -1579,8 +1577,7 @@ func (lc *LightningChannel) logUpdateToPayDesc(logUpdate *channeldb.LogUpdate, // to re-sync states with the remote party, and also verify/extend new proposed // commitment states. func (lc *LightningChannel) restoreCommitState( - localCommitState, remoteCommitState *channeldb.ChannelCommitment, - localUpdateLog, remoteUpdateLog *updateLog) error { + localCommitState, remoteCommitState *channeldb.ChannelCommitment) error { // In order to reconstruct the pkScripts on each of the pending HTLC // outputs (if any) we'll need to regenerate the current revocation for From 9324bf7b1d1ccecb3e4c873f61580148722620aa Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 4 May 2018 13:42:57 +0200 Subject: [PATCH 5/6] lnwallet/channel: return error in case of htlc index mismatch This commit make us return an error in case a restored HTLC from a pending remote commit has an index that is different from our local update log index. It is appended with the assumption that these indexes are the same, and if they are not we cannot really continue. --- lnwallet/channel.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 6ab16c9ac..f249e5555 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1765,6 +1765,16 @@ func (lc *LightningChannel) restoreStateLogs( } if payDesc.EntryType == Add { + // The HtlcIndex of the added HTLC _must_ be equal to + // the log's htlcCounter at this point. If it is not we + // panic to catch this. + // TODO(halseth): remove when cause of htlc entry bug + // is found. + if payDesc.HtlcIndex != lc.localUpdateLog.htlcCounter { + panic(fmt.Sprintf("htlc index mismatch: "+ + "%v vs %v", payDesc.HtlcIndex, + lc.localUpdateLog.htlcCounter)) + } lc.localUpdateLog.appendHtlc(payDesc) } else { lc.localUpdateLog.appendUpdate(payDesc) From f26ec38a74ccb890781288518935c77552c43b62 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Fri, 4 May 2018 15:25:05 +0200 Subject: [PATCH 6/6] channeldb: return ErrNoPendingCommit in case of no channel bucket --- channeldb/channel.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/channeldb/channel.go b/channeldb/channel.go index 12e942be7..ee308e71c 100644 --- a/channeldb/channel.go +++ b/channeldb/channel.go @@ -1223,7 +1223,9 @@ func (c *OpenChannel) RemoteCommitChainTip() (*CommitDiff, error) { err := c.Db.View(func(tx *bolt.Tx) error { chanBucket, err := readChanBucket(tx, c.IdentityPub, &c.FundingOutpoint, c.ChainHash) - if err != nil { + if err == ErrNoActiveChannels || err == ErrNoChanDBExists { + return ErrNoPendingCommit + } else if err != nil { return err }