funding: forget funding tx if conflict tx confirms by Crypt-iQ · Pull Request #7036 · lightningnetwork/lnd

Crypt-iQ · 2022-10-13T22:08:47Z

Replaces #5567
Fixes #1494
Fixes #1623
Fixes #386

Tried to give commit credit, but it didn't seem to work. This allows the initiator to forget the funding transaction a conflict transaction has confirmed.

morehouse · 2022-11-09T18:48:14Z

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?

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

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.

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

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.

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

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.

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.

@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

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.

morehouse · 2022-11-09T19:36:42Z

Can we also test that Alice is able to respend any UTXOs that should be unlocked after the pending channel is cancelled?

morehouse · 2022-11-09T19:38:20Z

Can we also add a test where the double-spend is reorged out and the funding tx is reorged in?

morehouse · 2022-11-09T22:08:49Z

Fixes #386

I think there is still one more piece to fix #386. We need to unlock our UTXOs after a timeout, to allow the double spend to happen in the first place. This PR only detects the double spend once it happens.

carlaKC

Only minor comments remaining, looks good.

carlaKC · 2022-12-14T21:15:15Z

+// 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 {


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?

carlaKC · 2022-12-14T21:21:22Z

+	// 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.


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.

carlaKC · 2022-12-14T21:23:45Z

+					err := fmt.Errorf("spend chan closed")
+					errChan <- err


Rather use chainntfs.ErrChainNotifierShuttingDown ?

carlaKC · 2022-12-14T21:32:55Z

