From 422928b8a3e82a52cfa2c5b8c5183872d4a53ab6 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Thu, 2 Apr 2020 17:38:54 -0700 Subject: [PATCH 1/5] lnwallet/channel: remove unused RemoteCommitHeight method --- lnwallet/channel.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 6f64d34d..9d820c8f 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -6667,14 +6667,6 @@ func (lc *LightningChannel) NextLocalHtlcIndex() (uint64, error) { return lc.channelState.NextLocalHtlcIndex() } -// RemoteCommitHeight returns the commitment height of the remote chain. -func (lc *LightningChannel) RemoteCommitHeight() uint64 { - lc.RLock() - defer lc.RUnlock() - - return lc.channelState.RemoteCommitment.CommitHeight -} - // FwdMinHtlc returns the minimum HTLC value required by the remote node, i.e. // the minimum value HTLC we can forward on this channel. func (lc *LightningChannel) FwdMinHtlc() lnwire.MilliSatoshi { From 9385b8cdc637e7fbdae819338adec13c6e997747 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Thu, 2 Apr 2020 17:39:10 -0700 Subject: [PATCH 2/5] channeldb+lnwallet: move ActiveHtlcs calc to OpenChannel --- channeldb/channel.go | 31 +++++++++++++++++++++++++++++++ lnwallet/channel.go | 22 +--------------------- 2 files changed, 32 insertions(+), 21 deletions(-) diff --git a/channeldb/channel.go b/channeldb/channel.go index 7189e56a..21468b7f 100644 --- a/channeldb/channel.go +++ b/channeldb/channel.go @@ -2,6 +2,7 @@ package channeldb import ( "bytes" + "crypto/sha256" "encoding/binary" "errors" "fmt" @@ -1483,6 +1484,36 @@ func (c *OpenChannel) BalancesAtHeight(height uint64) (lnwire.MilliSatoshi, return commit.LocalBalance, commit.RemoteBalance, nil } +// ActiveHtlcs returns a slice of HTLC's which are currently active on *both* +// commitment transactions. +func (c *OpenChannel) ActiveHtlcs() []HTLC { + c.RLock() + defer c.RUnlock() + + // We'll only return HTLC's that are locked into *both* commitment + // transactions. So we'll iterate through their set of HTLC's to note + // which ones are present on their commitment. + remoteHtlcs := make(map[[32]byte]struct{}) + for _, htlc := range c.RemoteCommitment.Htlcs { + onionHash := sha256.Sum256(htlc.OnionBlob) + remoteHtlcs[onionHash] = struct{}{} + } + + // Now that we know which HTLC's they have, we'll only mark the HTLC's + // as active if *we* know them as well. + activeHtlcs := make([]HTLC, 0, len(remoteHtlcs)) + for _, htlc := range c.LocalCommitment.Htlcs { + onionHash := sha256.Sum256(htlc.OnionBlob) + if _, ok := remoteHtlcs[onionHash]; !ok { + continue + } + + activeHtlcs = append(activeHtlcs, htlc) + } + + return activeHtlcs +} + // HTLC is the on-disk representation of a hash time-locked contract. HTLCs are // contained within ChannelDeltas which encode the current state of the // commitment between state updates. diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 9d820c8f..fbf6116f 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -6628,27 +6628,7 @@ func (lc *LightningChannel) ActiveHtlcs() []channeldb.HTLC { lc.RLock() defer lc.RUnlock() - // We'll only return HTLC's that are locked into *both* commitment - // transactions. So we'll iterate through their set of HTLC's to note - // which ones are present on their commitment. - remoteHtlcs := make(map[[32]byte]struct{}) - for _, htlc := range lc.channelState.RemoteCommitment.Htlcs { - onionHash := sha256.Sum256(htlc.OnionBlob[:]) - remoteHtlcs[onionHash] = struct{}{} - } - - // Now that we know which HTLC's they have, we'll only mark the HTLC's - // as active if *we* know them as well. - activeHtlcs := make([]channeldb.HTLC, 0, len(remoteHtlcs)) - for _, htlc := range lc.channelState.LocalCommitment.Htlcs { - if _, ok := remoteHtlcs[sha256.Sum256(htlc.OnionBlob[:])]; !ok { - continue - } - - activeHtlcs = append(activeHtlcs, htlc) - } - - return activeHtlcs + return lc.channelState.ActiveHtlcs() } // LocalChanReserve returns our local ChanReserve requirement for the remote party. From ec784db511edfc8182fbb5d252b2ac22fbfd310a Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Thu, 2 Apr 2020 17:39:29 -0700 Subject: [PATCH 3/5] multi: remove returned error from WipeChannel The linter complains about not checking the return value from WipeChannel in certain places. Instead of checking we simply remove the returned error because the in-memory modifications cannot fail. --- discovery/mock_test.go | 4 ++-- fundingmanager_test.go | 4 +--- htlcswitch/link.go | 6 +----- htlcswitch/link_test.go | 4 +--- htlcswitch/mock.go | 4 +--- lnpeer/peer.go | 2 +- peer.go | 24 ++++-------------------- rpcserver.go | 5 +---- 8 files changed, 12 insertions(+), 41 deletions(-) diff --git a/discovery/mock_test.go b/discovery/mock_test.go index f3f707d5..714f6b4a 100644 --- a/discovery/mock_test.go +++ b/discovery/mock_test.go @@ -45,8 +45,8 @@ func (p *mockPeer) SendMessageLazy(sync bool, msgs ...lnwire.Message) error { func (p *mockPeer) AddNewChannel(_ *channeldb.OpenChannel, _ <-chan struct{}) error { return nil } -func (p *mockPeer) WipeChannel(_ *wire.OutPoint) error { return nil } -func (p *mockPeer) IdentityKey() *btcec.PublicKey { return p.pk } +func (p *mockPeer) WipeChannel(_ *wire.OutPoint) {} +func (p *mockPeer) IdentityKey() *btcec.PublicKey { return p.pk } func (p *mockPeer) PubKey() [33]byte { var pubkey [33]byte copy(pubkey[:], p.pk.SerializeCompressed()) diff --git a/fundingmanager_test.go b/fundingmanager_test.go index f1ff528d..c7247136 100644 --- a/fundingmanager_test.go +++ b/fundingmanager_test.go @@ -201,9 +201,7 @@ func (n *testNode) SendMessageLazy(sync bool, msgs ...lnwire.Message) error { return n.SendMessage(sync, msgs...) } -func (n *testNode) WipeChannel(_ *wire.OutPoint) error { - return nil -} +func (n *testNode) WipeChannel(_ *wire.OutPoint) {} func (n *testNode) QuitSignal() <-chan struct{} { return n.shutdownChannel diff --git a/htlcswitch/link.go b/htlcswitch/link.go index d91bc856..6eec8d38 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -1084,11 +1084,7 @@ out: // TODO(roasbeef): remove all together go func() { chanPoint := l.channel.ChannelPoint() - err := l.cfg.Peer.WipeChannel(chanPoint) - if err != nil { - l.log.Errorf("unable to wipe channel "+ - "%v", err) - } + l.cfg.Peer.WipeChannel(chanPoint) }() break out diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index d150ddc5..965ecdb2 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -1652,9 +1652,7 @@ func (m *mockPeer) AddNewChannel(_ *channeldb.OpenChannel, _ <-chan struct{}) error { return nil } -func (m *mockPeer) WipeChannel(*wire.OutPoint) error { - return nil -} +func (m *mockPeer) WipeChannel(*wire.OutPoint) {} func (m *mockPeer) PubKey() [33]byte { return [33]byte{} } diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index 0c7f1ed8..cefb673a 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -602,9 +602,7 @@ func (s *mockServer) AddNewChannel(channel *channeldb.OpenChannel, return nil } -func (s *mockServer) WipeChannel(*wire.OutPoint) error { - return nil -} +func (s *mockServer) WipeChannel(*wire.OutPoint) {} func (s *mockServer) LocalFeatures() *lnwire.FeatureVector { return nil diff --git a/lnpeer/peer.go b/lnpeer/peer.go index 53f57131..1b118bfb 100644 --- a/lnpeer/peer.go +++ b/lnpeer/peer.go @@ -30,7 +30,7 @@ type Peer interface { // WipeChannel removes the channel uniquely identified by its channel // point from all indexes associated with the peer. - WipeChannel(*wire.OutPoint) error + WipeChannel(*wire.OutPoint) // PubKey returns the serialized public key of the remote peer. PubKey() [33]byte diff --git a/peer.go b/peer.go index df77c4fd..5cd431c8 100644 --- a/peer.go +++ b/peer.go @@ -2401,13 +2401,7 @@ func (p *peer) handleLocalCloseReq(req *htlcswitch.ChanClose) { // TODO(roasbeef): no longer need with newer beach logic? peerLog.Infof("ChannelPoint(%v) has been breached, wiping "+ "channel", req.ChanPoint) - if err := p.WipeChannel(req.ChanPoint); err != nil { - peerLog.Infof("Unable to wipe channel after detected "+ - "breach: %v", err) - req.Err <- err - return - } - return + p.WipeChannel(req.ChanPoint) } } @@ -2434,11 +2428,7 @@ func (p *peer) handleLinkFailure(failure linkFailureReport) { // link and cancel back any adds in its mailboxes such that we can // safely force close without the link being added again and updates // being applied. - if err := p.WipeChannel(&failure.chanPoint); err != nil { - peerLog.Errorf("Unable to wipe link for chanpoint=%v", - failure.chanPoint) - return - } + p.WipeChannel(&failure.chanPoint) // If the error encountered was severe enough, we'll now force close the // channel to prevent readding it to the switch in the future. @@ -2490,11 +2480,7 @@ func (p *peer) finalizeChanClosure(chanCloser *channelCloser) { // First, we'll clear all indexes related to the channel in question. chanPoint := chanCloser.cfg.channel.ChannelPoint() - if err := p.WipeChannel(chanPoint); err != nil { - if closeReq != nil { - closeReq.Err <- err - } - } + p.WipeChannel(chanPoint) // Next, we'll launch a goroutine which will request to be notified by // the ChainNotifier once the closure transaction obtains a single @@ -2584,7 +2570,7 @@ func waitForChanToClose(bestHeight uint32, notifier chainntnfs.ChainNotifier, // WipeChannel removes the passed channel point from all indexes associated with // the peer, and the switch. -func (p *peer) WipeChannel(chanPoint *wire.OutPoint) error { +func (p *peer) WipeChannel(chanPoint *wire.OutPoint) { chanID := lnwire.NewChanIDFromOutPoint(chanPoint) p.activeChanMtx.Lock() @@ -2594,8 +2580,6 @@ func (p *peer) WipeChannel(chanPoint *wire.OutPoint) error { // Instruct the HtlcSwitch to close this link as the channel is no // longer active. p.server.htlcSwitch.RemoveLink(chanID) - - return nil } // handleInitMsg handles the incoming init message which contains global and diff --git a/rpcserver.go b/rpcserver.go index c016ab5a..18a81b90 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -2259,10 +2259,7 @@ func (r *rpcServer) AbandonChannel(ctx context.Context, } remotePub := dbChan.IdentityPub if peer, err := r.server.FindPeer(remotePub); err == nil { - if err := peer.WipeChannel(chanPoint); err != nil { - return nil, fmt.Errorf("unable to wipe "+ - "channel state: %v", err) - } + peer.WipeChannel(chanPoint) } default: From 3c371dd63346728e2ec2767ba6eb14aeca916498 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Thu, 2 Apr 2020 17:39:47 -0700 Subject: [PATCH 4/5] rpcserver: remove NewLightningChannel usage in CloseChannel --- rpcserver.go | 33 +++++++-------------------------- 1 file changed, 7 insertions(+), 26 deletions(-) diff --git a/rpcserver.go b/rpcserver.go index 18a81b90..bb540d2c 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1985,7 +1985,7 @@ func (r *rpcServer) CloseChannel(in *lnrpc.CloseChannelRequest, // First, we'll fetch the channel as is, as we'll need to examine it // regardless of if this is a force close or not. - channel, err := r.fetchActiveChannel(*chanPoint) + channel, err := r.server.chanDB.FetchChannel(*chanPoint) if err != nil { return err } @@ -1996,12 +1996,12 @@ func (r *rpcServer) CloseChannel(in *lnrpc.CloseChannelRequest, if err != nil { return err } - if channel.State().ChanType.IsFrozen() && channel.IsInitiator() && - uint32(bestHeight) < channel.State().ThawHeight { + if channel.ChanType.IsFrozen() && channel.IsInitiator && + uint32(bestHeight) < channel.ThawHeight { return fmt.Errorf("cannot co-op close frozen channel as "+ "initiator until height=%v, (current_height=%v)", - channel.State().ThawHeight, bestHeight) + channel.ThawHeight, bestHeight) } // If a force closure was requested, then we'll handle all the details @@ -2014,14 +2014,14 @@ func (r *rpcServer) CloseChannel(in *lnrpc.CloseChannelRequest, // ensure that the switch doesn't continue to see this channel // as eligible for forwarding HTLC's. If the peer is online, // then we'll also purge all of its indexes. - remotePub := &channel.StateSnapshot().RemoteIdentity + remotePub := channel.IdentityPub if peer, err := r.server.FindPeer(remotePub); err == nil { // TODO(roasbeef): actually get the active channel // instead too? // * so only need to grab from database - peer.WipeChannel(channel.ChannelPoint()) + peer.WipeChannel(&channel.FundingOutpoint) } else { - chanID := lnwire.NewChanIDFromOutPoint(channel.ChannelPoint()) + chanID := lnwire.NewChanIDFromOutPoint(&channel.FundingOutpoint) r.server.htlcSwitch.RemoveLink(chanID) } @@ -2302,25 +2302,6 @@ func (r *rpcServer) AbandonChannel(ctx context.Context, return &lnrpc.AbandonChannelResponse{}, nil } -// fetchActiveChannel attempts to locate a channel identified by its channel -// point from the database's set of all currently opened channels and -// return it as a fully populated state machine -func (r *rpcServer) fetchActiveChannel(chanPoint wire.OutPoint) ( - *lnwallet.LightningChannel, error) { - - dbChan, err := r.server.chanDB.FetchChannel(chanPoint) - if err != nil { - return nil, err - } - - // If the channel is successfully fetched from the database, - // we create a fully populated channel state machine which - // uses the db channel as backing storage. - return lnwallet.NewLightningChannel( - r.server.cc.wallet.Cfg.Signer, dbChan, nil, - ) -} - // GetInfo returns general information concerning the lightning node including // its identity pubkey, alias, the chains it is connected to, and information // concerning the number of open+pending channels. From d1fa33c8eb2b1333a5525b194faa1f1eb15f7898 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Thu, 2 Apr 2020 17:40:08 -0700 Subject: [PATCH 5/5] rpcserver: only block co-op close for frozen chans This commit fixes a recent issue from #4081 that would prevent a frozen channel from being force closed via the rpc. We correct this, so that only the co-op path is inhibited. --- rpcserver.go | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/rpcserver.go b/rpcserver.go index bb540d2c..e0e119eb 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1990,19 +1990,12 @@ func (r *rpcServer) CloseChannel(in *lnrpc.CloseChannelRequest, return err } - // If this is a frozen channel, then we only allow the close to proceed - // if we were the responder to this channel. + // Retrieve the best height of the chain, which we'll use to complete + // either closing flow. _, bestHeight, err := r.server.cc.chainIO.GetBestBlock() if err != nil { return err } - if channel.ChanType.IsFrozen() && channel.IsInitiator && - uint32(bestHeight) < channel.ThawHeight { - - return fmt.Errorf("cannot co-op close frozen channel as "+ - "initiator until height=%v, (current_height=%v)", - channel.ThawHeight, bestHeight) - } // If a force closure was requested, then we'll handle all the details // around the creation and broadcast of the unilateral closure @@ -2057,6 +2050,17 @@ func (r *rpcServer) CloseChannel(in *lnrpc.CloseChannelRequest, } }) } else { + // If this is a frozen channel, then we only allow the co-op + // close to proceed if we were the responder to this channel. + if channel.ChanType.IsFrozen() && channel.IsInitiator && + uint32(bestHeight) < channel.ThawHeight { + + return fmt.Errorf("cannot co-op close frozen channel "+ + "as initiator until height=%v, "+ + "(current_height=%v)", channel.ThawHeight, + bestHeight) + } + // If the link is not known by the switch, we cannot gracefully close // the channel. channelID := lnwire.NewChanIDFromOutPoint(chanPoint)