From ec2377db79db6ddfee938ac2e60be4963f7f2a27 Mon Sep 17 00:00:00 2001 From: Yong Date: Mon, 9 Oct 2023 16:58:18 +0800 Subject: [PATCH] funding: remove dead code and sanity check pending chan ID (#7887) * funding: remove unused field `newChanBarriers` This commit removes the occurance of `newChanBarriers` as it's not used anywhere. * funding: rename method names to clear the funding flow Slightly refactored the names so it's easier to see which side is processing what messages. * lnwallet: sanity check empty pending channel ID This commit adds a sanity check to make sure an empty pending channel ID will not be accepted. --- funding/manager.go | 73 ++++++++++++--------------------- lnwallet/test/test_interface.go | 7 ++-- lnwallet/wallet.go | 18 +++++++- lnwallet/wallet_test.go | 34 +++++++++++++++ 4 files changed, 80 insertions(+), 52 deletions(-) create mode 100644 lnwallet/wallet_test.go diff --git a/funding/manager.go b/funding/manager.go index f88644f0d..374ad7adf 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -610,12 +610,6 @@ type Manager struct { // requests from a local subsystem within the daemon. fundingRequests chan *InitFundingMsg - // newChanBarriers is a map from a channel ID to a 'barrier' which will - // be signalled once the channel is fully open. This barrier acts as a - // synchronization point for any incoming/outgoing HTLCs before the - // channel has been fully opened. - newChanBarriers *lnutils.SyncMap[lnwire.ChannelID, chan struct{}] - localDiscoverySignals *lnutils.SyncMap[lnwire.ChannelID, chan struct{}] handleChannelReadyBarriers *lnutils.SyncMap[lnwire.ChannelID, struct{}] @@ -672,9 +666,6 @@ func NewFundingManager(cfg Config) (*Manager, error) { signedReservations: make( map[lnwire.ChannelID][32]byte, ), - newChanBarriers: &lnutils.SyncMap[ - lnwire.ChannelID, chan struct{}, - ]{}, fundingMsgs: make( chan *fundingMsg, msgBufferSize, ), @@ -729,7 +720,6 @@ func (f *Manager) start() error { "creating chan barrier", channel.FundingOutpoint) - f.newChanBarriers.Store(chanID, make(chan struct{})) f.localDiscoverySignals.Store( chanID, make(chan struct{}), ) @@ -993,16 +983,16 @@ func (f *Manager) reservationCoordinator() { case fmsg := <-f.fundingMsgs: switch msg := fmsg.msg.(type) { case *lnwire.OpenChannel: - f.handleFundingOpen(fmsg.peer, msg) + f.fundeeProcessOpenChannel(fmsg.peer, msg) case *lnwire.AcceptChannel: - f.handleFundingAccept(fmsg.peer, msg) + f.funderProcessAcceptChannel(fmsg.peer, msg) case *lnwire.FundingCreated: - f.handleFundingCreated(fmsg.peer, msg) + f.fundeeProcessFundingCreated(fmsg.peer, msg) case *lnwire.FundingSigned: - f.handleFundingSigned(fmsg.peer, msg) + f.funderProcessFundingSigned(fmsg.peer, msg) case *lnwire.ChannelReady: f.wg.Add(1) @@ -1359,13 +1349,15 @@ func (f *Manager) ProcessFundingMsg(msg lnwire.Message, peer lnpeer.Peer) { } } -// handleFundingOpen creates an initial 'ChannelReservation' within the wallet, -// then responds to the source peer with an accept channel message progressing -// the funding workflow. +// fundeeProcessOpenChannel creates an initial 'ChannelReservation' within the +// wallet, then responds to the source peer with an accept channel message +// progressing the funding workflow. // // TODO(roasbeef): add error chan to all, let channelManager handle // error+propagate. -func (f *Manager) handleFundingOpen(peer lnpeer.Peer, +// +//nolint:funlen +func (f *Manager) fundeeProcessOpenChannel(peer lnpeer.Peer, msg *lnwire.OpenChannel) { // Check number of pending channels to be smaller than maximum allowed @@ -1888,10 +1880,12 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer, } } -// handleFundingAccept processes a response to the workflow initiation sent by -// the remote peer. This message then queues a message with the funding +// funderProcessAcceptChannel processes a response to the workflow initiation +// sent by the remote peer. This message then queues a message with the funding // outpoint, and a commitment signature to the remote peer. -func (f *Manager) handleFundingAccept(peer lnpeer.Peer, +// +//nolint:funlen +func (f *Manager) funderProcessAcceptChannel(peer lnpeer.Peer, msg *lnwire.AcceptChannel) { pendingChanID := msg.PendingChannelID @@ -2240,7 +2234,6 @@ func (f *Manager) continueFundingAccept(resCtx *reservationWithCtx, // fully open. channelID := lnwire.NewChanIDFromOutPoint(outPoint) log.Debugf("Creating chan barrier for ChanID(%v)", channelID) - f.newChanBarriers.Store(channelID, make(chan struct{})) // The next message that advances the funding flow will reference the // channel via its permanent channel ID, so we'll set up this mapping @@ -2303,11 +2296,14 @@ func (f *Manager) continueFundingAccept(resCtx *reservationWithCtx, } } -// handleFundingCreated progresses the funding workflow when the daemon is on -// the responding side of a single funder workflow. Once this message has been -// processed, a signature is sent to the remote peer allowing it to broadcast -// the funding transaction, progressing the workflow into the final stage. -func (f *Manager) handleFundingCreated(peer lnpeer.Peer, +// fundeeProcessFundingCreated progresses the funding workflow when the daemon +// is on the responding side of a single funder workflow. Once this message has +// been processed, a signature is sent to the remote peer allowing it to +// broadcast the funding transaction, progressing the workflow into the final +// stage. +// +//nolint:funlen +func (f *Manager) fundeeProcessFundingCreated(peer lnpeer.Peer, msg *lnwire.FundingCreated) { peerKey := peer.IdentityKey() @@ -2410,7 +2406,6 @@ func (f *Manager) handleFundingCreated(peer lnpeer.Peer, // fully open. channelID := lnwire.NewChanIDFromOutPoint(&fundingOut) log.Debugf("Creating chan barrier for ChanID(%v)", channelID) - f.newChanBarriers.Store(channelID, make(chan struct{})) fundingSigned := &lnwire.FundingSigned{} @@ -2513,12 +2508,12 @@ func (f *Manager) handleFundingCreated(peer lnpeer.Peer, go f.advanceFundingState(completeChan, pendingChanID, nil) } -// handleFundingSigned processes the final message received in a single funder -// workflow. Once this message is processed, the funding transaction is +// funderProcessFundingSigned processes the final message received in a single +// funder workflow. Once this message is processed, the funding transaction is // broadcast. Once the funding transaction reaches a sufficient number of // confirmations, a message is sent to the responding peer along with a compact // encoding of the location of the channel within the blockchain. -func (f *Manager) handleFundingSigned(peer lnpeer.Peer, +func (f *Manager) funderProcessFundingSigned(peer lnpeer.Peer, msg *lnwire.FundingSigned) { // As the funding signed message will reference the reservation by its @@ -3918,20 +3913,6 @@ func (f *Manager) handleChannelReady(peer lnpeer.Peer, //nolint:funlen return } - // Launch a defer so we _ensure_ that the channel barrier is properly - // closed even if the target peer is no longer online at this point. - defer func() { - // Close the active channel barrier signaling the readHandler - // that commitment related modifications to this channel can - // now proceed. - chanBarrier, ok := f.newChanBarriers.LoadAndDelete(chanID) - if ok { - log.Tracef("Closing chan barrier for ChanID(%v)", - chanID) - close(chanBarrier) - } - }() - // Before we can add the channel to the peer, we'll need to ensure that // we have an initial forwarding policy set. if err := f.ensureInitialForwardingPolicy(chanID, channel); err != nil { @@ -4937,8 +4918,6 @@ func (f *Manager) cancelReservationCtx(peerKey *btcec.PublicKey, func (f *Manager) deleteReservationCtx(peerKey *btcec.PublicKey, pendingChanID [32]byte) { - // TODO(roasbeef): possibly cancel funding barrier in peer's - // channelManager? peerIDKey := newSerializedKey(peerKey) f.resMtx.Lock() defer f.resMtx.Unlock() diff --git a/lnwallet/test/test_interface.go b/lnwallet/test/test_interface.go index 563a975f8..66958278e 100644 --- a/lnwallet/test/test_interface.go +++ b/lnwallet/test/test_interface.go @@ -773,6 +773,7 @@ func testReservationInitiatorBalanceBelowDustCancel(miner *rpctest.Harness, PushMSat: 0, Flags: lnwire.FFAnnounceChannel, CommitType: lnwallet.CommitmentTypeTweakless, + PendingChanID: [32]byte{1}, } _, err = alice.InitChannelReservation(req) switch { @@ -2744,7 +2745,7 @@ var walletTests = []walletTestCase{ testSingleFunderReservationWorkflow( miner, alice, bob, t, lnwallet.CommitmentTypeLegacy, nil, - nil, [32]byte{}, 0, + nil, [32]byte{1}, 0, ) }, }, @@ -2756,7 +2757,7 @@ var walletTests = []walletTestCase{ testSingleFunderReservationWorkflow( miner, alice, bob, t, lnwallet.CommitmentTypeTweakless, nil, - nil, [32]byte{}, 0, + nil, [32]byte{1}, 0, ) }, }, @@ -2768,7 +2769,7 @@ var walletTests = []walletTestCase{ testSingleFunderReservationWorkflow( miner, alice, bob, t, lnwallet.CommitmentTypeSimpleTaproot, nil, - nil, [32]byte{}, 0, + nil, [32]byte{1}, 0, ) }, }, diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index b0a59d603..5f0748097 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -62,6 +62,14 @@ var ( ErrReservedValueInvalidated = errors.New("reserved wallet balance " + "invalidated: transaction would leave insufficient funds for " + "fee bumping anchor channel closings (see debug log for details)") + + // ErrEmptyPendingChanID is returned when an empty value is used for + // the pending channel ID. + ErrEmptyPendingChanID = errors.New("pending channel ID is empty") + + // ErrDuplicatePendingChanID is returned when an existing pending + // channel ID is registered again. + ErrDuplicatePendingChanID = errors.New("duplicate pending channel ID") ) // PsbtFundingRequired is a type that implements the error interface and @@ -670,9 +678,15 @@ func (l *LightningWallet) RegisterFundingIntent(expectedID [32]byte, l.intentMtx.Lock() defer l.intentMtx.Unlock() + // Sanity check the pending channel ID is not empty. + var zeroID [32]byte + if expectedID == zeroID { + return ErrEmptyPendingChanID + } + if _, ok := l.fundingIntents[expectedID]; ok { - return fmt.Errorf("pendingChanID(%x) already has intent "+ - "registered", expectedID[:]) + return fmt.Errorf("%w: already has intent registered: %v", + ErrDuplicatePendingChanID, expectedID[:]) } l.fundingIntents[expectedID] = shimIntent diff --git a/lnwallet/wallet_test.go b/lnwallet/wallet_test.go new file mode 100644 index 000000000..fc6055ac2 --- /dev/null +++ b/lnwallet/wallet_test.go @@ -0,0 +1,34 @@ +package lnwallet + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestRegisterFundingIntent checks RegisterFundingIntent behaves as expected. +func TestRegisterFundingIntent(t *testing.T) { + t.Parallel() + + require := require.New(t) + + // Create a testing wallet. + lw, err := NewLightningWallet(Config{}) + require.NoError(err) + + // Init an empty testing channel ID. + var testID [32]byte + + // Call the method with empty ID should give us an error. + err = lw.RegisterFundingIntent(testID, nil) + require.ErrorIs(err, ErrEmptyPendingChanID) + + // Modify the ID and call the method again should result in no error. + testID[0] = 1 + err = lw.RegisterFundingIntent(testID, nil) + require.NoError(err) + + // Call the method using the same ID should give us an error. + err = lw.RegisterFundingIntent(testID, nil) + require.ErrorIs(err, ErrDuplicatePendingChanID) +}