+
+	var (
+		chanAmt = btcutil.Amount(1_000_000)
+		pushAmt = btcutil.Amount(0)


unnecessary?

carlaKC · 2022-12-14T21:33:36Z

+	if zeroConf {
+		openReq.CommitmentType = lnrpc.CommitmentType_ANCHORS
+	}


Comment explaining this?

This allows the funder to remove the channel if a conflict transaction confirms. Co-authored by: John Griffith <me@joh.ng>

Co-authored by: John Griffith <me@joh.ng>

yyforyongyu

It'd be great if we could extend a function on top of RegisterConfirmationsNtfn such that when the inputs are double spent we will be notified. Meanwhile left some questions, and I think this one is almost LGTM, except the itest needs to be updated so we can see all cases are hit!

yyforyongyu · 2023-01-05T08:33:19Z

+	// 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


So I guess we could use ReorgSafetyLimit instead?

yyforyongyu · 2023-01-05T09:22:58Z

+// forget the channel.
+func (f *Manager) checkFundingInputsSpent(c *channeldb.OpenChannel) error {
+	numInputs := len(c.FundingTxn.TxIn)
+	errChan := make(chan error, numInputs)


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: }

yyforyongyu · 2023-01-05T09:29:49Z

+					// confirmations to be considered
+					// reorg-safe. It is possible that the
+					// conflict transaction was reorg'd
+					// out. In this case, we'll just fall


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,

RegisterSpendNtfn for the inputs

upon receiving the spend notification, register two RegisterConfirmationsNtfn, one for the conflict, one for the funding tx

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.

ellemouton · 2023-07-28T16:07:27Z

adding comment so bot doesnt close. @Crypt-iQ - consider marking these as drafts

Crypt-iQ · 2023-07-28T16:08:02Z

comment

lightninglabs-deploy · 2024-01-23T23:20:09Z

@positiveblue: review reminder
@Crypt-iQ, remember to re-request review from reviewers when ready

lightninglabs-deploy · 2024-01-24T16:11:28Z

Closing due to inactivity

lightninglabs-deploy · 2024-01-24T17:14:04Z

Closing due to inactivity

morehouse · 2024-01-24T17:18:32Z

^

guggero · 2024-01-29T12:26:57Z

!lightninglabs-deploy mute

AbhinavAnand241201

PR Review: "funding: forget funding tx if conflict tx confirms"

Review Process

I've performed the following review steps:

Checked out PR #7036 locally and compiled the code successfully
Run unit tests for the funding package (go test ./funding -v)
Attempted to run integration tests, though the specific test case may require a complete test suite run
Reviewed each file change commit by commit
Analyzed error handling, logging, and potential side effects
Verified CI is passing on the PR

Code Analysis

Changes Overview

This PR addresses three long-standing issues (#386, #1494, #1623) ,out of which two are closed now...., related to handling funding transactions whose inputs have been double-spent. Currently, if a funding transaction's inputs are spent by another transaction, the channel remains stuck in a pending state indefinitely, creating "zombie" channels. This PR adds logic to:

Detect when inputs to a funding transaction are spent by another transaction
Wait for the conflicting transaction to receive sufficient confirmations (144 blocks)
Properly clean up and forget the pending channel when the conflict is confirmed

File-by-File Review

1. `funding/manager.go`

New constants:

fundingBroadcastBuffer = 12  // blocks before broadcast height to use as hint
conflictReorgBuffer = 144    // number of confirmations to consider conflict tx safe
conflictReorgCeiling = 160   // max wait time for conflict tx confirmations

The constants use reasonable values - 144 blocks (approximately 1 day) is a good balance for considering a transaction safely confirmed, while 160 provides sufficient buffer for detecting reorgs.

New error:

errFundingInputSpent = errors.New("a funding input has been spent")

This error message could be more descriptive to help debugging.

New functions:

checkFundingInputsSpent: Methodically checks each input to see if it's been spent
registerConflictTx: Handles notification and confirmation logic for conflict transactions

Modified functions:

waitForFundingConfirmation: Now checks for spent inputs
fundingTimeout: Updated to handle both timeout and input-spent cases
advancePendingChannelState: Handles the new error case
waitForZeroConfChannel: Added handling for input-spent case

The error handling flow is well-designed, propagating the error correctly through the call stack. The solution is comprehensive, handling both regular and zero-conf channels.

2. `funding/manager_test.go`

The test updates are primarily focused on making the existing mock implementation work with the new code paths. The mockNotifier struct now includes a spentChan to allow testing of the spend notification path.

While the existing tests ensure the normal flow continues to work, there isn't a dedicated test case for the conflict detection logic itself.

3. `lntest/harness_net.go`

A simple refactoring of the OpenPendingChannel function to accept a complete OpenChannelRequest struct rather than individual parameters. This is a good change that improves flexibility and readability.

4. `lntest/itest/lnd_funding_test.go`

A new integration test testFundingConflict that:

Creates a pending channel
Creates a conflicting transaction that spends the same inputs
Mines enough blocks to confirm the conflict
Verifies the channel is properly forgotten

This test provides good coverage of the core functionality added in this PR.

5. `lntest/itest/lnd_open_channel_test.go`

Minor update to use the new OpenPendingChannel function signature.

6. `lntest/itest/lnd_test_list_on_test.go`

Adds the new test to the integration test list.

Potential Issues and Improvements

Error Message Clarity:
The errFundingInputSpent error message could be more descriptive to help with debugging and understanding logs.
Documentation
The code would benefit from additional documentation explaining the conflict resolution process in detail, especially around the confirmation thresholds and fallback logic.
Unit Test Coverage
While there is a good integration test, the unit tests could be improved by:
- Adding a dedicated test case for conflict detection
- Testing the reorg fallback case (when a conflict tx doesn't get enough confirmations)
Code Readability:
The registerConflictTx function has complex nested select/channel logic that could be refactored into smaller helper functions to improve readability.

Recommendations

Error Message Improvement:

// Current:
errFundingInputSpent = errors.New("a funding input has been spent")

// Suggested:
errFundingInputSpent = errors.New("channel funding failed: input has been spent by another transaction")

Documentation Additions:

// checkFundingInputsSpent checks if any inputs to the funding transaction
// have been spent by a different transaction. This can happen if the same
// UTXO is used for multiple purposes or if a double-spend attack occurs.

// registerConflictTx watches for transactions that spend the same inputs as
// our funding transaction. If such a transaction receives conflictReorgBuffer
// confirmations (144 blocks), we consider the funding transaction permanently
// failed and clean up the pending channel. If the conflict doesn't reach this
// threshold within conflictReorgCeiling blocks (160), we assume it might have
// been reorg'd out and fall back to normal confirmation behavior.

Additional Unit Tests:
- Add a test where the mock notifier sends a conflict transaction notification
  -Add a test for the reorg fallback path where conflictReorgCeiling is reached
Integration Test Enhancement:
- Add a test case that creates a conflict transaction but doesn't mine enough blocks to make it permanent, then verify the node still waits for the original funding tx.
optional improvements :
=> Consider refactoring the nested channel/select structure in registerConflictTx to improve readability, perhaps by breaking out some of the logic into separate helper functions.

ACK - The approach and implementation are sound, addressing a real problem in a well-structured way. ..... have good test coverage.

tests pass, which is a good sign! The unit tests for the funding manager run successfully,.....

Crypt-iQ added the funding Related to the opening of new channels with funding transactions on the blockchain label Oct 13, 2022

Crypt-iQ mentioned this pull request Oct 13, 2022

Funding detect double spends #5567

Closed

Crypt-iQ force-pushed the 386_unconf_funding branch 4 times, most recently from 1d5eb90 to 6bdccbd Compare October 17, 2022 16:06

saubyk mentioned this pull request Oct 26, 2022

Handling non-confirmed funding transactions #386

Open

2 tasks

morehouse reviewed Nov 9, 2022

View reviewed changes

carlaKC reviewed Nov 15, 2022

View reviewed changes

Comment thread funding/manager.go Outdated

Comment thread funding/manager.go Outdated

Comment thread funding/manager.go Outdated

saubyk requested review from positiveblue and yyforyongyu November 22, 2022 18:19

Crypt-iQ force-pushed the 386_unconf_funding branch from 6bdccbd to 8d8d6f6 Compare December 7, 2022 21:02

Crypt-iQ requested a review from carlaKC December 7, 2022 21:03

Crypt-iQ force-pushed the 386_unconf_funding branch from 8d8d6f6 to 0f914c2 Compare December 7, 2022 21:07

carlaKC reviewed Dec 14, 2022

View reviewed changes

Crypt-iQ added 2 commits December 23, 2022 12:10

funding: let initiator forget pending channel in case of conflict tx

f41ff36

This allows the funder to remove the channel if a conflict transaction confirms. Co-authored by: John Griffith <me@joh.ng>

itest: add itest for funding conflict tx confirming

b56b83c

Co-authored by: John Griffith <me@joh.ng>

Crypt-iQ force-pushed the 386_unconf_funding branch from 0f914c2 to b56b83c Compare December 23, 2022 17:10

yyforyongyu reviewed Jan 5, 2023

View reviewed changes

saubyk assigned Crypt-iQ May 18, 2023

saubyk added this to the v0.17.1 milestone Jun 15, 2023

ellemouton closed this Jul 28, 2023

Crypt-iQ reopened this Jul 28, 2023

saubyk added the P1 MUST be fixed or reviewed label Aug 8, 2023

saubyk removed this from the High Priority milestone Aug 8, 2023

AbhinavAnand241201 reviewed Apr 16, 2025

View reviewed changes

Conversation

Crypt-iQ commented Oct 13, 2022 • edited by saubyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

morehouse Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morehouse commented Nov 9, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellemouton commented Jul 28, 2023

Uh oh!

Crypt-iQ commented Jul 28, 2023

Uh oh!

lightninglabs-deploy commented Jan 23, 2024

Uh oh!

lightninglabs-deploy commented Jan 24, 2024

Uh oh!

lightninglabs-deploy commented Jan 24, 2024

Uh oh!

morehouse commented Jan 24, 2024

Uh oh!

guggero commented Jan 29, 2024

Uh oh!

AbhinavAnand241201 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

PR Review: "funding: forget funding tx if conflict tx confirms"

File-by-File Review

1. funding/manager.go

2. funding/manager_test.go

Crypt-iQ commented Oct 13, 2022 •

edited by saubyk

Loading

morehouse Nov 9, 2022 •

edited

Loading

AbhinavAnand241201 left a comment •

edited

Loading

1. `funding/manager.go`

2. `funding/manager_test.go`

3. `lntest/harness_net.go`

4. `lntest/itest/lnd_funding_test.go`

5. `lntest/itest/lnd_open_channel_test.go`

6. `lntest/itest/lnd_test_list_on_test.go`