From f8da26ee30c00041f63e02ee1ce9e6426d161995 Mon Sep 17 00:00:00 2001 From: Roei Erez Date: Mon, 3 Feb 2020 12:46:21 +0200 Subject: [PATCH 1/2] contractcourt: change shouldGoOnChain signature. This commit changes the shouldGoOnChain signature to get the htlc as parameter. I will allow the function to take decisions based on whether the htlc is Incoming or Outgoing. --- contractcourt/channel_arbitrator.go | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index bb082170..8b6896e1 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -1113,17 +1113,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, - currentHeight uint32) bool { +func (c *ChannelArbitrator) shouldGoOnChain(htlc channeldb.HTLC, + 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 @@ -1162,8 +1162,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( - htlc.RefundTimeout, c.cfg.OutgoingBroadcastDelta, + toChain := c.shouldGoOnChain(htlc, c.cfg.OutgoingBroadcastDelta, height, ) @@ -1194,8 +1193,7 @@ func (c *ChannelArbitrator) checkCommitChainActions(height uint32, continue } - toChain := c.shouldGoOnChain( - htlc.RefundTimeout, c.cfg.IncomingBroadcastDelta, + toChain := c.shouldGoOnChain(htlc, c.cfg.IncomingBroadcastDelta, height, ) @@ -1245,8 +1243,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( - htlc.RefundTimeout, c.cfg.OutgoingBroadcastDelta, + case !c.shouldGoOnChain(htlc, c.cfg.OutgoingBroadcastDelta, height, ): // TODO(roasbeef): also need to be able to query @@ -1415,8 +1412,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( - htlc.RefundTimeout, c.cfg.OutgoingBroadcastDelta, + goToChain := c.shouldGoOnChain(htlc, c.cfg.OutgoingBroadcastDelta, height, ) From 0407b37fceb8370d79cd6e56745588d58d0d27ca Mon Sep 17 00:00:00 2001 From: Roei Erez Date: Mon, 3 Feb 2020 12:52:22 +0200 Subject: [PATCH 2/2] contractcourt+switch: keep channels with timed-out initiated htlcs. This commit enables the user to specify he is not interested in automatically close channels with pending payments that their corresponding htlcs have timed-out. By requiring a configurable grace period uptime of our node before closing such channels, we give a chance to the other node to properly cancel the htlc and avoid unnecessary on-chain transaction. In mobile it is very important for the user experience as otherwise channels will be force closed more frequently. --- config.go | 47 ++++++------ contractcourt/chain_arbitrator.go | 15 ++++ contractcourt/chain_arbitrator_test.go | 3 + contractcourt/channel_arbitrator.go | 29 +++++++- contractcourt/channel_arbitrator_test.go | 91 ++++++++++++++++++++++++ htlcswitch/switch.go | 12 ++++ htlcswitch/switch_test.go | 7 ++ server.go | 13 ++-- test_utils.go | 7 ++ 9 files changed, 196 insertions(+), 28 deletions(-) diff --git a/config.go b/config.go index 66d096a2..7923b7a0 100644 --- a/config.go +++ b/config.go @@ -58,15 +58,16 @@ const ( // pending channels permitted per peer. DefaultMaxPendingChannels = 1 - defaultNoSeedBackup = false - defaultTrickleDelay = 90 * 1000 - defaultChanStatusSampleInterval = time.Minute - defaultChanEnableTimeout = 19 * time.Minute - defaultChanDisableTimeout = 20 * time.Minute - defaultMaxLogFiles = 3 - defaultMaxLogFileSize = 10 - defaultMinBackoff = time.Second - defaultMaxBackoff = time.Hour + defaultNoSeedBackup = false + defaultPaymentsExpirationGracePeriod = time.Duration(0) + defaultTrickleDelay = 90 * 1000 + defaultChanStatusSampleInterval = time.Minute + defaultChanEnableTimeout = 19 * time.Minute + defaultChanDisableTimeout = 20 * time.Minute + defaultMaxLogFiles = 3 + defaultMaxLogFileSize = 10 + defaultMinBackoff = time.Second + defaultMaxBackoff = time.Hour defaultTorSOCKSPort = 9050 defaultTorDNSHost = "soa.nodes.lightning.directory" @@ -298,10 +299,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"` - 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)."` - 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)"` + 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."` + TrickleDelay int `long:"trickledelay" description:"Time in milliseconds between each release of announcements to the network"` + 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)."` + 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"` @@ -416,15 +418,16 @@ func loadConfig() (*config, error) { "preferential": 1.0, }, }, - TrickleDelay: defaultTrickleDelay, - ChanStatusSampleInterval: defaultChanStatusSampleInterval, - ChanEnableTimeout: defaultChanEnableTimeout, - ChanDisableTimeout: defaultChanDisableTimeout, - Alias: defaultAlias, - Color: defaultColor, - MinChanSize: int64(minChanFundingSize), - NumGraphSyncPeers: defaultMinPeers, - HistoricalSyncInterval: discovery.DefaultHistoricalSyncInterval, + PaymentsExpirationGracePeriod: defaultPaymentsExpirationGracePeriod, + TrickleDelay: defaultTrickleDelay, + ChanStatusSampleInterval: defaultChanStatusSampleInterval, + ChanEnableTimeout: defaultChanEnableTimeout, + ChanDisableTimeout: defaultChanDisableTimeout, + Alias: defaultAlias, + Color: defaultColor, + MinChanSize: int64(minChanFundingSize), + NumGraphSyncPeers: defaultMinPeers, + HistoricalSyncInterval: discovery.DefaultHistoricalSyncInterval, Tor: &torConfig{ SOCKS: defaultTorSOCKS, DNS: defaultTorDNS, diff --git a/contractcourt/chain_arbitrator.go b/contractcourt/chain_arbitrator.go index add69c0f..552549f4 100644 --- a/contractcourt/chain_arbitrator.go +++ b/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 diff --git a/contractcourt/chain_arbitrator_test.go b/contractcourt/chain_arbitrator_test.go index 07c31b28..5710b14a 100644 --- a/contractcourt/chain_arbitrator_test.go +++ b/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, diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index 8b6896e1..b11c81aa 100644 --- a/contractcourt/channel_arbitrator.go +++ b/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 @@ -1132,7 +1137,29 @@ func (c *ChannelArbitrator) shouldGoOnChain(htlc channeldb.HTLC, // 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 diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index af4681a3..9375d895 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/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, + ) +} diff --git a/htlcswitch/switch.go b/htlcswitch/switch.go index b78f43b1..bdd4d446 100644 --- a/htlcswitch/switch.go +++ b/htlcswitch/switch.go @@ -460,6 +460,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. diff --git a/htlcswitch/switch_test.go b/htlcswitch/switch_test.go index 28493c83..5a8ace21 100644 --- a/htlcswitch/switch_test.go +++ b/htlcswitch/switch_test.go @@ -322,6 +322,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. @@ -1892,6 +1896,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, diff --git a/server.go b/server.go index a5538483..9ce272f2 100644 --- a/server.go +++ b/server.go @@ -915,11 +915,14 @@ func newServer(listenAddrs []net.Addr, chanDB *channeldb.DB, return ErrServerShuttingDown } }, - DisableChannel: s.chanStatusMgr.RequestDisable, - Sweeper: s.sweeper, - Registry: s.invoices, - NotifyClosedChannel: s.channelNotifier.NotifyClosedChannelEvent, - OnionProcessor: s.sphinx, + DisableChannel: s.chanStatusMgr.RequestDisable, + Sweeper: s.sweeper, + Registry: s.invoices, + NotifyClosedChannel: s.channelNotifier.NotifyClosedChannelEvent, + OnionProcessor: s.sphinx, + PaymentsExpirationGracePeriod: cfg.PaymentsExpirationGracePeriod, + IsForwardedHTLC: s.htlcSwitch.IsForwardedHTLC, + Clock: clock.NewDefaultClock(), }, chanDB) s.breachArbiter = newBreachArbiter(&BreachConfig{ diff --git a/test_utils.go b/test_utils.go index afb16cd5..11c025e2 100644 --- a/test_utils.go +++ b/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)