lnwallet/channeldb: restore fee updates after restart

Fix a bug where UpdateFee entries persisted under
remoteUnsignedLocalUpdatesKey were silently skipped during
restoreStateLogs, leaving the update log empty after a restart.

This caused a retransmitted commit_sig (covering the new fee rate)
to fail verification after channel reestablishment, incorrectly
triggering a force close.

Also refactor AdvanceCommitChainTail to avoid an early return when
unsignedAckedUpdates is nil, so that the full validation path always
runs.

Adds TestChanSyncUpdateFeeRestartAfterRevoke (lnwallet) and
TestChannelLinkFailDuringRestart (htlcswitch) to reproduce the
scenario at both the channel and link layers.

Author: ziggie1984

channeldb/channel.go

@@ -3423,18 +3423,14 @@ func (c *OpenChannel) AdvanceCommitChainTail(fwdPkg *FwdPkg,
 		// Persist the unsigned acked updates that are not included
 		// in their new commitment.
 		updateBytes := chanBucket.Get(unsignedAckedUpdatesKey)
-		if updateBytes == nil {
-			// This shouldn't normally happen as we always store
-			// the number of updates, but could still be
-			// encountered by nodes that are upgrading.
-			newRemoteCommit = &newCommit.Commitment
-			return nil
-		}
 
-		r := bytes.NewReader(updateBytes)
-		unsignedUpdates, err := deserializeLogUpdates(r)
-		if err != nil {
-			return err
+		var unsignedUpdates []LogUpdate
+		if updateBytes != nil {
+			r := bytes.NewReader(updateBytes)
+			unsignedUpdates, err = deserializeLogUpdates(r)
+			if err != nil {
+				return err
+			}
 		}
 
 		var validUpdates []LogUpdate

htlcswitch/link_test.go

@@ -445,6 +445,122 @@ func TestChannelLinkSigThenRev(t *testing.T) {
 	ctx.receiveRevAndAckAliceToBob()
 }
 
+// TestChannelLinkFailDuringRestart tests that the link fails (force-closing
+// the channel) if the commit dance is not completed after sharing the
+// update_fee and one of the peers restarts before completion.
+//
+// Specifically, this tests the following scenario:
+//
+//	Alice                    Bob
+//	----update_fee----->
+//	-----commit_sig---->
+//	<--revoke_and_ack----
+//	==== restart node ====
+//	<-----commit_sig----
+//	==== force close ====
+func TestChannelLinkFailDuringRestart(t *testing.T) {
+	const chanAmt = btcutil.SatoshiPerBitcoin * 5
+
+	harness, err := newSingleLinkTestHarness(t, chanAmt, 0)
+	require.NoError(t, err)
+
+	err = harness.start()
+	require.NoError(t, err)
+	defer harness.aliceLink.Stop()
+
+	alice := newPersistentLinkHarness(
+		t, harness.aliceSwitch, harness.aliceLink,
+		harness.aliceBatchTicker, harness.aliceRestore,
+	)
+	alice.coreLink.markReestablished()
+
+	// ----update_fee----->
+	// -----commit_sig---->
+	err = alice.coreLink.updateChannelFee(t.Context(), 85727)
+	require.NoError(t, err)
+
+	// Bob receive update_fee.
+	select {
+	case msg := <-alice.msgs:
+		updateFee := msg.(*lnwire.UpdateFee)
+		harness.bobChannel.ReceiveUpdateFee(
+			chainfee.SatPerKWeight(updateFee.FeePerKw),
+		)
+	case <-time.After(5 * time.Second):
+		t.Fatalf("did not receive update_fee message")
+	}
+
+	// Bob receive commit_sig, respond with revoke_and_ack.
+	select {
+	case msg := <-alice.msgs:
+		commitSig := msg.(*lnwire.CommitSig)
+
+		err = harness.bobChannel.ReceiveNewCommitment(&lnwallet.CommitSigs{
+			CommitSig:  commitSig.CommitSig,
+			HtlcSigs:   commitSig.HtlcSigs,
+			PartialSig: commitSig.PartialSig,
+		})
+		require.NoError(t, err)
+
+		// <--revoke_and_ack----
+		nextRevocation, _, _, err := harness.bobChannel.RevokeCurrentCommitment()
+		require.NoError(t, err)
+		harness.aliceLink.HandleChannelUpdate(nextRevocation)
+
+		// Bob will generate a commit sig, but this message will not be
+		// received by Alice, and before that, one of the sides restarts
+		harness.bobChannel.SignNextCommitment(t.Context())
+
+	case <-time.After(5 * time.Second):
+		t.Fatalf("did not receive commit_sig message")
+	}
+
+	// ==== restart node ====
+	// Restart Alice so she sends and accepts ChannelReestablish.
+	alice.restart(true, true)
+
+	// Restart Bob by re-instantiating NewLightningChannel.
+	bobSigner := harness.bobChannel.Signer
+	signerMock := lnwallet.NewDefaultAuxSignerMock(t)
+	bobPool := lnwallet.NewSigPool(runtime.NumCPU(), bobSigner)
+	bobChannel, err := lnwallet.NewLightningChannel(
+		bobSigner, harness.bobChannel.State(), bobPool,
+		lnwallet.WithLeafStore(&lnwallet.MockAuxLeafStore{}),
+		lnwallet.WithAuxSigner(signerMock),
+	)
+	require.NoError(t, err)
+	err = bobPool.Start()
+	require.NoError(t, err)
+
+	// <--channel_reestablish--
+	bobReest, err := bobChannel.State().ChanSyncMsg()
+	require.NoError(t, err)
+	alice.coreLink.HandleChannelUpdate(bobReest)
+
+	// --channel_reestablish-->
+	// Bob processes Alice's reestablish and re-sends commit_sig.
+	// Alice then force-closes upon receiving the commit_sig, which should
+	// not happen, indicating a bug.
+	select {
+	case msg := <-alice.msgs:
+		reestMsg, ok := msg.(*lnwire.ChannelReestablish)
+		require.True(t, ok)
+
+		msgsToReSend, _, _, err := bobChannel.ProcessChanSyncMsg(
+			t.Context(), reestMsg,
+		)
+		require.NoError(t, err)
+
+		// <-----commit_sig----
+		// ==== force close ====
+		alice.coreLink.HandleChannelUpdate(msgsToReSend[0])
+		time.Sleep(5 * time.Second)
+
+	case <-time.After(5 * time.Second):
+		t.Fatalf("did not receive channel_reestablish message")
+	}
+}
+
 // TestChannelLinkSingleHopPayment in this test we checks the interaction
 // between Alice and Bob within scope of one channel.
 func TestChannelLinkSingleHopPayment(t *testing.T) {
@@ -4905,9 +5021,10 @@ func (h *persistentLinkHarness) restartLink(
 		},
 		FetchLastChannelUpdate: mockGetChanUpdateMessage,
 		PreimageCache:          pCache,
-		OnChannelFailure: func(lnwire.ChannelID,
-			lnwire.ShortChannelID, LinkFailureError) {
+		OnChannelFailure: func(_ lnwire.ChannelID,
+			_ lnwire.ShortChannelID, linkErr LinkFailureError) {
 
+			require.NotEqual(t, linkErr.FailureAction, LinkFailureForceClose)
 		},
 		UpdateContractSignals: func(*contractcourt.ContractSignals) error {
 			return nil

lnwallet/channel_test.go

@@ -2895,6 +2895,137 @@ func TestUpdateFeeReceiverCommits(t *testing.T) {
 	require.NoError(t, err, "bob unable to process alice's revocation")
 }
 
+// TestChanSyncUpdateFeeRestartAfterRevoke tests a scenario where Alice (channel
+// initiator) sends update_fee and commit_sig, Bob responds with revoke_and_ack
+// and commit_sig, but Alice receives only the revoke_and_ack and then restarts
+// before Bob's commit_sig arrives.
+//
+// After restarting, channel reestablishment causes Bob to retransmit his
+// commit_sig. Alice should be able to validate it, but currently fails because
+// the fee update is not properly restored in her update log from disk.
+//
+// Root cause: during ReceiveRevocation, the fee update is persisted under
+// remoteUnsignedLocalUpdatesKey. However, restoreStateLogs only handles
+// HTLC-type updates (UpdateFulfillHTLC, UpdateFailHTLC, etc.) in that
+// restoration path and silently skips UpdateFee via "default: continue". So
+// after restart Alice's update log is empty, she computes the expected
+// commitment with the old fee rate, Bob's signature (covering the new fee
+// rate) fails verification, and LND incorrectly triggers a force close.
+func TestChanSyncUpdateFeeRestartAfterRevoke(t *testing.T) {
+	t.Parallel()
+
+	aliceChannel, bobChannel, err := CreateTestChannels(
+		t, channeldb.SingleFunderTweaklessBit,
+	)
+	require.NoError(t, err, "unable to create test channels")
+
+	// Alice is the channel initiator and sends an update_fee to Bob.
+	newFee := chainfee.SatPerKWeight(12500)
+	err = aliceChannel.UpdateFee(newFee)
+	require.NoError(t, err, "alice unable to update fee")
+	err = bobChannel.ReceiveUpdateFee(newFee)
+	require.NoError(t, err, "bob unable to receive fee update")
+
+	// Alice signs the next commitment, which includes the fee update.
+	aliceNewCommit, err := aliceChannel.SignNextCommitment(ctxb)
+	require.NoError(t, err, "alice unable to sign commitment")
+
+	// Bob receives and processes Alice's commitment.
+	err = bobChannel.ReceiveNewCommitment(aliceNewCommit.CommitSigs)
+	require.NoError(t, err, "bob unable to process alice's new commitment")
+
+	// Bob revokes his old commitment (producing a revoke_and_ack)...
+	bobRevocation, _, _, err := bobChannel.RevokeCurrentCommitment()
+	require.NoError(t, err, "bob unable to revoke commitment")
+
+	// ...and signs a new commitment for Alice.
+	_, err = bobChannel.SignNextCommitment(ctxb)
+	require.NoError(t, err, "bob unable to sign alice's commitment")
+
+	// Alice receives Bob's revoke_and_ack. This advances Bob's remote tail
+	// in Alice's view. During ReceiveRevocation, the still-unacknowledged
+	// fee update is written to disk under remoteUnsignedLocalUpdatesKey.
+	//
+	// The connection then drops before Alice receives Bob's commit_sig.
+	_, _, err = aliceChannel.ReceiveRevocation(bobRevocation)
+	require.NoError(t, err, "alice unable to process bob's revocation")
+
+	// Alice restarts. Due to the bug, the fee update stored under
+	// remoteUnsignedLocalUpdatesKey is not restored into Alice's update
+	// log (UpdateFee is skipped by the default: continue in restoreStateLogs).
+	aliceChannel, err = restartChannel(aliceChannel)
+	require.NoError(t, err, "unable to restart alice channel")
+
+	// Both sides produce their ChannelReestablish messages.
+	aliceSyncMsg, err := aliceChannel.channelState.ChanSyncMsg()
+	require.NoError(t, err, "alice unable to produce chan sync msg")
+	bobSyncMsg, err := bobChannel.channelState.ChanSyncMsg()
+	require.NoError(t, err, "bob unable to produce chan sync msg")
+
+	// Alice processes Bob's ChannelReestablish. Bob's NextLocalCommitHeight
+	// is 2, and Alice's remoteTipHeight is 1, so 2 == 1+1: Alice concludes
+	// the remote side is in sync and has nothing to retransmit.
+	aliceMsgsToSend, _, _, err := aliceChannel.ProcessChanSyncMsg(
+		ctxb, bobSyncMsg,
+	)
+	require.NoError(t, err, "alice unable to process bob's chan sync msg")
+	require.Empty(t, aliceMsgsToSend,
+		"alice should have no messages to retransmit after reestablish",
+	)
+
+	// Bob processes Alice's ChannelReestablish. Alice's NextLocalCommitHeight
+	// is 1, which equals Bob's remoteTipHeight (1), so Bob detects he owes
+	// Alice a commitment signature and prepares to retransmit it.
+	bobMsgsToSend, _, _, err := bobChannel.ProcessChanSyncMsg(
+		ctxb, aliceSyncMsg,
+	)
+	require.NoError(t, err, "bob unable to process alice's chan sync msg")
+	require.Len(t, bobMsgsToSend, 1,
+		"bob should retransmit exactly one message (his commit_sig)",
+	)
+
+	retransmittedSig, ok := bobMsgsToSend[0].(*lnwire.CommitSig)
+	require.True(t, ok, "expected bob to retransmit a CommitSig")
+
+	// Alice receives Bob's retransmitted commit_sig. This should succeed:
+	// Alice must have the fee update in her update log to compute the
+	// correct commitment and verify Bob's signature.
+	//
+	// Currently this FAILS because the fee update is not restored after
+	// restart, so Alice uses the old fee rate and Bob's signature (covering
+	// the new fee rate) does not verify. In the link layer this causes
+	// LinkFailureForceClose — an incorrect force close of a healthy channel.
+	err = aliceChannel.ReceiveNewCommitment(&CommitSigs{
+		CommitSig: retransmittedSig.CommitSig,
+		HtlcSigs:  retransmittedSig.HtlcSigs,
+	})
+	require.NoError(t, err,
+		"alice should process bob's retransmitted commit_sig without "+
+			"error; fee update not restored after restart causes "+
+			"spurious force close",
+	)
+
+	// Complete the commit dance and verify the fee is locked in on both sides.
+	aliceRevocation, _, _, err := aliceChannel.RevokeCurrentCommitment()
+	require.NoError(t, err, "alice unable to revoke commitment")
+
+	_, _, err = bobChannel.ReceiveRevocation(aliceRevocation)
+	require.NoError(t, err, "bob unable to process alice's revocation")
+
+	require.Equal(t, newFee,
+		chainfee.SatPerKWeight(
+			aliceChannel.channelState.LocalCommitment.FeePerKw,
+		),
+		"alice's fee rate was not properly locked in",
+	)
+	require.Equal(t, newFee,
+		chainfee.SatPerKWeight(
+			bobChannel.channelState.LocalCommitment.FeePerKw,
+		),
+		"bob's fee rate was not properly locked in",
+	)
+}
+
 // TestUpdateFeeReceiverSendsUpdate tests that receiving a fee update as channel
 // initiator fails, and that trying to initiate fee update as non-initiation
 // fails.