diff --git a/lnwallet/channel.go b/lnwallet/channel.go index e52efc76..074a02fe 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -57,6 +57,18 @@ var ( // message, the state machine deems that is unable to properly // synchronize states with the remote peer. ErrCannotSyncCommitChains = fmt.Errorf("unable to sync commit chains") + + // ErrInvalidLastCommitSecret is returned in the case that the + // commitment secret sent by the remote party in their + // ChannelReestablish message doesn't match the last secret we sent. + ErrInvalidLastCommitSecret = fmt.Errorf("commit secret is incorrect") + + // ErrCommitSyncDataLoss is returned in the case that we receive a + // valid commit secret within the ChannelReestablish message from the + // remote node AND they advertise a RemoteCommitTailHeight higher than + // our current known height. + ErrCommitSyncDataLoss = fmt.Errorf("possible commitment state data " + + "loss") ) // channelState is an enum like type which represents the current state of a @@ -2022,6 +2034,8 @@ func (lc *LightningChannel) closeObserver(channelCloseNtfn *chainntnfs.SpendEven err) } + // TODO(roasbeef): need to handle case of if > + // First, we'll generate the commitment point and the // revocation point so we can re-construct the HTLC state and // also our payment key. @@ -3054,10 +3068,41 @@ func (lc *LightningChannel) ProcessChanSyncMsg(msg *lnwire.ChannelReestablish) ( // resync. var updates []lnwire.Message + // If the remote party included the optional fields, then we'll verify + // their correctness first, as it will influence our decisions below. + hasRecoveryOptions := msg.LocalUnrevokedCommitPoint != nil + commitSecretCorrect := true + if hasRecoveryOptions && msg.RemoteCommitTailHeight != 0 { + // We'll check that they've really sent a valid commit + // secret from our shachain for our prior height, but only if + // this isn't the first state. + heightSecret, err := lc.channelState.RevocationProducer.AtIndex( + msg.RemoteCommitTailHeight - 1, + ) + if err != nil { + return nil, err + } + commitSecretCorrect = bytes.Equal( + heightSecret[:], msg.LastRemoteCommitSecret[:], + ) + } + + // TODO(roasbeef): check validity of commitment point after the fact + + // If the commit secret they sent is incorrect then we'll fail the + // channel as the remote node has an inconsistent state. + if !commitSecretCorrect { + // In this case, we'll return an error to indicate the remote + // node sent us the wrong values. This will let the caller act + // accordingly. + return nil, ErrInvalidLastCommitSecret + } + + switch { // If we owe the remote party a revocation message, then we'll re-send // the last revocation message that we sent. This will be the // revocation message for our prior chain tail. - if oweRevocation { + case oweRevocation: revocationMsg, err := lc.generateRevocation( localChainTail.height - 1, ) @@ -3087,10 +3132,21 @@ func (lc *LightningChannel) ProcessChanSyncMsg(msg *lnwire.ChannelReestablish) ( }) } - } else if !oweRevocation && localChainTail.height != msg.RemoteCommitTailHeight { - // If we don't owe them a revocation, and the height of our - // commitment chain reported by the remote party is not equal - // to our chain tail, then we cannot sync. + // If we don't owe the remote party a revocation, but their value for + // what our remote chain tail should be doesn't match up, and their + // purported commitment secrete matches up, then we'll behind! + case (msg.RemoteCommitTailHeight > localChainTail.height && + hasRecoveryOptions && commitSecretCorrect): + + // In this case, we've likely lost data and shouldn't proceed + // with channel updates. So we'll return the appropriate error + // to signal to the caller the current state. + return nil, ErrCommitSyncDataLoss + + // If we don't owe them a revocation, and the height of our commitment + // chain reported by the remote party is not equal to our chain tail, + // then we cannot sync. + case !oweRevocation && localChainTail.height != msg.RemoteCommitTailHeight: return nil, ErrCannotSyncCommitChains } diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index dd4c7b32..4b517a00 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -3407,6 +3407,97 @@ func TestChanSyncUnableToSync(t *testing.T) { } } +// TestChanSyncInvalidLastSecret ensures that if Alice and Bob have completed +// state transitions in an existing channel, and then send a ChannelReestablish +// message after a restart, the following holds: if Alice has lost data, so she +// sends an invalid commit secret then both parties recognize this as possible +// data loss. +func TestChanSyncInvalidLastSecret(t *testing.T) { + t.Parallel() + + // Create a test channel which will be used for the duration of this + // unittest. The channel will be funded evenly with Alice having 5 BTC, + // and Bob having 5 BTC. + aliceChannel, bobChannel, cleanUp, err := createTestChannels(1) + if err != nil { + t.Fatalf("unable to create test channels: %v", err) + } + defer cleanUp() + + // We'll create a new instances of Alice before doing any state updates + // such that we have the initial in memory state at the start of the + // channel. + aliceOld, err := restartChannel(aliceChannel) + if err != nil { + t.Fatalf("unable to restart alice") + } + + // First, we'll add an HTLC, and then initiate a state transition + // between the two parties such that we actually have a prior + // revocation to send. + var paymentPreimage [32]byte + copy(paymentPreimage[:], bytes.Repeat([]byte{1}, 32)) + paymentHash := sha256.Sum256(paymentPreimage[:]) + htlcAmt := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin) + htlc := &lnwire.UpdateAddHTLC{ + PaymentHash: paymentHash, + Amount: htlcAmt, + Expiry: uint32(5), + } + if _, err := aliceChannel.AddHTLC(htlc); err != nil { + t.Fatalf("unable to add htlc: %v", err) + } + if _, err := bobChannel.ReceiveHTLC(htlc); err != nil { + t.Fatalf("unable to recv htlc: %v", err) + } + + // Then we'll initiate a state transition to lock in this new HTLC. + if err := forceStateTransition(aliceChannel, bobChannel); err != nil { + t.Fatalf("unable to complete alice's state transition: %v", err) + } + + // Next, we'll restart both parties in order to simulate a connection + // re-establishment. + aliceChannel, err = restartChannel(aliceChannel) + if err != nil { + t.Fatalf("unable to restart alice: %v", err) + } + bobChannel, err = restartChannel(bobChannel) + if err != nil { + t.Fatalf("unable to restart bob: %v", err) + } + + // Next, we'll produce the ChanSync messages for both parties. + aliceChanSync, err := aliceChannel.ChanSyncMsg() + if err != nil { + t.Fatalf("unable to generate chan sync msg: %v", err) + } + bobChanSync, err := bobChannel.ChanSyncMsg() + if err != nil { + t.Fatalf("unable to generate chan sync msg: %v", err) + } + + // We'll modify Alice's sync message to have an invalid commitment + // secret. + aliceChanSync.LastRemoteCommitSecret[4] ^= 0x01 + + // Alice's former self should conclude that she possibly lost data as + // Bob is sending a valid commit secret for the latest state. + _, err = aliceOld.ProcessChanSyncMsg(bobChanSync) + if err != ErrCommitSyncDataLoss { + t.Fatalf("wrong error, expected ErrCommitSyncDataLoss "+ + "instead got: %v", err) + } + + // Bob should conclude that he should force close the channel, as Alice + // cannot continue operation. + _, err = bobChannel.ProcessChanSyncMsg(aliceChanSync) + if err != ErrInvalidLastCommitSecret { + t.Fatalf("wrong error, expected ErrInvalidLastCommitSecret, "+ + "instead got: %v", err) + } +} + // TestChanAvailableBandwidth tests the accuracy of the AvailableBalance() // method. The value returned from this message should reflect the value // returned within the commitment state of a channel after the transition is