From 5f0fad85be963a5c2401b9b5318dd43f01a4aeef Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 16 May 2019 17:23:26 -0700 Subject: [PATCH 01/11] multi: address lingering TODO by no longer wiping out local HTLCs on remote close In this commit, we fix a lingering TOOD statement in the channel arb. Before this commitment, we would simply wipe our our local HTLC set of the HTLC set that was on the remote commitment transaction on force close. This was incorrect as if our commitment transaction had an HTLC that the remote commitment didn't, then we would fail to cancel that back, and cause both channels to time out on chain. In order to remedy this, we introduce a new `HtlcSetKey` struct to track all 3 possible in-flight set of HTLCs: ours, theirs, and their pending. We also we start to tack on additional data to all the unilateral close messages we send to subscribers. This new data is the CommitSet, or the set of valid commitments at channel closure time. This new information will be used by the channel arb in an upcoming commit to ensure it will cancel back HTLCs in the case of split commitment state. Finally, we start to thread through an optional *CommitSet to the advanceState method. This additional information will give the channel arb addition information it needs to ensure it properly cancels back HTLCs that are about to time out or may time out depending on which commitment is played. Within the htlcswitch pakage, we modify the `SignNextCommitment` method to return the new set of pending HTLCs for the remote party's commitment transaction and `ReceiveRevocation` to return the latest set of commitment transactions on the remote party's commitment as well. This is a preparatory change which is part of a larger change to address a lingering TODO in the cnct. Additionally, rather than just send of the set of HTLCs after the we revoke, we'll also send of the set of HTLCs after the remote party revokes, and we create a pending commitment state for it. --- breacharbiter_test.go | 10 +- contractcourt/chain_arbitrator.go | 39 ++++- contractcourt/chain_watcher.go | 138 ++++++++++++---- contractcourt/chain_watcher_test.go | 8 +- contractcourt/channel_arbitrator.go | 113 ++++++++----- contractcourt/channel_arbitrator_test.go | 73 ++++++--- htlcswitch/link.go | 39 ++++- htlcswitch/link_test.go | 16 +- lnwallet/channel.go | 45 +++--- lnwallet/channel_test.go | 197 ++++++++++++----------- lnwallet/test_utils.go | 8 +- 11 files changed, 449 insertions(+), 237 deletions(-) diff --git a/breacharbiter_test.go b/breacharbiter_test.go index 3d4ceb73..d9738409 100644 --- a/breacharbiter_test.go +++ b/breacharbiter_test.go @@ -1946,7 +1946,7 @@ func createHTLC(data int, amount lnwire.MilliSatoshi) (*lnwire.UpdateAddHTLC, [3 // pending updates. // TODO(conner) remove code duplication func forceStateTransition(chanA, chanB *lnwallet.LightningChannel) error { - aliceSig, aliceHtlcSigs, err := chanA.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := chanA.SignNextCommitment() if err != nil { return err } @@ -1958,12 +1958,13 @@ func forceStateTransition(chanA, chanB *lnwallet.LightningChannel) error { if err != nil { return err } - bobSig, bobHtlcSigs, err := chanB.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := chanB.SignNextCommitment() if err != nil { return err } - if _, _, _, err := chanA.ReceiveRevocation(bobRevocation); err != nil { + _, _, _, _, err = chanA.ReceiveRevocation(bobRevocation) + if err != nil { return err } if err := chanA.ReceiveNewCommitment(bobSig, bobHtlcSigs); err != nil { @@ -1974,7 +1975,8 @@ func forceStateTransition(chanA, chanB *lnwallet.LightningChannel) error { if err != nil { return err } - if _, _, _, err := chanB.ReceiveRevocation(aliceRevocation); err != nil { + _, _, _, _, err = chanB.ReceiveRevocation(aliceRevocation) + if err != nil { return err } diff --git a/contractcourt/chain_arbitrator.go b/contractcourt/chain_arbitrator.go index 712dcc71..3c0dd21a 100644 --- a/contractcourt/chain_arbitrator.go +++ b/contractcourt/chain_arbitrator.go @@ -296,8 +296,25 @@ func newActiveChannelArbitrator(channel *channeldb.OpenChannel, return c.resolveContract(chanPoint, chanLog) } + // Finally, we'll need to construct a series of htlc Sets based on all + // currently known valid commitments. + htlcSets := make(map[HtlcSetKey]htlcSet) + htlcSets[LocalHtlcSet] = newHtlcSet(channel.LocalCommitment.Htlcs) + htlcSets[RemoteHtlcSet] = newHtlcSet(channel.RemoteCommitment.Htlcs) + + pendingRemoteCommitment, err := channel.RemoteCommitChainTip() + if err != nil && err != channeldb.ErrNoPendingCommit { + blockEpoch.Cancel() + return nil, err + } + if pendingRemoteCommitment != nil { + htlcSets[RemotePendingHtlcSet] = newHtlcSet( + pendingRemoteCommitment.Commitment.Htlcs, + ) + } + return NewChannelArbitrator( - arbCfg, channel.LocalCommitment.Htlcs, chanLog, + arbCfg, htlcSets, chanLog, ), nil } @@ -557,15 +574,27 @@ func (c *ChainArbitrator) Stop() error { return nil } +// ContractUpdate is a message packages the latest set of active HTLCs on a +// commitment, and also identifies which commitment received a new set of +// HTLCs. +type ContractUpdate struct { + // HtlcKey identifies which commitment the HTLCs below are present on. + HtlcKey HtlcSetKey + + // Htlcs are the of active HTLCs on the commitment identified by the + // above HtlcKey. + Htlcs []channeldb.HTLC +} + // ContractSignals wraps the two signals that affect the state of a channel // being watched by an arbitrator. The two signals we care about are: the // channel has a new set of HTLC's, and the remote party has just broadcast // their version of the commitment transaction. type ContractSignals struct { - // HtlcUpdates is a channel that once we new commitment updates takes - // place, the later set of HTLC's on the commitment transaction should - // be sent over. - HtlcUpdates chan []channeldb.HTLC + // HtlcUpdates is a channel that the link will use to update the + // designated channel arbitrator when the set of HTLCs on any valid + // commitment changes. + HtlcUpdates chan *ContractUpdate // ShortChanID is the up to date short channel ID for a contract. This // can change either if when the contract was added it didn't yet have diff --git a/contractcourt/chain_watcher.go b/contractcourt/chain_watcher.go index 746d8011..b4b2865e 100644 --- a/contractcourt/chain_watcher.go +++ b/contractcourt/chain_watcher.go @@ -30,20 +30,77 @@ const ( maxCommitPointPollTimeout = 10 * time.Minute ) -// LocalUnilateralCloseInfo encapsulates all the informnation we need to act -// on a local force close that gets confirmed. +// LocalUnilateralCloseInfo encapsulates all the information we need to act on +// a local force close that gets confirmed. type LocalUnilateralCloseInfo struct { *chainntnfs.SpendDetail *lnwallet.LocalForceCloseSummary *channeldb.ChannelCloseSummary + + // CommitSet is the set of known valid commitments at the time the + // remote party's commitment hit the chain. + CommitSet CommitSet } -// CooperativeCloseInfo encapsulates all the informnation we need to act -// on a cooperative close that gets confirmed. +// CooperativeCloseInfo encapsulates all the information we need to act on a +// cooperative close that gets confirmed. type CooperativeCloseInfo struct { *channeldb.ChannelCloseSummary } +// RemoteUnilateralCloseInfo wraps the normal UnilateralCloseSummary to couple +// the CommitSet at the time of channel closure. +type RemoteUnilateralCloseInfo struct { + *lnwallet.UnilateralCloseSummary + + // CommitSet is the set of known valid commitments at the time the + // remote party's commitemnt hit the chain. + CommitSet CommitSet +} + +// CommitSet is a collection of the set of known valid commitments at a given +// instant. If ConfCommitKey is set, then the commitment identified by the +// HtlcSetKey has hit the chain. This struct will be used to examine all live +// 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 + // confirmed in the chain. + ConfCommitKey *HtlcSetKey + + // HtlcSets stores the set of all known active HTLC for each active + // commitment at the time of channel closure. + HtlcSets map[HtlcSetKey][]channeldb.HTLC +} + +// IsEmpty returns true if there are no HTLCs at all within all commitments +// that are a part of this commitment diff. +func (c *CommitSet) IsEmpty() bool { + if c == nil { + return true + } + + for _, htlcs := range c.HtlcSets { + if len(htlcs) != 0 { + return false + } + } + + return true +} + +// toActiveHTLCSets returns the set of all active HTLCs across all commitment +// transactions. +func (c *CommitSet) toActiveHTLCSets() map[HtlcSetKey]htlcSet { + htlcSets := make(map[HtlcSetKey]htlcSet) + + for htlcSetKey, htlcs := range c.HtlcSets { + htlcSets[htlcSetKey] = newHtlcSet(htlcs) + } + + return htlcSets +} + // ChainEventSubscription is a struct that houses a subscription to be notified // for any on-chain events related to a channel. There are three types of // possible on-chain events: a cooperative channel closure, a unilateral @@ -55,7 +112,7 @@ type ChainEventSubscription struct { // RemoteUnilateralClosure is a channel that will be sent upon in the // event that the remote party's commitment transaction is confirmed. - RemoteUnilateralClosure chan *lnwallet.UnilateralCloseSummary + RemoteUnilateralClosure chan *RemoteUnilateralCloseInfo // LocalUnilateralClosure is a channel that will be sent upon in the // event that our commitment transaction is confirmed. @@ -249,7 +306,7 @@ func (c *chainWatcher) SubscribeChannelEvents() *ChainEventSubscription { sub := &ChainEventSubscription{ ChanPoint: c.cfg.chanState.FundingOutpoint, - RemoteUnilateralClosure: make(chan *lnwallet.UnilateralCloseSummary, 1), + RemoteUnilateralClosure: make(chan *RemoteUnilateralCloseInfo, 1), LocalUnilateralClosure: make(chan *LocalUnilateralCloseInfo, 1), CooperativeClosure: make(chan *CooperativeCloseInfo, 1), ContractBreach: make(chan *lnwallet.BreachRetribution, 1), @@ -373,6 +430,30 @@ func (c *chainWatcher) closeObserver(spendNtfn *chainntnfs.SpendEvent) { return } + // Fetch the current known commit height for the remote party, + // and their pending commitment chain tip if it exist. + remoteStateNum := remoteCommit.CommitHeight + remoteChainTip, err := c.cfg.chanState.RemoteCommitChainTip() + if err != nil && err != channeldb.ErrNoPendingCommit { + log.Errorf("unable to obtain chain tip for "+ + "ChannelPoint(%v): %v", + c.cfg.chanState.FundingOutpoint, err) + return + } + + // Now that we have all the possible valid commitments, we'll + // make the CommitSet the ChannelArbitrator will need it in + // order to carry out its duty. + commitSet := CommitSet{ + HtlcSets: make(map[HtlcSetKey][]channeldb.HTLC), + } + commitSet.HtlcSets[LocalHtlcSet] = localCommit.Htlcs + commitSet.HtlcSets[RemoteHtlcSet] = remoteCommit.Htlcs + if remoteChainTip != nil { + htlcs := remoteChainTip.Commitment.Htlcs + commitSet.HtlcSets[RemotePendingHtlcSet] = htlcs + } + // We'll not retrieve the latest sate of the revocation store // so we can populate the information within the channel state // object that we have. @@ -411,8 +492,10 @@ func (c *chainWatcher) closeObserver(spendNtfn *chainntnfs.SpendEvent) { // as we don't have any further processing we need to do (we // can't cheat ourselves :p). if isOurCommit { + commitSet.ConfCommitKey = &LocalHtlcSet + if err := c.dispatchLocalForceClose( - commitSpend, *localCommit, + commitSpend, *localCommit, commitSet, ); err != nil { log.Errorf("unable to handle local"+ "close for chan_point=%v: %v", @@ -439,17 +522,6 @@ func (c *chainWatcher) closeObserver(spendNtfn *chainntnfs.SpendEvent) { log.Warnf("Unprompted commitment broadcast for "+ "ChannelPoint(%v) ", c.cfg.chanState.FundingOutpoint) - // Fetch the current known commit height for the remote party, - // and their pending commitment chain tip if it exist. - remoteStateNum := remoteCommit.CommitHeight - remoteChainTip, err := c.cfg.chanState.RemoteCommitChainTip() - if err != nil && err != channeldb.ErrNoPendingCommit { - log.Errorf("unable to obtain chain tip for "+ - "ChannelPoint(%v): %v", - c.cfg.chanState.FundingOutpoint, err) - return - } - // If this channel has been recovered, then we'll modify our // behavior as it isn't possible for us to close out the // channel off-chain ourselves. It can only be the remote party @@ -465,9 +537,10 @@ func (c *chainWatcher) closeObserver(spendNtfn *chainntnfs.SpendEvent) { // we'll trigger the unilateral close signal so subscribers can // clean up the state as necessary. case broadcastStateNum == remoteStateNum && !isRecoveredChan: + commitSet.ConfCommitKey = &RemoteHtlcSet err := c.dispatchRemoteForceClose( - commitSpend, *remoteCommit, + commitSpend, *remoteCommit, commitSet, c.cfg.chanState.RemoteCurrentRevocation, ) if err != nil { @@ -484,8 +557,10 @@ func (c *chainWatcher) closeObserver(spendNtfn *chainntnfs.SpendEvent) { case broadcastStateNum == remoteStateNum+1 && remoteChainTip != nil && !isRecoveredChan: + commitSet.ConfCommitKey = &RemotePendingHtlcSet + err := c.dispatchRemoteForceClose( - commitSpend, remoteChainTip.Commitment, + commitSpend, *remoteCommit, commitSet, c.cfg.chanState.RemoteNextRevocation, ) if err != nil { @@ -553,14 +628,15 @@ func (c *chainWatcher) closeObserver(spendNtfn *chainntnfs.SpendEvent) { c.cfg.chanState.FundingOutpoint) // Since we don't have the commitment stored for this - // state, we'll just pass an empty commitment. Note - // that this means we won't be able to recover any HTLC - // funds. + // state, we'll just pass an empty commitment within + // the commitment set. Note that this means we won't be + // able to recover any HTLC funds. // // TODO(halseth): can we try to recover some HTLCs? + commitSet.ConfCommitKey = &RemoteHtlcSet err = c.dispatchRemoteForceClose( commitSpend, channeldb.ChannelCommitment{}, - commitPoint, + commitSet, commitPoint, ) if err != nil { log.Errorf("unable to handle remote "+ @@ -691,7 +767,7 @@ func (c *chainWatcher) dispatchCooperativeClose(commitSpend *chainntnfs.SpendDet // dispatchLocalForceClose processes a unilateral close by us being confirmed. func (c *chainWatcher) dispatchLocalForceClose( commitSpend *chainntnfs.SpendDetail, - localCommit channeldb.ChannelCommitment) error { + localCommit channeldb.ChannelCommitment, commitSet CommitSet) error { log.Infof("Local unilateral close of ChannelPoint(%v) "+ "detected", c.cfg.chanState.FundingOutpoint) @@ -749,7 +825,10 @@ func (c *chainWatcher) dispatchLocalForceClose( // With the event processed, we'll now notify all subscribers of the // event. closeInfo := &LocalUnilateralCloseInfo{ - commitSpend, forceClose, closeSummary, + SpendDetail: commitSpend, + LocalForceCloseSummary: forceClose, + ChannelCloseSummary: closeSummary, + CommitSet: commitSet, } c.Lock() for _, sub := range c.clientSubscriptions { @@ -781,7 +860,7 @@ func (c *chainWatcher) dispatchLocalForceClose( func (c *chainWatcher) dispatchRemoteForceClose( commitSpend *chainntnfs.SpendDetail, remoteCommit channeldb.ChannelCommitment, - commitPoint *btcec.PublicKey) error { + commitSet CommitSet, commitPoint *btcec.PublicKey) error { log.Infof("Unilateral close of ChannelPoint(%v) "+ "detected", c.cfg.chanState.FundingOutpoint) @@ -802,7 +881,10 @@ func (c *chainWatcher) dispatchRemoteForceClose( c.Lock() for _, sub := range c.clientSubscriptions { select { - case sub.RemoteUnilateralClosure <- uniClose: + case sub.RemoteUnilateralClosure <- &RemoteUnilateralCloseInfo{ + UnilateralCloseSummary: uniClose, + CommitSet: commitSet, + }: case <-c.quit: c.Unlock() return fmt.Errorf("exiting") diff --git a/contractcourt/chain_watcher_test.go b/contractcourt/chain_watcher_test.go index f5bb0b12..2a511b65 100644 --- a/contractcourt/chain_watcher_test.go +++ b/contractcourt/chain_watcher_test.go @@ -104,7 +104,7 @@ func TestChainWatcherRemoteUnilateralClose(t *testing.T) { // We should get a new spend event over the remote unilateral close // event channel. - var uniClose *lnwallet.UnilateralCloseSummary + var uniClose *RemoteUnilateralCloseInfo select { case uniClose = <-chanEvents.RemoteUnilateralClosure: case <-time.After(time.Second * 15): @@ -186,7 +186,7 @@ func TestChainWatcherRemoteUnilateralClosePendingCommit(t *testing.T) { // With the HTLC added, we'll now manually initiate a state transition // from Alice to Bob. - _, _, err = aliceChannel.SignNextCommitment() + _, _, _, err = aliceChannel.SignNextCommitment() if err != nil { t.Fatal(err) } @@ -211,7 +211,7 @@ func TestChainWatcherRemoteUnilateralClosePendingCommit(t *testing.T) { // We should get a new spend event over the remote unilateral close // event channel. - var uniClose *lnwallet.UnilateralCloseSummary + var uniClose *RemoteUnilateralCloseInfo select { case uniClose = <-chanEvents.RemoteUnilateralClosure: case <-time.After(time.Second * 15): @@ -343,7 +343,7 @@ func TestChainWatcherDataLossProtect(t *testing.T) { // We should get a new uni close resolution that indicates we // processed the DLP scenario. - var uniClose *lnwallet.UnilateralCloseSummary + var uniClose *RemoteUnilateralCloseInfo select { case uniClose = <-chanEvents.RemoteUnilateralClosure: // If we processed this as a DLP case, then the remote diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index bb16a3af..703827f5 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -161,14 +161,14 @@ type ContractReport struct { // htlcSet represents the set of active HTLCs on a given commitment // transaction. type htlcSet struct { - // incomingHTLCs is a map of all incoming HTLCs on our commitment - // transaction. We may potentially go onchain to claim the funds sent - // to us within this set. + // incomingHTLCs is a map of all incoming HTLCs on the target + // commitment transaction. We may potentially go onchain to claim the + // funds sent to us within this set. incomingHTLCs map[uint64]channeldb.HTLC - // outgoingHTLCs is a map of all outgoing HTLCs on our commitment - // transaction. We may potentially go onchain to reclaim the funds that - // are currently in limbo. + // outgoingHTLCs is a map of all outgoing HTLCs on the target + // commitment transaction. We may potentially go onchain to reclaim the + // funds that are currently in limbo. outgoingHTLCs map[uint64]channeldb.HTLC } @@ -191,6 +191,30 @@ func newHtlcSet(htlcs []channeldb.HTLC) htlcSet { } } +// HtlcSetKey is a two-tuple that uniquely identifies a set of HTLCs on a +// commitment transaction. +type HtlcSetKey struct { + // IsRemote denotes if the HTLCs are on the remote commitment + // transaction. + IsRemote bool + + // IsPending denotes if the commitment transaction that HTLCS are on + // are pending (the higher of two unrevoked commitments). + IsPending bool +} + +var ( + // LocalHtlcSet is the HtlcSetKey used for local commitments. + LocalHtlcSet = HtlcSetKey{IsRemote: false, IsPending: false} + + // RemoteHtlcSet is the HtlcSetKey used for remote commitments. + RemoteHtlcSet = HtlcSetKey{IsRemote: true, IsPending: false} + + // RemotePendingHtlcSet is the HtlcSetKey used for dangling remote + // commitment transactions. + RemotePendingHtlcSet = HtlcSetKey{IsRemote: true, IsPending: true} +) + // ChannelArbitrator is the on-chain arbitrator for a particular channel. The // struct will keep in sync with the current set of HTLCs on the commitment // transaction. The job of the attendant is to go on-chain to either settle or @@ -207,9 +231,9 @@ type ChannelArbitrator struct { // its next action, and the state of any unresolved contracts. log ArbitratorLog - // activeHTLCs is the set of active incoming/outgoing HTLC's on the - // commitment transaction. - activeHTLCs htlcSet + // activeHTLCs is the set of active incoming/outgoing HTLC's on all + // currently valid commitment transactions. + activeHTLCs map[HtlcSetKey]htlcSet // cfg contains all the functionality that the ChannelArbitrator requires // to do its duty. @@ -222,7 +246,7 @@ type ChannelArbitrator struct { // htlcUpdates is a channel that is sent upon with new updates from the // active channel. Each time a new commitment state is accepted, the // set of HTLC's on the new state should be sent across this channel. - htlcUpdates <-chan []channeldb.HTLC + htlcUpdates <-chan *ContractUpdate // activeResolvers is a slice of any active resolvers. This is used to // be able to signal them for shutdown in the case that we shutdown. @@ -252,15 +276,15 @@ type ChannelArbitrator struct { // NewChannelArbitrator returns a new instance of a ChannelArbitrator backed by // the passed config struct. func NewChannelArbitrator(cfg ChannelArbitratorConfig, - startingHTLCs []channeldb.HTLC, log ArbitratorLog) *ChannelArbitrator { + htlcSets map[HtlcSetKey]htlcSet, log ArbitratorLog) *ChannelArbitrator { return &ChannelArbitrator{ log: log, signalUpdates: make(chan *signalUpdateMsg), - htlcUpdates: make(<-chan []channeldb.HTLC), + htlcUpdates: make(<-chan *ContractUpdate), resolutionSignal: make(chan struct{}), forceCloseReqs: make(chan *forceCloseReq), - activeHTLCs: newHtlcSet(startingHTLCs), + activeHTLCs: htlcSets, cfg: cfg, quit: make(chan struct{}), } @@ -335,7 +359,9 @@ func (c *ChannelArbitrator) Start() error { // We'll now attempt to advance our state forward based on the current // on-chain state, and our set of active contracts. startingState := c.state - nextState, _, err := c.advanceState(triggerHeight, trigger) + nextState, _, err := c.advanceState( + triggerHeight, trigger, nil, + ) if err != nil { switch err { @@ -600,8 +626,9 @@ func (t transitionTrigger) String() string { // the appropriate state transition if necessary. The next state we transition // to is returned, Additionally, if the next transition results in a commitment // broadcast, the commitment transaction itself is returned. -func (c *ChannelArbitrator) stateStep(triggerHeight uint32, - trigger transitionTrigger) (ArbitratorState, *wire.MsgTx, error) { +func (c *ChannelArbitrator) stateStep( + triggerHeight uint32, trigger transitionTrigger, + confCommitSet *CommitSet) (ArbitratorState, *wire.MsgTx, error) { var ( nextState ArbitratorState @@ -932,8 +959,9 @@ func (c *ChannelArbitrator) launchResolvers(resolvers []ContractResolver) { // redundant transition, meaning that the state transition is a noop. The final // param is a callback that allows the caller to execute an arbitrary action // after each state transition. -func (c *ChannelArbitrator) advanceState(triggerHeight uint32, - trigger transitionTrigger) (ArbitratorState, *wire.MsgTx, error) { +func (c *ChannelArbitrator) advanceState( + triggerHeight uint32, trigger transitionTrigger, + confCommitSet *CommitSet) (ArbitratorState, *wire.MsgTx, error) { var ( priorState ArbitratorState @@ -949,7 +977,7 @@ func (c *ChannelArbitrator) advanceState(triggerHeight uint32, priorState) nextState, closeTx, err := c.stateStep( - triggerHeight, trigger, + triggerHeight, trigger, confCommitSet, ) if err != nil { log.Errorf("ChannelArbitrator(%v): unable to advance "+ @@ -1099,7 +1127,8 @@ func (c *ChannelArbitrator) checkChainActions(height uint32, // First, we'll make an initial pass over the set of incoming and // outgoing HTLC's to decide if we need to go on chain at all. haveChainActions := false - for _, htlc := range c.activeHTLCs.outgoingHTLCs { + localHTLCs := c.activeHTLCs[LocalHtlcSet] + for _, htlc := range localHTLCs.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( @@ -1120,7 +1149,7 @@ func (c *ChannelArbitrator) checkChainActions(height uint32, haveChainActions = haveChainActions || toChain } - for _, htlc := range c.activeHTLCs.incomingHTLCs { + for _, htlc := range localHTLCs.incomingHTLCs { // We'll need to go on-chain to pull an incoming HTLC iff we // know the pre-image and it's close to timing out. We need to // ensure that we claim the funds that our rightfully ours @@ -1166,7 +1195,7 @@ func (c *ChannelArbitrator) checkChainActions(height uint32, // active outgoing HTLC's to see if we either need to: sweep them after // a timeout (then cancel backwards), cancel them backwards // immediately, or watch them as they're still active contracts. - for _, htlc := range c.activeHTLCs.outgoingHTLCs { + for _, htlc := range localHTLCs.outgoingHTLCs { switch { // If the HTLC is dust, then we can cancel it backwards // immediately as there's no matching contract to arbitrate @@ -1221,7 +1250,7 @@ func (c *ChannelArbitrator) checkChainActions(height uint32, // observe the output on-chain if we don't In this last, case we'll // either learn of it eventually from the outgoing HTLC, or the sender // will timeout the HTLC. - for _, htlc := range c.activeHTLCs.incomingHTLCs { + for _, htlc := range localHTLCs.incomingHTLCs { log.Tracef("ChannelArbitrator(%v): watching chain to decide "+ "action for incoming htlc=%x", c.cfg.ChanPoint, htlc.RHash[:]) @@ -1671,7 +1700,7 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) { // Now that a new block has arrived, we'll attempt to // advance our state forward. nextState, _, err := c.advanceState( - uint32(bestHeight), chainTrigger, + uint32(bestHeight), chainTrigger, nil, ) if err != nil { log.Errorf("unable to advance state: %v", err) @@ -1703,16 +1732,19 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) { // A new set of HTLC's has been added or removed from the // commitment transaction. So we'll update our activeHTLCs map // accordingly. - case newStateHTLCs := <-c.htlcUpdates: - // We'll wipe out our old set of HTLC's and instead - // monitor only the HTLC's that are still active on the - // current commitment state. - c.activeHTLCs = newHtlcSet(newStateHTLCs) + case htlcUpdate := <-c.htlcUpdates: + // We'll wipe out our old set of HTLC's for each + // htlcSetKey type included in this update in order to + // only monitor the HTLCs that are still active on this + // target commitment. + c.activeHTLCs[htlcUpdate.HtlcKey] = newHtlcSet( + htlcUpdate.Htlcs, + ) - log.Tracef("ChannelArbitrator(%v): fresh set of "+ - "htlcs=%v", c.cfg.ChanPoint, + log.Tracef("ChannelArbitrator(%v): fresh set of htlcs=%v", + c.cfg.ChanPoint, newLogClosure(func() string { - return spew.Sdump(c.activeHTLCs) + return spew.Sdump(htlcUpdate) }), ) @@ -1734,7 +1766,7 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) { // We'll now advance our state machine until it reaches // a terminal state, and the channel is marked resolved. _, _, err = c.advanceState( - closeInfo.CloseHeight, coopCloseTrigger, + closeInfo.CloseHeight, coopCloseTrigger, nil, ) if err != nil { log.Errorf("unable to advance state: %v", err) @@ -1794,7 +1826,7 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) { // a terminal state. _, _, err = c.advanceState( uint32(closeInfo.SpendingHeight), - localCloseTrigger, + localCloseTrigger, &closeInfo.CommitSet, ) if err != nil { log.Errorf("unable to advance state: %v", err) @@ -1804,7 +1836,6 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) { // We'll examine our state to determine if we need to act at // all. case uniClosure := <-c.cfg.ChainEvents.RemoteUnilateralClosure: - log.Infof("ChannelArbitrator(%v): remote party has "+ "closed channel out on-chain", c.cfg.ChanPoint) @@ -1817,12 +1848,6 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) { HtlcResolutions: *uniClosure.HtlcResolutions, } - // As we're now acting upon an event triggered by the - // broadcast of the remote commitment transaction, - // we'll swap out our active HTLC set with the set - // present on their commitment. - c.activeHTLCs = newHtlcSet(uniClosure.RemoteCommit.Htlcs) - // When processing a unilateral close event, we'll // transition to the ContractClosed state. We'll log // out the set of resolutions such that they are @@ -1856,7 +1881,7 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) { // a terminal state. _, _, err = c.advanceState( uint32(uniClosure.SpendingHeight), - remoteCloseTrigger, + remoteCloseTrigger, &uniClosure.CommitSet, ) if err != nil { log.Errorf("unable to advance state: %v", err) @@ -1870,7 +1895,7 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) { "fully resolved!", c.cfg.ChanPoint) nextState, _, err := c.advanceState( - uint32(bestHeight), chainTrigger, + uint32(bestHeight), chainTrigger, nil, ) if err != nil { log.Errorf("unable to advance state: %v", err) @@ -1904,7 +1929,7 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) { } nextState, closeTx, err := c.advanceState( - uint32(bestHeight), userTrigger, + uint32(bestHeight), userTrigger, nil, ) if err != nil { log.Errorf("unable to advance state: %v", err) diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index addf2c50..cae8636b 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/contractcourt/channel_arbitrator_test.go @@ -152,7 +152,7 @@ func createTestChannelArbitrator(log ArbitratorLog) (*ChannelArbitrator, chanPoint := wire.OutPoint{} shortChanID := lnwire.ShortChannelID{} chanEvents := &ChainEventSubscription{ - RemoteUnilateralClosure: make(chan *lnwallet.UnilateralCloseSummary, 1), + RemoteUnilateralClosure: make(chan *RemoteUnilateralCloseInfo, 1), LocalUnilateralClosure: make(chan *LocalUnilateralCloseInfo, 1), CooperativeClosure: make(chan *CooperativeCloseInfo, 1), ContractBreach: make(chan *lnwallet.BreachRetribution, 1), @@ -328,7 +328,13 @@ func TestChannelArbitratorRemoteForceClose(t *testing.T) { SpendDetail: commitSpend, HtlcResolutions: &lnwallet.HtlcResolutions{}, } - chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- uniClose + chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- &RemoteUnilateralCloseInfo{ + UnilateralCloseSummary: uniClose, + CommitSet: CommitSet{ + ConfCommitKey: &RemoteHtlcSet, + HtlcSets: make(map[HtlcSetKey][]channeldb.HTLC), + }, + } // It should transition StateDefault -> StateContractClosed -> // StateFullyResolved. @@ -430,12 +436,12 @@ func TestChannelArbitratorLocalForceClose(t *testing.T) { // Now notify about the local force close getting confirmed. chanArb.cfg.ChainEvents.LocalUnilateralClosure <- &LocalUnilateralCloseInfo{ - &chainntnfs.SpendDetail{}, - &lnwallet.LocalForceCloseSummary{ + SpendDetail: &chainntnfs.SpendDetail{}, + LocalForceCloseSummary: &lnwallet.LocalForceCloseSummary{ CloseTx: &wire.MsgTx{}, HtlcResolutions: &lnwallet.HtlcResolutions{}, }, - &channeldb.ChannelCloseSummary{}, + ChannelCloseSummary: &channeldb.ChannelCloseSummary{}, } // It should transition StateContractClosed -> StateFullyResolved. @@ -483,7 +489,7 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { defer chanArb.Stop() // Create htlcUpdates channel. - htlcUpdates := make(chan []channeldb.HTLC) + htlcUpdates := make(chan *ContractUpdate) signals := &ContractSignals{ HtlcUpdates: htlcUpdates, @@ -492,14 +498,16 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { chanArb.UpdateContractSignals(signals) // Add HTLC to channel arbitrator. + htlcIndex := uint64(99) htlc := channeldb.HTLC{ Incoming: false, Amt: 10000, - HtlcIndex: 0, + HtlcIndex: htlcIndex, } - htlcUpdates <- []channeldb.HTLC{ - htlc, + htlcUpdates <- &ContractUpdate{ + HtlcKey: LocalHtlcSet, + Htlcs: []channeldb.HTLC{htlc}, } errChan := make(chan error, 1) @@ -572,8 +580,8 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { } chanArb.cfg.ChainEvents.LocalUnilateralClosure <- &LocalUnilateralCloseInfo{ - &chainntnfs.SpendDetail{}, - &lnwallet.LocalForceCloseSummary{ + SpendDetail: &chainntnfs.SpendDetail{}, + LocalForceCloseSummary: &lnwallet.LocalForceCloseSummary{ CloseTx: closeTx, HtlcResolutions: &lnwallet.HtlcResolutions{ OutgoingHTLCs: []lnwallet.OutgoingHtlcResolution{ @@ -581,7 +589,13 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { }, }, }, - &channeldb.ChannelCloseSummary{}, + ChannelCloseSummary: &channeldb.ChannelCloseSummary{}, + CommitSet: CommitSet{ + ConfCommitKey: &LocalHtlcSet, + HtlcSets: map[HtlcSetKey][]channeldb.HTLC{ + LocalHtlcSet: {htlc}, + }, + }, } assertStateTransitions( @@ -627,7 +641,6 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { // At this point channel should be marked as resolved. assertStateTransitions(t, arbLog.newStates, StateFullyResolved) - select { case <-resolved: case <-time.After(5 * time.Second): @@ -726,7 +739,9 @@ func TestChannelArbitratorLocalForceCloseRemoteConfirmed(t *testing.T) { SpendDetail: commitSpend, HtlcResolutions: &lnwallet.HtlcResolutions{}, } - chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- uniClose + chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- &RemoteUnilateralCloseInfo{ + UnilateralCloseSummary: uniClose, + } // It should transition StateContractClosed -> StateFullyResolved. assertStateTransitions(t, log.newStates, StateContractClosed, @@ -832,7 +847,9 @@ func TestChannelArbitratorLocalForceDoubleSpend(t *testing.T) { SpendDetail: commitSpend, HtlcResolutions: &lnwallet.HtlcResolutions{}, } - chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- uniClose + chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- &RemoteUnilateralCloseInfo{ + UnilateralCloseSummary: uniClose, + } // It should transition StateContractClosed -> StateFullyResolved. assertStateTransitions(t, log.newStates, StateContractClosed, @@ -878,7 +895,9 @@ func TestChannelArbitratorPersistence(t *testing.T) { SpendDetail: commitSpend, HtlcResolutions: &lnwallet.HtlcResolutions{}, } - chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- uniClose + chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- &RemoteUnilateralCloseInfo{ + UnilateralCloseSummary: uniClose, + } // Since writing the resolutions fail, the arbitrator should not // advance to the next state. @@ -909,7 +928,9 @@ func TestChannelArbitratorPersistence(t *testing.T) { } // Send a new remote force close event. - chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- uniClose + chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- &RemoteUnilateralCloseInfo{ + UnilateralCloseSummary: uniClose, + } // Since closing the channel failed, the arbitrator should stay in the // default state. @@ -934,7 +955,9 @@ func TestChannelArbitratorPersistence(t *testing.T) { // Now make fetching the resolutions fail. log.failFetch = fmt.Errorf("intentional fetch failure") - chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- uniClose + chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- &RemoteUnilateralCloseInfo{ + UnilateralCloseSummary: uniClose, + } // Since logging the resolutions and closing the channel now succeeds, // it should advance to StateContractClosed. @@ -1015,7 +1038,9 @@ func TestChannelArbitratorCommitFailure(t *testing.T) { SpendDetail: commitSpend, HtlcResolutions: &lnwallet.HtlcResolutions{}, } - chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- uniClose + chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- &RemoteUnilateralCloseInfo{ + UnilateralCloseSummary: uniClose, + } }, expectedStates: []ArbitratorState{StateContractClosed, StateFullyResolved}, }, @@ -1023,12 +1048,12 @@ func TestChannelArbitratorCommitFailure(t *testing.T) { closeType: channeldb.LocalForceClose, sendEvent: func(chanArb *ChannelArbitrator) { chanArb.cfg.ChainEvents.LocalUnilateralClosure <- &LocalUnilateralCloseInfo{ - &chainntnfs.SpendDetail{}, - &lnwallet.LocalForceCloseSummary{ + SpendDetail: &chainntnfs.SpendDetail{}, + LocalForceCloseSummary: &lnwallet.LocalForceCloseSummary{ CloseTx: &wire.MsgTx{}, HtlcResolutions: &lnwallet.HtlcResolutions{}, }, - &channeldb.ChannelCloseSummary{}, + ChannelCloseSummary: &channeldb.ChannelCloseSummary{}, } }, expectedStates: []ArbitratorState{StateContractClosed, StateFullyResolved}, @@ -1192,8 +1217,8 @@ func TestChannelArbitratorAlreadyForceClosed(t *testing.T) { case <-chanArb.quit: } - // Finally, we should ensure that we are not able to do so by seeing the - // expected errAlreadyForceClosed error. + // Finally, we should ensure that we are not able to do so by seeing + // the expected errAlreadyForceClosed error. select { case err = <-errChan: if err != errAlreadyForceClosed { diff --git a/htlcswitch/link.go b/htlcswitch/link.go index dc50b4fd..6f3a8316 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -320,7 +320,7 @@ type channelLink struct { // htlcUpdates is a channel that we'll use to update outside // sub-systems with the latest set of active HTLC's on our channel. - htlcUpdates chan []channeldb.HTLC + htlcUpdates chan *contractcourt.ContractUpdate // logCommitTimer is a timer which is sent upon if we go an interval // without receiving/sending a commitment update. It's role is to @@ -372,7 +372,7 @@ func NewChannelLink(cfg ChannelLinkConfig, // TODO(roasbeef): just do reserve here? logCommitTimer: time.NewTimer(300 * time.Millisecond), overflowQueue: newPacketQueue(input.MaxHTLCNumber / 2), - htlcUpdates: make(chan []channeldb.HTLC), + htlcUpdates: make(chan *contractcourt.ContractUpdate), hodlMap: make(map[lntypes.Hash][]hodlHtlc), hodlQueue: queue.NewConcurrentQueue(10), quit: make(chan struct{}), @@ -1721,7 +1721,10 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) { // of HTLC's on our commitment, so we'll send them over our // HTLC update channel so any callers can be notified. select { - case l.htlcUpdates <- currentHtlcs: + case l.htlcUpdates <- &contractcourt.ContractUpdate{ + HtlcKey: contractcourt.LocalHtlcSet, + Htlcs: currentHtlcs, + }: case <-l.quit: return } @@ -1761,7 +1764,9 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) { // We've received a revocation from the remote chain, if valid, // this moves the remote chain forward, and expands our // revocation window. - fwdPkg, adds, settleFails, err := l.channel.ReceiveRevocation(msg) + fwdPkg, adds, settleFails, remoteHTLCs, err := l.channel.ReceiveRevocation( + msg, + ) if err != nil { // TODO(halseth): force close? l.fail(LinkFailureError{code: ErrInvalidRevocation}, @@ -1769,6 +1774,18 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) { return } + // The remote party now has a new primary commitment, so we'll + // update the contract court to be aware of this new set (the + // prior old remote pending). + select { + case l.htlcUpdates <- &contractcourt.ContractUpdate{ + HtlcKey: contractcourt.RemoteHtlcSet, + Htlcs: remoteHTLCs, + }: + case <-l.quit: + return + } + l.processRemoteSettleFails(fwdPkg, settleFails) needUpdate := l.processRemoteAdds(fwdPkg, adds) @@ -1894,7 +1911,7 @@ func (l *channelLink) updateCommitTx() error { return nil } - theirCommitSig, htlcSigs, err := l.channel.SignNextCommitment() + theirCommitSig, htlcSigs, pendingHTLCs, err := l.channel.SignNextCommitment() if err == lnwallet.ErrNoWindow { l.tracef("revocation window exhausted, unable to send: %v, "+ "dangling_opens=%v, dangling_closes%v", @@ -1910,6 +1927,18 @@ func (l *channelLink) updateCommitTx() error { return err } + // The remote party now has a new pending commitment, so we'll update + // the contract court to be aware of this new set (the prior old remote + // pending). + select { + case l.htlcUpdates <- &contractcourt.ContractUpdate{ + HtlcKey: contractcourt.RemotePendingHtlcSet, + Htlcs: pendingHTLCs, + }: + case <-l.quit: + return nil + } + if err := l.ackDownStreamPackets(); err != nil { return err } diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index fb3cfc56..55184e40 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -1761,7 +1761,7 @@ func handleStateUpdate(link *channelLink, } link.HandleChannelUpdate(remoteRev) - remoteSig, remoteHtlcSigs, err := remoteChannel.SignNextCommitment() + remoteSig, remoteHtlcSigs, _, err := remoteChannel.SignNextCommitment() if err != nil { return err } @@ -1782,7 +1782,7 @@ func handleStateUpdate(link *channelLink, if !ok { return fmt.Errorf("expected RevokeAndAck got %T", msg) } - _, _, _, err = remoteChannel.ReceiveRevocation(revoke) + _, _, _, _, err = remoteChannel.ReceiveRevocation(revoke) if err != nil { return fmt.Errorf("unable to receive "+ "revocation: %v", err) @@ -1812,7 +1812,7 @@ func updateState(batchTick chan time.Time, link *channelLink, // The remote is triggering the state update, emulate this by // signing and sending CommitSig to the link. - remoteSig, remoteHtlcSigs, err := remoteChannel.SignNextCommitment() + remoteSig, remoteHtlcSigs, _, err := remoteChannel.SignNextCommitment() if err != nil { return err } @@ -1836,7 +1836,7 @@ func updateState(batchTick chan time.Time, link *channelLink, return fmt.Errorf("expected RevokeAndAck got %T", msg) } - _, _, _, err = remoteChannel.ReceiveRevocation(revoke) + _, _, _, _, err = remoteChannel.ReceiveRevocation(revoke) if err != nil { return fmt.Errorf("unable to receive "+ "revocation: %v", err) @@ -4397,7 +4397,7 @@ func sendCommitSigBobToAlice(t *testing.T, aliceLink ChannelLink, t.Helper() - sig, htlcSigs, err := bobChannel.SignNextCommitment() + sig, htlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("error signing commitment: %v", err) } @@ -4435,7 +4435,7 @@ func receiveRevAndAckAliceToBob(t *testing.T, aliceMsgs chan lnwire.Message, t.Fatalf("expected RevokeAndAck, got %T", msg) } - _, _, _, err := bobChannel.ReceiveRevocation(rev) + _, _, _, _, err := bobChannel.ReceiveRevocation(rev) if err != nil { t.Fatalf("bob failed receiving revocation: %v", err) } @@ -5326,7 +5326,7 @@ func TestChannelLinkFail(t *testing.T) { // Sign a commitment that will include // signature for the HTLC just sent. - sig, htlcSigs, err := + sig, htlcSigs, _, err := remoteChannel.SignNextCommitment() if err != nil { t.Fatalf("error signing commitment: %v", @@ -5358,7 +5358,7 @@ func TestChannelLinkFail(t *testing.T) { // Sign a commitment that will include // signature for the HTLC just sent. - sig, htlcSigs, err := + sig, htlcSigs, _, err := remoteChannel.SignNextCommitment() if err != nil { t.Fatalf("error signing commitment: %v", diff --git a/lnwallet/channel.go b/lnwallet/channel.go index e82293bd..dddbe7e7 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -3035,8 +3035,10 @@ func (lc *LightningChannel) createCommitDiff( // The first return parameter is the signature for the commitment transaction // itself, while the second parameter is a slice of all HTLC signatures (if // any). The HTLC signatures are sorted according to the BIP 69 order of the -// HTLC's on the commitment transaction. -func (lc *LightningChannel) SignNextCommitment() (lnwire.Sig, []lnwire.Sig, error) { +// HTLC's on the commitment transaction. Finally, the new set of pending HTLCs +// for the remote party's commitment are also returned. +func (lc *LightningChannel) SignNextCommitment() (lnwire.Sig, []lnwire.Sig, []channeldb.HTLC, error) { + lc.Lock() defer lc.Unlock() @@ -3052,7 +3054,7 @@ func (lc *LightningChannel) SignNextCommitment() (lnwire.Sig, []lnwire.Sig, erro commitPoint := lc.channelState.RemoteNextRevocation if lc.remoteCommitChain.hasUnackedCommitment() || commitPoint == nil { - return sig, htlcSigs, ErrNoWindow + return sig, htlcSigs, nil, ErrNoWindow } // Determine the last update on the remote log that has been locked in. @@ -3067,7 +3069,7 @@ func (lc *LightningChannel) SignNextCommitment() (lnwire.Sig, []lnwire.Sig, erro remoteACKedIndex, lc.localUpdateLog.logIndex, true, nil, ) if err != nil { - return sig, htlcSigs, err + return sig, htlcSigs, nil, err } // Grab the next commitment point for the remote party. This will be @@ -3089,7 +3091,7 @@ func (lc *LightningChannel) SignNextCommitment() (lnwire.Sig, []lnwire.Sig, erro remoteACKedIndex, remoteHtlcIndex, keyRing, ) if err != nil { - return sig, htlcSigs, err + return sig, htlcSigs, nil, err } walletLog.Tracef("ChannelPoint(%v): extending remote chain to height %v, "+ @@ -3114,7 +3116,7 @@ func (lc *LightningChannel) SignNextCommitment() (lnwire.Sig, []lnwire.Sig, erro lc.localChanCfg, lc.remoteChanCfg, newCommitView, ) if err != nil { - return sig, htlcSigs, err + return sig, htlcSigs, nil, err } lc.sigPool.SubmitSignBatch(sigBatch) @@ -3125,12 +3127,12 @@ func (lc *LightningChannel) SignNextCommitment() (lnwire.Sig, []lnwire.Sig, erro rawSig, err := lc.Signer.SignOutputRaw(newCommitView.txn, lc.signDesc) if err != nil { close(cancelChan) - return sig, htlcSigs, err + return sig, htlcSigs, nil, err } sig, err = lnwire.NewSigFromRawSignature(rawSig) if err != nil { close(cancelChan) - return sig, htlcSigs, err + return sig, htlcSigs, nil, err } // We'll need to send over the signatures to the remote party in the @@ -3150,7 +3152,7 @@ func (lc *LightningChannel) SignNextCommitment() (lnwire.Sig, []lnwire.Sig, erro // jobs. if jobResp.Err != nil { close(cancelChan) - return sig, htlcSigs, err + return sig, htlcSigs, nil, err } htlcSigs = append(htlcSigs, jobResp.Sig) @@ -3161,11 +3163,11 @@ func (lc *LightningChannel) SignNextCommitment() (lnwire.Sig, []lnwire.Sig, erro // can retransmit it if necessary. commitDiff, err := lc.createCommitDiff(newCommitView, sig, htlcSigs) if err != nil { - return sig, htlcSigs, err + return sig, htlcSigs, nil, err } err = lc.channelState.AppendRemoteCommitChain(commitDiff) if err != nil { - return sig, htlcSigs, err + return sig, htlcSigs, nil, err } // TODO(roasbeef): check that one eclair bug @@ -3176,7 +3178,7 @@ func (lc *LightningChannel) SignNextCommitment() (lnwire.Sig, []lnwire.Sig, erro // latest commitment update. lc.remoteCommitChain.addCommitment(newCommitView) - return sig, htlcSigs, nil + return sig, htlcSigs, commitDiff.Commitment.Htlcs, nil } // ProcessChanSyncMsg processes a ChannelReestablish message sent by the remote @@ -3352,7 +3354,7 @@ func (lc *LightningChannel) ProcessChanSyncMsg( // revocation, but also initiate a state transition to re-sync // them. if !lc.FullySynced() { - commitSig, htlcSigs, err := lc.SignNextCommitment() + commitSig, htlcSigs, _, err := lc.SignNextCommitment() switch { // If we signed this state, then we'll accumulate @@ -4277,8 +4279,11 @@ func (lc *LightningChannel) RevokeCurrentCommitment() (*lnwire.RevokeAndAck, []c // revocation. // 3. The PaymentDescriptor of any Settle/Fail HTLCs that were locked in by // this revocation. +// 4. The set of HTLCs present on the current valid commitment transaction +// for the remote party. func (lc *LightningChannel) ReceiveRevocation(revMsg *lnwire.RevokeAndAck) ( - *channeldb.FwdPkg, []*PaymentDescriptor, []*PaymentDescriptor, error) { + *channeldb.FwdPkg, []*PaymentDescriptor, []*PaymentDescriptor, + []channeldb.HTLC, error) { lc.Lock() defer lc.Unlock() @@ -4287,10 +4292,10 @@ func (lc *LightningChannel) ReceiveRevocation(revMsg *lnwire.RevokeAndAck) ( store := lc.channelState.RevocationStore revocation, err := chainhash.NewHash(revMsg.Revocation[:]) if err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } if err := store.AddNextEntry(revocation); err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } // Verify that if we use the commitment point computed based off of the @@ -4299,7 +4304,7 @@ func (lc *LightningChannel) ReceiveRevocation(revMsg *lnwire.RevokeAndAck) ( currentCommitPoint := lc.channelState.RemoteCurrentRevocation derivedCommitPoint := input.ComputeCommitmentPoint(revMsg.Revocation[:]) if !derivedCommitPoint.IsEqual(currentCommitPoint) { - return nil, nil, nil, fmt.Errorf("revocation key mismatch") + return nil, nil, nil, nil, fmt.Errorf("revocation key mismatch") } // Now that we've verified that the prior commitment has been properly @@ -4470,7 +4475,7 @@ func (lc *LightningChannel) ReceiveRevocation(revMsg *lnwire.RevokeAndAck) ( // commitment chain. err = lc.channelState.AdvanceCommitChainTail(fwdPkg) if err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } // Since they revoked the current lowest height in their commitment @@ -4485,7 +4490,9 @@ func (lc *LightningChannel) ReceiveRevocation(revMsg *lnwire.RevokeAndAck) ( remoteChainTail, ) - return fwdPkg, addsToForward, settleFailsToForward, nil + remoteHTLCs := lc.channelState.RemoteCommitment.Htlcs + + return fwdPkg, addsToForward, settleFailsToForward, remoteHTLCs, nil } // LoadFwdPkgs loads any pending log updates from disk and returns the payment diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 3132fc56..cdea5644 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -99,7 +99,7 @@ func TestSimpleAddSettleWorkflow(t *testing.T) { // we expect the messages to be ordered, Bob will receive the HTLC we // just sent before he receives this signature, so the signature will // cover the HTLC. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("alice unable to sign commitment: %v", err) } @@ -123,7 +123,7 @@ func TestSimpleAddSettleWorkflow(t *testing.T) { // This signature will cover the HTLC, since Bob will first send the // revocation just created. The revocation also acks every received // HTLC up to the point where Alice sent here signature. - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("bob unable to sign alice's commitment: %v", err) } @@ -131,7 +131,7 @@ func TestSimpleAddSettleWorkflow(t *testing.T) { // Alice then processes this revocation, sending her own revocation for // her prior commitment transaction. Alice shouldn't have any HTLCs to // forward since she's sending an outgoing HTLC. - fwdPkg, _, _, err := aliceChannel.ReceiveRevocation(bobRevocation) + fwdPkg, _, _, _, err := aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatalf("alice unable to process bob's revocation: %v", err) } @@ -162,7 +162,7 @@ func TestSimpleAddSettleWorkflow(t *testing.T) { // is fully locked in within both commitment transactions. Bob should // also be able to forward an HTLC now that the HTLC has been locked // into both commitment transactions. - fwdPkg, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + fwdPkg, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) if err != nil { t.Fatalf("bob unable to process alice's revocation: %v", err) } @@ -239,7 +239,7 @@ func TestSimpleAddSettleWorkflow(t *testing.T) { t.Fatalf("alice unable to accept settle of outbound htlc: %v", err) } - bobSig2, bobHtlcSigs2, err := bobChannel.SignNextCommitment() + bobSig2, bobHtlcSigs2, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("bob unable to sign settle commitment: %v", err) } @@ -252,12 +252,12 @@ func TestSimpleAddSettleWorkflow(t *testing.T) { if err != nil { t.Fatalf("alice unable to generate revocation: %v", err) } - aliceSig2, aliceHtlcSigs2, err := aliceChannel.SignNextCommitment() + aliceSig2, aliceHtlcSigs2, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("alice unable to sign new commitment: %v", err) } - fwdPkg, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation2) + fwdPkg, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation2) if err != nil { t.Fatalf("bob unable to process alice's revocation: %v", err) } @@ -279,7 +279,7 @@ func TestSimpleAddSettleWorkflow(t *testing.T) { if err != nil { t.Fatalf("bob unable to revoke commitment: %v", err) } - fwdPkg, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation2) + fwdPkg, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation2) if err != nil { t.Fatalf("alice unable to process bob's revocation: %v", err) } @@ -1106,7 +1106,7 @@ func TestHTLCSigNumber(t *testing.T) { aboveDust) defer cleanUp() - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("Error signing next commitment: %v", err) } @@ -1130,7 +1130,7 @@ func TestHTLCSigNumber(t *testing.T) { aliceChannel, bobChannel, cleanUp = createChanWithHTLC(aboveDust) defer cleanUp() - aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("Error signing next commitment: %v", err) } @@ -1153,7 +1153,7 @@ func TestHTLCSigNumber(t *testing.T) { aliceChannel, bobChannel, cleanUp = createChanWithHTLC(belowDust) defer cleanUp() - aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("Error signing next commitment: %v", err) } @@ -1176,7 +1176,7 @@ func TestHTLCSigNumber(t *testing.T) { aliceChannel, bobChannel, cleanUp = createChanWithHTLC(aboveDust) defer cleanUp() - aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("Error signing next commitment: %v", err) } @@ -1203,7 +1203,7 @@ func TestHTLCSigNumber(t *testing.T) { // Alice should produce only one signature, since one HTLC is below // dust. - aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("Error signing next commitment: %v", err) } @@ -1989,7 +1989,7 @@ func TestUpdateFeeFail(t *testing.T) { // Alice sends signature for commitment that does not cover any fee // update. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("alice unable to sign commitment: %v", err) } @@ -2040,13 +2040,13 @@ func TestUpdateFeeConcurrentSig(t *testing.T) { } // Alice signs a commitment, and sends this to bob. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("alice unable to sign commitment: %v", err) } // At the same time, Bob signs a commitment. - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("bob unable to sign alice's commitment: %v", err) } @@ -2127,7 +2127,7 @@ func TestUpdateFeeSenderCommits(t *testing.T) { // Alice signs a commitment, which will cover everything sent to Bob // (the HTLC and the fee update), and everything acked by Bob (nothing // so far). - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("alice unable to sign commitment: %v", err) } @@ -2157,7 +2157,7 @@ func TestUpdateFeeSenderCommits(t *testing.T) { // Bob commits to all updates he has received from Alice. This includes // the HTLC he received, and the fee update. - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("bob unable to sign alice's commitment: %v", err) } @@ -2165,7 +2165,8 @@ func TestUpdateFeeSenderCommits(t *testing.T) { // Alice receives the revocation of the old one, and can now assume // that Bob's received everything up to the signature she sent, // including the HTLC and fee update. - if _, _, _, err := aliceChannel.ReceiveRevocation(bobRevocation); err != nil { + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + if err != nil { t.Fatalf("alice unable to process bob's revocation: %v", err) } @@ -2192,7 +2193,8 @@ func TestUpdateFeeSenderCommits(t *testing.T) { } // Bob receives revocation from Alice. - if _, _, _, err := bobChannel.ReceiveRevocation(aliceRevocation); err != nil { + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { t.Fatalf("bob unable to process alice's revocation: %v", err) } @@ -2239,7 +2241,7 @@ func TestUpdateFeeReceiverCommits(t *testing.T) { // Bob commits to every change he has sent since last time (none). He // does not commit to the received HTLC and fee update, since Alice // cannot know if he has received them. - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("alice unable to sign commitment: %v", err) } @@ -2260,14 +2262,15 @@ func TestUpdateFeeReceiverCommits(t *testing.T) { } // Bob receives the revocation of the old commitment - if _, _, _, err := bobChannel.ReceiveRevocation(aliceRevocation); err != nil { + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { t.Fatalf("alice unable to process bob's revocation: %v", err) } // Alice will sign next commitment. Since she sent the revocation, she // also ack'ed everything received, but in this case this is nothing. // Since she sent the two updates, this signature will cover those two. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("bob unable to sign alice's commitment: %v", err) } @@ -2297,14 +2300,15 @@ func TestUpdateFeeReceiverCommits(t *testing.T) { // Bob will send a new signature, which will cover what he just acked: // the HTLC and fee update. - bobSig, bobHtlcSigs, err = bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err = bobChannel.SignNextCommitment() if err != nil { t.Fatalf("alice unable to sign commitment: %v", err) } // Alice receives revocation from Bob, and can now be sure that Bob // received the two updates, and they are considered locked in. - if _, _, _, err := aliceChannel.ReceiveRevocation(bobRevocation); err != nil { + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + if err != nil { t.Fatalf("bob unable to process alice's revocation: %v", err) } @@ -2331,7 +2335,8 @@ func TestUpdateFeeReceiverCommits(t *testing.T) { } // Bob receives revocation from Alice. - if _, _, _, err := bobChannel.ReceiveRevocation(aliceRevocation); err != nil { + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { t.Fatalf("bob unable to process alice's revocation: %v", err) } } @@ -2391,7 +2396,7 @@ func TestUpdateFeeMultipleUpdates(t *testing.T) { // Alice signs a commitment, which will cover everything sent to Bob // (the HTLC and the fee update), and everything acked by Bob (nothing // so far). - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("alice unable to sign commitment: %v", err) } @@ -2437,7 +2442,7 @@ func TestUpdateFeeMultipleUpdates(t *testing.T) { // Bob commits to all updates he has received from Alice. This includes // the HTLC he received, and the fee update. - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("bob unable to sign alice's commitment: %v", err) } @@ -2445,7 +2450,8 @@ func TestUpdateFeeMultipleUpdates(t *testing.T) { // Alice receives the revocation of the old one, and can now assume that // Bob's received everything up to the signature she sent, including the // HTLC and fee update. - if _, _, _, err := aliceChannel.ReceiveRevocation(bobRevocation); err != nil { + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + if err != nil { t.Fatalf("alice unable to process bob's revocation: %v", err) } @@ -2471,7 +2477,8 @@ func TestUpdateFeeMultipleUpdates(t *testing.T) { } // Bob receives revocation from Alice. - if _, _, _, err := bobChannel.ReceiveRevocation(aliceRevocation); err != nil { + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { t.Fatalf("bob unable to process alice's revocation: %v", err) } } @@ -2764,7 +2771,7 @@ func TestChanSyncOweCommitment(t *testing.T) { // Now we'll begin the core of the test itself. Alice will extend a new // commitment to Bob, but the connection drops before Bob can process // it. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -2892,11 +2899,11 @@ func TestChanSyncOweCommitment(t *testing.T) { if err != nil { t.Fatalf("unable to revoke bob commitment: %v", err) } - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("bob unable to sign commitment: %v", err) } - _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatalf("alice unable to recv revocation: %v", err) } @@ -2908,7 +2915,8 @@ func TestChanSyncOweCommitment(t *testing.T) { if err != nil { t.Fatalf("alice unable to revoke commitment: %v", err) } - if _, _, _, err := bobChannel.ReceiveRevocation(aliceRevocation); err != nil { + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { t.Fatalf("bob unable to recv revocation: %v", err) } @@ -3048,7 +3056,7 @@ func TestChanSyncOweRevocation(t *testing.T) { // // Alice signs the next state, then Bob receives and sends his // revocation message. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -3061,12 +3069,12 @@ func TestChanSyncOweRevocation(t *testing.T) { if err != nil { t.Fatalf("unable to revoke bob commitment: %v", err) } - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("bob unable to sign commitment: %v", err) } - _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatalf("alice unable to recv revocation: %v", err) } @@ -3148,7 +3156,8 @@ func TestChanSyncOweRevocation(t *testing.T) { // TODO(roasbeef): restart bob too??? // We'll continue by then allowing bob to process Alice's revocation message. - if _, _, _, err := bobChannel.ReceiveRevocation(aliceRevocation); err != nil { + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { t.Fatalf("bob unable to recv revocation: %v", err) } @@ -3230,7 +3239,7 @@ func TestChanSyncOweRevocationAndCommit(t *testing.T) { // Progressing the exchange: Alice will send her signature, Bob will // receive, send a revocation and also a signature for Alice's state. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -3245,7 +3254,7 @@ func TestChanSyncOweRevocationAndCommit(t *testing.T) { if err != nil { t.Fatalf("unable to revoke bob commitment: %v", err) } - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("bob unable to sign commitment: %v", err) } @@ -3326,7 +3335,7 @@ func TestChanSyncOweRevocationAndCommit(t *testing.T) { // We'll now finish the state transition by having Alice process both // messages, and send her final revocation. - _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatalf("alice unable to recv revocation: %v", err) } @@ -3338,7 +3347,8 @@ func TestChanSyncOweRevocationAndCommit(t *testing.T) { if err != nil { t.Fatalf("alice unable to revoke commitment: %v", err) } - if _, _, _, err := bobChannel.ReceiveRevocation(aliceRevocation); err != nil { + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { t.Fatalf("bob unable to recv revocation: %v", err) } } @@ -3407,7 +3417,7 @@ func TestChanSyncOweRevocationAndCommitForceTransition(t *testing.T) { } // Bob signs the new state update, and sends the signature to Alice. - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("bob unable to sign commitment: %v", err) } @@ -3425,7 +3435,8 @@ func TestChanSyncOweRevocationAndCommitForceTransition(t *testing.T) { if err != nil { t.Fatalf("alice unable to revoke commitment: %v", err) } - if _, _, _, err := bobChannel.ReceiveRevocation(aliceRevocation); err != nil { + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { t.Fatalf("bob unable to recv revocation: %v", err) } @@ -3442,7 +3453,7 @@ func TestChanSyncOweRevocationAndCommitForceTransition(t *testing.T) { // Progressing the exchange: Alice will send her signature, with Bob // processing the new state locally. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -3554,7 +3565,7 @@ func TestChanSyncOweRevocationAndCommitForceTransition(t *testing.T) { // Now, we'll continue the exchange, sending Bob's revocation and // signature message to Alice, ending with Alice sending her revocation // message to Bob. - _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatalf("alice unable to recv revocation: %v", err) } @@ -3568,7 +3579,8 @@ func TestChanSyncOweRevocationAndCommitForceTransition(t *testing.T) { if err != nil { t.Fatalf("alice unable to revoke commitment: %v", err) } - if _, _, _, err := bobChannel.ReceiveRevocation(aliceRevocation); err != nil { + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { t.Fatalf("bob unable to recv revocation: %v", err) } } @@ -3651,7 +3663,7 @@ func TestChanSyncFailure(t *testing.T) { t.Fatalf("unable to recv bob's htlc: %v", err) } - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign next commit: %v", err) } @@ -3883,7 +3895,7 @@ func TestChannelRetransmissionFeeUpdate(t *testing.T) { // Now, Alice will send a new commitment to Bob, but we'll simulate a // connection failure, so Bob doesn't get her signature. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -3986,11 +3998,11 @@ func TestChannelRetransmissionFeeUpdate(t *testing.T) { if err != nil { t.Fatalf("unable to revoke bob commitment: %v", err) } - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("bob unable to sign commitment: %v", err) } - _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatalf("alice unable to recv revocation: %v", err) } @@ -4002,7 +4014,8 @@ func TestChannelRetransmissionFeeUpdate(t *testing.T) { if err != nil { t.Fatalf("alice unable to revoke commitment: %v", err) } - if _, _, _, err := bobChannel.ReceiveRevocation(aliceRevocation); err != nil { + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + if err != nil { t.Fatalf("bob unable to recv revocation: %v", err) } @@ -4131,7 +4144,7 @@ func TestFeeUpdateOldDiskFormat(t *testing.T) { // Now, Alice will send a new commitment to Bob, but we'll simulate a // connection failure, so Bob doesn't get the signature. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -4198,11 +4211,11 @@ func TestFeeUpdateOldDiskFormat(t *testing.T) { if err != nil { t.Fatalf("unable to revoke bob commitment: %v", err) } - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("bob unable to sign commitment: %v", err) } - _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatalf("alice unable to recv revocation: %v", err) } @@ -4214,7 +4227,7 @@ func TestFeeUpdateOldDiskFormat(t *testing.T) { if err != nil { t.Fatalf("alice unable to revoke commitment: %v", err) } - _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) if err != nil { t.Fatalf("bob unable to recv revocation: %v", err) } @@ -4524,7 +4537,7 @@ func TestSignCommitmentFailNotLockedIn(t *testing.T) { // If we now try to initiate a state update, then it should fail as // Alice is unable to actually create a new state. - _, _, err = aliceChannel.SignNextCommitment() + _, _, _, err = aliceChannel.SignNextCommitment() if err != ErrNoWindow { t.Fatalf("expected ErrNoWindow, instead have: %v", err) } @@ -4563,7 +4576,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { // We'll now manually initiate a state transition between Alice and // bob. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatal(err) } @@ -4577,7 +4590,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { } // Alice should detect that she doesn't need to forward any HTLC's. - fwdPkg, _, _, err := aliceChannel.ReceiveRevocation(bobRevocation) + fwdPkg, _, _, _, err := aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatal(err) } @@ -4592,7 +4605,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { // Now, have Bob initiate a transition to lock in the Adds sent by // Alice. - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatal(err) } @@ -4608,7 +4621,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { // Bob should now detect that he now has 2 incoming HTLC's that he can // forward along. - fwdPkg, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + fwdPkg, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) if err != nil { t.Fatal(err) } @@ -4645,7 +4658,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { // We'll now initiate another state transition, but this time Bob will // lead. - bobSig, bobHtlcSigs, err = bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err = bobChannel.SignNextCommitment() if err != nil { t.Fatal(err) } @@ -4661,7 +4674,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { // At this point, Bob receives the revocation from Alice, which is now // his signal to examine all the HTLC's that have been locked in to // process. - fwdPkg, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + fwdPkg, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) if err != nil { t.Fatal(err) } @@ -4680,7 +4693,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { // Now, begin another state transition led by Alice, and fail the second // HTLC part-way through the dance. - aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment() if err != nil { t.Fatal(err) } @@ -4707,7 +4720,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { // Alice should detect that she doesn't need to forward any Adds's, but // that the Fail has been locked in an can be forwarded. - _, adds, settleFails, err := aliceChannel.ReceiveRevocation(bobRevocation) + _, adds, settleFails, _, err := aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatal(err) } @@ -4749,7 +4762,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { // Have Alice initiate a state transition, which does not include the // HTLCs just readded to the channel state. - aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment() if err != nil { t.Fatal(err) } @@ -4764,7 +4777,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { // Alice should detect that she doesn't need to forward any HTLC's, as // the updates haven't been committed by Bob yet. - fwdPkg, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + fwdPkg, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatal(err) } @@ -4778,7 +4791,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { } // Now initiate a final update from Bob to lock in the final Fail. - bobSig, bobHtlcSigs, err = bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err = bobChannel.SignNextCommitment() if err != nil { t.Fatal(err) } @@ -4795,7 +4808,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { // Bob should detect that he has nothing to forward, as he hasn't // received any HTLCs. - fwdPkg, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + fwdPkg, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) if err != nil { t.Fatal(err) } @@ -4810,7 +4823,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { // Finally, have Bob initiate a state transition that locks in the Fail // added after the restart. - aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment() if err != nil { t.Fatal(err) } @@ -4825,7 +4838,7 @@ func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) { // When Alice receives the revocation, she should detect that she // can now forward the freshly locked-in Fail. - _, adds, settleFails, err = aliceChannel.ReceiveRevocation(bobRevocation) + _, adds, settleFails, _, err = aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatal(err) } @@ -4869,7 +4882,7 @@ func TestInvalidCommitSigError(t *testing.T) { } // Alice will now attempt to initiate a state transition. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign new commit: %v", err) } @@ -5075,7 +5088,7 @@ func TestChannelUnilateralClosePendingCommit(t *testing.T) { // With the HTLC added, we'll now manually initiate a state transition // from Alice to Bob. - _, _, err = aliceChannel.SignNextCommitment() + _, _, _, err = aliceChannel.SignNextCommitment() if err != nil { t.Fatal(err) } @@ -5861,7 +5874,7 @@ func TestChannelRestoreUpdateLogs(t *testing.T) { } // Let Alice sign a new state, which will include the HTLC just sent. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -5881,7 +5894,7 @@ func TestChannelRestoreUpdateLogs(t *testing.T) { // sent. However her local commitment chain still won't include the // state with the HTLC, since she hasn't received a new commitment // signature from Bob yet. - _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatalf("unable to recive revocation: %v", err) } @@ -5899,7 +5912,7 @@ func TestChannelRestoreUpdateLogs(t *testing.T) { // and remote commit chains are updated in an async fashion. Since the // remote chain was updated with the latest state (since Bob sent the // revocation earlier) we can keep advancing the remote commit chain. - aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -6064,7 +6077,7 @@ func TestChannelRestoreUpdateLogsFailedHTLC(t *testing.T) { restoreAndAssert(t, aliceChannel, 1, 0, 0, 0) // Bob sends a signature. - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -6083,7 +6096,7 @@ func TestChannelRestoreUpdateLogsFailedHTLC(t *testing.T) { if err != nil { t.Fatalf("unable to revoke commitment: %v", err) } - _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) if err != nil { t.Fatalf("bob unable to process alice's revocation: %v", err) } @@ -6097,7 +6110,7 @@ func TestChannelRestoreUpdateLogsFailedHTLC(t *testing.T) { // Now send a signature from Alice. This will give Bob a new commitment // where the HTLC is removed. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -6119,7 +6132,7 @@ func TestChannelRestoreUpdateLogsFailedHTLC(t *testing.T) { if err != nil { t.Fatalf("unable to revoke commitment: %v", err) } - _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatalf("unable to receive revocation: %v", err) } @@ -6177,7 +6190,7 @@ func TestDuplicateFailRejection(t *testing.T) { // We'll now have Bob sign a new commitment to lock in the HTLC fail // for Alice. - _, _, err = bobChannel.SignNextCommitment() + _, _, _, err = bobChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commit: %v", err) } @@ -6257,7 +6270,7 @@ func TestDuplicateSettleRejection(t *testing.T) { // We'll now have Bob sign a new commitment to lock in the HTLC fail // for Alice. - _, _, err = bobChannel.SignNextCommitment() + _, _, _, err = bobChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commit: %v", err) } @@ -6350,7 +6363,7 @@ func TestChannelRestoreCommitHeight(t *testing.T) { } // Let Alice sign a new state, which will include the HTLC just sent. - aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -6376,7 +6389,7 @@ func TestChannelRestoreCommitHeight(t *testing.T) { bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 0, 1, 0) // Alice receives the revocation, ACKing her pending commitment. - _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) if err != nil { t.Fatalf("unable to recive revocation: %v", err) } @@ -6388,7 +6401,7 @@ func TestChannelRestoreCommitHeight(t *testing.T) { // Now let Bob send the commitment signature making the HTLC lock in on // Alice's commitment. - bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -6411,7 +6424,7 @@ func TestChannelRestoreCommitHeight(t *testing.T) { aliceChannel = restoreAndAssertCommitHeights(t, aliceChannel, false, 0, 1, 1) - _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) + _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) if err != nil { t.Fatalf("unable to recive revocation: %v", err) } @@ -6433,7 +6446,7 @@ func TestChannelRestoreCommitHeight(t *testing.T) { // Send a new signature from Alice to Bob, making Alice have a pending // remote commitment. - aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -6463,7 +6476,7 @@ func TestChannelRestoreCommitHeight(t *testing.T) { // Sign a new state for Alice, making Bob have a pending remote // commitment. - bobSig, bobHtlcSigs, err = bobChannel.SignNextCommitment() + bobSig, bobHtlcSigs, _, err = bobChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commitment: %v", err) } @@ -6519,7 +6532,7 @@ func TestForceCloseBorkedState(t *testing.T) { // Do the commitment dance until Bob sends a revocation so Alice is // able to receive the revocation, and then also make a new state // herself. - aliceSigs, aliceHtlcSigs, err := aliceChannel.SignNextCommitment() + aliceSigs, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commit: %v", err) } @@ -6531,7 +6544,7 @@ func TestForceCloseBorkedState(t *testing.T) { if err != nil { t.Fatalf("unable to revoke bob commitment: %v", err) } - bobSigs, bobHtlcSigs, err := bobChannel.SignNextCommitment() + bobSigs, bobHtlcSigs, _, err := bobChannel.SignNextCommitment() if err != nil { t.Fatalf("unable to sign commit: %v", err) } @@ -6563,7 +6576,7 @@ func TestForceCloseBorkedState(t *testing.T) { // At this point, all channel mutating methods should now fail as they // shouldn't be able to proceed if the channel is borked. - _, _, _, err = aliceChannel.ReceiveRevocation(revokeMsg) + _, _, _, _, err = aliceChannel.ReceiveRevocation(revokeMsg) if err != channeldb.ErrChanBorked { t.Fatalf("advance commitment tail should have failed") } @@ -6571,7 +6584,7 @@ func TestForceCloseBorkedState(t *testing.T) { // We manually advance the commitment tail here since the above // ReceiveRevocation call will fail before it's actually advanced. aliceChannel.remoteCommitChain.advanceTail() - _, _, err = aliceChannel.SignNextCommitment() + _, _, _, err = aliceChannel.SignNextCommitment() if err != channeldb.ErrChanBorked { t.Fatalf("sign commitment should have failed: %v", err) } diff --git a/lnwallet/test_utils.go b/lnwallet/test_utils.go index 7d2abff4..92010e9d 100644 --- a/lnwallet/test_utils.go +++ b/lnwallet/test_utils.go @@ -496,7 +496,7 @@ func calcStaticFee(numHTLCs int) btcutil.Amount { // pending updates. This method is useful when testing interactions between two // live state machines. func ForceStateTransition(chanA, chanB *LightningChannel) error { - aliceSig, aliceHtlcSigs, err := chanA.SignNextCommitment() + aliceSig, aliceHtlcSigs, _, err := chanA.SignNextCommitment() if err != nil { return err } @@ -508,12 +508,12 @@ func ForceStateTransition(chanA, chanB *LightningChannel) error { if err != nil { return err } - bobSig, bobHtlcSigs, err := chanB.SignNextCommitment() + bobSig, bobHtlcSigs, _, err := chanB.SignNextCommitment() if err != nil { return err } - if _, _, _, err := chanA.ReceiveRevocation(bobRevocation); err != nil { + if _, _, _, _, err := chanA.ReceiveRevocation(bobRevocation); err != nil { return err } if err := chanA.ReceiveNewCommitment(bobSig, bobHtlcSigs); err != nil { @@ -524,7 +524,7 @@ func ForceStateTransition(chanA, chanB *LightningChannel) error { if err != nil { return err } - if _, _, _, err := chanB.ReceiveRevocation(aliceRevocation); err != nil { + if _, _, _, _, err := chanB.ReceiveRevocation(aliceRevocation); err != nil { return err } From fc617cd041de9d05bf1b9c790f225c41a4583094 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 16 May 2019 17:34:46 -0700 Subject: [PATCH 02/11] contractcourt: add new checkLocalChainActions method use in main state step In this commit, we add a new `checkLocalChainActions` method. This method differs from the existing `checkChainActions` method in that it's only concerned with actions we should take on chain for our local state based on the local _and_ remote state. This change ensures that we'll now to go to chain order to cancel an HTLC that was on the remote party's commitment transaction, but not our own. --- contractcourt/chain_watcher.go | 2 +- contractcourt/channel_arbitrator.go | 145 +++++++++++++++++++++++++--- 2 files changed, 134 insertions(+), 13 deletions(-) diff --git a/contractcourt/chain_watcher.go b/contractcourt/chain_watcher.go index b4b2865e..8b17e75a 100644 --- a/contractcourt/chain_watcher.go +++ b/contractcourt/chain_watcher.go @@ -54,7 +54,7 @@ type RemoteUnilateralCloseInfo struct { *lnwallet.UnilateralCloseSummary // CommitSet is the set of known valid commitments at the time the - // remote party's commitemnt hit the chain. + // remote party's commitment hit the chain. CommitSet CommitSet } diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index 703827f5..acff6e19 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -646,10 +646,20 @@ func (c *ChannelArbitrator) stateStep( // As a new block has been connected to the end of the main // chain, we'll check to see if we need to make any on-chain // claims on behalf of the channel contract that we're - // arbitrating for. - chainActions, err := c.checkChainActions(triggerHeight, trigger) + // 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 { + htlcs = c.activeHTLCs + } + chainActions, err := c.checkLocalChainActions( + triggerHeight, trigger, htlcs, false, + ) if err != nil { - return StateError, closeTx, err + return StateDefault, nil, err } // If there are no actions to be made, then we'll remain in the @@ -1105,15 +1115,15 @@ func (c *ChannelArbitrator) shouldGoOnChain(htlcExpiry, broadcastDelta, return currentHeight >= broadcastCutOff } -// checkChainActions is called for each new block connected to the end of the -// main chain. Given the new block height, this new method will examine all +// checkCommitChainActions is called for each new block connected to the end of +// the main chain. Given the new block height, this new method will examine all // active HTLC's, and determine if we need to go on-chain to claim any of them. // A map of action -> []htlc is returned, detailing what action (if any) should // be performed for each HTLC. For timed out HTLC's, once the commitment has // been sufficiently confirmed, the HTLC's should be canceled backwards. For // redeemed HTLC's, we should send the pre-image back to the incoming link. -func (c *ChannelArbitrator) checkChainActions(height uint32, - trigger transitionTrigger) (ChainActionMap, error) { +func (c *ChannelArbitrator) checkCommitChainActions(height uint32, + trigger transitionTrigger, htlcs htlcSet) (ChainActionMap, error) { // TODO(roasbeef): would need to lock channel? channel totem? // * race condition if adding and we broadcast, etc @@ -1127,8 +1137,7 @@ func (c *ChannelArbitrator) checkChainActions(height uint32, // First, we'll make an initial pass over the set of incoming and // outgoing HTLC's to decide if we need to go on chain at all. haveChainActions := false - localHTLCs := c.activeHTLCs[LocalHtlcSet] - for _, htlc := range localHTLCs.outgoingHTLCs { + 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( @@ -1149,7 +1158,7 @@ func (c *ChannelArbitrator) checkChainActions(height uint32, haveChainActions = haveChainActions || toChain } - for _, htlc := range localHTLCs.incomingHTLCs { + for _, htlc := range htlcs.incomingHTLCs { // We'll need to go on-chain to pull an incoming HTLC iff we // know the pre-image and it's close to timing out. We need to // ensure that we claim the funds that our rightfully ours @@ -1195,7 +1204,7 @@ func (c *ChannelArbitrator) checkChainActions(height uint32, // active outgoing HTLC's to see if we either need to: sweep them after // a timeout (then cancel backwards), cancel them backwards // immediately, or watch them as they're still active contracts. - for _, htlc := range localHTLCs.outgoingHTLCs { + for _, htlc := range htlcs.outgoingHTLCs { switch { // If the HTLC is dust, then we can cancel it backwards // immediately as there's no matching contract to arbitrate @@ -1250,7 +1259,7 @@ func (c *ChannelArbitrator) checkChainActions(height uint32, // observe the output on-chain if we don't In this last, case we'll // either learn of it eventually from the outgoing HTLC, or the sender // will timeout the HTLC. - for _, htlc := range localHTLCs.incomingHTLCs { + for _, htlc := range htlcs.incomingHTLCs { log.Tracef("ChannelArbitrator(%v): watching chain to decide "+ "action for incoming htlc=%x", c.cfg.ChanPoint, htlc.RHash[:]) @@ -1298,6 +1307,118 @@ func (c *ChannelArbitrator) isPreimageAvailable(hash lntypes.Hash) (bool, return preimageAvailable, nil } +// checkLocalChainActions is similar to checkCommitChainActions, but it also +// examines the set of HTLCs on the remote party's commitment. This allows us +// to ensure we're able to satisfy the HTLC timeout constraints for incoming vs +// outgoing HTLCs. +func (c *ChannelArbitrator) checkLocalChainActions( + height uint32, trigger transitionTrigger, + activeHTLCs map[HtlcSetKey]htlcSet, + commitsConfirmed bool) (ChainActionMap, error) { + + // First, we'll check our local chain actions as normal. This will only + // examine HTLCs on our local commitment (timeout or settle). + localCommitActions, err := c.checkCommitChainActions( + height, trigger, activeHTLCs[LocalHtlcSet], + ) + if err != nil { + return nil, err + } + + // Next, we'll examine the remote commitment (and maybe a dangling one) + // to see if the set difference of our HTLCs is non-empty. If so, then + // we may need to cancel back some HTLCs if we decide go to chain. + remoteDanglingActions := c.checkRemoteDanglingActions( + height, activeHTLCs, commitsConfirmed, + ) + + // Finally, we'll merge the two set of chain actions. + localActions := make(ChainActionMap) + for chainAction, htlcs := range localCommitActions { + localActions[chainAction] = append( + localActions[chainAction], htlcs..., + ) + } + for chainAction, htlcs := range remoteDanglingActions { + localActions[chainAction] = append( + localActions[chainAction], htlcs..., + ) + } + + return localActions, nil +} + +// checkRemoteChainActions examines the set of remote commitments for any HTLCs +// that are close to timing out. If we find any, then we'll return a set of +// chain actions for HTLCs that are on our commitment, but not theirs to cancel +// immediately. +func (c *ChannelArbitrator) checkRemoteDanglingActions( + height uint32, activeHTLCs map[HtlcSetKey]htlcSet, + commitsConfirmed bool) ChainActionMap { + + var ( + pendingRemoteHTLCs []channeldb.HTLC + localHTLCs = make(map[uint64]struct{}) + remoteHTLCs = make(map[uint64]channeldb.HTLC) + actionMap = make(ChainActionMap) + ) + + // First, we'll construct two sets of the outgoing HTLCs: those on our + // local commitment, and those that are on the remote commitment(s). + for htlcSetKey, htlcs := range activeHTLCs { + if htlcSetKey.IsRemote { + for _, htlc := range htlcs.outgoingHTLCs { + remoteHTLCs[htlc.HtlcIndex] = htlc + } + } else { + for _, htlc := range htlcs.outgoingHTLCs { + localHTLCs[htlc.HtlcIndex] = struct{}{} + } + } + } + + // With both sets constructed, we'll now compute the set difference of + // our two sets of HTLCs. This'll give us the HTLCs that exist on the + // remote commitment transaction, but not on ours. + for htlcIndex, htlc := range remoteHTLCs { + if _, ok := localHTLCs[htlcIndex]; ok { + continue + } + + pendingRemoteHTLCs = append(pendingRemoteHTLCs, htlc) + } + + // Finally, we'll examine all the pending remote HTLCs for those that + // have expired. If we find any, then we'll recommend that they be + // failed now so we can free up the incoming HTLC. + 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, + height, + ) + + // If we don't need to go to chain, and no commitments have + // been confirmed, then we can move on. Otherwise, if + // commitments have been confirmed, then we need to cancel back + // *all* of the pending remote HTLCS. + if !goToChain && !commitsConfirmed { + continue + } + + log.Tracef("ChannelArbitrator(%v): immediately "+ + "htlc=%x about to timeout on remote commitment", + c.cfg.ChanPoint, htlc.RHash[:]) + + actionMap[HtlcFailNowAction] = append( + actionMap[HtlcFailNowAction], htlc, + ) + } + + return actionMap +} + // prepContractResolutions is called either int he case that we decide we need // to go to chain, or the remote party goes to chain. Given a set of actions we // need to take for each HTLC, this method will return a set of contract From fb91f0be70f8192a7d1cbded8c9d8acc14c16062 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 16 May 2019 17:37:03 -0700 Subject: [PATCH 03/11] contractcourt: reconstruct chain actions at time of commitment confirmation In this commit, we change the behavior of the channel arb to no longer write chain actions to disk. Instead, using the new CommitSet struct, we'll replay our set of prior actions based on what actually got into the chain. As a result, we no longer need to write the chain actions at all, instead they're reconstructed at run time to determine decisions, and before any commitments are broadcast in order to determine if we need to go to chain at all. --- contractcourt/channel_arbitrator.go | 184 ++++++++++++++++++++++++---- 1 file changed, 157 insertions(+), 27 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index acff6e19..48b5853e 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -215,6 +215,20 @@ var ( RemotePendingHtlcSet = HtlcSetKey{IsRemote: true, IsPending: true} ) +// String returns a human readable string describing the target HtlcSetKey. +func (h HtlcSetKey) String() string { + switch h { + case LocalHtlcSet: + return "LocalHtlcSet" + case RemoteHtlcSet: + return "RemoteHtlcSet" + case RemotePendingHtlcSet: + return "RemotePendingHtlcSet" + default: + return "unknown HtlcSetKey" + } +} + // ChannelArbitrator is the on-chain arbitrator for a particular channel. The // struct will keep in sync with the current set of HTLCs on the commitment // transaction. The job of the attendant is to go on-chain to either settle or @@ -824,7 +838,7 @@ func (c *ChannelArbitrator) stateStep( case StateContractClosed: // First, we'll fetch our chain actions, and both sets of // resolutions so we can process them. - chainActions, err := c.log.FetchChainActions() + _, err := c.log.FetchChainActions() if err != nil { log.Errorf("unable to fetch chain actions: %v", err) return StateError, closeTx, err @@ -836,10 +850,10 @@ func (c *ChannelArbitrator) stateStep( return StateError, closeTx, err } - // If the resolution is empty, then we're done here. We don't - // need to launch any resolvers, and can go straight to our - // final state. - if contractResolutions.IsEmpty() { + // If the resolution is empty, and we have no HTLCs at all to + // tend to, then we're done here. We don't need to launch any + // resolvers, and can go straight to our final state. + if contractResolutions.IsEmpty() && confCommitSet.IsEmpty() { log.Infof("ChannelArbitrator(%v): contract "+ "resolutions empty, marking channel as fully resolved!", c.cfg.ChanPoint) @@ -872,7 +886,8 @@ func (c *ChannelArbitrator) stateStep( // actions, wen create the structures we need to resolve all // outstanding contracts. htlcResolvers, pktsToSend, err := c.prepContractResolutions( - chainActions, contractResolutions, triggerHeight, + contractResolutions, triggerHeight, trigger, + confCommitSet, ) if err != nil { log.Errorf("ChannelArbitrator(%v): unable to "+ @@ -1088,6 +1103,13 @@ func (c ChainAction) String() string { // be acted upon for a given action type. The channel type ChainActionMap map[ChainAction][]channeldb.HTLC +// Merge merges the passed chain actions with the target chain action map. +func (c ChainActionMap) Merge(actions ChainActionMap) { + for chainAction, htlcs := range actions { + c[chainAction] = append(c[chainAction], htlcs...) + } +} + // shouldGoOnChain takes into account the absolute timeout of the HTLC, if the // confirmation delta that we need is close, and returns a bool indicating if // we should go on chain to claim. We do this rather than waiting up until the @@ -1192,7 +1214,7 @@ func (c *ChannelArbitrator) checkCommitChainActions(height uint32, // If we don't have any actions to make, then we'll return an empty // action map. We only do this if this was a chain trigger though, as - // if we're going to broadcast the commitment (or the remote party) did + // if we're going to broadcast the commitment (or the remote party did) // we're *forced* to act on each HTLC. if !haveChainActions && trigger == chainTrigger { log.Tracef("ChannelArbitrator(%v): no actions to take at "+ @@ -1333,25 +1355,15 @@ func (c *ChannelArbitrator) checkLocalChainActions( ) // Finally, we'll merge the two set of chain actions. - localActions := make(ChainActionMap) - for chainAction, htlcs := range localCommitActions { - localActions[chainAction] = append( - localActions[chainAction], htlcs..., - ) - } - for chainAction, htlcs := range remoteDanglingActions { - localActions[chainAction] = append( - localActions[chainAction], htlcs..., - ) - } + localCommitActions.Merge(remoteDanglingActions) - return localActions, nil + return localCommitActions, nil } -// checkRemoteChainActions examines the set of remote commitments for any HTLCs -// that are close to timing out. If we find any, then we'll return a set of -// chain actions for HTLCs that are on our commitment, but not theirs to cancel -// immediately. +// checkRemoteDanglingActions examines the set of remote commitments for any +// HTLCs that are close to timing out. If we find any, then we'll return a set +// of chain actions for HTLCs that are on our commitment, but not theirs to +// cancel immediately. func (c *ChannelArbitrator) checkRemoteDanglingActions( height uint32, activeHTLCs map[HtlcSetKey]htlcSet, commitsConfirmed bool) ChainActionMap { @@ -1407,8 +1419,8 @@ func (c *ChannelArbitrator) checkRemoteDanglingActions( continue } - log.Tracef("ChannelArbitrator(%v): immediately "+ - "htlc=%x about to timeout on remote commitment", + log.Tracef("ChannelArbitrator(%v): immediately failing "+ + "htlc=%x from remote commitment", c.cfg.ChanPoint, htlc.RHash[:]) actionMap[HtlcFailNowAction] = append( @@ -1419,15 +1431,133 @@ func (c *ChannelArbitrator) checkRemoteDanglingActions( return actionMap } +// checkRemoteChainActions examines the two possible remote commitment chains +// and returns the set of chain actions we need to carry out if the remote +// commitment (non pending) confirms. The pendingConf indicates if the pending +// remote commitment confirmed. This is similar to checkCommitChainActions, but +// we'll immediately fail any HTLCs on the pending remote commit, but not the +// remote commit (or the other way around). +func (c *ChannelArbitrator) checkRemoteChainActions( + height uint32, trigger transitionTrigger, + activeHTLCs map[HtlcSetKey]htlcSet, + pendingConf bool) (ChainActionMap, error) { + + // First, we'll examine all the normal chain actions on the remote + // commitment that confirmed. + confHTLCs := activeHTLCs[RemoteHtlcSet] + if pendingConf { + confHTLCs = activeHTLCs[RemotePendingHtlcSet] + } + remoteCommitActions, err := c.checkCommitChainActions( + height, trigger, confHTLCs, + ) + if err != nil { + return nil, err + } + + // With this actions computed, we'll now check the diff of the HTLCs on + // the commitments, and cancel back any that are on the pending but not + // the non-pending. + remoteDiffActions := c.checkRemoteDiffActions( + height, activeHTLCs, pendingConf, + ) + + // Finally, we'll merge all the chain actions and the final set of + // chain actions. + remoteCommitActions.Merge(remoteDiffActions) + return remoteCommitActions, nil +} + +// checkRemoteDiffActions checks the set difference of the HTLCs on the remote +// confirmed commit and remote dangling 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(height uint32, + activeHTLCs map[HtlcSetKey]htlcSet, + pendingConf bool) ChainActionMap { + + // First, we'll partition the HTLCs into those that are present on the + // confirmed commitment, and those on the dangling commitment. + confHTLCs := activeHTLCs[RemoteHtlcSet] + danglingHTLCs := activeHTLCs[RemotePendingHtlcSet] + if pendingConf { + confHTLCs = activeHTLCs[RemotePendingHtlcSet] + danglingHTLCs = activeHTLCs[RemoteHtlcSet] + } + + // Next, we'll create a set of all the HTLCs confirmed commitment. + remoteHtlcs := make(map[uint64]struct{}) + for _, htlc := range confHTLCs.outgoingHTLCs { + remoteHtlcs[htlc.HtlcIndex] = struct{}{} + } + + // With the remote HTLCs assembled, we'll mark any HTLCs only on the + // remote dangling commitment to be failed asap. + actionMap := make(ChainActionMap) + for _, htlc := range danglingHTLCs.outgoingHTLCs { + if _, ok := remoteHtlcs[htlc.HtlcIndex]; ok { + continue + } + + actionMap[HtlcFailNowAction] = append( + actionMap[HtlcFailNowAction], htlc, + ) + + log.Tracef("ChannelArbitrator(%v): immediately failing "+ + "htlc=%x from remote commitment", + c.cfg.ChanPoint, htlc.RHash[:]) + } + + return actionMap +} + // prepContractResolutions is called either int he case that we decide we need // to go to chain, or the remote party goes to chain. Given a set of actions we // need to take for each HTLC, this method will return a set of contract // resolvers that will resolve the contracts on-chain if needed, and also a set // of packets to send to the htlcswitch in order to ensure all incoming HTLC's // are properly resolved. -func (c *ChannelArbitrator) prepContractResolutions(htlcActions ChainActionMap, +func (c *ChannelArbitrator) prepContractResolutions( contractResolutions *ContractResolutions, height uint32, -) ([]ContractResolver, []ResolutionMsg, error) { + trigger transitionTrigger, + confCommitSet *CommitSet) ([]ContractResolver, []ResolutionMsg, error) { + + htlcSets := confCommitSet.toActiveHTLCSets() + + // 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. + var ( + htlcActions ChainActionMap + err error + ) + switch *confCommitSet.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: + htlcActions, err = c.checkLocalChainActions( + height, trigger, htlcSets, true, + ) + + // If the remote commitment confirmed, then we'll grab all the chain + // actions for the remote commit, and check the pending commit for any + // HTLCS we need to handle immediately (dust). + case RemoteHtlcSet: + htlcActions, err = c.checkRemoteChainActions( + height, trigger, htlcSets, false, + ) + + // Otherwise, the remote pending commitment confirmed, so we'll examine + // the HTLCs on that unrevoked dangling commitment. + case RemotePendingHtlcSet: + htlcActions, err = c.checkRemoteChainActions( + height, trigger, htlcSets, true, + ) + } + if err != nil { + return nil, 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 From 877b8c55d3d191d6c7ea6cbd9b2c278487d89787 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 16 May 2019 17:37:58 -0700 Subject: [PATCH 04/11] contractcourt: stop writing chain actions to disk We don't need them as we'll just reconstruct the chain actions once a commitment has actually confirmed. --- contractcourt/channel_arbitrator.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index 48b5853e..0861aab0 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -693,9 +693,6 @@ func (c *ChannelArbitrator) stateStep( newLogClosure(func() string { return spew.Sdump(chainActions) })) - if err := c.log.LogChainActions(chainActions); 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 @@ -838,11 +835,6 @@ func (c *ChannelArbitrator) stateStep( case StateContractClosed: // First, we'll fetch our chain actions, and both sets of // resolutions so we can process them. - _, err := c.log.FetchChainActions() - if err != nil { - log.Errorf("unable to fetch chain actions: %v", err) - return StateError, closeTx, err - } contractResolutions, err := c.log.FetchContractResolutions() if err != nil { log.Errorf("unable to fetch contract resolutions: %v", From 8a34b1ae8897189144e7fd1d8478273f2590ad17 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 16 May 2019 17:38:44 -0700 Subject: [PATCH 05/11] contractcourt: only send resolution messages if we have any to send --- contractcourt/channel_arbitrator.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index 0861aab0..28040961 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -895,11 +895,14 @@ func (c *ChannelArbitrator) stateStep( // With the commitment broadcast, we'll then send over all // messages we can send immediately. - err = c.cfg.DeliverResolutionMsg(pktsToSend...) - if err != nil { - // TODO(roasbeef): make sure packet sends are idempotent - log.Errorf("unable to send pkts: %v", err) - return StateError, closeTx, err + if len(pktsToSend) != 0 { + err := c.cfg.DeliverResolutionMsg(pktsToSend...) + if err != nil { + // TODO(roasbeef): make sure packet sends are + // idempotent + log.Errorf("unable to send pkts: %v", err) + return StateError, closeTx, err + } } log.Debugf("ChannelArbitrator(%v): inserting %v contract "+ From 087e22d81798571fbe20ce01aac7007af5ac9010 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 16 May 2019 17:40:06 -0700 Subject: [PATCH 06/11] contractcourt: obtain supplementary HTLC info from the htlcSets Since we no longer have up to date chain actions on disk, we'll use the HTLC sets in memory which contain the necessary information we need to in order to obtain the HTLC amounts. --- contractcourt/channel_arbitrator.go | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index 28040961..b8862a89 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -438,18 +438,24 @@ func (c *ChannelArbitrator) relaunchResolvers() error { commitHash := contractResolutions.CommitHash // Reconstruct the htlc outpoints and data from the chain action log. - // The purpose of the constructed htlc map is to supplement to resolvers - // restored from database with extra data. Ideally this data is stored - // as part of the resolver in the log. This is a workaround to prevent a - // db migration. + // The purpose of the constructed htlc map is to supplement to + // resolvers restored from database with extra data. Ideally this data + // is stored as part of the resolver in the log. This is a workaround + // to prevent a db migration. We use all available htlc sets here in + // order to ensure we have complete coverage. htlcMap := make(map[wire.OutPoint]*channeldb.HTLC) - chainActions, err := c.log.FetchChainActions() - if err != nil { - log.Errorf("unable to fetch chain actions: %v", err) - return err - } - for _, htlcs := range chainActions { - for _, htlc := range htlcs { + for _, htlcs := range c.activeHTLCs { + for _, htlc := range htlcs.incomingHTLCs { + htlc := htlc + outpoint := wire.OutPoint{ + Hash: commitHash, + Index: uint32(htlc.OutputIndex), + } + htlcMap[outpoint] = &htlc + } + + for _, htlc := range htlcs.outgoingHTLCs { + htlc := htlc outpoint := wire.OutPoint{ Hash: commitHash, Index: uint32(htlc.OutputIndex), From ea7bae849202007103e1a7d7cf77d55facef83b7 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 16 May 2019 17:41:48 -0700 Subject: [PATCH 07/11] contractcourt: remove the now unused chain actions methods We still keep the `actionsBucketKey` variable around so current contracts will clean up the existing state once they've been fully resolved. --- contractcourt/briefcase.go | 95 ----------------------- contractcourt/briefcase_test.go | 130 -------------------------------- 2 files changed, 225 deletions(-) diff --git a/contractcourt/briefcase.go b/contractcourt/briefcase.go index 3af1da08..43a7c722 100644 --- a/contractcourt/briefcase.go +++ b/contractcourt/briefcase.go @@ -9,7 +9,6 @@ import ( "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/wire" "github.com/coreos/bbolt" - "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/lnwallet" ) @@ -82,17 +81,6 @@ type ArbitratorLog interface { // contract resolutions from persistent storage. FetchContractResolutions() (*ContractResolutions, error) - // LogChainActions stores a set of chain actions which are derived from - // our set of active contracts, and the on-chain state. We'll write - // this et of cations when: we decide to go on-chain to resolve a - // contract, or we detect that the remote party has gone on-chain. - LogChainActions(ChainActionMap) error - - // FetchChainActions attempts to fetch the set of previously stored - // chain actions. We'll use this upon restart to properly advance our - // state machine forward. - FetchChainActions() (ChainActionMap, error) - // WipeHistory is to be called ONLY once *all* contracts have been // fully resolved, and the channel closure if finalized. This method // will delete all on-disk state within the persistent log. @@ -720,88 +708,6 @@ func (b *boltArbitratorLog) FetchContractResolutions() (*ContractResolutions, er return c, err } -// LogChainActions stores a set of chain actions which are derived from our set -// of active contracts, and the on-chain state. We'll write this et of cations -// when: we decide to go on-chain to resolve a contract, or we detect that the -// remote party has gone on-chain. -// -// NOTE: Part of the ContractResolver interface. -func (b *boltArbitratorLog) LogChainActions(actions ChainActionMap) error { - return b.db.Batch(func(tx *bbolt.Tx) error { - scopeBucket, err := tx.CreateBucketIfNotExists(b.scopeKey[:]) - if err != nil { - return err - } - - actionsBucket, err := scopeBucket.CreateBucketIfNotExists( - actionsBucketKey, - ) - if err != nil { - return err - } - - for chainAction, htlcs := range actions { - var htlcBuf bytes.Buffer - err := channeldb.SerializeHtlcs(&htlcBuf, htlcs...) - if err != nil { - return err - } - - actionKey := []byte{byte(chainAction)} - err = actionsBucket.Put(actionKey, htlcBuf.Bytes()) - if err != nil { - return err - } - } - - return nil - }) -} - -// FetchChainActions attempts to fetch the set of previously stored chain -// actions. We'll use this upon restart to properly advance our state machine -// forward. -// -// NOTE: Part of the ContractResolver interface. -func (b *boltArbitratorLog) FetchChainActions() (ChainActionMap, error) { - actionsMap := make(ChainActionMap) - - err := b.db.View(func(tx *bbolt.Tx) error { - scopeBucket := tx.Bucket(b.scopeKey[:]) - if scopeBucket == nil { - return errScopeBucketNoExist - } - - actionsBucket := scopeBucket.Bucket(actionsBucketKey) - if actionsBucket == nil { - return errNoActions - } - - return actionsBucket.ForEach(func(action, htlcBytes []byte) error { - if htlcBytes == nil { - return nil - } - - chainAction := ChainAction(action[0]) - - htlcReader := bytes.NewReader(htlcBytes) - htlcs, err := channeldb.DeserializeHtlcs(htlcReader) - if err != nil { - return err - } - - actionsMap[chainAction] = htlcs - - return nil - }) - }) - if err != nil { - return nil, err - } - - return actionsMap, nil -} - // WipeHistory is to be called ONLY once *all* contracts have been fully // resolved, and the channel closure if finalized. This method will delete all // on-disk state within the persistent log. @@ -835,7 +741,6 @@ func (b *boltArbitratorLog) WipeHistory() error { return err } if err := scopeBucket.DeleteBucket(contractsBucketKey); err != nil { - fmt.Println("nah") return err } diff --git a/contractcourt/briefcase_test.go b/contractcourt/briefcase_test.go index 81a78afc..0777f2ba 100644 --- a/contractcourt/briefcase_test.go +++ b/contractcourt/briefcase_test.go @@ -15,11 +15,8 @@ import ( "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcd/wire" "github.com/coreos/bbolt" - "github.com/davecgh/go-spew/spew" - "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/lnwallet" - "github.com/lightningnetwork/lnd/lnwire" ) var ( @@ -565,133 +562,6 @@ func TestContractResolutionsStorage(t *testing.T) { } } -// TestChainActionStorage tests that were able to properly store a set of chain -// actions, and then retrieve the same set of chain actions from disk. -func TestChainActionStorage(t *testing.T) { - t.Parallel() - - // First, we'll create a test instance of the ArbitratorLog - // implementation backed by boltdb. - testLog, cleanUp, err := newTestBoltArbLog( - testChainHash, testChanPoint2, - ) - if err != nil { - t.Fatalf("unable to create test log: %v", err) - } - defer cleanUp() - - chainActions := ChainActionMap{ - NoAction: []channeldb.HTLC{ - { - RHash: testPreimage, - Amt: lnwire.MilliSatoshi(prand.Uint64()), - RefundTimeout: prand.Uint32(), - OutputIndex: int32(prand.Uint32()), - Incoming: true, - HtlcIndex: prand.Uint64(), - LogIndex: prand.Uint64(), - OnionBlob: make([]byte, 0), - Signature: make([]byte, 0), - }, - }, - HtlcTimeoutAction: []channeldb.HTLC{ - { - RHash: testPreimage, - Amt: lnwire.MilliSatoshi(prand.Uint64()), - RefundTimeout: prand.Uint32(), - OutputIndex: int32(prand.Uint32()), - Incoming: true, - HtlcIndex: prand.Uint64(), - LogIndex: prand.Uint64(), - OnionBlob: make([]byte, 0), - Signature: make([]byte, 0), - }, - }, - HtlcClaimAction: []channeldb.HTLC{ - { - RHash: testPreimage, - Amt: lnwire.MilliSatoshi(prand.Uint64()), - RefundTimeout: prand.Uint32(), - OutputIndex: int32(prand.Uint32()), - Incoming: true, - HtlcIndex: prand.Uint64(), - LogIndex: prand.Uint64(), - OnionBlob: make([]byte, 0), - Signature: make([]byte, 0), - }, - }, - HtlcFailNowAction: []channeldb.HTLC{ - { - RHash: testPreimage, - Amt: lnwire.MilliSatoshi(prand.Uint64()), - RefundTimeout: prand.Uint32(), - OutputIndex: int32(prand.Uint32()), - Incoming: true, - HtlcIndex: prand.Uint64(), - LogIndex: prand.Uint64(), - OnionBlob: make([]byte, 0), - Signature: make([]byte, 0), - }, - }, - HtlcOutgoingWatchAction: []channeldb.HTLC{ - { - RHash: testPreimage, - Amt: lnwire.MilliSatoshi(prand.Uint64()), - RefundTimeout: prand.Uint32(), - OutputIndex: int32(prand.Uint32()), - Incoming: true, - HtlcIndex: prand.Uint64(), - LogIndex: prand.Uint64(), - OnionBlob: make([]byte, 0), - Signature: make([]byte, 0), - }, - }, - HtlcIncomingWatchAction: []channeldb.HTLC{ - { - RHash: testPreimage, - Amt: lnwire.MilliSatoshi(prand.Uint64()), - RefundTimeout: prand.Uint32(), - OutputIndex: int32(prand.Uint32()), - Incoming: true, - HtlcIndex: prand.Uint64(), - LogIndex: prand.Uint64(), - OnionBlob: make([]byte, 0), - Signature: make([]byte, 0), - }, - }, - } - - // With our set of test chain actions constructed, we'll now insert - // them into the database, retrieve them, then assert equality with the - // set of chain actions create above. - if err := testLog.LogChainActions(chainActions); err != nil { - t.Fatalf("unable to write chain actions: %v", err) - } - diskActions, err := testLog.FetchChainActions() - if err != nil { - t.Fatalf("unable to read chain actions: %v", err) - } - - for k, contracts := range chainActions { - diskContracts := diskActions[k] - if !reflect.DeepEqual(contracts, diskContracts) { - t.Fatalf("chain action mismatch: expected %v, got %v", - spew.Sdump(contracts), spew.Sdump(diskContracts)) - } - } - - // We'll now delete the state, then attempt to retrieve the set of - // chain actions, no resolutions should be found. - if err := testLog.WipeHistory(); err != nil { - t.Fatalf("unable to wipe log: %v", err) - } - actions, err := testLog.FetchChainActions() - if len(actions) != 0 { - t.Fatalf("expected no chain actions, instead found: %v", - len(actions)) - } -} - // TestStateMutation tests that we're able to properly mutate the state of the // log, then retrieve that same mutated state from disk. func TestStateMutation(t *testing.T) { From b4a116fd07a94b0bb756521687a0dd1bbe37b52d Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 16 May 2019 17:45:26 -0700 Subject: [PATCH 08/11] contractcourt: update TestChannelArbitratorLocalForceClosePendingHtlc to assert resolution msg delivery --- contractcourt/channel_arbitrator_test.go | 70 +++++++++++++++--------- 1 file changed, 43 insertions(+), 27 deletions(-) diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index cae8636b..c0a87e1b 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/contractcourt/channel_arbitrator_test.go @@ -108,16 +108,6 @@ func (b *mockArbitratorLog) FetchContractResolutions() (*ContractResolutions, er return b.resolutions, nil } -func (b *mockArbitratorLog) LogChainActions(actions ChainActionMap) error { - b.chainActions = actions - return nil -} - -func (b *mockArbitratorLog) FetchChainActions() (ChainActionMap, error) { - actionsMap := b.chainActions - return actionsMap, nil -} - func (b *mockArbitratorLog) WipeHistory() error { return nil } @@ -144,8 +134,11 @@ func (*mockChainIO) GetBlock(blockHash *chainhash.Hash) (*wire.MsgBlock, error) } func createTestChannelArbitrator(log ArbitratorLog) (*ChannelArbitrator, - chan struct{}, error) { + chan struct{}, chan []ResolutionMsg, chan *chainntnfs.BlockEpoch, error) { + + blockEpochs := make(chan *chainntnfs.BlockEpoch) blockEpoch := &chainntnfs.BlockEpochEvent{ + Epochs: blockEpochs, Cancel: func() {}, } @@ -158,13 +151,16 @@ func createTestChannelArbitrator(log ArbitratorLog) (*ChannelArbitrator, ContractBreach: make(chan *lnwallet.BreachRetribution, 1), } + resolutionChan := make(chan []ResolutionMsg, 1) + chainIO := &mockChainIO{} chainArbCfg := ChainArbitratorConfig{ ChainIO: chainIO, PublishTx: func(*wire.MsgTx) error { return nil }, - DeliverResolutionMsg: func(...ResolutionMsg) error { + DeliverResolutionMsg: func(msgs ...ResolutionMsg) error { + resolutionChan <- msgs return nil }, OutgoingBroadcastDelta: 5, @@ -213,7 +209,9 @@ func createTestChannelArbitrator(log ArbitratorLog) (*ChannelArbitrator, ChainEvents: chanEvents, } - return NewChannelArbitrator(arbCfg, nil, log), resolvedChan, nil + htlcSets := make(map[HtlcSetKey]htlcSet) + return NewChannelArbitrator(arbCfg, htlcSets, log), resolvedChan, + resolutionChan, blockEpochs, nil } // assertState checks that the ChannelArbitrator is in the state we expect it @@ -233,7 +231,7 @@ func TestChannelArbitratorCooperativeClose(t *testing.T) { newStates: make(chan ArbitratorState, 5), } - chanArb, resolved, err := createTestChannelArbitrator(log) + chanArb, resolved, _, _, err := createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -306,7 +304,7 @@ func TestChannelArbitratorRemoteForceClose(t *testing.T) { newStates: make(chan ArbitratorState, 5), } - chanArb, resolved, err := createTestChannelArbitrator(log) + chanArb, resolved, _, _, err := createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -360,7 +358,7 @@ func TestChannelArbitratorLocalForceClose(t *testing.T) { newStates: make(chan ArbitratorState, 5), } - chanArb, resolved, err := createTestChannelArbitrator(log) + chanArb, resolved, _, _, err := createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -467,7 +465,9 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { resolvers: make(map[ContractResolver]struct{}), } - chanArb, resolved, err := createTestChannelArbitrator(arbLog) + chanArb, resolved, resolutions, _, err := createTestChannelArbitrator( + arbLog, + ) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -627,6 +627,22 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { // spent. notifier.spendChan <- &chainntnfs.SpendDetail{SpendingTx: closeTx} + // Finally, we should also receive a resolution message instructing the + // switch to cancel back the HTLC. + select { + case msgs := <-resolutions: + if len(msgs) != 1 { + t.Fatalf("expected 1 message, instead got %v", len(msgs)) + } + + if msgs[0].HtlcIndex != htlcIndex { + t.Fatalf("wrong htlc index: expected %v, got %v", + htlcIndex, msgs[0].HtlcIndex) + } + case <-time.After(5 * time.Second): + t.Fatalf("resolution msgs not sent") + } + // As this is our own commitment transaction, the HTLC will go through // to the second level. Channel arbitrator should still not be marked // as resolved. @@ -657,7 +673,7 @@ func TestChannelArbitratorLocalForceCloseRemoteConfirmed(t *testing.T) { newStates: make(chan ArbitratorState, 5), } - chanArb, resolved, err := createTestChannelArbitrator(log) + chanArb, resolved, _, _, err := createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -766,7 +782,7 @@ func TestChannelArbitratorLocalForceDoubleSpend(t *testing.T) { newStates: make(chan ArbitratorState, 5), } - chanArb, resolved, err := createTestChannelArbitrator(log) + chanArb, resolved, _, _, err := createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -874,7 +890,7 @@ func TestChannelArbitratorPersistence(t *testing.T) { failLog: true, } - chanArb, resolved, err := createTestChannelArbitrator(log) + chanArb, resolved, _, _, err := createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -908,7 +924,7 @@ func TestChannelArbitratorPersistence(t *testing.T) { chanArb.Stop() // Create a new arbitrator with the same log. - chanArb, resolved, err = createTestChannelArbitrator(log) + chanArb, resolved, _, _, err = createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -941,7 +957,7 @@ func TestChannelArbitratorPersistence(t *testing.T) { chanArb.Stop() // Create yet another arbitrator with the same log. - chanArb, resolved, err = createTestChannelArbitrator(log) + chanArb, resolved, _, _, err = createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -975,7 +991,7 @@ func TestChannelArbitratorPersistence(t *testing.T) { // Create a new arbitrator, and now make fetching resolutions succeed. log.failFetch = nil - chanArb, resolved, err = createTestChannelArbitrator(log) + chanArb, resolved, _, _, err = createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -1071,7 +1087,7 @@ func TestChannelArbitratorCommitFailure(t *testing.T) { failCommitState: test.expectedStates[0], } - chanArb, resolved, err := createTestChannelArbitrator(log) + chanArb, resolved, _, _, err := createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -1111,7 +1127,7 @@ func TestChannelArbitratorCommitFailure(t *testing.T) { // Start the arbitrator again, with IsPendingClose reporting // the channel closed in the database. - chanArb, resolved, err = createTestChannelArbitrator(log) + chanArb, resolved, _, _, err = createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -1157,7 +1173,7 @@ func TestChannelArbitratorEmptyResolutions(t *testing.T) { failFetch: errNoResolutions, } - chanArb, _, err := createTestChannelArbitrator(log) + chanArb, _, _, _, err := createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } @@ -1195,7 +1211,7 @@ func TestChannelArbitratorAlreadyForceClosed(t *testing.T) { log := &mockArbitratorLog{ state: StateCommitmentBroadcasted, } - chanArb, _, err := createTestChannelArbitrator(log) + chanArb, _, _, _, err := createTestChannelArbitrator(log) if err != nil { t.Fatalf("unable to create ChannelArbitrator: %v", err) } From 086f4eb8b3fe9d92f7f3063269e8971a0b0f752a Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 16 May 2019 17:47:51 -0700 Subject: [PATCH 09/11] contractcourt: add new TestChannelArbitratorDanglingCommitForceClose test --- contractcourt/channel_arbitrator_test.go | 228 +++++++++++++++++++++++ 1 file changed, 228 insertions(+) diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index c0a87e1b..903ca564 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/contractcourt/channel_arbitrator_test.go @@ -1244,3 +1244,231 @@ func TestChannelArbitratorAlreadyForceClosed(t *testing.T) { t.Fatal("expected to receive error response") } } + +// TestChannelArbitratorDanglingCommitForceClose tests that if there're HTLCs +// on the remote party's commitment, but not ours, and they're about to time +// out, then we'll go on chain so we can cancel back the HTLCs on the incoming +// commitment. +func TestChannelArbitratorDanglingCommitForceClose(t *testing.T) { + t.Parallel() + + type testCase struct { + htlcExpired bool + remotePendingHTLC bool + confCommit HtlcSetKey + } + var testCases []testCase + + testOptions := []bool{true, false} + confOptions := []HtlcSetKey{ + LocalHtlcSet, RemoteHtlcSet, RemotePendingHtlcSet, + } + for _, htlcExpired := range testOptions { + for _, remotePendingHTLC := range testOptions { + for _, commitConf := range confOptions { + switch { + // If the HTLC is on the remote commitment, and + // that one confirms, then there's no special + // behavior, we should play all the HTLCs on + // that remote commitment as normal. + case !remotePendingHTLC && commitConf == RemoteHtlcSet: + fallthrough + + // If the HTLC is on the remote pending, and + // that confirms, then we don't have any + // special actions. + case remotePendingHTLC && commitConf == RemotePendingHtlcSet: + continue + } + + testCases = append(testCases, testCase{ + htlcExpired: htlcExpired, + remotePendingHTLC: remotePendingHTLC, + confCommit: commitConf, + }) + } + } + } + + fmt.Println(len(testCases)) + + for _, testCase := range testCases { + testCase := testCase + testName := fmt.Sprintf("testCase: htlcExpired=%v,"+ + "remotePendingHTLC=%v,remotePendingCommitConf=%v", + testCase.htlcExpired, testCase.remotePendingHTLC, + testCase.confCommit) + + t.Run(testName, func(t *testing.T) { + t.Parallel() + + arbLog := &mockArbitratorLog{ + state: StateDefault, + newStates: make(chan ArbitratorState, 5), + resolvers: make(map[ContractResolver]struct{}), + } + + chanArb, _, resolutions, blockEpochs, err := createTestChannelArbitrator( + arbLog, + ) + if err != nil { + t.Fatalf("unable to create ChannelArbitrator: %v", err) + } + if err := chanArb.Start(); err != nil { + t.Fatalf("unable to start ChannelArbitrator: %v", err) + } + defer chanArb.Stop() + + // 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) + + htlcKey := RemoteHtlcSet + if testCase.remotePendingHTLC { + htlcKey = RemotePendingHtlcSet + } + + // Next, we'll send it a new HTLC that is set to expire + // in 10 blocks, this HTLC will only appear on the + // commitment transaction of the _remote_ party. + htlcIndex := uint64(99) + htlcExpiry := uint32(10) + danglingHTLC := channeldb.HTLC{ + Incoming: false, + Amt: 10000, + HtlcIndex: htlcIndex, + RefundTimeout: htlcExpiry, + } + htlcUpdates <- &ContractUpdate{ + HtlcKey: htlcKey, + Htlcs: []channeldb.HTLC{danglingHTLC}, + } + + // At this point, we now have a split commitment state + // from the PoV of the channel arb. There's now an HTLC + // that only exists on the commitment transaction of + // the remote party. + errChan := make(chan error, 1) + respChan := make(chan *wire.MsgTx, 1) + switch { + // If we want an HTLC expiration trigger, then We'll + // now mine a block (height 5), which is 5 blocks away + // (our grace delta) from the expiry of that HTLC. + case testCase.htlcExpired: + blockEpochs <- &chainntnfs.BlockEpoch{Height: 5} + + // Otherwise, we'll just trigger a regular force close + // request. + case !testCase.htlcExpired: + chanArb.forceCloseReqs <- &forceCloseReq{ + errResp: errChan, + closeTx: respChan, + } + + } + + // At this point, the resolver should now have + // determined that it needs to go to chain in order to + // block off the redemption path so it can cancel the + // incoming HTLC. + assertStateTransitions( + t, arbLog.newStates, StateBroadcastCommit, + StateCommitmentBroadcasted, + ) + + // Next we'll craft a fake commitment transaction to + // send to signal that the channel has closed out on + // chain. + closeTx := &wire.MsgTx{ + TxIn: []*wire.TxIn{ + { + PreviousOutPoint: wire.OutPoint{}, + Witness: [][]byte{ + {0x9}, + }, + }, + }, + } + + // We'll now signal to the channel arb that the HTLC + // has fully closed on chain. Our local commit set + // shows now HTLC on our commitment, but one on the + // remote commitment. This should result in the HTLC + // being canalled back. Also note that there're no HTLC + // resolutions sent since we have none on our + // commitment transaction. + uniCloseInfo := &LocalUnilateralCloseInfo{ + SpendDetail: &chainntnfs.SpendDetail{}, + LocalForceCloseSummary: &lnwallet.LocalForceCloseSummary{ + CloseTx: closeTx, + HtlcResolutions: &lnwallet.HtlcResolutions{}, + }, + ChannelCloseSummary: &channeldb.ChannelCloseSummary{}, + CommitSet: CommitSet{ + ConfCommitKey: &testCase.confCommit, + HtlcSets: make(map[HtlcSetKey][]channeldb.HTLC), + }, + } + + // If the HTLC was meant to expire, then we'll mark the + // closing transaction at the proper expiry height + // since our comparison "need to timeout" comparison is + // based on the confirmation height. + if testCase.htlcExpired { + uniCloseInfo.SpendDetail.SpendingHeight = 5 + } + + // Depending on if we're testing the remote pending + // commitment or not, we'll populate either a fake + // dangling remote commitment, or a regular locked in + // one. + htlcs := []channeldb.HTLC{danglingHTLC} + if testCase.remotePendingHTLC { + uniCloseInfo.CommitSet.HtlcSets[RemotePendingHtlcSet] = htlcs + } else { + uniCloseInfo.CommitSet.HtlcSets[RemoteHtlcSet] = htlcs + } + + chanArb.cfg.ChainEvents.LocalUnilateralClosure <- uniCloseInfo + + // The channel arb should now transition to waiting + // until the HTLCs have been fully resolved. + assertStateTransitions( + t, arbLog.newStates, StateContractClosed, + StateWaitingFullResolution, + ) + + // Now that we've sent this signal, we should have that + // HTLC be cancelled back immediately. + select { + case msgs := <-resolutions: + if len(msgs) != 1 { + t.Fatalf("expected 1 message, "+ + "instead got %v", len(msgs)) + } + + if msgs[0].HtlcIndex != htlcIndex { + t.Fatalf("wrong htlc index: expected %v, got %v", + htlcIndex, msgs[0].HtlcIndex) + } + case <-time.After(5 * time.Second): + t.Fatalf("resolution msgs not sent") + } + + // There's no contract to send a fully resolve message, + // so instead, we'll mine another block which'll cause + // it to re-examine its state and realize there're no + // more HTLCs. + blockEpochs <- &chainntnfs.BlockEpoch{Height: 6} + assertStateTransitions( + t, arbLog.newStates, StateFullyResolved, + ) + }) + } +} From 364c0dd9f12d894f777874e4a733fa6bad331c26 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 23 May 2019 20:20:06 -0700 Subject: [PATCH 10/11] contractcourt: add storage for the confirmed CommitSet In this commit, we add storage to the Briefcase for reading/writing a confirmed CommitSet. This will be used in follow up commits to ensure that we're able to survive restarts after we mark a channel as pending closed. Along the way, we also re-add the FetchChainActions struct as legacy nodes will need this storage. --- contractcourt/briefcase.go | 199 ++++++++++++++++++++++++++++++++ contractcourt/briefcase_test.go | 58 ++++++++++ 2 files changed, 257 insertions(+) diff --git a/contractcourt/briefcase.go b/contractcourt/briefcase.go index 43a7c722..853be443 100644 --- a/contractcourt/briefcase.go +++ b/contractcourt/briefcase.go @@ -9,6 +9,7 @@ import ( "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/wire" "github.com/coreos/bbolt" + "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/lnwallet" ) @@ -81,6 +82,24 @@ type ArbitratorLog interface { // contract resolutions from persistent storage. FetchContractResolutions() (*ContractResolutions, error) + // InsertConfirmedCommitSet stores the known set of active HTLCs at the + // time channel closure. We'll use this to reconstruct our set of chain + // actions anew based on the confirmed and pending commitment state. + InsertConfirmedCommitSet(c *CommitSet) error + + // FetchConfirmedCommitSet fetches the known confirmed active HTLC set + // from the database. + FetchConfirmedCommitSet() (*CommitSet, error) + + // FetchChainActions attempts to fetch the set of previously stored + // chain actions. We'll use this upon restart to properly advance our + // state machine forward. + // + // NOTE: This method only exists in order to be able to serve nodes had + // channels in the process of closing before the CommitSet struct was + // introduced. + FetchChainActions() (ChainActionMap, error) + // WipeHistory is to be called ONLY once *all* contracts have been // fully resolved, and the channel closure if finalized. This method // will delete all on-disk state within the persistent log. @@ -247,6 +266,11 @@ var ( // actionsBucketKey is the key under the logScope that we'll use to // store all chain actions once they're determined. actionsBucketKey = []byte("chain-actions") + + // commitSetKey is the primary key under the logScope that we'll use to + // store the confirmed active HTLC sets once we learn that a channel + // has closed out on chain. + commitSetKey = []byte("commit-set") ) var ( @@ -265,6 +289,11 @@ var ( // errNoActions is retuned when the log doesn't contain any stored // chain actions. errNoActions = fmt.Errorf("no chain actions exist") + + // errNoCommitSet is return when the log doesn't contained a CommitSet. + // This can happen if the channel hasn't closed yet, or a client is + // running an older version that didn't yet write this state. + errNoCommitSet = fmt.Errorf("no commit set exists") ) // boltArbitratorLog is an implementation of the ArbitratorLog interface backed @@ -708,6 +737,103 @@ func (b *boltArbitratorLog) FetchContractResolutions() (*ContractResolutions, er return c, err } +// FetchChainActions attempts to fetch the set of previously stored chain +// actions. We'll use this upon restart to properly advance our state machine +// forward. +// +// NOTE: Part of the ContractResolver interface. +func (b *boltArbitratorLog) FetchChainActions() (ChainActionMap, error) { + actionsMap := make(ChainActionMap) + + err := b.db.View(func(tx *bbolt.Tx) error { + scopeBucket := tx.Bucket(b.scopeKey[:]) + if scopeBucket == nil { + return errScopeBucketNoExist + } + + actionsBucket := scopeBucket.Bucket(actionsBucketKey) + if actionsBucket == nil { + return errNoActions + } + + return actionsBucket.ForEach(func(action, htlcBytes []byte) error { + if htlcBytes == nil { + return nil + } + + chainAction := ChainAction(action[0]) + + htlcReader := bytes.NewReader(htlcBytes) + htlcs, err := channeldb.DeserializeHtlcs(htlcReader) + if err != nil { + return err + } + + actionsMap[chainAction] = htlcs + + return nil + }) + }) + if err != nil { + return nil, err + } + + return actionsMap, nil +} + +// InsertConfirmedCommitSet stores the known set of active HTLCs at the time +// channel closure. We'll use this to reconstruct our set of chain actions anew +// based on the confirmed and pending commitment state. +// +// NOTE: Part of the ContractResolver interface. +func (b *boltArbitratorLog) InsertConfirmedCommitSet(c *CommitSet) error { + return b.db.Update(func(tx *bbolt.Tx) error { + scopeBucket, err := tx.CreateBucketIfNotExists(b.scopeKey[:]) + if err != nil { + return err + } + + var b bytes.Buffer + if err := encodeCommitSet(&b, c); err != nil { + return err + } + + return scopeBucket.Put(commitSetKey, b.Bytes()) + }) +} + +// FetchConfirmedCommitSet fetches the known confirmed active HTLC set from the +// database. +// +// NOTE: Part of the ContractResolver interface. +func (b *boltArbitratorLog) FetchConfirmedCommitSet() (*CommitSet, error) { + var c *CommitSet + err := b.db.View(func(tx *bbolt.Tx) error { + scopeBucket := tx.Bucket(b.scopeKey[:]) + if scopeBucket == nil { + return errScopeBucketNoExist + } + + commitSetBytes := scopeBucket.Get(commitSetKey) + if commitSetBytes == nil { + return errNoCommitSet + } + + commitSet, err := decodeCommitSet(bytes.NewReader(commitSetBytes)) + if err != nil { + return err + } + + c = commitSet + return nil + }) + if err != nil { + return nil, err + } + + return c, nil +} + // WipeHistory is to be called ONLY once *all* contracts have been fully // resolved, and the channel closure if finalized. This method will delete all // on-disk state within the persistent log. @@ -957,3 +1083,76 @@ func decodeCommitResolution(r io.Reader, return binary.Read(r, endian, &c.MaturityDelay) } + +func encodeHtlcSetKey(w io.Writer, h *HtlcSetKey) error { + err := binary.Write(w, endian, h.IsRemote) + if err != nil { + return err + } + return binary.Write(w, endian, h.IsPending) +} + +func encodeCommitSet(w io.Writer, c *CommitSet) error { + if err := encodeHtlcSetKey(w, c.ConfCommitKey); err != nil { + return err + } + + numSets := uint8(len(c.HtlcSets)) + if err := binary.Write(w, endian, numSets); err != nil { + return err + } + + for htlcSetKey, htlcs := range c.HtlcSets { + htlcSetKey := htlcSetKey + if err := encodeHtlcSetKey(w, &htlcSetKey); err != nil { + return err + } + + if err := channeldb.SerializeHtlcs(w, htlcs...); err != nil { + return err + } + } + + return nil +} + +func decodeHtlcSetKey(r io.Reader, h *HtlcSetKey) error { + err := binary.Read(r, endian, &h.IsRemote) + if err != nil { + return err + } + + return binary.Read(r, endian, &h.IsPending) +} + +func decodeCommitSet(r io.Reader) (*CommitSet, error) { + c := &CommitSet{ + ConfCommitKey: &HtlcSetKey{}, + HtlcSets: make(map[HtlcSetKey][]channeldb.HTLC), + } + + if err := decodeHtlcSetKey(r, c.ConfCommitKey); err != nil { + return nil, err + } + + var numSets uint8 + if err := binary.Read(r, endian, &numSets); err != nil { + return nil, err + } + + for i := uint8(0); i < numSets; i++ { + var htlcSetKey HtlcSetKey + if err := decodeHtlcSetKey(r, &htlcSetKey); err != nil { + return nil, err + } + + htlcs, err := channeldb.DeserializeHtlcs(r) + if err != nil { + return nil, err + } + + c.HtlcSets[htlcSetKey] = htlcs + } + + return c, nil +} diff --git a/contractcourt/briefcase_test.go b/contractcourt/briefcase_test.go index 0777f2ba..b3b90665 100644 --- a/contractcourt/briefcase_test.go +++ b/contractcourt/briefcase_test.go @@ -15,6 +15,8 @@ import ( "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcd/wire" "github.com/coreos/bbolt" + "github.com/davecgh/go-spew/spew" + "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/lnwallet" ) @@ -672,6 +674,62 @@ func TestScopeIsolation(t *testing.T) { } } +// TestCommitSetStorage tests that we're able to properly read/write active +// commitment sets. +func TestCommitSetStorage(t *testing.T) { + t.Parallel() + + testLog, cleanUp, err := newTestBoltArbLog( + testChainHash, testChanPoint1, + ) + if err != nil { + t.Fatalf("unable to create test log: %v", err) + } + defer cleanUp() + + activeHTLCs := []channeldb.HTLC{ + { + Amt: 1000, + OnionBlob: make([]byte, 0), + Signature: make([]byte, 0), + }, + } + + confTypes := []HtlcSetKey{ + LocalHtlcSet, RemoteHtlcSet, RemotePendingHtlcSet, + } + for _, pendingRemote := range []bool{true, false} { + for _, confType := range confTypes { + commitSet := &CommitSet{ + ConfCommitKey: &confType, + HtlcSets: make(map[HtlcSetKey][]channeldb.HTLC), + } + commitSet.HtlcSets[LocalHtlcSet] = activeHTLCs + commitSet.HtlcSets[RemoteHtlcSet] = activeHTLCs + + if pendingRemote { + commitSet.HtlcSets[RemotePendingHtlcSet] = activeHTLCs + } + + err := testLog.InsertConfirmedCommitSet(commitSet) + if err != nil { + t.Fatalf("unable to write commit set: %v", err) + } + + diskCommitSet, err := testLog.FetchConfirmedCommitSet() + if err != nil { + t.Fatalf("unable to read commit set: %v", err) + } + + if !reflect.DeepEqual(commitSet, diskCommitSet) { + t.Fatalf("commit set mismatch: expected %v, got %v", + spew.Sdump(commitSet), spew.Sdump(diskCommitSet)) + } + } + } + +} + func init() { testSignDesc.KeyDesc.PubKey, _ = btcec.ParsePubKey(key1, btcec.S256()) From 2011ccc571633b7ba5ba72a1e6132310521327ef Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 23 May 2019 20:27:04 -0700 Subject: [PATCH 11/11] contractcourt: write confirmed CommitSet to disk before MarkChannelClosed In this commit, we make a series of changes to ensure that we'll be able to properly survive restarts if we crash right after we call MarkChannelClosed. In order to ensure we can survive restarts, we'll now long the confirmed CommitSet to disk right before we close the channel. Upon restart, we'll read these from disk so we can pick up where we left over. Additionally, we also will now consult the legacy chain actions if it turns out that the channel has been closed, but we don't have a confCommitSet written to disk. This will only be the case for nodes that had pending close channels before this commitment. --- contractcourt/chain_arbitrator.go | 2 +- contractcourt/channel_arbitrator.go | 112 ++++++++++++++++------- contractcourt/channel_arbitrator_test.go | 17 +++- 3 files changed, 96 insertions(+), 35 deletions(-) diff --git a/contractcourt/chain_arbitrator.go b/contractcourt/chain_arbitrator.go index 3c0dd21a..b3cd497d 100644 --- a/contractcourt/chain_arbitrator.go +++ b/contractcourt/chain_arbitrator.go @@ -463,7 +463,7 @@ func (c *ChainArbitrator) Start() error { // We can also leave off the set of HTLC's here as since the // channel is already in the process of being full resolved, no - // new HTLC's we be added. + // new HTLC's will be added. c.activeChannels[chanPoint] = NewChannelArbitrator( arbCfg, nil, chanLog, ) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index b8862a89..7bb1899c 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -3,6 +3,7 @@ package contractcourt import ( "bytes" "errors" + "fmt" "sync" "sync/atomic" @@ -370,11 +371,21 @@ func (c *ChannelArbitrator) Start() error { } } + // Next we'll fetch our confirmed commitment set. This will only exist + // if the channel has been closed out on chain for modern nodes. For + // older nodes, this won't be found at all, and will rely on the + // existing written chain actions. Additionally, if this channel hasn't + // logged any actions in the log, then this field won't be present. + commitSet, err := c.log.FetchConfirmedCommitSet() + if err != nil && err != errNoCommitSet && err != errScopeBucketNoExist { + return err + } + // We'll now attempt to advance our state forward based on the current // on-chain state, and our set of active contracts. startingState := c.state nextState, _, err := c.advanceState( - triggerHeight, trigger, nil, + triggerHeight, trigger, commitSet, ) if err != nil { switch err { @@ -1512,6 +1523,52 @@ func (c *ChannelArbitrator) checkRemoteDiffActions(height uint32, return actionMap } +// constructChainActions returns the set of actions that should be taken for +// confirmed HTLCs at the specified height. Our actions will depend on the set +// of HTLCs that were active across all channels at the time of channel +// closure. +func (c *ChannelArbitrator) constructChainActions(confCommitSet *CommitSet, + height uint32, trigger transitionTrigger) (ChainActionMap, error) { + + // If we've reached this point and have not confirmed commitment set, + // 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 { + 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 { + + // If the local commitment transaction confirmed, then we'll examine + // that as well as their commitments to the set of chain actions. + case LocalHtlcSet: + return c.checkLocalChainActions( + height, trigger, htlcSets, true, + ) + + // If the remote commitment confirmed, then we'll grab all the chain + // actions for the remote commit, and check the pending commit for any + // HTLCS we need to handle immediately (dust). + case RemoteHtlcSet: + return c.checkRemoteChainActions( + height, trigger, htlcSets, false, + ) + + // Otherwise, the remote pending commitment confirmed, so we'll examine + // the HTLCs on that unrevoked dangling commitment. + case RemotePendingHtlcSet: + return c.checkRemoteChainActions( + height, trigger, htlcSets, true, + ) + } + + return nil, fmt.Errorf("unable to locate chain actions") +} + // prepContractResolutions is called either int he case that we decide we need // to go to chain, or the remote party goes to chain. Given a set of actions we // need to take for each HTLC, this method will return a set of contract @@ -1523,39 +1580,12 @@ func (c *ChannelArbitrator) prepContractResolutions( trigger transitionTrigger, confCommitSet *CommitSet) ([]ContractResolver, []ResolutionMsg, error) { - htlcSets := confCommitSet.toActiveHTLCSets() - // 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. - var ( - htlcActions ChainActionMap - err error + htlcActions, err := c.constructChainActions( + confCommitSet, height, trigger, ) - switch *confCommitSet.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: - htlcActions, err = c.checkLocalChainActions( - height, trigger, htlcSets, true, - ) - - // If the remote commitment confirmed, then we'll grab all the chain - // actions for the remote commit, and check the pending commit for any - // HTLCS we need to handle immediately (dust). - case RemoteHtlcSet: - htlcActions, err = c.checkRemoteChainActions( - height, trigger, htlcSets, false, - ) - - // Otherwise, the remote pending commitment confirmed, so we'll examine - // the HTLCs on that unrevoked dangling commitment. - case RemotePendingHtlcSet: - htlcActions, err = c.checkRemoteChainActions( - height, trigger, htlcSets, true, - ) - } if err != nil { return nil, nil, err } @@ -2047,13 +2077,22 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) { // When processing a unilateral close event, we'll // transition to the ContractClosed state. We'll log // out the set of resolutions such that they are - // available to fetch in that state. + // available to fetch in that state, we'll also write + // the commit set so we can reconstruct our chain + // actions on restart. err := c.log.LogContractResolutions(contractRes) if err != nil { log.Errorf("unable to write resolutions: %v", err) return } + err = c.log.InsertConfirmedCommitSet( + &closeInfo.CommitSet, + ) + if err != nil { + log.Errorf("unable to write commit set: %v", err) + return + } // After the set of resolutions are successfully // logged, we can safely close the channel. After this @@ -2103,13 +2142,22 @@ func (c *ChannelArbitrator) channelAttendant(bestHeight int32) { // When processing a unilateral close event, we'll // transition to the ContractClosed state. We'll log // out the set of resolutions such that they are - // available to fetch in that state. + // available to fetch in that state, we'll also write + // the commit set so we can reconstruct our chain + // actions on restart. err := c.log.LogContractResolutions(contractRes) if err != nil { log.Errorf("unable to write resolutions: %v", err) return } + err = c.log.InsertConfirmedCommitSet( + &uniClosure.CommitSet, + ) + if err != nil { + log.Errorf("unable to write commit set: %v", err) + return + } // After the set of resolutions are successfully // logged, we can safely close the channel. After this diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index 903ca564..8468e53d 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/contractcourt/channel_arbitrator_test.go @@ -26,6 +26,8 @@ type mockArbitratorLog struct { chainActions ChainActionMap resolvers map[ContractResolver]struct{} + commitSet *CommitSet + sync.Mutex } @@ -108,6 +110,19 @@ func (b *mockArbitratorLog) FetchContractResolutions() (*ContractResolutions, er return b.resolutions, nil } +func (b *mockArbitratorLog) FetchChainActions() (ChainActionMap, error) { + return nil, nil +} + +func (b *mockArbitratorLog) InsertConfirmedCommitSet(c *CommitSet) error { + b.commitSet = c + return nil +} + +func (b *mockArbitratorLog) FetchConfirmedCommitSet() (*CommitSet, error) { + return b.commitSet, nil +} + func (b *mockArbitratorLog) WipeHistory() error { return nil } @@ -1290,8 +1305,6 @@ func TestChannelArbitratorDanglingCommitForceClose(t *testing.T) { } } - fmt.Println(len(testCases)) - for _, testCase := range testCases { testCase := testCase testName := fmt.Sprintf("testCase: htlcExpired=%v,"+