From d71a4ee03342fe34222353a1d81ec0df4daef8b8 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Wed, 8 Jan 2020 09:06:59 -0300 Subject: [PATCH 1/4] contractcourt+itest: fix lingering pending channel after breach This fixes an issue where the contract court could leave a completely swept commit tx unresolved if it was swept by the remote party. This could happen if (our) commit tx just published was actually a previously revoked state, in which case the remote party would claim the funds via a justice transaction. This manifested itself in the testRevokedCloseRetribution integration test where at the end of the test Bob was left with a pending channel that never resolved itself. --- contractcourt/commit_sweep_resolver.go | 26 +++++++++++++++++++++----- lntest/itest/lnd_test.go | 9 +++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/contractcourt/commit_sweep_resolver.go b/contractcourt/commit_sweep_resolver.go index fe09dc09f..c184010a1 100644 --- a/contractcourt/commit_sweep_resolver.go +++ b/contractcourt/commit_sweep_resolver.go @@ -239,24 +239,40 @@ func (c *commitSweepResolver) Resolve() (ContractResolver, error) { // possible and publish the sweep tx. When the sweep tx // confirms, it signals us through the result channel with the // outcome. Wait for this to happen. + recovered := true select { case sweepResult := <-resultChan: - if sweepResult.Err != nil { + switch sweepResult.Err { + case sweep.ErrRemoteSpend: + // If the remote party was able to sweep this output + // it's likely what we sent was actually a revoked + // commitment. Report the error and continue to wrap up + // the contract. + c.log.Errorf("local commitment output was swept by "+ + "remote party via %v", sweepResult.Tx.TxHash()) + recovered = false + case nil: + // No errors, therefore continue processing. + c.log.Infof("local commitment output fully resolved by "+ + "sweep tx: %v", sweepResult.Tx.TxHash()) + default: + // Unknown errors. c.log.Errorf("unable to sweep input: %v", sweepResult.Err) return nil, sweepResult.Err } - - c.log.Infof("commit tx fully resolved by sweep tx: %v", - sweepResult.Tx.TxHash()) case <-c.quit: return nil, errResolverShuttingDown } // Funds have been swept and balance is no longer in limbo. c.reportLock.Lock() - c.currentReport.RecoveredBalance = c.currentReport.LimboBalance + if recovered { + // We only record the balance as recovered if it actually came + // back to us. + c.currentReport.RecoveredBalance = c.currentReport.LimboBalance + } c.currentReport.LimboBalance = 0 c.reportLock.Unlock() diff --git a/lntest/itest/lnd_test.go b/lntest/itest/lnd_test.go index 6c48ddc8b..475507bab 100644 --- a/lntest/itest/lnd_test.go +++ b/lntest/itest/lnd_test.go @@ -7586,6 +7586,15 @@ func testRevokedCloseRetribution(net *lntest.NetworkHarness, t *harnessTest) { } assertNodeNumChannels(t, carol, 0) + + // Mine enough blocks for Bob's channel arbitrator to wrap up the + // references to the breached channel. The chanarb waits for commitment + // tx's confHeight+CSV-1 blocks and since we've already mined one that + // included the justice tx we only need to mine extra DefaultCSV-2 + // blocks to unlock it. + mineBlocks(t, net, lntest.DefaultCSV-2, 0) + + assertNumPendingChannels(t, net.Bob, 0, 0) } // testRevokedCloseRetributionZeroValueRemoteOutput tests that Dave is able From 2110bfa40bcc4bf544fd7604e7039a356ae78d75 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Wed, 8 Jan 2020 09:08:47 -0300 Subject: [PATCH 2/4] contractcourt: test sweep of locally breached commit tx This adds a test to the commit sweeper resolver to ensure it behaves properly if the local node breaches a channel. In this situation the remote party is expected to sweep the breached output to itself and therefore the local party won't be able to recover any funds. --- contractcourt/commit_sweep_resolver_test.go | 93 ++++++++++++++++++++- 1 file changed, 92 insertions(+), 1 deletion(-) diff --git a/contractcourt/commit_sweep_resolver_test.go b/contractcourt/commit_sweep_resolver_test.go index 279cc6e84..da1fca11d 100644 --- a/contractcourt/commit_sweep_resolver_test.go +++ b/contractcourt/commit_sweep_resolver_test.go @@ -96,6 +96,7 @@ func (i *commitSweepResolverTestContext) waitForResult() { type mockSweeper struct { sweptInputs chan input.Input updatedInputs chan wire.OutPoint + sweepErr error } func newMockSweeper() *mockSweeper { @@ -112,7 +113,8 @@ func (s *mockSweeper) SweepInput(input input.Input, params sweep.Params) ( result := make(chan sweep.Result, 1) result <- sweep.Result{ - Tx: &wire.MsgTx{}, + Tx: &wire.MsgTx{}, + Err: s.sweepErr, } return result, nil } @@ -244,3 +246,92 @@ func TestCommitSweepResolverDelay(t *testing.T) { t.Fatal("unexpected resolver report") } } + +// TestCommitSweepResolverLocalBreach tests resolution when the local node +// publishes a breached output (one that is swept by the remote party). +func TestCommitSweepResolverLocalBreach(t *testing.T) { + t.Parallel() + defer timeout(t)() + + amt := int64(100) + outpoint := wire.OutPoint{ + Index: 5, + } + res := lnwallet.CommitOutputResolution{ + SelfOutputSignDesc: input.SignDescriptor{ + Output: &wire.TxOut{ + Value: amt, + }, + WitnessScript: []byte{0}, + }, + MaturityDelay: 3, + SelfOutPoint: outpoint, + } + + ctx := newCommitSweepResolverTestContext(t, &res) + + // Setup the sweeper so that it returns an error of the output being + // already swept. + ctx.sweeper.sweepErr = sweep.ErrRemoteSpend + + report := ctx.resolver.report() + expectedReport := ContractReport{ + Outpoint: outpoint, + Type: ReportOutputUnencumbered, + Amount: btcutil.Amount(amt), + LimboBalance: btcutil.Amount(amt), + } + if *report != expectedReport { + t.Fatalf("unexpected resolver report. want=%v got=%v", + expectedReport, report) + } + + ctx.resolve() + + ctx.notifier.confChan <- &chainntnfs.TxConfirmation{ + BlockHeight: testInitialBlockHeight - 1, + } + + // Allow resolver to process confirmation. + time.Sleep(100 * time.Millisecond) + + // Expect report to be updated. + report = ctx.resolver.report() + if report.MaturityHeight != testInitialBlockHeight+2 { + t.Fatal("report maturity height incorrect") + } + + // Notify initial block height. The csv lock is still in effect, so we + // don't expect any sweep to happen yet. + ctx.notifyEpoch(testInitialBlockHeight) + + select { + case <-ctx.sweeper.sweptInputs: + t.Fatal("no sweep expected") + case <-time.After(100 * time.Millisecond): + } + + // A new block arrives. The commit tx confirmed at height -1 and the csv + // is 3, so a spend will be valid in the first block after height +1. + ctx.notifyEpoch(testInitialBlockHeight + 1) + + <-ctx.sweeper.sweptInputs + + ctx.waitForResult() + + report = ctx.resolver.report() + expectedReport = ContractReport{ + Outpoint: outpoint, + Type: ReportOutputUnencumbered, + Amount: btcutil.Amount(amt), + MaturityHeight: testInitialBlockHeight + 2, + + // RecoveredBalance is zero due to the output having been swept + // by the remote party. + RecoveredBalance: 0, + } + if *report != expectedReport { + t.Fatalf("unexpected resolver report. want=%v got=%v", + expectedReport, report) + } +} From 9ed14a50a41037c5123ea95749cdcb30077e4856 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Thu, 7 May 2020 08:14:38 -0300 Subject: [PATCH 3/4] contractcourt: switch to table based testCommitSweepResolverDelay This improves readability since both instances of the test are very similar. --- contractcourt/commit_sweep_resolver_test.go | 147 +++++++------------- 1 file changed, 51 insertions(+), 96 deletions(-) diff --git a/contractcourt/commit_sweep_resolver_test.go b/contractcourt/commit_sweep_resolver_test.go index da1fca11d..866cb22b5 100644 --- a/contractcourt/commit_sweep_resolver_test.go +++ b/contractcourt/commit_sweep_resolver_test.go @@ -1,7 +1,6 @@ package contractcourt import ( - "reflect" "testing" "time" @@ -169,12 +168,13 @@ func TestCommitSweepResolverNoDelay(t *testing.T) { ctx.waitForResult() } -// TestCommitSweepResolverDelay tests resolution of a direct commitment output -// that is encumbered by a time lock. -func TestCommitSweepResolverDelay(t *testing.T) { - t.Parallel() +// testCommitSweepResolverDelay tests resolution of a direct commitment output +// that is encumbered by a time lock. sweepErr indicates whether the local node +// fails to sweep the output. +func testCommitSweepResolverDelay(t *testing.T, sweepErr error) { defer timeout(t)() + const sweepProcessInterval = 100 * time.Millisecond amt := int64(100) outpoint := wire.OutPoint{ Index: 5, @@ -192,87 +192,9 @@ func TestCommitSweepResolverDelay(t *testing.T) { ctx := newCommitSweepResolverTestContext(t, &res) - report := ctx.resolver.report() - if !reflect.DeepEqual(report, &ContractReport{ - Outpoint: outpoint, - Type: ReportOutputUnencumbered, - Amount: btcutil.Amount(amt), - LimboBalance: btcutil.Amount(amt), - }) { - t.Fatal("unexpected resolver report") - } - - ctx.resolve() - - ctx.notifier.confChan <- &chainntnfs.TxConfirmation{ - BlockHeight: testInitialBlockHeight - 1, - } - - // Allow resolver to process confirmation. - time.Sleep(100 * time.Millisecond) - - // Expect report to be updated. - report = ctx.resolver.report() - if report.MaturityHeight != testInitialBlockHeight+2 { - t.Fatal("report maturity height incorrect") - } - - // Notify initial block height. The csv lock is still in effect, so we - // don't expect any sweep to happen yet. - ctx.notifyEpoch(testInitialBlockHeight) - - select { - case <-ctx.sweeper.sweptInputs: - t.Fatal("no sweep expected") - case <-time.After(100 * time.Millisecond): - } - - // A new block arrives. The commit tx confirmed at height -1 and the csv - // is 3, so a spend will be valid in the first block after height +1. - ctx.notifyEpoch(testInitialBlockHeight + 1) - - <-ctx.sweeper.sweptInputs - - ctx.waitForResult() - - report = ctx.resolver.report() - if !reflect.DeepEqual(report, &ContractReport{ - Outpoint: outpoint, - Type: ReportOutputUnencumbered, - Amount: btcutil.Amount(amt), - RecoveredBalance: btcutil.Amount(amt), - MaturityHeight: testInitialBlockHeight + 2, - }) { - t.Fatal("unexpected resolver report") - } -} - -// TestCommitSweepResolverLocalBreach tests resolution when the local node -// publishes a breached output (one that is swept by the remote party). -func TestCommitSweepResolverLocalBreach(t *testing.T) { - t.Parallel() - defer timeout(t)() - - amt := int64(100) - outpoint := wire.OutPoint{ - Index: 5, - } - res := lnwallet.CommitOutputResolution{ - SelfOutputSignDesc: input.SignDescriptor{ - Output: &wire.TxOut{ - Value: amt, - }, - WitnessScript: []byte{0}, - }, - MaturityDelay: 3, - SelfOutPoint: outpoint, - } - - ctx := newCommitSweepResolverTestContext(t, &res) - - // Setup the sweeper so that it returns an error of the output being - // already swept. - ctx.sweeper.sweepErr = sweep.ErrRemoteSpend + // Setup whether we expect the sweeper to receive a sweep error in this + // test case. + ctx.sweeper.sweepErr = sweepErr report := ctx.resolver.report() expectedReport := ContractReport{ @@ -293,7 +215,7 @@ func TestCommitSweepResolverLocalBreach(t *testing.T) { } // Allow resolver to process confirmation. - time.Sleep(100 * time.Millisecond) + time.Sleep(sweepProcessInterval) // Expect report to be updated. report = ctx.resolver.report() @@ -308,7 +230,7 @@ func TestCommitSweepResolverLocalBreach(t *testing.T) { select { case <-ctx.sweeper.sweptInputs: t.Fatal("no sweep expected") - case <-time.After(100 * time.Millisecond): + case <-time.After(sweepProcessInterval): } // A new block arrives. The commit tx confirmed at height -1 and the csv @@ -319,19 +241,52 @@ func TestCommitSweepResolverLocalBreach(t *testing.T) { ctx.waitForResult() + // If this test case generates a sweep error, we don't expect to be + // able to recover anything. This might happen if the local commitment + // output was swept by a justice transaction by the remote party. + expectedRecoveredBalance := btcutil.Amount(amt) + if sweepErr != nil { + expectedRecoveredBalance = 0 + } + report = ctx.resolver.report() expectedReport = ContractReport{ - Outpoint: outpoint, - Type: ReportOutputUnencumbered, - Amount: btcutil.Amount(amt), - MaturityHeight: testInitialBlockHeight + 2, - - // RecoveredBalance is zero due to the output having been swept - // by the remote party. - RecoveredBalance: 0, + Outpoint: outpoint, + Type: ReportOutputUnencumbered, + Amount: btcutil.Amount(amt), + MaturityHeight: testInitialBlockHeight + 2, + RecoveredBalance: expectedRecoveredBalance, } if *report != expectedReport { t.Fatalf("unexpected resolver report. want=%v got=%v", expectedReport, report) } + +} + +// TestCommitSweepResolverDelay tests resolution of a direct commitment output +// that is encumbered by a time lock. +func TestCommitSweepResolverDelay(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + sweepErr error + }{{ + name: "success", + sweepErr: nil, + }, { + name: "remote spend", + sweepErr: sweep.ErrRemoteSpend, + }} + + for _, tc := range testCases { + tc := tc + ok := t.Run(tc.name, func(t *testing.T) { + testCommitSweepResolverDelay(t, tc.sweepErr) + }) + if !ok { + break + } + } } From 43b9318b2d806cf22549b297b41ba9d2405ab00d Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Fri, 8 May 2020 09:34:55 -0300 Subject: [PATCH 4/4] contractcourt/commit_sweep_resolver: reduce log msg to warn --- contractcourt/commit_sweep_resolver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contractcourt/commit_sweep_resolver.go b/contractcourt/commit_sweep_resolver.go index c184010a1..5c4990778 100644 --- a/contractcourt/commit_sweep_resolver.go +++ b/contractcourt/commit_sweep_resolver.go @@ -248,7 +248,7 @@ func (c *commitSweepResolver) Resolve() (ContractResolver, error) { // it's likely what we sent was actually a revoked // commitment. Report the error and continue to wrap up // the contract. - c.log.Errorf("local commitment output was swept by "+ + c.log.Warnf("local commitment output was swept by "+ "remote party via %v", sweepResult.Tx.TxHash()) recovered = false case nil: