From 98a270bd308db8c7cd1941da8e361000d462919f Mon Sep 17 00:00:00 2001 From: ziggie Date: Thu, 17 Oct 2024 12:49:19 +0200 Subject: [PATCH 1/9] contractcourt: improve robustness --- contractcourt/channel_arbitrator.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index 4ddb52f64..082e3f71a 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -682,7 +682,7 @@ func (c *ChannelArbitrator) relaunchResolvers(commitSet *CommitSet, // chain actions may exclude some information, but we cannot recover it // for these older nodes at the moment. var confirmedHTLCs []channeldb.HTLC - if commitSet != nil { + if commitSet != nil && commitSet.ConfCommitKey != nil { confirmedHTLCs = commitSet.HtlcSets[*commitSet.ConfCommitKey] } else { chainActions, err := c.log.FetchChainActions() @@ -2203,7 +2203,7 @@ func (c *ChannelArbitrator) constructChainActions(confCommitSet *CommitSet, // then this is an older node that had a pending close channel before // the CommitSet was introduced. In this case, we'll just return the // existing ChainActionMap they had on disk. - if confCommitSet == nil { + if confCommitSet == nil || confCommitSet.ConfCommitKey == nil { return c.log.FetchChainActions() } From 8890f83bde51d041ea1d34f5d33c11ca30882f59 Mon Sep 17 00:00:00 2001 From: ziggie Date: Thu, 17 Oct 2024 13:17:38 +0200 Subject: [PATCH 2/9] contratcourt: refactor resolving htlc logic Refactor the part where we are failing back the incoming htlc when the channel of the corresponding outgoing htlc is force closed. We do this because in furture commits we separate the logic when we fail back the incoming htlc (abandonForward). Right now we fail abandon dust forwards and non-dust forwards only when the commitment transaction is confirmed. Later we will move the canceling of the upstream htlc when the commitment transaction is broadcasted instead of waiting until the commitment tx is confirmed. The reason for that is that dust cannot be enforced onchain anyways so there is no reason to wait. --- contractcourt/channel_arbitrator.go | 273 ++++++++++++++++------------ 1 file changed, 156 insertions(+), 117 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index 082e3f71a..1b61a100b 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -1204,12 +1204,78 @@ func (c *ChannelArbitrator) stateStep( break } + // First, we'll reconstruct a fresh set of chain actions as the + // set of actions we need to act on may differ based on if it + // was our commitment, or they're commitment that hit the chain. + htlcActions, err := c.constructChainActions( + confCommitSet, triggerHeight, trigger, + ) + if err != nil { + return StateError, closeTx, err + } + + // In case its a breach transaction we fail back all outgoing + // HTLCs on the remote commitment set. + if contractResolutions.BreachResolution != nil { + // cancelBreachedHTLCs is a set which holds HTLCs whose + // corresponding incoming HTLCs will be failed back + // because the peer broadcasted an old state. + cancelBreachedHTLCs := fn.NewSet[uint64]() + + // We'll use the CommitSet, we'll fail back all outgoing + // HTLC's that exist on either of the remote + // commitments. The map is used to deduplicate any + // shared HTLC's. + for htlcSetKey, htlcs := range confCommitSet.HtlcSets { + if !htlcSetKey.IsRemote { + continue + } + + for _, htlc := range htlcs { + // Only outgoing HTLCs have a + // corresponding incoming HTLC. + if htlc.Incoming { + continue + } + + cancelBreachedHTLCs.Add(htlc.HtlcIndex) + } + } + + err := c.abandonForwards(cancelBreachedHTLCs) + if err != nil { + return StateError, closeTx, err + } + } else { + // If it's not a breach, we resolve all incoming dust + // HTLCs immediately after the commitment is confirmed. + err = c.failIncomingDust( + htlcActions[HtlcIncomingDustFinalAction], + ) + if err != nil { + return StateError, closeTx, err + } + + // If we can fail an HTLC immediately (an outgoing HTLC + // with no contract and it was not canceled before), + // then we'll assemble an HTLC fail packet to send. + getIdx := func(htlc channeldb.HTLC) uint64 { + return htlc.HtlcIndex + } + remoteDangling := fn.NewSet(fn.Map( + getIdx, htlcActions[HtlcFailNowAction], + )...) + err := c.abandonForwards(remoteDangling) + if err != nil { + return StateError, closeTx, err + } + } + // Now that we know we'll need to act, we'll process all the // resolvers, then create the structures we need to resolve all // outstanding contracts. - resolvers, pktsToSend, err := c.prepContractResolutions( - contractResolutions, triggerHeight, trigger, - confCommitSet, + resolvers, err := c.prepContractResolutions( + contractResolutions, triggerHeight, htlcActions, ) if err != nil { log.Errorf("ChannelArbitrator(%v): unable to "+ @@ -1217,20 +1283,6 @@ func (c *ChannelArbitrator) stateStep( return StateError, closeTx, err } - // With the commitment broadcast, we'll then send over all - // messages we can send immediately. - if len(pktsToSend) != 0 { - log.Debugf("ChannelArbitrator(%v): sending "+ - "resolution message=%v", c.cfg.ChanPoint, - lnutils.SpewLogClosure(pktsToSend)) - - err := c.cfg.DeliverResolutionMsg(pktsToSend...) - if err != nil { - log.Errorf("unable to send pkts: %v", err) - return StateError, closeTx, err - } - } - log.Debugf("ChannelArbitrator(%v): inserting %v contract "+ "resolvers", c.cfg.ChanPoint, len(resolvers)) @@ -2246,24 +2298,13 @@ func (c *ChannelArbitrator) constructChainActions(confCommitSet *CommitSet, // are properly resolved. func (c *ChannelArbitrator) prepContractResolutions( contractResolutions *ContractResolutions, height uint32, - trigger transitionTrigger, - confCommitSet *CommitSet) ([]ContractResolver, []ResolutionMsg, error) { - - // First, we'll reconstruct a fresh set of chain actions as the set of - // actions we need to act on may differ based on if it was our - // commitment, or they're commitment that hit the chain. - htlcActions, err := c.constructChainActions( - confCommitSet, height, trigger, - ) - if err != nil { - return nil, nil, err - } + htlcActions ChainActionMap) ([]ContractResolver, error) { // We'll also fetch the historical state of this channel, as it should // have been marked as closed by now, and supplement it to each resolver // such that we can properly resolve our pending contracts. var chanState *channeldb.OpenChannel - chanState, err = c.cfg.FetchHistoricalChannel() + chanState, err := c.cfg.FetchHistoricalChannel() switch { // If we don't find this channel, then it may be the case that it // was closed before we started to retain the final state @@ -2275,16 +2316,9 @@ func (c *ChannelArbitrator) prepContractResolutions( "state", c.cfg.ChanPoint) case err != nil: - return nil, nil, err + return nil, err } - // There may be a class of HTLC's which we can fail back immediately, - // for those we'll prepare a slice of packets to add to our outbox. Any - // packets we need to send, will be cancels. - var ( - msgsToSend []ResolutionMsg - ) - incomingResolutions := contractResolutions.HtlcResolutions.IncomingHTLCs outgoingResolutions := contractResolutions.HtlcResolutions.OutgoingHTLCs @@ -2313,7 +2347,6 @@ func (c *ChannelArbitrator) prepContractResolutions( } commitHash := contractResolutions.CommitHash - failureMsg := &lnwire.FailPermanentChannelFailure{} var htlcResolvers []ContractResolver @@ -2337,37 +2370,7 @@ func (c *ChannelArbitrator) prepContractResolutions( breachResolver := newBreachResolver(resolverCfg) htlcResolvers = append(htlcResolvers, breachResolver) - // We'll use the CommitSet, we'll fail back all outgoing HTLC's - // that exist on either of the remote commitments. The map is - // used to deduplicate any shared htlc's. - remoteOutgoing := make(map[uint64]channeldb.HTLC) - for htlcSetKey, htlcs := range confCommitSet.HtlcSets { - if !htlcSetKey.IsRemote { - continue - } - - for _, htlc := range htlcs { - if htlc.Incoming { - continue - } - - remoteOutgoing[htlc.HtlcIndex] = htlc - } - } - - // Now we'll loop over the map and create ResolutionMsgs for - // each of them. - for _, htlc := range remoteOutgoing { - failMsg := ResolutionMsg{ - SourceChan: c.cfg.ShortChanID, - HtlcIndex: htlc.HtlcIndex, - Failure: failureMsg, - } - - msgsToSend = append(msgsToSend, failMsg) - } - - return htlcResolvers, msgsToSend, nil + return htlcResolvers, nil } // For each HTLC, we'll either act immediately, meaning we'll instantly @@ -2375,20 +2378,6 @@ func (c *ChannelArbitrator) prepContractResolutions( // confirmed, in which case we'll need an HTLC resolver. for htlcAction, htlcs := range htlcActions { switch htlcAction { - - // If we can fail an HTLC immediately (an outgoing HTLC with no - // contract), then we'll assemble an HTLC fail packet to send. - case HtlcFailNowAction: - for _, htlc := range htlcs { - failMsg := ResolutionMsg{ - SourceChan: c.cfg.ShortChanID, - HtlcIndex: htlc.HtlcIndex, - Failure: failureMsg, - } - - msgsToSend = append(msgsToSend, failMsg) - } - // If we can claim this HTLC, we'll create an HTLC resolver to // claim the HTLC (second-level or directly), then add the pre case HtlcClaimAction: @@ -2487,36 +2476,6 @@ func (c *ChannelArbitrator) prepContractResolutions( htlcResolvers = append(htlcResolvers, resolver) } - // We've lost an htlc because it isn't manifested on the - // commitment transaction that closed the channel. - case HtlcIncomingDustFinalAction: - for _, htlc := range htlcs { - htlc := htlc - - key := models.CircuitKey{ - ChanID: c.cfg.ShortChanID, - HtlcID: htlc.HtlcIndex, - } - - // Mark this dust htlc as final failed. - chainArbCfg := c.cfg.ChainArbitratorConfig - err := chainArbCfg.PutFinalHtlcOutcome( - key.ChanID, key.HtlcID, false, - ) - if err != nil { - return nil, nil, err - } - - // Send notification. - chainArbCfg.HtlcNotifier.NotifyFinalHtlcEvent( - key, - channeldb.FinalHtlcInfo{ - Settled: false, - Offchain: false, - }, - ) - } - // Finally, if this is an outgoing HTLC we've sent, then we'll // launch a resolver to watch for the pre-image (and settle // backwards), or just timeout. @@ -2531,9 +2490,11 @@ func (c *ChannelArbitrator) prepContractResolutions( resolution, ok := outResolutionMap[htlcOp] if !ok { - log.Errorf("ChannelArbitrator(%v) unable to find "+ - "outgoing resolution: %v", + log.Errorf("ChannelArbitrator(%v) "+ + "unable to find outgoing "+ + "resolution: %v", c.cfg.ChanPoint, htlcOp) + continue } @@ -2570,7 +2531,7 @@ func (c *ChannelArbitrator) prepContractResolutions( htlcResolvers = append(htlcResolvers, resolver) } - return htlcResolvers, msgsToSend, nil + return htlcResolvers, nil } // replaceResolver replaces a in the list of active resolvers. If the resolver @@ -3159,3 +3120,81 @@ func (c *ChannelArbitrator) checkLegacyBreach() (ArbitratorState, error) { // This is a modern breach close with resolvers. return StateContractClosed, nil } + +// failIncomingDust resolves the incoming dust HTLCs because they do not have +// an output on the commitment transaction and cannot be resolved onchain. We +// mark them as failed here. +func (c *ChannelArbitrator) failIncomingDust( + incomingDustHTLCs []channeldb.HTLC) error { + + for _, htlc := range incomingDustHTLCs { + if !htlc.Incoming || htlc.OutputIndex >= 0 { + return fmt.Errorf("htlc with index %v is not incoming "+ + "dust", htlc.OutputIndex) + } + + key := models.CircuitKey{ + ChanID: c.cfg.ShortChanID, + HtlcID: htlc.HtlcIndex, + } + + // Mark this dust htlc as final failed. + chainArbCfg := c.cfg.ChainArbitratorConfig + err := chainArbCfg.PutFinalHtlcOutcome( + key.ChanID, key.HtlcID, false, + ) + if err != nil { + return err + } + + // Send notification. + chainArbCfg.HtlcNotifier.NotifyFinalHtlcEvent( + key, + channeldb.FinalHtlcInfo{ + Settled: false, + Offchain: false, + }, + ) + } + + return nil +} + +// abandonForwards cancels back the incoming HTLCs for their corresponding +// outgoing HTLCs. We use a set here to avoid sending duplicate failure messages +// for the same HTLC. This also needs to be done for locally initiated outgoing +// HTLCs they are special cased in the switch. +func (c *ChannelArbitrator) abandonForwards(htlcs fn.Set[uint64]) error { + log.Debugf("ChannelArbitrator(%v): cancelling back %v incoming "+ + "HTLC(s)", c.cfg.ChanPoint, + len(htlcs)) + + msgsToSend := make([]ResolutionMsg, 0, len(htlcs)) + failureMsg := &lnwire.FailPermanentChannelFailure{} + + for idx := range htlcs { + failMsg := ResolutionMsg{ + SourceChan: c.cfg.ShortChanID, + HtlcIndex: idx, + Failure: failureMsg, + } + + msgsToSend = append(msgsToSend, failMsg) + } + + // Send the msges to the switch, if there are any. + if len(msgsToSend) == 0 { + return nil + } + + log.Debugf("ChannelArbitrator(%v): sending resolution message=%v", + c.cfg.ChanPoint, lnutils.SpewLogClosure(msgsToSend)) + + err := c.cfg.DeliverResolutionMsg(msgsToSend...) + if err != nil { + log.Errorf("Unable to send resolution msges to switch: %v", err) + return err + } + + return nil +} From 328a711dbe11636ee8d3311240321accd74389c8 Mon Sep 17 00:00:00 2001 From: ziggie Date: Thu, 17 Oct 2024 13:21:39 +0200 Subject: [PATCH 3/9] contractcourt: enhance chainAction type We distinguish between dangling and dust htlcs. This does not change any logic but only introduces new types to later act on them differently when we begin to fail dust htlcs earlier in a later commit. --- contractcourt/channel_arbitrator.go | 83 +++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 16 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index 1b61a100b..2bb0dbbc5 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -1256,19 +1256,28 @@ func (c *ChannelArbitrator) stateStep( return StateError, closeTx, err } - // If we can fail an HTLC immediately (an outgoing HTLC - // with no contract and it was not canceled before), - // then we'll assemble an HTLC fail packet to send. + // We can fail the upstream HTLC for an outgoing + // dangling and dust HTLC because we can be sure they + // will not be resolved onchain. getIdx := func(htlc channeldb.HTLC) uint64 { return htlc.HtlcIndex } remoteDangling := fn.NewSet(fn.Map( - getIdx, htlcActions[HtlcFailNowAction], + getIdx, htlcActions[HtlcFailDanglingAction], )...) err := c.abandonForwards(remoteDangling) if err != nil { return StateError, closeTx, err } + + dustHTLCs := fn.NewSet(fn.Map( + getIdx, htlcActions[HtlcFailDustAction], + )...) + + err = c.abandonForwards(dustHTLCs) + if err != nil { + return StateError, closeTx, err + } } // Now that we know we'll need to act, we'll process all the @@ -1716,10 +1725,11 @@ const ( // before its timeout period. HtlcClaimAction = 2 - // HtlcFailNowAction indicates that we should fail an outgoing HTLC - // immediately by cancelling it backwards as it has no corresponding - // output in our commitment transaction. - HtlcFailNowAction = 3 + // HtlcFailDustAction indicates that we should fail the upstream HTLC + // for an outgoing dust HTLC immediately (even before the commitment + // transaction is confirmed) because it has no output on the commitment + // transaction. This also includes remote pending outgoing dust HTLCs. + HtlcFailDustAction = 3 // HtlcOutgoingWatchAction indicates that we can't yet timeout this // HTLC, but we had to go to chain on order to resolve an existing @@ -1738,6 +1748,13 @@ const ( // HtlcIncomingDustFinalAction indicates that we should mark an incoming // dust htlc as final because it can't be claimed on-chain. HtlcIncomingDustFinalAction = 6 + + // HtlcFailDanglingAction indicates that we should fail the upstream + // HTLC for an outgoing HTLC immediately after the commitment + // transaction has confirmed because it has no corresponding output on + // the commitment transaction. This category does NOT include any dust + // HTLCs which are mapped in the "HtlcFailDustAction" category. + HtlcFailDanglingAction = 7 ) // String returns a human readable string describing a chain action. @@ -1752,8 +1769,8 @@ func (c ChainAction) String() string { case HtlcClaimAction: return "HtlcClaimAction" - case HtlcFailNowAction: - return "HtlcFailNowAction" + case HtlcFailDustAction: + return "HtlcFailDustAction" case HtlcOutgoingWatchAction: return "HtlcOutgoingWatchAction" @@ -1764,6 +1781,9 @@ func (c ChainAction) String() string { case HtlcIncomingDustFinalAction: return "HtlcIncomingDustFinalAction" + case HtlcFailDanglingAction: + return "HtlcFailDanglingAction" + default: return "" } @@ -1944,8 +1964,8 @@ func (c *ChannelArbitrator) checkCommitChainActions(height uint32, "failing dust htlc=%x", c.cfg.ChanPoint, htlc.RHash[:]) - actionMap[HtlcFailNowAction] = append( - actionMap[HtlcFailNowAction], htlc, + actionMap[HtlcFailDustAction] = append( + actionMap[HtlcFailDustAction], htlc, ) // If we don't need to immediately act on this HTLC, then we'll @@ -2138,12 +2158,30 @@ func (c *ChannelArbitrator) checkRemoteDanglingActions( continue } + // Dust htlcs can be canceled back even before the commitment + // transaction confirms. Dust htlcs are not enforceable onchain. + // If another version of the commit tx would confirm we either + // gain or lose those dust amounts but there is no other way + // than cancelling the incoming back because we will never learn + // the preimage. + if htlc.OutputIndex < 0 { + log.Infof("ChannelArbitrator(%v): fail dangling dust "+ + "htlc=%x from local/remote commitments diff", + c.cfg.ChanPoint, htlc.RHash[:]) + + actionMap[HtlcFailDustAction] = append( + actionMap[HtlcFailDustAction], htlc, + ) + + continue + } + log.Infof("ChannelArbitrator(%v): fail dangling htlc=%x from "+ "local/remote commitments diff", c.cfg.ChanPoint, htlc.RHash[:]) - actionMap[HtlcFailNowAction] = append( - actionMap[HtlcFailNowAction], htlc, + actionMap[HtlcFailDanglingAction] = append( + actionMap[HtlcFailDanglingAction], htlc, ) } @@ -2232,8 +2270,21 @@ func (c *ChannelArbitrator) checkRemoteDiffActions( continue } - actionMap[HtlcFailNowAction] = append( - actionMap[HtlcFailNowAction], htlc, + // Dust HTLCs on the remote commitment can be failed back. + if htlc.OutputIndex < 0 { + log.Infof("ChannelArbitrator(%v): fail dangling dust "+ + "htlc=%x from remote commitments diff", + c.cfg.ChanPoint, htlc.RHash[:]) + + actionMap[HtlcFailDustAction] = append( + actionMap[HtlcFailDustAction], htlc, + ) + + continue + } + + actionMap[HtlcFailDanglingAction] = append( + actionMap[HtlcFailDanglingAction], htlc, ) log.Infof("ChannelArbitrator(%v): fail dangling htlc=%x from "+ From 20dc7f29f83da5ae715cf5ac0dcd89fc97e9e485 Mon Sep 17 00:00:00 2001 From: ziggie Date: Thu, 17 Oct 2024 13:25:45 +0200 Subject: [PATCH 4/9] multi: fix typo and add comment. --- contractcourt/htlc_timeout_resolver.go | 14 +++++++++----- htlcswitch/switch.go | 2 +- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/contractcourt/htlc_timeout_resolver.go b/contractcourt/htlc_timeout_resolver.go index 670da607d..1c5620fc6 100644 --- a/contractcourt/htlc_timeout_resolver.go +++ b/contractcourt/htlc_timeout_resolver.go @@ -461,19 +461,23 @@ func (h *htlcTimeoutResolver) Resolve( return h.claimCleanUp(commitSpend) } - log.Infof("%T(%v): resolving htlc with incoming fail msg, fully "+ - "confirmed", h, h.htlcResolution.ClaimOutpoint) - // At this point, the second-level transaction is sufficiently // confirmed, or a transaction directly spending the output is. // Therefore, we can now send back our clean up message, failing the // HTLC on the incoming link. + // + // NOTE: This can be called twice if the outgoing resolver restarts + // before the second-stage timeout transaction is confirmed. + log.Infof("%T(%v): resolving htlc with incoming fail msg, "+ + "fully confirmed", h, h.htlcResolution.ClaimOutpoint) + failureMsg := &lnwire.FailPermanentChannelFailure{} - if err := h.DeliverResolutionMsg(ResolutionMsg{ + err = h.DeliverResolutionMsg(ResolutionMsg{ SourceChan: h.ShortChanID, HtlcIndex: h.htlc.HtlcIndex, Failure: failureMsg, - }); err != nil { + }) + if err != nil { return nil, err } diff --git a/htlcswitch/switch.go b/htlcswitch/switch.go index 10e4d37bf..cbc2a16da 100644 --- a/htlcswitch/switch.go +++ b/htlcswitch/switch.go @@ -1632,7 +1632,7 @@ out: defer s.wg.Done() if err := s.FlushForwardingEvents(); err != nil { - log.Errorf("unable to flush "+ + log.Errorf("Unable to flush "+ "forwarding events: %v", err) } }() From 8ed8665d50d2c9a40de649d7fd06b3a66e6146fa Mon Sep 17 00:00:00 2001 From: ziggie Date: Thu, 17 Oct 2024 13:29:31 +0200 Subject: [PATCH 5/9] contractcourt: Cancel dust htlcs prematurely We will now cancel dust htlcs on the local/remote commits after we decided to go onchain. This can be done because dust cannot be enforced onchain and therefore there is no way to also reveil the preimage onchain. --- contractcourt/channel_arbitrator.go | 86 ++++++++++++++++-------- contractcourt/channel_arbitrator_test.go | 34 +++++----- 2 files changed, 75 insertions(+), 45 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index 2bb0dbbc5..ce67908cd 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -932,21 +932,36 @@ func (c *ChannelArbitrator) stateStep( // arbitrating for. If a commitment has confirmed, then we'll // use the set snapshot from the chain, otherwise we'll use our // current set. - var htlcs map[HtlcSetKey]htlcSet - if confCommitSet != nil { - htlcs = confCommitSet.toActiveHTLCSets() - } else { + var ( + chainActions ChainActionMap + err error + ) + + // Normally if we force close the channel locally we will have + // no confCommitSet. However when the remote commitment confirms + // without us ever broadcasting our local commitment we need to + // make sure we cancel all upstream HTLCs for outgoing dust + // HTLCs as well hence we need to fetch the chain actions here + // as well. + if confCommitSet == nil { // Update the set of activeHTLCs so // checkLocalChainActions has an up-to-date view of the // commitments. c.updateActiveHTLCs() - htlcs = c.activeHTLCs - } - chainActions, err := c.checkLocalChainActions( - triggerHeight, trigger, htlcs, false, - ) - if err != nil { - return StateDefault, nil, err + htlcs := c.activeHTLCs + chainActions, err = c.checkLocalChainActions( + triggerHeight, trigger, htlcs, false, + ) + if err != nil { + return StateDefault, nil, err + } + } else { + chainActions, err = c.constructChainActions( + confCommitSet, triggerHeight, trigger, + ) + if err != nil { + return StateDefault, nil, err + } } // If there are no actions to be made, then we'll remain in the @@ -964,6 +979,25 @@ func (c *ChannelArbitrator) stateStep( log.Tracef("ChannelArbitrator(%v): logging chain_actions=%v", c.cfg.ChanPoint, lnutils.SpewLogClosure(chainActions)) + // Cancel upstream HTLCs for all outgoing dust HTLCs available + // either on the local or the remote/remote pending commitment + // transaction. + dustHTLCs := chainActions[HtlcFailDustAction] + if len(dustHTLCs) > 0 { + log.Debugf("ChannelArbitrator(%v): canceling %v dust "+ + "HTLCs backwards", c.cfg.ChanPoint, + len(dustHTLCs)) + + getIdx := func(htlc channeldb.HTLC) uint64 { + return htlc.HtlcIndex + } + dustHTLCSet := fn.NewSet(fn.Map(getIdx, dustHTLCs)...) + err = c.abandonForwards(dustHTLCSet) + if err != nil { + return StateError, closeTx, err + } + } + // Depending on the type of trigger, we'll either "tunnel" // through to a farther state, or just proceed linearly to the // next state. @@ -1214,18 +1248,19 @@ func (c *ChannelArbitrator) stateStep( return StateError, closeTx, err } - // In case its a breach transaction we fail back all outgoing - // HTLCs on the remote commitment set. + // In case its a breach transaction we fail back all upstream + // HTLCs for their corresponding outgoing HTLCs on the remote + // commitment set (remote and remote pending set). if contractResolutions.BreachResolution != nil { // cancelBreachedHTLCs is a set which holds HTLCs whose // corresponding incoming HTLCs will be failed back // because the peer broadcasted an old state. cancelBreachedHTLCs := fn.NewSet[uint64]() - // We'll use the CommitSet, we'll fail back all outgoing - // HTLC's that exist on either of the remote - // commitments. The map is used to deduplicate any - // shared HTLC's. + // We'll use the CommitSet, we'll fail back all + // upstream HTLCs for their corresponding outgoing + // HTLC that exist on either of the remote commitments. + // The map is used to deduplicate any shared HTLC's. for htlcSetKey, htlcs := range confCommitSet.HtlcSets { if !htlcSetKey.IsRemote { continue @@ -1256,9 +1291,11 @@ func (c *ChannelArbitrator) stateStep( return StateError, closeTx, err } - // We can fail the upstream HTLC for an outgoing - // dangling and dust HTLC because we can be sure they - // will not be resolved onchain. + // We fail the upstream HTLCs for all remote pending + // outgoing HTLCs as soon as the commitment is + // confirmed. The upstream HTLCs for outgoing dust + // HTLCs have already been resolved before we reach + // this point. getIdx := func(htlc channeldb.HTLC) uint64 { return htlc.HtlcIndex } @@ -1269,15 +1306,6 @@ func (c *ChannelArbitrator) stateStep( if err != nil { return StateError, closeTx, err } - - dustHTLCs := fn.NewSet(fn.Map( - getIdx, htlcActions[HtlcFailDustAction], - )...) - - err = c.abandonForwards(dustHTLCs) - if err != nil { - return StateError, closeTx, err - } } // Now that we know we'll need to act, we'll process all the diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index 916cd5f58..ba78139ab 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/contractcourt/channel_arbitrator_test.go @@ -931,6 +931,24 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { t.Fatalf("no response received") } + // We expect an immediate resolution message for the outgoing dust htlc. + // It is not resolvable on-chain and it can be canceled back even before + // the commitment transaction confirmed. + select { + case msgs := <-chanArbCtx.resolutions: + if len(msgs) != 1 { + t.Fatalf("expected 1 message, instead got %v", + len(msgs)) + } + + if msgs[0].HtlcIndex != outgoingDustHtlc.HtlcIndex { + t.Fatalf("wrong htlc index: expected %v, got %v", + outgoingDustHtlc.HtlcIndex, msgs[0].HtlcIndex) + } + case <-time.After(defaultTimeout): + t.Fatalf("resolution msgs not sent") + } + // Now notify about the local force close getting confirmed. closeTx := &wire.MsgTx{ TxIn: []*wire.TxIn{ @@ -993,22 +1011,6 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { StateWaitingFullResolution, ) - // We expect an immediate resolution message for the outgoing dust htlc. - // It is not resolvable on-chain. - select { - case msgs := <-chanArbCtx.resolutions: - if len(msgs) != 1 { - t.Fatalf("expected 1 message, instead got %v", len(msgs)) - } - - if msgs[0].HtlcIndex != outgoingDustHtlc.HtlcIndex { - t.Fatalf("wrong htlc index: expected %v, got %v", - outgoingDustHtlc.HtlcIndex, msgs[0].HtlcIndex) - } - case <-time.After(defaultTimeout): - t.Fatalf("resolution msgs not sent") - } - // We'll grab the old notifier here as our resolvers are still holding // a reference to this instance, and a new one will be created when we // restart the channel arb below. From 7aa9ade4c280894921a639c5beca19280f1b4607 Mon Sep 17 00:00:00 2001 From: ziggie Date: Thu, 17 Oct 2024 13:24:24 +0200 Subject: [PATCH 6/9] contratcourt: dont consider dust htlc for anchor sweep Now that we cancel dust htlcs prematurely even before the commitment tx is confirmed we don't consider dust htlcs when creating the cpfp transaction. --- contractcourt/channel_arbitrator.go | 323 ++++++++++++++--------- contractcourt/channel_arbitrator_test.go | 116 +++++++- 2 files changed, 312 insertions(+), 127 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index ce67908cd..a76fcd330 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -1390,133 +1390,22 @@ func (c *ChannelArbitrator) stateStep( func (c *ChannelArbitrator) sweepAnchors(anchors *lnwallet.AnchorResolutions, heightHint uint32) error { - // Use the chan id as the exclusive group. This prevents any of the - // anchors from being batched together. - exclusiveGroup := c.cfg.ShortChanID.ToUint64() - - // sweepWithDeadline is a helper closure that takes an anchor - // resolution and sweeps it with its corresponding deadline. - sweepWithDeadline := func(anchor *lnwallet.AnchorResolution, - htlcs htlcSet, anchorPath string) error { - - // Find the deadline for this specific anchor. - deadline, value, err := c.findCommitmentDeadlineAndValue( - heightHint, htlcs, - ) - if err != nil { - return err - } - - // If we cannot find a deadline, it means there's no HTLCs at - // stake, which means we can relax our anchor sweeping - // conditions as we don't have any time sensitive outputs to - // sweep. However we need to register the anchor output with the - // sweeper so we are later able to bump the close fee. - if deadline.IsNone() { - log.Infof("ChannelArbitrator(%v): no HTLCs at stake, "+ - "sweeping anchor with default deadline", - c.cfg.ChanPoint) - } - - witnessType := input.CommitmentAnchor - - // For taproot channels, we need to use the proper witness - // type. - if txscript.IsPayToTaproot( - anchor.AnchorSignDescriptor.Output.PkScript, - ) { - - witnessType = input.TaprootAnchorSweepSpend - } - - // Prepare anchor output for sweeping. - anchorInput := input.MakeBaseInput( - &anchor.CommitAnchor, - witnessType, - &anchor.AnchorSignDescriptor, - heightHint, - &input.TxInfo{ - Fee: anchor.CommitFee, - Weight: anchor.CommitWeight, - }, - ) - - // If we have a deadline, we'll use it to calculate the - // deadline height, otherwise default to none. - deadlineDesc := "None" - deadlineHeight := fn.MapOption(func(d int32) int32 { - deadlineDesc = fmt.Sprintf("%d", d) - - return d + int32(heightHint) - })(deadline) - - // Calculate the budget based on the value under protection, - // which is the sum of all HTLCs on this commitment subtracted - // by their budgets. - // The anchor output in itself has a small output value of 330 - // sats so we also include it in the budget to pay for the - // cpfp transaction. - budget := calculateBudget( - value, c.cfg.Budget.AnchorCPFPRatio, - c.cfg.Budget.AnchorCPFP, - ) + AnchorOutputValue - - log.Infof("ChannelArbitrator(%v): offering anchor from %s "+ - "commitment %v to sweeper with deadline=%v, budget=%v", - c.cfg.ChanPoint, anchorPath, anchor.CommitAnchor, - deadlineDesc, budget) - - // Sweep anchor output with a confirmation target fee - // preference. Because this is a cpfp-operation, the anchor - // will only be attempted to sweep when the current fee - // estimate for the confirmation target exceeds the commit fee - // rate. - _, err = c.cfg.Sweeper.SweepInput( - &anchorInput, - sweep.Params{ - ExclusiveGroup: &exclusiveGroup, - Budget: budget, - DeadlineHeight: deadlineHeight, - }, - ) - if err != nil { - return err - } - - return nil - } - // Update the set of activeHTLCs so that the sweeping routine has an // up-to-date view of the set of commitments. c.updateActiveHTLCs() - // Sweep anchors based on different HTLC sets. Notice the HTLC sets may - // differ across commitments, thus their deadline values could vary. - for htlcSet, htlcs := range c.activeHTLCs { - switch { - case htlcSet == LocalHtlcSet && anchors.Local != nil: - err := sweepWithDeadline(anchors.Local, htlcs, "local") - if err != nil { - return err - } + // Prepare the sweeping requests for all possible versions of + // commitments. + sweepReqs, err := c.prepareAnchorSweeps(heightHint, anchors) + if err != nil { + return err + } - case htlcSet == RemoteHtlcSet && anchors.Remote != nil: - err := sweepWithDeadline( - anchors.Remote, htlcs, "remote", - ) - if err != nil { - return err - } - - case htlcSet == RemotePendingHtlcSet && - anchors.RemotePending != nil: - - err := sweepWithDeadline( - anchors.RemotePending, htlcs, "remote pending", - ) - if err != nil { - return err - } + // Send out the sweeping requests to the sweeper. + for _, req := range sweepReqs { + _, err = c.cfg.Sweeper.SweepInput(req.input, req.params) + if err != nil { + return err } } @@ -2254,7 +2143,7 @@ func (c *ChannelArbitrator) checkRemoteChainActions( } // checkRemoteDiffActions checks the set difference of the HTLCs on the remote -// confirmed commit and remote dangling commit for HTLCS that we need to cancel +// confirmed commit and remote pending commit for HTLCS that we need to cancel // back. If we find any HTLCs on the remote pending but not the remote, then // we'll mark them to be failed immediately. func (c *ChannelArbitrator) checkRemoteDiffActions( @@ -2277,7 +2166,7 @@ func (c *ChannelArbitrator) checkRemoteDiffActions( } // With the remote HTLCs assembled, we'll mark any HTLCs only on the - // remote dangling commitment to be failed asap. + // remote pending commitment to be failed asap. actionMap := make(ChainActionMap) for _, htlc := range danglingHTLCs.outgoingHTLCs { if _, ok := remoteHtlcs[htlc.HtlcIndex]; ok { @@ -3200,6 +3089,192 @@ func (c *ChannelArbitrator) checkLegacyBreach() (ArbitratorState, error) { return StateContractClosed, nil } +// sweepRequest wraps the arguments used when calling `SweepInput`. +type sweepRequest struct { + // input is the input to be swept. + input input.Input + + // params holds the sweeping parameters. + params sweep.Params +} + +// createSweepRequest creates an anchor sweeping request for a particular +// version (local/remote/remote pending) of the commitment. +func (c *ChannelArbitrator) createSweepRequest( + anchor *lnwallet.AnchorResolution, htlcs htlcSet, anchorPath string, + heightHint uint32) (sweepRequest, error) { + + // Use the chan id as the exclusive group. This prevents any of the + // anchors from being batched together. + exclusiveGroup := c.cfg.ShortChanID.ToUint64() + + // Find the deadline for this specific anchor. + deadline, value, err := c.findCommitmentDeadlineAndValue( + heightHint, htlcs, + ) + if err != nil { + return sweepRequest{}, err + } + + // If we cannot find a deadline, it means there's no HTLCs at stake, + // which means we can relax our anchor sweeping conditions as we don't + // have any time sensitive outputs to sweep. However we need to + // register the anchor output with the sweeper so we are later able to + // bump the close fee. + if deadline.IsNone() { + log.Infof("ChannelArbitrator(%v): no HTLCs at stake, "+ + "sweeping anchor with default deadline", + c.cfg.ChanPoint) + } + + witnessType := input.CommitmentAnchor + + // For taproot channels, we need to use the proper witness type. + if txscript.IsPayToTaproot( + anchor.AnchorSignDescriptor.Output.PkScript, + ) { + + witnessType = input.TaprootAnchorSweepSpend + } + + // Prepare anchor output for sweeping. + anchorInput := input.MakeBaseInput( + &anchor.CommitAnchor, + witnessType, + &anchor.AnchorSignDescriptor, + heightHint, + &input.TxInfo{ + Fee: anchor.CommitFee, + Weight: anchor.CommitWeight, + }, + ) + + // If we have a deadline, we'll use it to calculate the deadline + // height, otherwise default to none. + deadlineDesc := "None" + deadlineHeight := fn.MapOption(func(d int32) int32 { + deadlineDesc = fmt.Sprintf("%d", d) + + return d + int32(heightHint) + })(deadline) + + // Calculate the budget based on the value under protection, which is + // the sum of all HTLCs on this commitment subtracted by their budgets. + // The anchor output in itself has a small output value of 330 sats so + // we also include it in the budget to pay for the cpfp transaction. + budget := calculateBudget( + value, c.cfg.Budget.AnchorCPFPRatio, c.cfg.Budget.AnchorCPFP, + ) + AnchorOutputValue + + log.Infof("ChannelArbitrator(%v): offering anchor from %s commitment "+ + "%v to sweeper with deadline=%v, budget=%v", c.cfg.ChanPoint, + anchorPath, anchor.CommitAnchor, deadlineDesc, budget) + + // Sweep anchor output with a confirmation target fee preference. + // Because this is a cpfp-operation, the anchor will only be attempted + // to sweep when the current fee estimate for the confirmation target + // exceeds the commit fee rate. + return sweepRequest{ + input: &anchorInput, + params: sweep.Params{ + ExclusiveGroup: &exclusiveGroup, + Budget: budget, + DeadlineHeight: deadlineHeight, + }, + }, nil +} + +// prepareAnchorSweeps creates a list of requests to be used by the sweeper for +// all possible commitment versions. +func (c *ChannelArbitrator) prepareAnchorSweeps(heightHint uint32, + anchors *lnwallet.AnchorResolutions) ([]sweepRequest, error) { + + // requests holds all the possible anchor sweep requests. We can have + // up to 3 different versions of commitments (local/remote/remote + // dangling) to be CPFPed by the anchors. + requests := make([]sweepRequest, 0, 3) + + // remotePendingReq holds the request for sweeping the anchor output on + // the remote pending commitment. It's only set when there's an actual + // pending remote commitment and it's used to decide whether we need to + // update the fee budget when sweeping the anchor output on the local + // commitment. + remotePendingReq := fn.None[sweepRequest]() + + // First we check on the remote pending commitment and optionally + // create an anchor sweeping request. + htlcs, ok := c.activeHTLCs[RemotePendingHtlcSet] + if ok && anchors.RemotePending != nil { + req, err := c.createSweepRequest( + anchors.RemotePending, htlcs, "remote pending", + heightHint, + ) + if err != nil { + return nil, err + } + + // Save the request. + requests = append(requests, req) + + // Set the optional variable. + remotePendingReq = fn.Some(req) + } + + // Check the local commitment and optionally create an anchor sweeping + // request. The params used in this request will be influenced by the + // anchor sweeping request made from the pending remote commitment. + htlcs, ok = c.activeHTLCs[LocalHtlcSet] + if ok && anchors.Local != nil { + req, err := c.createSweepRequest( + anchors.Local, htlcs, "local", heightHint, + ) + if err != nil { + return nil, err + } + + // If there's an anchor sweeping request from the pending + // remote commitment, we will compare its budget against the + // budget used here and choose the params that has a larger + // budget. The deadline when choosing the remote pending budget + // instead of the local one will always be earlier or equal to + // the local deadline because outgoing HTLCs are resolved on + // the local commitment first before they are removed from the + // remote one. + remotePendingReq.WhenSome(func(s sweepRequest) { + if s.params.Budget <= req.params.Budget { + return + } + + log.Infof("ChannelArbitrator(%v): replaced local "+ + "anchor(%v) sweep params with pending remote "+ + "anchor sweep params, \nold:[%v], \nnew:[%v]", + c.cfg.ChanPoint, anchors.Local.CommitAnchor, + req.params, s.params) + + req.params = s.params + }) + + // Save the request. + requests = append(requests, req) + } + + // Check the remote commitment and create an anchor sweeping request if + // needed. + htlcs, ok = c.activeHTLCs[RemoteHtlcSet] + if ok && anchors.Remote != nil { + req, err := c.createSweepRequest( + anchors.Remote, htlcs, "remote", heightHint, + ) + if err != nil { + return nil, err + } + + requests = append(requests, req) + } + + return requests, nil +} + // failIncomingDust resolves the incoming dust HTLCs because they do not have // an output on the commitment transaction and cannot be resolved onchain. We // mark them as failed here. diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index ba78139ab..441c36907 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/contractcourt/channel_arbitrator_test.go @@ -2256,7 +2256,7 @@ func TestFindCommitmentDeadlineAndValue(t *testing.T) { mockPreimageDB := newMockWitnessBeacon() mockPreimageDB.lookupPreimage[rHash] = rHash - // Attack a mock PreimageDB and Registry to channel arbitrator. + // Attach a mock PreimageDB and Registry to channel arbitrator. chanArb := chanArbCtx.chanArb chanArb.cfg.PreimageDB = mockPreimageDB chanArb.cfg.Registry = &mockRegistry{} @@ -2447,7 +2447,7 @@ func TestSweepAnchors(t *testing.T) { mockPreimageDB := newMockWitnessBeacon() mockPreimageDB.lookupPreimage[rHash] = rHash - // Attack a mock PreimageDB and Registry to channel arbitrator. + // Attach a mock PreimageDB and Registry to channel arbitrator. chanArb := chanArbCtx.chanArb chanArb.cfg.PreimageDB = mockPreimageDB chanArb.cfg.Registry = &mockRegistry{} @@ -2598,6 +2598,116 @@ func TestSweepAnchors(t *testing.T) { ) } +// TestSweepLocalAnchor checks the sweep params used for the local anchor will +// be updated optionally based on the pending remote commit. +func TestSweepLocalAnchor(t *testing.T) { + // Create a testing channel arbitrator. + log := &mockArbitratorLog{ + state: StateDefault, + newStates: make(chan ArbitratorState, 5), + } + chanArbCtx, err := createTestChannelArbitrator(t, log) + require.NoError(t, err, "unable to create ChannelArbitrator") + + // Attach a mock PreimageDB and Registry to channel arbitrator. + chanArb := chanArbCtx.chanArb + mockPreimageDB := newMockWitnessBeacon() + chanArb.cfg.PreimageDB = mockPreimageDB + chanArb.cfg.Registry = &mockRegistry{} + + // Set current block height. + heightHint := uint32(1000) + chanArbCtx.chanArb.blocks <- int32(heightHint) + + htlcIndex := uint64(99) + deadlineDelta := uint32(10) + + htlcAmt := lnwire.MilliSatoshi(1_000_000) + + // Create one testing HTLC. + deadlineSmallDelta := deadlineDelta + 4 + htlcSmallExipry := channeldb.HTLC{ + HtlcIndex: htlcIndex, + RefundTimeout: heightHint + deadlineSmallDelta, + Amt: htlcAmt, + } + + // Setup our local HTLC set such that it doesn't have any HTLCs. We + // expect an anchor sweeping request to be made using the params + // created from sweeping the anchor from the pending remote commit. + chanArb.activeHTLCs[LocalHtlcSet] = htlcSet{} + + // Setup our remote HTLC set such that no valid HTLCs can be used, thus + // the anchor sweeping is skipped. + chanArb.activeHTLCs[RemoteHtlcSet] = htlcSet{} + + // Setup out pending remote HTLC set such that we will use the HTLC's + // CLTV from the outgoing HTLC set. + // Only half of the deadline is used since the anchor cpfp sweep. The + // other half of the deadline is used to sweep the HTLCs at stake. + expectedPendingDeadline := heightHint + deadlineSmallDelta/2 + chanArb.activeHTLCs[RemotePendingHtlcSet] = htlcSet{ + outgoingHTLCs: map[uint64]channeldb.HTLC{ + htlcSmallExipry.HtlcIndex: htlcSmallExipry, + }, + } + + // Mock FindOutgoingHTLCDeadline so the pending remote's outgoing HTLC + // returns the small expiry value. + chanArb.cfg.FindOutgoingHTLCDeadline = func( + htlc channeldb.HTLC) fn.Option[int32] { + + if htlc.RHash != htlcSmallExipry.RHash { + return fn.None[int32]() + } + + return fn.Some(int32(htlcSmallExipry.RefundTimeout)) + } + + // Create AnchorResolutions. + anchors := &lnwallet.AnchorResolutions{ + Local: &lnwallet.AnchorResolution{ + AnchorSignDescriptor: input.SignDescriptor{ + Output: &wire.TxOut{Value: 1}, + }, + }, + Remote: &lnwallet.AnchorResolution{ + AnchorSignDescriptor: input.SignDescriptor{ + Output: &wire.TxOut{Value: 1}, + }, + }, + RemotePending: &lnwallet.AnchorResolution{ + AnchorSignDescriptor: input.SignDescriptor{ + Output: &wire.TxOut{Value: 1}, + }, + }, + } + + // Sweep anchors and check there's no error. + err = chanArb.sweepAnchors(anchors, heightHint) + require.NoError(t, err) + + // Verify deadlines are used as expected. + deadlines := chanArbCtx.sweeper.deadlines + + // We should see two `SweepInput` calls - one for sweeping the local + // anchor, the other from the remote pending anchor. + require.Len(t, deadlines, 2) + + // Both deadlines should be the same since the local anchor uses the + // parameters from the pending remote commitment. + require.EqualValues( + t, expectedPendingDeadline, deadlines[0], + "local deadline not matched, want %v, got %v", + expectedPendingDeadline, deadlines[0], + ) + require.EqualValues( + t, expectedPendingDeadline, deadlines[1], + "pending remote deadline not matched, want %v, got %v", + expectedPendingDeadline, deadlines[1], + ) +} + // TestChannelArbitratorAnchors asserts that the commitment tx anchor is swept. func TestChannelArbitratorAnchors(t *testing.T) { log := &mockArbitratorLog{ @@ -2621,7 +2731,7 @@ func TestChannelArbitratorAnchors(t *testing.T) { mockPreimageDB := newMockWitnessBeacon() mockPreimageDB.lookupPreimage[rHash] = rHash - // Attack a mock PreimageDB and Registry to channel arbitrator. + // Attach a mock PreimageDB and Registry to channel arbitrator. chanArb := chanArbCtx.chanArb chanArb.cfg.PreimageDB = mockPreimageDB chanArb.cfg.Registry = &mockRegistry{} From 50d6864137d4efb4408618bcd78d4475189c37e5 Mon Sep 17 00:00:00 2001 From: ziggie Date: Thu, 17 Oct 2024 13:30:28 +0200 Subject: [PATCH 7/9] itest: adopt itests for the new behavior Now outgoing dust-htlcs are canceled back before the commitment is confirmed onchain. --- itest/lnd_multi-hop_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/itest/lnd_multi-hop_test.go b/itest/lnd_multi-hop_test.go index 67dcf9d16..f08a748c3 100644 --- a/itest/lnd_multi-hop_test.go +++ b/itest/lnd_multi-hop_test.go @@ -250,6 +250,10 @@ func runMultiHopHtlcLocalTimeout(ht *lntest.HarnessTest, op := ht.OutPointFromChannelPoint(bobChanPoint) closeTx := ht.AssertOutpointInMempool(op) + // Dust HTLCs are immediately canceled backwards as soon as the local + // commitment tx is successfully broadcasted to the local mempool. + ht.AssertActiveHtlcs(alice, payHash) + // Bob's anchor output should be offered to his sweep since Bob has // time-sensitive HTLCs - we expect both anchors are offered. ht.AssertNumPendingSweeps(bob, 2) @@ -257,11 +261,6 @@ func runMultiHopHtlcLocalTimeout(ht *lntest.HarnessTest, // Mine a block to confirm the closing transaction. ht.MineBlocksAndAssertNumTxes(1, 1) - // At this point, Bob should have canceled backwards the dust HTLC - // that we sent earlier. This means Alice should now only have a single - // HTLC on her channel. - ht.AssertActiveHtlcs(alice, payHash) - // With the closing transaction confirmed, we should expect Bob's HTLC // timeout transaction to be offered to the sweeper due to the expiry // being reached. we also expect Bon and Carol's anchor sweeps. From d5e8df7067e035fd8a5881ea0695adc4de26c792 Mon Sep 17 00:00:00 2001 From: ziggie Date: Thu, 17 Oct 2024 13:26:44 +0200 Subject: [PATCH 8/9] docs: add release-notes for LND 19 --- docs/release-notes/release-notes-0.19.0.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/release-notes/release-notes-0.19.0.md b/docs/release-notes/release-notes-0.19.0.md index 2bbcc4d58..31f5c603a 100644 --- a/docs/release-notes/release-notes-0.19.0.md +++ b/docs/release-notes/release-notes-0.19.0.md @@ -39,6 +39,11 @@ * [Fix a bug](https://github.com/lightningnetwork/lnd/pull/9137) that prevented a graceful shutdown of LND during the main chain backend sync check in certain cases. + +* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/9068) where dust + htlcs although not being able to be resolved onchain were not canceled + back before the commitment tx was confirmed causing potentially force closes + of the incoming channel. # New Features ## Functional Enhancements From 59f32682a5162d71db0c3c6bce67e81e13a06578 Mon Sep 17 00:00:00 2001 From: ziggie Date: Mon, 28 Oct 2024 12:46:31 +0100 Subject: [PATCH 9/9] contractcourt: introduce option for commitKey. --- contractcourt/briefcase.go | 29 +++++++++++++++--------- contractcourt/briefcase_test.go | 3 ++- contractcourt/chain_watcher.go | 16 ++++++------- contractcourt/channel_arbitrator.go | 20 ++++++++++++---- contractcourt/channel_arbitrator_test.go | 18 +++++++++------ 5 files changed, 55 insertions(+), 31 deletions(-) diff --git a/contractcourt/briefcase.go b/contractcourt/briefcase.go index 26df50b30..acc49be44 100644 --- a/contractcourt/briefcase.go +++ b/contractcourt/briefcase.go @@ -1475,16 +1475,23 @@ func decodeBreachResolution(r io.Reader, b *BreachResolution) error { return binary.Read(r, endian, &b.FundingOutPoint.Index) } -func encodeHtlcSetKey(w io.Writer, h *HtlcSetKey) error { - err := binary.Write(w, endian, h.IsRemote) +func encodeHtlcSetKey(w io.Writer, htlcSetKey HtlcSetKey) error { + err := binary.Write(w, endian, htlcSetKey.IsRemote) if err != nil { return err } - return binary.Write(w, endian, h.IsPending) + + return binary.Write(w, endian, htlcSetKey.IsPending) } func encodeCommitSet(w io.Writer, c *CommitSet) error { - if err := encodeHtlcSetKey(w, c.ConfCommitKey); err != nil { + confCommitKey, err := c.ConfCommitKey.UnwrapOrErr( + fmt.Errorf("HtlcSetKey is not set"), + ) + if err != nil { + return err + } + if err := encodeHtlcSetKey(w, confCommitKey); err != nil { return err } @@ -1494,8 +1501,7 @@ func encodeCommitSet(w io.Writer, c *CommitSet) error { } for htlcSetKey, htlcs := range c.HtlcSets { - htlcSetKey := htlcSetKey - if err := encodeHtlcSetKey(w, &htlcSetKey); err != nil { + if err := encodeHtlcSetKey(w, htlcSetKey); err != nil { return err } @@ -1517,13 +1523,14 @@ func decodeHtlcSetKey(r io.Reader, h *HtlcSetKey) error { } func decodeCommitSet(r io.Reader) (*CommitSet, error) { - c := &CommitSet{ - ConfCommitKey: &HtlcSetKey{}, - HtlcSets: make(map[HtlcSetKey][]channeldb.HTLC), + confCommitKey := HtlcSetKey{} + if err := decodeHtlcSetKey(r, &confCommitKey); err != nil { + return nil, err } - if err := decodeHtlcSetKey(r, c.ConfCommitKey); err != nil { - return nil, err + c := &CommitSet{ + ConfCommitKey: fn.Some(confCommitKey), + HtlcSets: make(map[HtlcSetKey][]channeldb.HTLC), } var numSets uint8 diff --git a/contractcourt/briefcase_test.go b/contractcourt/briefcase_test.go index 89e017fd7..0f44db2ab 100644 --- a/contractcourt/briefcase_test.go +++ b/contractcourt/briefcase_test.go @@ -14,6 +14,7 @@ import ( "github.com/btcsuite/btcd/wire" "github.com/davecgh/go-spew/spew" "github.com/lightningnetwork/lnd/channeldb" + "github.com/lightningnetwork/lnd/fn" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/kvdb" "github.com/lightningnetwork/lnd/lnmock" @@ -753,7 +754,7 @@ func TestCommitSetStorage(t *testing.T) { for _, pendingRemote := range []bool{true, false} { for _, confType := range confTypes { commitSet := &CommitSet{ - ConfCommitKey: &confType, + ConfCommitKey: fn.Some(confType), HtlcSets: make(map[HtlcSetKey][]channeldb.HTLC), } commitSet.HtlcSets[LocalHtlcSet] = activeHTLCs diff --git a/contractcourt/chain_watcher.go b/contractcourt/chain_watcher.go index ef4a4e200..f25ca7e0a 100644 --- a/contractcourt/chain_watcher.go +++ b/contractcourt/chain_watcher.go @@ -93,9 +93,9 @@ type BreachCloseInfo struct { // HTLCs to determine if any additional actions need to be made based on the // remote party's commitments. type CommitSet struct { - // ConfCommitKey if non-nil, identifies the commitment that was + // When the ConfCommitKey is set, it signals that the commitment tx was // confirmed in the chain. - ConfCommitKey *HtlcSetKey + ConfCommitKey fn.Option[HtlcSetKey] // HtlcSets stores the set of all known active HTLC for each active // commitment at the time of channel closure. @@ -509,7 +509,7 @@ func (c *chainWatcher) handleUnknownLocalState( // If this is our commitment transaction, then we try to act even // though we won't be able to sweep HTLCs. - chainSet.commitSet.ConfCommitKey = &LocalHtlcSet + chainSet.commitSet.ConfCommitKey = fn.Some(LocalHtlcSet) if err := c.dispatchLocalForceClose( commitSpend, broadcastStateNum, chainSet.commitSet, ); err != nil { @@ -806,7 +806,7 @@ func (c *chainWatcher) handleKnownLocalState( return false, nil } - chainSet.commitSet.ConfCommitKey = &LocalHtlcSet + chainSet.commitSet.ConfCommitKey = fn.Some(LocalHtlcSet) if err := c.dispatchLocalForceClose( commitSpend, broadcastStateNum, chainSet.commitSet, ); err != nil { @@ -844,7 +844,7 @@ func (c *chainWatcher) handleKnownRemoteState( log.Infof("Remote party broadcast base set, "+ "commit_num=%v", chainSet.remoteStateNum) - chainSet.commitSet.ConfCommitKey = &RemoteHtlcSet + chainSet.commitSet.ConfCommitKey = fn.Some(RemoteHtlcSet) err := c.dispatchRemoteForceClose( commitSpend, chainSet.remoteCommit, chainSet.commitSet, @@ -869,7 +869,7 @@ func (c *chainWatcher) handleKnownRemoteState( log.Infof("Remote party broadcast pending set, "+ "commit_num=%v", chainSet.remoteStateNum+1) - chainSet.commitSet.ConfCommitKey = &RemotePendingHtlcSet + chainSet.commitSet.ConfCommitKey = fn.Some(RemotePendingHtlcSet) err := c.dispatchRemoteForceClose( commitSpend, *chainSet.remotePendingCommit, chainSet.commitSet, @@ -936,7 +936,7 @@ func (c *chainWatcher) handlePossibleBreach(commitSpend *chainntnfs.SpendDetail, // only used to ensure a nil-pointer-dereference doesn't occur and is // not used otherwise. The HTLC's may not exist for the // RemotePendingHtlcSet. - chainSet.commitSet.ConfCommitKey = &RemoteHtlcSet + chainSet.commitSet.ConfCommitKey = fn.Some(RemoteHtlcSet) // THEY'RE ATTEMPTING TO VIOLATE THE CONTRACT LAID OUT WITHIN THE // PAYMENT CHANNEL. Therefore we close the signal indicating a revoked @@ -997,7 +997,7 @@ func (c *chainWatcher) handleUnknownRemoteState( // means we won't be able to recover any HTLC funds. // // TODO(halseth): can we try to recover some HTLCs? - chainSet.commitSet.ConfCommitKey = &RemoteHtlcSet + chainSet.commitSet.ConfCommitKey = fn.Some(RemoteHtlcSet) err := c.dispatchRemoteForceClose( commitSpend, channeldb.ChannelCommitment{}, chainSet.commitSet, commitPoint, diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index a76fcd330..cc1ee6958 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -682,8 +682,14 @@ func (c *ChannelArbitrator) relaunchResolvers(commitSet *CommitSet, // chain actions may exclude some information, but we cannot recover it // for these older nodes at the moment. var confirmedHTLCs []channeldb.HTLC - if commitSet != nil && commitSet.ConfCommitKey != nil { - confirmedHTLCs = commitSet.HtlcSets[*commitSet.ConfCommitKey] + if commitSet != nil && commitSet.ConfCommitKey.IsSome() { + confCommitKey, err := commitSet.ConfCommitKey.UnwrapOrErr( + fmt.Errorf("no commitKey available"), + ) + if err != nil { + return err + } + confirmedHTLCs = commitSet.HtlcSets[confCommitKey] } else { chainActions, err := c.log.FetchChainActions() if err != nil { @@ -2223,15 +2229,21 @@ func (c *ChannelArbitrator) constructChainActions(confCommitSet *CommitSet, // then this is an older node that had a pending close channel before // the CommitSet was introduced. In this case, we'll just return the // existing ChainActionMap they had on disk. - if confCommitSet == nil || confCommitSet.ConfCommitKey == nil { + if confCommitSet == nil || confCommitSet.ConfCommitKey.IsNone() { return c.log.FetchChainActions() } // Otherwise, we have the full commitment set written to disk, and can // proceed as normal. htlcSets := confCommitSet.toActiveHTLCSets() - switch *confCommitSet.ConfCommitKey { + confCommitKey, err := confCommitSet.ConfCommitKey.UnwrapOrErr( + fmt.Errorf("no commitKey available"), + ) + if err != nil { + return nil, err + } + switch confCommitKey { // If the local commitment transaction confirmed, then we'll examine // that as well as their commitments to the set of chain actions. case LocalHtlcSet: diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index 441c36907..1353770d8 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/contractcourt/channel_arbitrator_test.go @@ -590,7 +590,7 @@ func TestChannelArbitratorRemoteForceClose(t *testing.T) { chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- &RemoteUnilateralCloseInfo{ UnilateralCloseSummary: uniClose, CommitSet: CommitSet{ - ConfCommitKey: &RemoteHtlcSet, + ConfCommitKey: fn.Some(RemoteHtlcSet), HtlcSets: make(map[HtlcSetKey][]channeldb.HTLC), }, } @@ -777,7 +777,7 @@ func TestChannelArbitratorBreachClose(t *testing.T) { }, AnchorResolution: anchorRes, CommitSet: CommitSet{ - ConfCommitKey: &RemoteHtlcSet, + ConfCommitKey: fn.Some(RemoteHtlcSet), HtlcSets: map[HtlcSetKey][]channeldb.HTLC{ RemoteHtlcSet: {htlc1, htlc2}, }, @@ -999,7 +999,7 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { }, ChannelCloseSummary: &channeldb.ChannelCloseSummary{}, CommitSet: CommitSet{ - ConfCommitKey: &LocalHtlcSet, + ConfCommitKey: fn.Some(LocalHtlcSet), HtlcSets: map[HtlcSetKey][]channeldb.HTLC{ LocalHtlcSet: htlcSet, }, @@ -1527,7 +1527,7 @@ func TestChannelArbitratorForceCloseBreachedChannel(t *testing.T) { }, } log.commitSet = &CommitSet{ - ConfCommitKey: &RemoteHtlcSet, + ConfCommitKey: fn.Some(RemoteHtlcSet), HtlcSets: map[HtlcSetKey][]channeldb.HTLC{ RemoteHtlcSet: {}, }, @@ -1954,8 +1954,12 @@ func TestChannelArbitratorDanglingCommitForceClose(t *testing.T) { }, ChannelCloseSummary: &channeldb.ChannelCloseSummary{}, CommitSet: CommitSet{ - ConfCommitKey: &testCase.confCommit, - HtlcSets: make(map[HtlcSetKey][]channeldb.HTLC), + ConfCommitKey: fn.Some( + testCase.confCommit, + ), + HtlcSets: make( + map[HtlcSetKey][]channeldb.HTLC, + ), }, } @@ -2875,7 +2879,7 @@ func TestChannelArbitratorAnchors(t *testing.T) { }, ChannelCloseSummary: &channeldb.ChannelCloseSummary{}, CommitSet: CommitSet{ - ConfCommitKey: &LocalHtlcSet, + ConfCommitKey: fn.Some(LocalHtlcSet), HtlcSets: map[HtlcSetKey][]channeldb.HTLC{}, }, }