From f757bf48bdbfd4c115aa896d85210cf83351bb2e Mon Sep 17 00:00:00 2001 From: nsa Date: Mon, 9 Mar 2020 13:36:06 -0400 Subject: [PATCH 1/3] channeldb: use RemoteCommitment in NextLocalHtlcIndex This commit changes the fallback in NextLocalHtlcIndex to RemoteCommitment since the LocalHtlcIndex field lags behind on the LocalCommitment. Without this bug fix, open circuits would get prematurely trimmed, resulting in more erroneous logs. A test case is included to check that the fix works. --- channeldb/channel.go | 6 +- htlcswitch/link_test.go | 155 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 3 deletions(-) diff --git a/channeldb/channel.go b/channeldb/channel.go index 27f8887c..7189e56a 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_test.go b/htlcswitch/link_test.go index 0906141a..d7f95df1 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. From 4fe174ba4e0ea200f826c3e5ae6ccfedb7a49b72 Mon Sep 17 00:00:00 2001 From: nsa Date: Mon, 9 Mar 2020 13:36:33 -0400 Subject: [PATCH 2/3] htlcswitch: switch ackDownStreamPackets order with contract update call This commit modifies updateCommitTx to error with ErrLinkShuttingDown when we try to send a ContractUpdate on the htlcUpdates chan and the link has closed the quit chan. It also changes the order of the call to ackDownStreamPackets and contract update call for consistency since the packets should be acknowledged before the link goes down. --- htlcswitch/link.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 7c834827..cac199b6 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{ From 8c0c53eac3bb2f640407dfebe6d86743f4f9dcd2 Mon Sep 17 00:00:00 2001 From: nsa Date: Mon, 9 Mar 2020 13:37:08 -0400 Subject: [PATCH 3/3] htlcswitch: only error in closeCircuit if the htlc was failed This commit changes the switch to only log an error if update_fail_htlc comes in and closeCircuit returns ErrUnknownCircuit. Rationale being that only settles should hit this code path, anything else is a result of a link flap and should be treated as an error. --- htlcswitch/switch.go | 29 ++++++++++++++++++++++------- htlcswitch/switch_test.go | 4 ++-- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/htlcswitch/switch.go b/htlcswitch/switch.go index 9d03b1b7..52d05be8 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 9cfed117..bcd98dcf 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") } }