diff --git a/channeldb/channel.go b/channeldb/channel.go index 27f8887c69..7189e56af7 100644 --- a/channeldb/channel.go +++ b/channeldb/channel.go @@ -2171,7 +2171,7 @@ func (c *OpenChannel) AdvanceCommitChainTail(fwdPkg *FwdPkg) error { // NextLocalHtlcIndex returns the next unallocated local htlc index. To ensure // this always returns the next index that has been not been allocated, this // will first try to examine any pending commitments, before falling back to the -// last locked-in local commitment. +// last locked-in remote commitment. func (c *OpenChannel) NextLocalHtlcIndex() (uint64, error) { // First, load the most recent commit diff that we initiated for the // remote party. If no pending commit is found, this is not treated as @@ -2187,8 +2187,8 @@ func (c *OpenChannel) NextLocalHtlcIndex() (uint64, error) { return pendingRemoteCommit.Commitment.LocalHtlcIndex, nil } - // Otherwise, fallback to using the local htlc index of our commitment. - return c.LocalCommitment.LocalHtlcIndex, nil + // Otherwise, fallback to using the local htlc index of their commitment. + return c.RemoteCommitment.LocalHtlcIndex, nil } // LoadFwdPkgs scans the forwarding log for any packages that haven't been diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 9b8323cd5e..ff631a59db 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -2060,6 +2060,10 @@ func (l *channelLink) updateCommitTx() error { return err } + if err := l.ackDownStreamPackets(); err != nil { + return err + } + // The remote party now has a new pending commitment, so we'll update // the contract court to be aware of this new set (the prior old remote // pending). @@ -2069,11 +2073,7 @@ func (l *channelLink) updateCommitTx() error { Htlcs: pendingHTLCs, }: case <-l.quit: - return nil - } - - if err := l.ackDownStreamPackets(); err != nil { - return err + return ErrLinkShuttingDown } commitSig := &lnwire.CommitSig{ diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 0906141ac1..d7f95df162 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -3158,6 +3158,161 @@ func TestChannelLinkTrimCircuitsNoCommit(t *testing.T) { assertLinkBandwidth(t, alice.link, aliceStartingBandwidth) } +// TestChannelLinkTrimCircuitsRemoteCommit checks that the switch and link +// don't trim circuits if the ADD is locked in on the remote commitment but +// not on our local commitment. +func TestChannelLinkTrimCircuitsRemoteCommit(t *testing.T) { + t.Parallel() + + const ( + chanAmt = btcutil.SatoshiPerBitcoin * 5 + numHtlcs = 2 + ) + + // We'll start by creating a new link with our chanAmt (5 BTC). + aliceLink, bobChan, batchTicker, start, cleanUp, restore, err := + newSingleLinkTestHarness(chanAmt, 0) + if err != nil { + t.Fatalf("unable to create link: %v", err) + } + + if err := start(); err != nil { + t.Fatalf("unable to start test harness: %v", err) + } + defer cleanUp() + + alice := newPersistentLinkHarness( + t, aliceLink, batchTicker, restore, + ) + + // Compute the static fees that will be used to determine the + // correctness of Alice's bandwidth when forwarding HTLCs. + estimator := chainfee.NewStaticEstimator(6000, 0) + feePerKw, err := estimator.EstimateFeePerKW(1) + if err != nil { + t.Fatalf("unable to query fee estimator: %v", err) + } + + defaultCommitFee := alice.channel.StateSnapshot().CommitFee + htlcFee := lnwire.NewMSatFromSatoshis( + feePerKw.FeeForWeight(input.HTLCWeight), + ) + + // The starting bandwidth of the channel should be exactly the amount + // that we created the channel between her and Bob, minus the commitment + // fee and fee of adding an HTLC. + expectedBandwidth := lnwire.NewMSatFromSatoshis( + chanAmt-defaultCommitFee, + ) - htlcFee + assertLinkBandwidth(t, alice.link, expectedBandwidth) + + // Capture Alice's starting bandwidth to perform later, relative + // bandwidth assertions. + aliceStartingBandwidth := alice.link.Bandwidth() + + // Next, we'll create an HTLC worth 1 BTC that will be used as a dummy + // message for the test. + var mockBlob [lnwire.OnionPacketSize]byte + htlcAmt := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin) + _, htlc, _, err := generatePayment(htlcAmt, htlcAmt, 5, mockBlob) + if err != nil { + t.Fatalf("unable to create payment: %v", err) + } + + // Create `numHtlc` htlcPackets and payment circuits that will be used + // to drive the test. All of the packets will use the same dummy HTLC. + addPkts, circuits := genAddsAndCircuits(numHtlcs, htlc) + + // To begin the test, start by committing the circuits for our first two + // HTLCs. + fwdActions := alice.commitCircuits(circuits) + + // Both of these circuits should have successfully added, as this is the + // first attempt to send them. + if len(fwdActions.Adds) != numHtlcs { + t.Fatalf("expected %d circuits to be added", numHtlcs) + } + alice.assertNumPendingNumOpenCircuits(2, 0) + + // Since both were committed successfully, we will now deliver them to + // Alice's link. + for _, addPkt := range addPkts { + if err := alice.link.HandleSwitchPacket(addPkt); err != nil { + t.Fatalf("unable to handle switch packet: %v", err) + } + } + + // Wait until Alice's link has sent both HTLCs via the peer. + alice.checkSent(addPkts) + + // Pass both of the htlcs to Bob. + for i, addPkt := range addPkts { + pkt, ok := addPkt.htlc.(*lnwire.UpdateAddHTLC) + if !ok { + t.Fatalf("unable to add packet") + } + + pkt.ID = uint64(i) + + _, err := bobChan.ReceiveHTLC(pkt) + if err != nil { + t.Fatalf("unable to receive htlc: %v", err) + } + } + + // The resulting bandwidth should reflect that Alice is paying both + // htlc amounts, in addition to both htlc fees. + assertLinkBandwidth(t, alice.link, + aliceStartingBandwidth-numHtlcs*(htlcAmt+htlcFee), + ) + + // Now, initiate a state transition by Alice so that the pending HTLCs + // are locked in. + alice.trySignNextCommitment() + alice.assertNumPendingNumOpenCircuits(2, 2) + + select { + case aliceMsg := <-alice.msgs: + // Pass the commitment signature to Bob. + sig, ok := aliceMsg.(*lnwire.CommitSig) + if !ok { + t.Fatalf("alice did not send commitment signature") + } + + err := bobChan.ReceiveNewCommitment(sig.CommitSig, sig.HtlcSigs) + if err != nil { + t.Fatalf("unable to receive new commitment: %v", err) + } + case <-time.After(time.Second): + } + + // Next, revoke Bob's current commitment and send it to Alice so that we + // can test that Alice's circuits aren't trimmed. + rev, _, err := bobChan.RevokeCurrentCommitment() + if err != nil { + t.Fatalf("unable to revoke current commitment: %v", err) + } + + _, _, _, _, err = alice.channel.ReceiveRevocation(rev) + if err != nil { + t.Fatalf("unable to receive revocation: %v", err) + } + + // Restart Alice's link, which simulates a disconnection with the remote + // peer. + cleanUp = alice.restart(false) + defer cleanUp() + + alice.assertNumPendingNumOpenCircuits(2, 2) + + // Restart the link + switch and check that the number of open circuits + // doesn't change. + cleanUp = alice.restart(true) + defer cleanUp() + + alice.assertNumPendingNumOpenCircuits(2, 2) +} + // TestChannelLinkBandwidthChanReserve checks that the bandwidth available // on the channel link reflects the channel reserve that must be kept // at all times. diff --git a/htlcswitch/switch.go b/htlcswitch/switch.go index 9d03b1b7b4..52d05be8b4 100644 --- a/htlcswitch/switch.go +++ b/htlcswitch/switch.go @@ -1164,6 +1164,12 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error { return err } + // closeCircuit returns a nil circuit when a settle packet returns an + // ErrUnknownCircuit error upon the inner call to CloseCircuit. + if circuit == nil { + return nil + } + fail, isFail := htlc.(*lnwire.UpdateFailHTLC) if isFail && !packet.hasSource { switch { @@ -1394,19 +1400,28 @@ func (s *Switch) closeCircuit(pkt *htlcPacket) (*PaymentCircuit, error) { // Failed to close circuit because it does not exist. This is likely // because the circuit was already successfully closed. case ErrUnknownCircuit: - err := fmt.Errorf("Unable to find target channel "+ - "for HTLC settle/fail: channel ID = %s, "+ - "HTLC ID = %d", pkt.outgoingChanID, - pkt.outgoingHTLCID) - log.Error(err) - if pkt.destRef != nil { // Add this SettleFailRef to the set of pending settle/fail entries // awaiting acknowledgement. s.pendingSettleFails = append(s.pendingSettleFails, *pkt.destRef) } - return nil, err + // If this is a settle, we will not log an error message as settles + // are expected to hit the ErrUnknownCircuit case. The only way fails + // can hit this case if the link restarts after having just sent a fail + // to the switch. + _, isSettle := pkt.htlc.(*lnwire.UpdateFulfillHTLC) + if !isSettle { + err := fmt.Errorf("unable to find target channel "+ + "for HTLC fail: channel ID = %s, "+ + "HTLC ID = %d", pkt.outgoingChanID, + pkt.outgoingHTLCID) + log.Error(err) + + return nil, err + } + + return nil, nil // Unexpected error. default: diff --git a/htlcswitch/switch_test.go b/htlcswitch/switch_test.go index 9cfed11774..bcd98dcf87 100644 --- a/htlcswitch/switch_test.go +++ b/htlcswitch/switch_test.go @@ -761,8 +761,8 @@ func TestSwitchForwardSettleAfterFullAdd(t *testing.T) { } // Send the settle packet again, which should fail. - if err := s2.forward(settle); err == nil { - t.Fatalf("expected failure when sending duplicate settle " + + if err := s2.forward(settle); err != nil { + t.Fatalf("expected success when sending duplicate settle " + "with no pending circuit") } }