htlcswitch+lnwallet+channeldb: invalid sig fix #3872

joostjager · 2019-12-23T10:33:39Z

This PR persistently stores updates received from the remote party until we've produced a signature that includes these updates. Previously those updates were only stored in memory and dropped across restarts. This lead to the production of an invalid signature and channel force closure.

Fixes #3822
Fixes #3126

When restoring an htlc fulfill update from disk, the payment hash wasn't restored previously.

…te updates

halseth

Great PR! I definitely think this is a nice way of solving this :)

My main concern is that we now restore the update logs from several sources: the local/remote commit, the pending remote commit and the dangling remote updates. Ideally we would reduce this, but that seems hard while still being backwards compatible, so this seems like the way to do it. We should just be careful that updates won't be restored from more that one of the sources, as that could lead to some subtle bugs.

lnwallet/channel.go

lnwallet/channel_test.go

channeldb/channel.go

halseth · 2020-01-08T08:40:41Z

lnwallet/channel.go

+	localCommitmentHeight uint64) error {
+
+	if len(danglingRemoteUpdates) == 0 {
+		lc.log.Debugf("No dangling remote updates to restore")


Debug felt appropriate to me, given that this functionality is newly introduced. The log volume is still acceptable for users out there to collect. Do you think this is too spammy? We are already quite verbose on debug and this seemed to fit into that.

Alternatively, we could conditionally call this method instead (lift the if statement out to the caller).

Removed special casing completely. The for loop below is skipped and the sanity checks may be useful also in case there are no updates.

This change uncovered that the sanity checks actually had false positives. A fee update may have already disappeared from the log, causing the last present update in the log not to equal the last committed update. Relaxed the check and brought it into the switch statement below.

lnwallet/channel.go

halseth · 2020-01-08T09:20:56Z

lnwallet/channel.go

+
+		switch payDesc.EntryType {
+		case Add:
+			lc.remoteUpdateLog.restoreHtlc(payDesc)


some adds will be restored twice now, both when checking incoming HTLCs on our local commit, and here. But maybe that's okay?

hmm... maybe it isn't necessary then to save and restore add msgs?

Thought about this a bit more. The operation is idempotent, so it doesn't seem to hurt. But maybe we do need the save/restore for add msgs in case an htlc is added and later settled on the local commit. If we aren't able to sign during that time, we still need to process those messages later.

Although this is probably not even possible. We can only settle if we've also added the add to our remote commit tx. But it seems safer to do it like it is now.

channeldb/channel.go

joostjager · 2020-01-08T14:58:13Z

My main concern is that we now restore the update logs from several sources: the local/remote commit, the pending remote commit and the dangling remote updates.

Yes, fully agree to this. Maybe it would have been better to persist the complete local and remote update logs, to not have to think so hard about reconstruction.

Roasbeef · 2020-01-10T22:53:42Z

Maybe it would have been better to persist the complete local and remote update logs, to not have to think so hard about reconstruction.

Any updates that aren't committed are to be forgotten upon reconnection. If we persisted all updates, we'd have issues w.r.t preemptively attempting to evaluate a commitment view using abandoned updates.

Roasbeef · 2020-01-10T22:55:25Z

party until we've produced a signature that includes these updates

Haven't dug into the PR yet, but this seems incorrect. Instead, we should only store the updates when we receive a signature from the remote party.

Roasbeef

Excited to finally, finally (for real this time) squash this annoying bug. Completed an initial pass through the PR and may have found a bug that would cause us to store more than necessary (updates of theirs sent after their signature ).

lnwallet/channel.go

lnwallet/channel_test.go

channeldb/channel.go

lnwallet/channel.go

Roasbeef · 2020-01-10T23:39:33Z

lnwallet/channel.go

+	localCommitmentHeight uint64) error {
+
+	if len(danglingRemoteUpdates) == 0 {
+		lc.log.Debugf("No dangling remote updates to restore")


Alternatively, we could conditionally call this method instead (lift the if statement out to the caller).

lnwallet/channel.go

halseth

I think this one is getting close!

I really like the way the commits are structured, with a simple buildup of commits before the last, important commit, where the real meat of the change is found.

lnwallet/channel_test.go

channeldb/channel.go

lnwallet/channel.go

halseth · 2020-01-15T08:33:24Z

lnwallet/channel.go

+
+	// Fetch the last remote updates that have been committed on both sides.
+	lastRemoteCommitted := lc.remoteCommitChain.tip().theirMessageIndex
+	lastLocalCommitted := lc.localCommitChain.tip().theirMessageIndex


Technically this should be tail since that is the oldest unrevoked commitment == the last index we acked. In this case I don't think it matters since we will always revoke after receiving commitment, but I think it'll be more correct.

This test asserts that remote updates that are locked-in on the local commitment, but haven't been signed for on the remote commitment, are properly restored after a restart.

This method is only used to update the local commitment transaction. Updated comment accordingly.

Extract functionality to methods as a preparation for serializing remote log updates.

Extract method in preparation for restoring dangling remote updates. We need to get rid of the early return.

This commit updates the channel state machine to persistently store remote updates that we have received a signature for, but that we haven't yet included in a commit signature of our own. Previously those updates were only stored in memory and dropped across restarts. This lead to the production of an invalid signature and channel force closure. The remote party expects us to include those updates.

halseth

LGTM 💯

halseth · 2020-01-16T08:59:18Z

lnwallet/channel.go

+
+			lc.localUpdateLog.markHtlcModified(payDesc.ParentIndex)
+		}
+	}


suggestion: also add sanity check that the last update index matches the log index after all pending updates are restored? (doesn't have to be in this method, just after both state logs are fully restored)

Yes, so I discovered that this does not always need to be the case. #3872 (comment)

halseth · 2020-01-16T09:03:39Z

lnwallet/channel.go

@@ -3092,6 +3277,96 @@ func (lc *LightningChannel) createCommitDiff(
 	}, nil
 }

+// getUnsignedAckedUpdates returns all remote log updates that we haven't


nit: "that we have acked, but haven't..."

Roasbeef · 2020-01-17T01:47:41Z

Running this patch on testnet now.

Roasbeef

LGTM 💴

Roasbeef · 2020-01-17T02:07:46Z

lnwallet/channel.go

+	chanID := lnwire.NewChanIDFromOutPoint(&lc.channelState.FundingOutpoint)
+
+	// Fetch the last remote update that we have signed for.
+	lastRemoteCommitted := lc.remoteCommitChain.tip().theirMessageIndex


👍 w.r.t the slight reshuffling of logic here, I find it much easier to follow now.

joostjager force-pushed the invalid-sig-fix branch 6 times, most recently from a422d10 to 88163f4 Compare December 23, 2019 13:03

joostjager force-pushed the invalid-sig-fix branch 14 times, most recently from fb66fd1 to 012a488 Compare January 6, 2020 13:55

lnwallet: restore missing field in local log update

b3bd470

When restoring an htlc fulfill update from disk, the payment hash wasn't restored previously.

joostjager force-pushed the invalid-sig-fix branch 2 times, most recently from c5d8cbb to 0e8a372 Compare January 6, 2020 14:15

lnwallet/test: describe existing bug after restart with dangling remo…

41c207e

…te updates

joostjager force-pushed the invalid-sig-fix branch from 0e8a372 to 5211a31 Compare January 6, 2020 14:30

joostjager requested review from halseth and Roasbeef January 6, 2020 14:31

joostjager self-assigned this Jan 6, 2020

joostjager added channels commitments Commitment transactions containing the state of the channel labels Jan 6, 2020

joostjager added the database Related to the database/storage of LND label Jan 6, 2020

joostjager marked this pull request as ready for review January 6, 2020 14:34

Roasbeef added this to the 0.9.1 milestone Jan 7, 2020

halseth suggested changes Jan 8, 2020

View reviewed changes

joostjager force-pushed the invalid-sig-fix branch from 5211a31 to c643ee5 Compare January 8, 2020 11:32

joostjager requested a review from halseth January 8, 2020 11:34

Roasbeef requested changes Jan 11, 2020

View reviewed changes

joostjager force-pushed the invalid-sig-fix branch 2 times, most recently from b7e45b9 to fce0658 Compare January 13, 2020 10:50

joostjager requested a review from Roasbeef January 13, 2020 10:54

Roasbeef mentioned this pull request Jan 14, 2020

reverse proxy causes invalid commit sig w/ CL #3586

Closed

halseth suggested changes Jan 15, 2020

View reviewed changes

joostjager added 5 commits January 15, 2020 12:37

lnwallet/test: test remote update after restart

6a86689

This test asserts that remote updates that are locked-in on the local commitment, but haven't been signed for on the remote commitment, are properly restored after a restart.

channeldb: update UpdateCommitment description

ea892fc

This method is only used to update the local commitment transaction. Updated comment accordingly.

channeldb: extract log update serialization

2d37d34

Extract functionality to methods as a preparation for serializing remote log updates.

lnwallet: extract pending local updates restore method

ed8fa35

Extract method in preparation for restoring dangling remote updates. We need to get rid of the early return.

joostjager force-pushed the invalid-sig-fix branch from fce0658 to 8257940 Compare January 15, 2020 12:09

joostjager requested a review from halseth January 15, 2020 17:17

halseth approved these changes Jan 16, 2020

View reviewed changes

Roasbeef approved these changes Jan 17, 2020

View reviewed changes

joostjager merged commit 1413995 into lightningnetwork:master Jan 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

htlcswitch+lnwallet+channeldb: invalid sig fix #3872

htlcswitch+lnwallet+channeldb: invalid sig fix #3872

joostjager commented Dec 23, 2019 •

edited by Roasbeef

Loading

halseth left a comment

halseth Jan 8, 2020

joostjager Jan 8, 2020

Roasbeef Jan 10, 2020

joostjager Jan 13, 2020

joostjager Jan 13, 2020

halseth Jan 8, 2020

joostjager Jan 8, 2020

joostjager Jan 8, 2020 •

edited

Loading

joostjager commented Jan 8, 2020

Roasbeef commented Jan 10, 2020

Roasbeef commented Jan 10, 2020

Roasbeef left a comment

Roasbeef Jan 10, 2020

halseth left a comment

halseth Jan 15, 2020

joostjager Jan 15, 2020

halseth left a comment

halseth Jan 16, 2020

joostjager Jan 16, 2020

halseth Jan 16, 2020

Roasbeef commented Jan 17, 2020

Roasbeef left a comment

Roasbeef Jan 17, 2020

htlcswitch+lnwallet+channeldb: invalid sig fix #3872

htlcswitch+lnwallet+channeldb: invalid sig fix #3872

Conversation

joostjager commented Dec 23, 2019 • edited by Roasbeef Loading

halseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

joostjager commented Jan 8, 2020

Roasbeef commented Jan 10, 2020

Roasbeef commented Jan 10, 2020

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Jan 17, 2020

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager commented Dec 23, 2019 •

edited by Roasbeef

Loading

joostjager Jan 8, 2020 •

edited

Loading