Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
259 changes: 244 additions & 15 deletions funding/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,24 @@ const (
// for the funding transaction to be confirmed before forgetting
// channels that aren't initiated by us. 2016 blocks is ~2 weeks.
maxWaitNumBlocksFundingConf = 2016

// fundingBroadcastBuffer is the amount of blocks before the broadcast
// height to use as a height hint when checking if any of the funding
// inputs have been double spent. This is done in case the funding flow
// takes longer than expected.
fundingBroadcastBuffer = 12

// conflictReorgBuffer is the number of blocks after which we'll
// consider a transaction that conflicts with a funding transaction
// safe from reorgs. This is the same value that the chainntnfs
// notifier code uses as a default reorg-safe number of blocks.
conflictReorgBuffer = 144
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess we could use ReorgSafetyLimit instead?


// conflictReorgCeiling is the maximum number of blocks we'll wait for
// the conflict transaction to receive conflictReorgBuffer
// confirmations. This is used in case there are several small reorgs
// along the way that drag out the confirmation time.
conflictReorgCeiling = conflictReorgBuffer + 16
)

var (
Expand All @@ -127,6 +145,11 @@ var (
errUpfrontShutdownScriptNotSupported = errors.New("peer does not support" +
"option upfront shutdown script")

// errFundingInputSpent is returned if the initiator sees that the
// inputs to the funding transaction have been spent after broadcasting
// the funding transaction.
errFundingInputSpent = errors.New("a funding input has been spent")

zeroID [32]byte
)

Expand Down Expand Up @@ -1214,8 +1237,8 @@ func (f *Manager) advancePendingChannelState(
}

confChannel, err := f.waitForFundingWithTimeout(channel)
if err == ErrConfirmationTimeout {
return f.fundingTimeout(channel, pendingChanID)
if err == ErrConfirmationTimeout || err == errFundingInputSpent {
return f.fundingTimeout(channel, pendingChanID, err)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. So now we close the channel immediately when a double spend is detected. What if there is a reorg and the funding tx confirms?

Would it make sense to unlock our UTXOs immediately, but not forget the pending channel until X confirmations?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then the channel is lost. this is an issue with RegisterSpendNtfn only waiting for 1 confirmation before notifying. i don't think adding the logic is worth it, the better fix is to fix the api

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unlock-immediately, forget channel later logic would be useful for dual funding, since without it the peer could double-spend and force our inputs into limbo for 3-6 blocks.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And even if the "channel is lost", are we able to force close it rather than losing funds?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with the existing API, it seems unnecessary to lose funds in this case. Looks like SpendEvent contains a Reorg and a Done channel specifically for this scenario.

So we can immediately unlock our UTXOs on a spend notification and completely forget about the pending channel once Done is sent upon.

For contract court, we should really be doing something similar.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided not to change the code here. If you want to make this and other code re-org safe, feel free. Already you can lose funds due to a funding transaction getting reorg'd out and replaced with a double-spend, so I don't think this patch makes things much worse

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already you can lose funds due to a funding transaction getting reorg'd out and replaced with a double-spend, so I don't think this patch makes things much worse

I don't understand this logic -- if we double-spend our own UTXO in a reorg, we should strictly benefit (able to spend the same UTXO both on and off chain). If the peer is initiator and could double-spend, that's our fault for choosing too small of a minimum_depth.

This patch does make things worse. Before this patch, we never forget the funding tx, so we can always recover if the funding tx is reorg'd in. After this patch, we cannot.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this patch, we never forget the funding tx, so we can always recover if the funding tx is reorg'd in. After this patch, we cannot.

The current issue with LND is that if there is a double spend it'll be stuck in funding flow never being able to initiate funding with the same peer again. There are various legit reasons to double spend so "whatever, the peer was an asshole anyway" isn't sufficient.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@morehouse Can you review the latest diff here? It should address the reorg situation. I chose not to use Done since it is not always sent on if the confirmation/spend notification is old. Instead of changing the txnotifier logic in this PR, I made the funding code just register for a confirmation notification of 144 blocks. If the conflict tx doesn't get 144 confirmations in a period of 160 blocks, we'll fall back to the old behavior in case the funding tx actually gets reorg'd back in. With the changes, I don't think anything should be worse now

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reorg approach LGTM, though it's a shame we can't use Done, as it would simplify the funding manager side.

Still need a test for funding tx being reorged in.

} else if err != nil {
return fmt.Errorf("error waiting for funding "+
"confirmation for ChannelPoint(%v): %v",
Expand Down Expand Up @@ -2441,11 +2464,10 @@ type confirmedChannel struct {
}

// fundingTimeout is called when callers of waitForFundingWithTimeout receive
// an ErrConfirmationTimeout. It is used to clean-up channel state and mark the
// channel as closed. The error is only returned for the responder of the
// channel flow.
// an ErrConfirmationTimeout or errFundingInputSpent error. It is used to
// clean-up channel state and mark the channel as closed.
func (f *Manager) fundingTimeout(c *channeldb.OpenChannel,
pendingID [32]byte) error {
pendingID [32]byte, fundingErr error) error {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the general error interface here means that we can technically pass anything in here. Seems like it would make sense to either make a specific error type or use an enum to restrict use?


// We'll get a timeout if the number of blocks mined since the channel
// was initiated reaches maxWaitNumBlocksFundingConf and we are not the
Expand All @@ -2472,9 +2494,6 @@ func (f *Manager) fundingTimeout(c *channeldb.OpenChannel,
c.FundingOutpoint, err)
}

timeoutErr := fmt.Errorf("timeout waiting for funding tx (%v) to "+
"confirm", c.FundingOutpoint)

// When the peer comes online, we'll notify it that we are now
// considering the channel flow canceled.
f.wg.Add(1)
Expand Down Expand Up @@ -2502,10 +2521,10 @@ func (f *Manager) fundingTimeout(c *channeldb.OpenChannel,

// The reservation won't exist at this point, but we'll send an
// Error message over anyways with ChanID set to pendingID.
f.failFundingFlow(peer, pendingID, timeoutErr)
f.failFundingFlow(peer, pendingID, fundingErr)
}()

return timeoutErr
return fundingErr
}

// waitForFundingWithTimeout is a wrapper around waitForFundingConfirmation and
Expand All @@ -2517,11 +2536,12 @@ func (f *Manager) waitForFundingWithTimeout(
ch *channeldb.OpenChannel) (*confirmedChannel, error) {

confChan := make(chan *confirmedChannel)
spentChan := make(chan error, 1)
timeoutChan := make(chan error, 1)
cancelChan := make(chan struct{})

f.wg.Add(1)
go f.waitForFundingConfirmation(ch, cancelChan, confChan)
go f.waitForFundingConfirmation(ch, cancelChan, confChan, spentChan)

// If we are not the initiator, we have no money at stake and will
// timeout waiting for the funding transaction to confirm after a
Expand All @@ -2533,6 +2553,9 @@ func (f *Manager) waitForFundingWithTimeout(
defer close(cancelChan)

select {
case err := <-spentChan:
return nil, err

case err := <-timeoutChan:
if err != nil {
return nil, err
Expand Down Expand Up @@ -2573,14 +2596,32 @@ func makeFundingScript(channel *channeldb.OpenChannel) ([]byte, error) {
// confirmation, and then to notify the other systems that must be notified
// when a channel has become active for lightning transactions.
// The wait can be canceled by closing the cancelChan. In case of success,
// a *lnwire.ShortChannelID will be passed to confChan.
// a *lnwire.ShortChannelID will be passed to confChan. If this function
// detects that the inputs to the funding transaction have been spent by a
// different transaction, an error will be sent along the spentChan. The
// spentChan MUST be buffered.
//
// NOTE: This MUST be run as a goroutine.
func (f *Manager) waitForFundingConfirmation(
completeChan *channeldb.OpenChannel, cancelChan <-chan struct{},
confChan chan<- *confirmedChannel) {
confChan chan<- *confirmedChannel, spentChan chan<- error) {
Comment thread
carlaKC marked this conversation as resolved.
Outdated

defer f.wg.Done()

// If we are the initiator, we'll know the inputs to the funding
// transaction and will check for the inputs being used in a different
// transaction. This is done before the defer close(confChan) call so
// that the select statement in waitForFundingWithTimeout doesn't
// accidentally trigger on the closed channel instead of the error
// channel.
Comment on lines +2613 to +2616
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit of a layering violation to reference the calling function, I think it's enough to just say we don't want to signal on two channels because it will cause races.

if completeChan.IsInitiator && completeChan.ChanType.HasFundingTx() {
err := f.checkFundingInputsSpent(completeChan)
if err != nil {
spentChan <- err
return
}
}

defer close(confChan)

// Register with the ChainNotifier for a notification once the funding
Expand Down Expand Up @@ -2728,6 +2769,192 @@ func (f *Manager) waitForTimeout(completeChan *channeldb.OpenChannel,
}
}

// checkFundingInputsSpent checks whether the inputs to the funding transaction
// are spent by another transaction. This will allow the funding manager to
// forget the channel.
func (f *Manager) checkFundingInputsSpent(c *channeldb.OpenChannel) error {
numInputs := len(c.FundingTxn.TxIn)
errChan := make(chan error, numInputs)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder what would happen if defer close(done) is called and yet another error is sent to this errChan. Does it mean we have a leaked channel? I think this might happen if we have two inputs being double spent by the same conflict tx, the second input, which is checked inside a goroutine, will send an error.

Meanwhile could we make its buffer size to be 1 and use this pattern in registerConfilicTx?

select {
case errChan <- err:
case <- doneChan:
}

done := make(chan interface{})

// Close the done channel when this function exits, so that the
// goroutines can clean up.
defer close(done)

for _, input := range c.FundingTxn.TxIn {
f.wg.Add(1)
go f.registerConflictTx(
input, c.BroadcastHeight(), c.FundingTxn.TxHash(),
errChan, done,
)
}

select {
case err := <-errChan:
return err

case <-f.quit:
return ErrFundingManagerShuttingDown
}
}

// registerConflictTx registers to be notified of a conflict transaction. If a
// conflict transaction is found, we will wait register for conflictReorgBuffer
// confirmations. If we receive the confirmation notification within
// conflictReorgCeiling blocks, we'll signal the caller to clean up the channel
// state. Else, if a notification is not received, we'll exit gracefully and
// allow the caller to register for a funding transaction confirmation. The
// conflict transaction may have been reorg'd out and replaced with the funding
// transaction.
//
// NOTE: This MUST be run as a goroutine and the calling function must
// increment the funding manager's waitgroup.
func (f *Manager) registerConflictTx(txIn *wire.TxIn, broadcastHeight uint32,
fundingTxid chainhash.Hash, errChan chan error,
doneChan chan interface{}) {

defer f.wg.Done()

// We use the zero-taproot-pk-script here since it will notify only on
// the outpoint being spent and not the outpoint+pkscript. This is
// because:
// - it's not necessary to be notified on the pkscript being spent.
// - we cannot use ComputePkScript for an input that spends a taproot
// output.
zeroScript := chainntnfs.ZeroTaprootPkScript.Script()
spendNtfn, err := f.cfg.Notifier.RegisterSpendNtfn(
&txIn.PreviousOutPoint, zeroScript,
broadcastHeight-fundingBroadcastBuffer,
)
if err != nil {
errChan <- err
return
}

defer spendNtfn.Cancel()

select {
case spend, ok := <-spendNtfn.Spend:
if !ok {
errChan <- fmt.Errorf("spend chan closed")
return
}

// If the spending transaction is the funding transaction,
// we'll send nil on errChan and exit.
if *spend.SpenderTxHash == fundingTxid {
errChan <- nil
return
}

// Before we register for a confirmation notification after
// conflictReorgBuffer blocks, we'll register for block
// notifications so that if conflictReorgCeiling blocks pass
// without us receiving the confirmation notification, we'll
// exit gracefully.
epochClient, err := f.cfg.Notifier.RegisterBlockEpochNtfn(nil)
if err != nil {
errChan <- err
return
}

defer epochClient.Cancel()

// We'll be immediately notified of the best block. This will
// inform our end height.
var bestHeight int32
select {
case epoch, ok := <-epochClient.Epochs:
if !ok {
errChan <- fmt.Errorf("epoch chan closed")
return
}

bestHeight = epoch.Height

case <-f.quit:
return

case <-doneChan:
return
}

// Set the ending height to conflictReorgCeiling blocks after
// the best height.
endHeight := bestHeight + conflictReorgCeiling

// Register for a confirmation notification of
// conflictReorgBuffer blocks for the conflict transaction.
// We'll choose the first output's pkScript to notify on since
// it doesn't matter which output we choose to notify on.
var (
conflictTxid = spend.SpenderTxHash
conflictPkScript = spend.SpendingTx.TxOut[0].PkScript
conflictHeightHint = spend.SpendingHeight
)

confNtfn, err := f.cfg.Notifier.RegisterConfirmationsNtfn(
conflictTxid, conflictPkScript, conflictReorgBuffer,
uint32(conflictHeightHint),
)
if err != nil {
errChan <- err
return
}

defer confNtfn.Cancel()

for {
select {
case _, ok := <-confNtfn.Confirmed:
if !ok {
err := fmt.Errorf("conf chan closed")
errChan <- err
return
}

// The conflict transaction is considered
// reorg-safe and we can signal the caller to
// clean up this channel.
errChan <- errFundingInputSpent
return

case epoch, ok := <-epochClient.Epochs:
if !ok {
err := fmt.Errorf("epoch chan closed")
errChan <- err
return
}

if epoch.Height >= endHeight {
// The conflict transaction did not
// reach a sufficient number of
// confirmations to be considered
// reorg-safe. It is possible that the
// conflict transaction was reorg'd
// out. In this case, we'll just fall
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the funding tx must have been confirmed at this point, given it's usually 6 and the reorg ceiling is 160. Having to consider reorg really makes things much more complicated...don't know an easy fix here, but maybe we could place the two confirmation ntfn in two goroutines, something like,

  1. RegisterSpendNtfn for the inputs
  2. upon receiving the spend notification, register two RegisterConfirmationsNtfn, one for the conflict, one for the funding tx
  3. exit whenever receiving the confirm notification(but then we have to think about the relationship between the user configured conf target for funding tx and the reorg ceiling we choose here).

Just a thought, non-blocking.

// back to the happy path behavior and
// wait for the funding tx to confirm.
errChan <- nil
return
}

case <-f.quit:
return

case <-doneChan:
return
}
}

case <-f.quit:
// The funding manager is shutting down.

case <-doneChan:
// The caller is signalling for us to clean up.
}
}

// makeLabelForTx updates the label for the confirmed funding transaction. If
// we opened the channel, and lnd's wallet published our funding tx (which is
// not the case for some channels) then we update our transaction label with
Expand Down Expand Up @@ -3256,7 +3483,9 @@ func (f *Manager) waitForZeroConfChannel(c *channeldb.OpenChannel,
// is already confirmed, the chainntnfs subsystem will return with the
// confirmed tx. Otherwise, we'll wait here until confirmation occurs.
confChan, err := f.waitForFundingWithTimeout(c)
if err != nil {
if err == errFundingInputSpent {
return f.fundingTimeout(c, pendingID, err)
} else if err != nil {
return fmt.Errorf("error waiting for zero-conf funding "+
"confirmation for ChannelPoint(%v): %v",
c.FundingOutpoint, err)
Expand Down
Loading