Merge pull request #3876 from breez/allow-timeouted-initiated-htlc

contractcourt: keep channels with timed-out initiated htlcs.

config.go

@@ -58,15 +58,16 @@ const (
 	// pending channels permitted per peer.
 	DefaultMaxPendingChannels = 1
 
-	defaultNoSeedBackup             = false
+	defaultNoSeedBackup                  = false
-	defaultTrickleDelay             = 90 * 1000
+	defaultPaymentsExpirationGracePeriod = time.Duration(0)
-	defaultChanStatusSampleInterval = time.Minute
+	defaultTrickleDelay                  = 90 * 1000
-	defaultChanEnableTimeout        = 19 * time.Minute
+	defaultChanStatusSampleInterval      = time.Minute
-	defaultChanDisableTimeout       = 20 * time.Minute
+	defaultChanEnableTimeout             = 19 * time.Minute
-	defaultMaxLogFiles              = 3
+	defaultChanDisableTimeout            = 20 * time.Minute
-	defaultMaxLogFileSize           = 10
+	defaultMaxLogFiles                   = 3
-	defaultMinBackoff               = time.Second
+	defaultMaxLogFileSize                = 10
-	defaultMaxBackoff               = time.Hour
+	defaultMinBackoff                    = time.Second
+	defaultMaxBackoff                    = time.Hour
 
 	defaultTorSOCKSPort            = 9050
 	defaultTorDNSHost              = "soa.nodes.lightning.directory"
@@ -301,10 +302,11 @@ type config struct {
 
 	NoSeedBackup bool `long:"noseedbackup" description:"If true, NO SEED WILL BE EXPOSED AND THE WALLET WILL BE ENCRYPTED USING THE DEFAULT PASSPHRASE -- EVER. THIS FLAG IS ONLY FOR TESTING AND IS BEING DEPRECATED."`
 
-	TrickleDelay             int           `long:"trickledelay" description:"Time in milliseconds between each release of announcements to the network"`
+	PaymentsExpirationGracePeriod time.Duration `long:"payments-expiration-grace-period" description:"A period to wait before force closing channels with outgoing htlcs that have timed-out and are a result of this node initiated payments."`
-	ChanEnableTimeout        time.Duration `long:"chan-enable-timeout" description:"The duration that a peer connection must be stable before attempting to send a channel update to reenable or cancel a pending disables of the peer's channels on the network (default: 19m)."`
+	TrickleDelay                  int           `long:"trickledelay" description:"Time in milliseconds between each release of announcements to the network"`
-	ChanDisableTimeout       time.Duration `long:"chan-disable-timeout" description:"The duration that must elapse after first detecting that an already active channel is actually inactive and sending channel update disabling it to the network. The pending disable can be canceled if the peer reconnects and becomes stable for chan-enable-timeout before the disable update is sent. (default: 20m)"`
+	ChanEnableTimeout             time.Duration `long:"chan-enable-timeout" description:"The duration that a peer connection must be stable before attempting to send a channel update to reenable or cancel a pending disables of the peer's channels on the network (default: 19m)."`
-	ChanStatusSampleInterval time.Duration `long:"chan-status-sample-interval" description:"The polling interval between attempts to detect if an active channel has become inactive due to its peer going offline. (default: 1m)"`
+	ChanDisableTimeout            time.Duration `long:"chan-disable-timeout" description:"The duration that must elapse after first detecting that an already active channel is actually inactive and sending channel update disabling it to the network. The pending disable can be canceled if the peer reconnects and becomes stable for chan-enable-timeout before the disable update is sent. (default: 20m)"`
+	ChanStatusSampleInterval      time.Duration `long:"chan-status-sample-interval" description:"The polling interval between attempts to detect if an active channel has become inactive due to its peer going offline. (default: 1m)"`
 
 	Alias       string `long:"alias" description:"The node alias. Used as a moniker by peers and intelligence services"`
 	Color       string `long:"color" description:"The color of the node in hex format (i.e. '#3399FF'). Used to customize node appearance in intelligence services"`
@@ -421,15 +423,16 @@ func loadConfig() (*config, error) {
 				"preferential": 1.0,
 			},
 		},
-		TrickleDelay:             defaultTrickleDelay,
+		PaymentsExpirationGracePeriod: defaultPaymentsExpirationGracePeriod,
-		ChanStatusSampleInterval: defaultChanStatusSampleInterval,
+		TrickleDelay:                  defaultTrickleDelay,
-		ChanEnableTimeout:        defaultChanEnableTimeout,
+		ChanStatusSampleInterval:      defaultChanStatusSampleInterval,
-		ChanDisableTimeout:       defaultChanDisableTimeout,
+		ChanEnableTimeout:             defaultChanEnableTimeout,
-		Alias:                    defaultAlias,
+		ChanDisableTimeout:            defaultChanDisableTimeout,
-		Color:                    defaultColor,
+		Alias:                         defaultAlias,
-		MinChanSize:              int64(minChanFundingSize),
+		Color:                         defaultColor,
-		NumGraphSyncPeers:        defaultMinPeers,
+		MinChanSize:                   int64(minChanFundingSize),
-		HistoricalSyncInterval:   discovery.DefaultHistoricalSyncInterval,
+		NumGraphSyncPeers:             defaultMinPeers,
+		HistoricalSyncInterval:        discovery.DefaultHistoricalSyncInterval,
 		Tor: &torConfig{
 			SOCKS:   defaultTorSOCKS,
 			DNS:     defaultTorDNS,

contractcourt/chain_arbitrator.go

@@ -5,12 +5,14 @@ import (
 	"fmt"
 	"sync"
 	"sync/atomic"
+	"time"
 
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/wire"
 	"github.com/btcsuite/btcutil"
 	"github.com/lightningnetwork/lnd/chainntnfs"
 	"github.com/lightningnetwork/lnd/channeldb"
+	"github.com/lightningnetwork/lnd/clock"
 	"github.com/lightningnetwork/lnd/input"
 	"github.com/lightningnetwork/lnd/lnwallet"
 	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
@@ -153,6 +155,19 @@ type ChainArbitratorConfig struct {
 	// OnionProcessor is used to decode onion payloads for on-chain
 	// resolution.
 	OnionProcessor OnionProcessor
+
+	// PaymentsExpirationGracePeriod indicates is a time window we let the
+	// other node to cancel an outgoing htlc that our node has initiated and
+	// has timed out.
+	PaymentsExpirationGracePeriod time.Duration
+
+	// IsForwardedHTLC checks for a given htlc, identified by channel id and
+	// htlcIndex, if it is a forwarded one.
+	IsForwardedHTLC func(chanID lnwire.ShortChannelID, htlcIndex uint64) bool
+
+	// Clock is the clock implementation that ChannelArbitrator uses.
+	// It is useful for testing.
+	Clock clock.Clock
 }
 
 // ChainArbitrator is a sub-system that oversees the on-chain resolution of all

contractcourt/chain_arbitrator_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/wire"
 	"github.com/lightningnetwork/lnd/channeldb"
+	"github.com/lightningnetwork/lnd/clock"
 	"github.com/lightningnetwork/lnd/lnwallet"
 )
 
@@ -83,6 +84,7 @@ func TestChainArbitratorRepublishCloses(t *testing.T) {
 			published[tx.TxHash()]++
 			return nil
 		},
+		Clock: clock.NewDefaultClock(),
 	}
 	chainArb := NewChainArbitrator(
 		chainArbCfg, db,
@@ -171,6 +173,7 @@ func TestResolveContract(t *testing.T) {
 		PublishTx: func(tx *wire.MsgTx) error {
 			return nil
 		},
+		Clock: clock.NewDefaultClock(),
 	}
 	chainArb := NewChainArbitrator(
 		chainArbCfg, db,

contractcourt/channel_arbitrator.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"sync"
 	"sync/atomic"
+	"time"
 
 	"github.com/btcsuite/btcd/wire"
 	"github.com/btcsuite/btcutil"
@@ -260,6 +261,9 @@ type ChannelArbitrator struct {
 	started int32 // To be used atomically.
 	stopped int32 // To be used atomically.
 
+	// startTimestamp is the time when this ChannelArbitrator was started.
+	startTimestamp time.Time
+
 	// log is a persistent log that the attendant will use to checkpoint
 	// its next action, and the state of any unresolved contracts.
 	log ArbitratorLog
@@ -328,6 +332,7 @@ func (c *ChannelArbitrator) Start() error {
 	if !atomic.CompareAndSwapInt32(&c.started, 0, 1) {
 		return nil
 	}
+	c.startTimestamp = c.cfg.Clock.Now()
 
 	var (
 		err error
@@ -1113,17 +1118,17 @@ func (c ChainActionMap) Merge(actions ChainActionMap) {
 // we should go on chain to claim.  We do this rather than waiting up until the
 // last minute as we want to ensure that when we *need* (HTLC is timed out) to
 // sweep, the commitment is already confirmed.
-func (c *ChannelArbitrator) shouldGoOnChain(htlcExpiry, broadcastDelta,
+func (c *ChannelArbitrator) shouldGoOnChain(htlc channeldb.HTLC,
-	currentHeight uint32) bool {
+	broadcastDelta, currentHeight uint32) bool {
 
 	// We'll calculate the broadcast cut off for this HTLC. This is the
 	// height that (based on our current fee estimation) we should
 	// broadcast in order to ensure the commitment transaction is confirmed
 	// before the HTLC fully expires.
-	broadcastCutOff := htlcExpiry - broadcastDelta
+	broadcastCutOff := htlc.RefundTimeout - broadcastDelta
 
 	log.Tracef("ChannelArbitrator(%v): examining outgoing contract: "+
-		"expiry=%v, cutoff=%v, height=%v", c.cfg.ChanPoint, htlcExpiry,
+		"expiry=%v, cutoff=%v, height=%v", c.cfg.ChanPoint, htlc.RefundTimeout,
 		broadcastCutOff, currentHeight)
 
 	// TODO(roasbeef): take into account default HTLC delta, don't need to
@@ -1132,7 +1137,29 @@ func (c *ChannelArbitrator) shouldGoOnChain(htlcExpiry, broadcastDelta,
 
 	// We should on-chain for this HTLC, iff we're within out broadcast
 	// cutoff window.
-	return currentHeight >= broadcastCutOff
+	if currentHeight < broadcastCutOff {
+		return false
+	}
+
+	// In case of incoming htlc we should go to chain.
+	if htlc.Incoming {
+		return true
+	}
+
+	// For htlcs that are result of our initiated payments we give some grace
+	// period before force closing the channel. During this time we expect
+	// both nodes to connect and give a chance to the other node to send its
+	// updates and cancel the htlc.
+	// This shouldn't add any security risk as there is no incoming htlc to
+	// fulfill at this case and the expectation is that when the channel is
+	// active the other node will send update_fail_htlc to remove the htlc
+	// without closing the channel. It is up to the user to force close the
+	// channel if the peer misbehaves and doesn't send the update_fail_htlc.
+	// It is useful when this node is most of the time not online and is
+	// likely to miss the time slot where the htlc may be cancelled.
+	isForwarded := c.cfg.IsForwardedHTLC(c.cfg.ShortChanID, htlc.HtlcIndex)
+	upTime := c.cfg.Clock.Now().Sub(c.startTimestamp)
+	return isForwarded || upTime > c.cfg.PaymentsExpirationGracePeriod
 }
 
 // checkCommitChainActions is called for each new block connected to the end of
@@ -1162,8 +1189,7 @@ func (c *ChannelArbitrator) checkCommitChainActions(height uint32,
 	for _, htlc := range htlcs.outgoingHTLCs {
 		// We'll need to go on-chain for an outgoing HTLC if it was
 		// never resolved downstream, and it's "close" to timing out.
-		toChain := c.shouldGoOnChain(
+		toChain := c.shouldGoOnChain(htlc, c.cfg.OutgoingBroadcastDelta,
-			htlc.RefundTimeout, c.cfg.OutgoingBroadcastDelta,
 			height,
 		)
 
@@ -1194,8 +1220,7 @@ func (c *ChannelArbitrator) checkCommitChainActions(height uint32,
 			continue
 		}
 
-		toChain := c.shouldGoOnChain(
+		toChain := c.shouldGoOnChain(htlc, c.cfg.IncomingBroadcastDelta,
-			htlc.RefundTimeout, c.cfg.IncomingBroadcastDelta,
 			height,
 		)
 
@@ -1245,8 +1270,7 @@ func (c *ChannelArbitrator) checkCommitChainActions(height uint32,
 		// mark it still "live". After we broadcast, we'll monitor it
 		// until the HTLC times out to see if we can also redeem it
 		// on-chain.
-		case !c.shouldGoOnChain(
+		case !c.shouldGoOnChain(htlc, c.cfg.OutgoingBroadcastDelta,
-			htlc.RefundTimeout, c.cfg.OutgoingBroadcastDelta,
 			height,
 		):
 			// TODO(roasbeef): also need to be able to query
@@ -1415,8 +1439,7 @@ func (c *ChannelArbitrator) checkRemoteDanglingActions(
 	for _, htlc := range pendingRemoteHTLCs {
 		// We'll now check if we need to go to chain in order to cancel
 		// the incoming HTLC.
-		goToChain := c.shouldGoOnChain(
+		goToChain := c.shouldGoOnChain(htlc, c.cfg.OutgoingBroadcastDelta,
-			htlc.RefundTimeout, c.cfg.OutgoingBroadcastDelta,
 			height,
 		)
 

contractcourt/channel_arbitrator_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/coreos/bbolt"
 	"github.com/lightningnetwork/lnd/chainntnfs"
 	"github.com/lightningnetwork/lnd/channeldb"
+	"github.com/lightningnetwork/lnd/clock"
 	"github.com/lightningnetwork/lnd/input"
 	"github.com/lightningnetwork/lnd/lnwallet"
 	"github.com/lightningnetwork/lnd/lnwire"
@@ -309,6 +310,12 @@ func createTestChannelArbitrator(t *testing.T, log ArbitratorLog) (*chanArbTestC
 			return nil
 		},
 		OnionProcessor: &mockOnionProcessor{},
+		IsForwardedHTLC: func(chanID lnwire.ShortChannelID,
+			htlcIndex uint64) bool {
+
+			return true
+		},
+		Clock: clock.NewDefaultClock(),
 	}
 
 	// We'll use the resolvedChan to synchronize on call to
@@ -1811,3 +1818,87 @@ func TestChannelArbitratorDanglingCommitForceClose(t *testing.T) {
 		})
 	}
 }
+
+// TestChannelArbitratorPendingExpiredHTLC tests that if we have pending htlc
+// that is expired we will only go to chain if we are running at least the
+// time defined in PaymentsExpirationGracePeriod.
+// During this time the remote party is expected to send his updates and cancel
+// The htlc.
+func TestChannelArbitratorPendingExpiredHTLC(t *testing.T) {
+	t.Parallel()
+
+	// We'll create the arbitrator and its backing log in a default state.
+	log := &mockArbitratorLog{
+		state:     StateDefault,
+		newStates: make(chan ArbitratorState, 5),
+		resolvers: make(map[ContractResolver]struct{}),
+	}
+	chanArbCtx, err := createTestChannelArbitrator(t, log)
+	if err != nil {
+		t.Fatalf("unable to create ChannelArbitrator: %v", err)
+	}
+	chanArb := chanArbCtx.chanArb
+
+	// We'll inject a test clock implementation so we can control the uptime.
+	startTime := time.Date(2020, time.February, 3, 13, 0, 0, 0, time.UTC)
+	testClock := clock.NewTestClock(startTime)
+	chanArb.cfg.Clock = testClock
+
+	// We also configure the grace period and the IsForwardedHTLC to identify
+	// the htlc as our initiated payment.
+	chanArb.cfg.PaymentsExpirationGracePeriod = time.Second * 15
+	chanArb.cfg.IsForwardedHTLC = func(chanID lnwire.ShortChannelID,
+		htlcIndex uint64) bool {
+
+		return false
+	}
+
+	if err := chanArb.Start(); err != nil {
+		t.Fatalf("unable to start ChannelArbitrator: %v", err)
+	}
+	defer func() {
+		if err := chanArb.Stop(); err != nil {
+			t.Fatalf("unable to stop chan arb: %v", err)
+		}
+	}()
+
+	// Now that our channel arb has started, we'll set up
+	// its contract signals channel so we can send it
+	// various HTLC updates for this test.
+	htlcUpdates := make(chan *ContractUpdate)
+	signals := &ContractSignals{
+		HtlcUpdates: htlcUpdates,
+		ShortChanID: lnwire.ShortChannelID{},
+	}
+	chanArb.UpdateContractSignals(signals)
+
+	// Next, we'll send it a new HTLC that is set to expire
+	// in 10 blocks.
+	htlcIndex := uint64(99)
+	htlcExpiry := uint32(10)
+	pendingHTLC := channeldb.HTLC{
+		Incoming:      false,
+		Amt:           10000,
+		HtlcIndex:     htlcIndex,
+		RefundTimeout: htlcExpiry,
+	}
+	htlcUpdates <- &ContractUpdate{
+		HtlcKey: RemoteHtlcSet,
+		Htlcs:   []channeldb.HTLC{pendingHTLC},
+	}
+
+	// We will advance the uptime to 10 seconds which should be still within
+	// the grace period and should not trigger going to chain.
+	testClock.SetTime(startTime.Add(time.Second * 10))
+	chanArbCtx.blockEpochs <- &chainntnfs.BlockEpoch{Height: 5}
+	chanArbCtx.AssertState(StateDefault)
+
+	// We will advance the uptime to 16 seconds which should trigger going
+	// to chain.
+	testClock.SetTime(startTime.Add(time.Second * 16))
+	chanArbCtx.blockEpochs <- &chainntnfs.BlockEpoch{Height: 6}
+	chanArbCtx.AssertStateTransitions(
+		StateBroadcastCommit,
+		StateCommitmentBroadcasted,
+	)
+}

htlcswitch/switch.go

@@ -459,6 +459,18 @@ func (s *Switch) UpdateForwardingPolicies(
 	s.indexMtx.RUnlock()
 }
 
+// IsForwardedHTLC checks for a given channel and htlc index if it is related
+// to an opened circuit that represents a forwarded payment.
+func (s *Switch) IsForwardedHTLC(chanID lnwire.ShortChannelID,
+	htlcIndex uint64) bool {
+
+	circuit := s.circuits.LookupOpenCircuit(channeldb.CircuitKey{
+		ChanID: chanID,
+		HtlcID: htlcIndex,
+	})
+	return circuit != nil && circuit.Incoming.ChanID != hop.Source
+}
+
 // forward is used in order to find next channel link and apply htlc update.
 // Also this function is used by channel links itself in order to forward the
 // update after it has been included in the channel.

htlcswitch/switch_test.go

@@ -336,6 +336,10 @@ func TestSwitchForward(t *testing.T) {
 		t.Fatal("wrong amount of circuits")
 	}
 
+	if !s.IsForwardedHTLC(bobChannelLink.ShortChanID(), 0) {
+		t.Fatal("htlc should be identified as forwarded")
+	}
+
 	// Create settle request pretending that bob link handled the add htlc
 	// request and sent the htlc settle request back. This request should
 	// be forwarder back to Alice link.
@@ -2120,6 +2124,9 @@ func TestSwitchSendPayment(t *testing.T) {
 		t.Fatalf("unable obfuscate failure: %v", err)
 	}
 
+	if s.IsForwardedHTLC(aliceChannelLink.ShortChanID(), update.ID) {
+		t.Fatal("htlc should be identified as not forwarded")
+	}
 	packet := &htlcPacket{
 		outgoingChanID: aliceChannelLink.ShortChanID(),
 		outgoingHTLCID: 0,

server.go

@@ -916,11 +916,14 @@ func newServer(listenAddrs []net.Addr, chanDB *channeldb.DB,
 				return ErrServerShuttingDown
 			}
 		},
-		DisableChannel:      s.chanStatusMgr.RequestDisable,
+		DisableChannel:                s.chanStatusMgr.RequestDisable,
-		Sweeper:             s.sweeper,
+		Sweeper:                       s.sweeper,
-		Registry:            s.invoices,
+		Registry:                      s.invoices,
-		NotifyClosedChannel: s.channelNotifier.NotifyClosedChannelEvent,
+		NotifyClosedChannel:           s.channelNotifier.NotifyClosedChannelEvent,
-		OnionProcessor:      s.sphinx,
+		OnionProcessor:                s.sphinx,
+		PaymentsExpirationGracePeriod: cfg.PaymentsExpirationGracePeriod,
+		IsForwardedHTLC:               s.htlcSwitch.IsForwardedHTLC,
+		Clock:                         clock.NewDefaultClock(),
 	}, chanDB)
 
 	s.breachArbiter = newBreachArbiter(&BreachConfig{

test_utils.go

@@ -17,6 +17,7 @@ import (
 	"github.com/btcsuite/btcutil"
 	"github.com/lightningnetwork/lnd/chainntnfs"
 	"github.com/lightningnetwork/lnd/channeldb"
+	"github.com/lightningnetwork/lnd/clock"
 	"github.com/lightningnetwork/lnd/contractcourt"
 	"github.com/lightningnetwork/lnd/htlcswitch"
 	"github.com/lightningnetwork/lnd/input"
@@ -360,6 +361,12 @@ func createTestPeer(notifier chainntnfs.ChainNotifier, publTx chan *wire.MsgTx,
 		contractcourt.ChainArbitratorConfig{
 			Notifier: notifier,
 			ChainIO:  chainIO,
+			IsForwardedHTLC: func(chanID lnwire.ShortChannelID,
+				htlcIndex uint64) bool {
+
+				return true
+			},
+			Clock: clock.NewDefaultClock(),
 		}, dbAlice,
 	)
 	chainArb.WatchNewChannel(aliceChannelState)