[breacharbiter] Split justice tx in case of delayed confirmation. #5036

halseth · 2021-02-17T13:55:30Z

This PR attempts to solve the theoretical pinning issue that can occur for anchor type channels in the breach case: lightning/bolts#803

If 4 blocks pass without the justie tx confirming (it is created using a confirmation target of 2), we will attempt to sweep the commitment outputs separately from the HTLC outputs.

TODO

Additional test

breacharbiter.go

Roasbeef

Nice work on this! The high level algo is much clearer from my PoV after another pass.

I definitely think we should bolster our test coverage to exercise these cases just to ensure we're not missing an edge case. It also might be helpful to document the state/transaction tree structure if something exceptional does happen.

With the current param of a delay of 4 blocks, I'd expect this to in practice be the default way lnd handles breaches. We end up spending more on chain fees, but could say the extra assurance that no funny businesses is happening may be worth it....

breacharbiter.go

Roasbeef · 2021-03-10T01:51:30Z

breacharbiter.go

 		for _, inp := range breachInfo.breachedOutputs {
+			// If the spent outpoint is not among the ouputs that
+			// were breached , we can ignore it.


Nit: extra space before the comma.

breacharbiter.go

cfromknecht

solid changes! only thing i see with this PR is that it may never actually broadcast the split justice transaction if lnd doesn't stay up long enough to observe 4 blocks

breacharbiter.go

cfromknecht · 2021-03-16T17:53:47Z

breacharbiter.go

+			// was published with a 2-block fee estimate, so it's
+			// not unexpected that four blocks without confirmation
+			// can pass.
+			if blocksPassed < 4 {


note that we may never pass this check on mobile, since lnd needs to observe 4 block consecutively without restarting. not sure what the best way to handle that is, is there a way we can load the broadcast height on startup?

Hmm, yeah this is a good point...since we no longer save the finalized transaction, we can't use the locktime there to mark our last broadcast height. We do have the CloseHeight as part of the close summary though, which tracks the height the breaching transaction was confirmed. I think that should work as a replacement?

Roasbeef · 2021-04-14T23:03:43Z

Tests still failing (to compile):

Error: ./breacharbiter_test.go:416:15: frs.rs.Finalize undefined (type RetributionStore has no field or method Finalize)
Error: ./breacharbiter_test.go:425:2: not enough arguments to return
Error: ./breacharbiter_test.go:425:15: frs.rs.GetFinalizedTxn undefined (type RetributionStore has no field or method GetFinalizedTxn)

breacharbiter.go

Roasbeef · 2021-04-23T19:34:00Z

Do we need #5229 if the first two commits are now included in this diff?

Roasbeef

Really nice iteration between the current and prior diff! Need to do one additional round on my end to review the diff as a hole to fully grok the new possible state transitinos in the main brar handing sate machine.

Roasbeef · 2021-04-23T19:38:49Z

contractcourt/chain_watcher.go

 	}

-	log.Infof("Breached channel=%v marked pending-closed",
-		c.cfg.chanState.FundingOutpoint)
+	// Hand the retribution info over to the breach arbiter.


Ah yeah this makes total sense in retrospect: we always need to ensure that we've persisted any state that might affect the downstream consumer of our message before we send it.

breacharbiter_test.go

Roasbeef · 2021-04-23T19:50:59Z

breacharbiter.go

+				signDesc.DoubleTweak.PubKey(),
+			).SerializeCompressed()
+
+			// If the HTLC output was spent using the revocation


What would happen otherwise, we'd have some empty iterations in the sweeper until it removed the input due to hitting the failed sweep counter ceiling?

Ah I now from below that this will be a new case since we'll at times just sweep the HTLCs in isolation.

Earlier we were only calling this if we detected a spend that wasn't ours, now we use it for our own spends as well, which is why we need this.

breacharbiter.go

Roasbeef · 2021-04-23T20:01:24Z

breacharbiter.go

+			// published with a 2-block fee estimate, so it's not
+			// unexpected that four blocks without confirmation can
+			// pass.
+			splitHeight := breachInfo.breachHeight +


Roasbeef · 2021-04-23T20:02:40Z

breacharbiter.go

+	// smaller variants of the justice tx. We do this to mitigate an attack
+	// the channel peer can do by pinning the HTLC outputs of the
+	// commitment with low-fee HTLC transactions.
+	blocksPassedSplitPublish = 4


Feels hard to properly tune these values in a way that can handle sustained fee bursts like we're seeing now, but nevertheless, this is a start to patch an unlikely, but possible attack vector.

Roasbeef · 2021-04-23T20:03:03Z

breacharbiter_test.go

@@ -1798,6 +1798,190 @@ func testBreachSpends(t *testing.T, test breachTest) {
 	assertBrarCleanup(t, brar, alice.ChanPoint, alice.State().Db)
 }

+// TestBreachDelayedJusticeConfirmation tests that the breach arbiter will


halseth · 2021-04-26T13:37:43Z

Do we need #5229 if the first two commits are now included in this diff?

Not really, I just separated it in case we wanted to merge it in isolation, as it is really an existing issue.

halseth · 2021-04-26T13:39:32Z

Added an itest in a separate PR, since we depend on a change to btcd to drive the test: #5242

PTAL

halseth · 2021-05-05T19:17:44Z

ping @cfromknecht @Roasbeef

Roasbeef · 2021-05-06T23:17:41Z

Needs a rebase w/ the first two commits merged in!

cfromknecht

Did another pass, looks pretty good. Only outstanding question was this one re: mobile #5036 (comment)

Any ideas of how to handle this?

cfromknecht · 2021-05-06T23:34:49Z

breacharbiter.go

+			if len(txIn.Witness) == 3 &&
+				bytes.Equal(txIn.Witness[1], revokeKey) {
+				break
+			}


can this section about use the IsHtlcSpendRevoke method created in the last commit?

oh i see, the commits are out of order on GH it seems

cfromknecht · 2021-05-06T23:38:43Z

breacharbiter.go


-		if err == lnwallet.ErrDoubleSpend {


do we still want to silence the log message if the error is ErrDoubleSpend?

I think we still want to log it? (we also logged any error before, even double spend)

cfromknecht · 2021-05-06T23:40:37Z

breacharbiter.go

 			brarLog.Infof("Attempting another justice tx "+
 				"with %d inputs",
 				len(breachInfo.breachedOutputs))

+			wg.Wait()


is there a reason we wait for this towards the end? or can it also be done as soon as the channel fires?

it's because for the case where we break the loop, we go straight into wg.Wait(). In this particular case we are using the goto, so we add the wait before we go back to launch a new go routine.

breacharbiter_test.go

Roasbeef

LGTM 💿

breacharbiter.go

Roasbeef · 2021-05-07T02:01:49Z

Did another pass, looks pretty good. Only outstanding question was this one re: mobile #5036 (comment)

The latest diff now uses the stored breach height compared to the current block height, and proceed w/ the new logic if that delta exceeds our target time window.

halseth · 2021-05-07T10:20:44Z

Did another pass, looks pretty good. Only outstanding question was this one re: mobile #5036 (comment)

The latest diff now uses the stored breach height compared to the current block height, and proceed w/ the new logic if that delta exceeds our target time window.

Yeah, exactly.

cfromknecht

LGTM 🏆

We split the method waitForSpendEvent into two, such that we can reuse it in case the commitment is spent by various transactions.

inputs Since we want to test more complex combinations of spends of the breached outputs, we use two maps tracking 1. which transaction will spend the outpoint 2. which outpoints we expect the breacharbiter to include in the justice tx This let us trigger spends of the individual outputs, and depending on what we want to test check whether the breacharbiter sweeps the expected outpoints.

Since we want to potentially broadcast multiple versions of the justice TX, instead of waiting for confirmation of a specific TXID, we instead wait for the breached outputs to be spent.

Now that we don't rely on the justice tx TXID anymore, we can remove finalization of it. Instead we'll recreate the transaction when needed from the persisted retribution info.

We define a new struct justiceTxVariants, which holds three different justice transactions: 1. The "normal" justice tx that spends all breached outputs 2. A tx that spends only the breached to_local output and to_remote output (can be nil if none of these exist) 3. A tx that spends all the breached HTLC outputs (can be nil if no HTLC outputs exist) This will later be used to sweep the time sensitive outputs separately, in case the normal justice tx doesn't confirm in time.

confirming In case 4 block passes without our justice tx confirming, we'll "split" it up, and separately sweep the commitment outs, and HTLC outs.

Since we also must count revoked funds swept from second level revoked outputs, we move the funds counting into the updateBreachInfo method, where we already are checking whether the spend is by us or the remote. We also clean up the logs a bit, to log the incremental sweep of funds that now can happen.

halseth added safety General label for issues/PRs related to the safety of using the software anchors v0.13 labels Feb 17, 2021

halseth added this to the 0.13.0 milestone Feb 17, 2021

halseth requested a review from cfromknecht February 17, 2021 13:55

Roasbeef added the P1 MUST be fixed or reviewed label Feb 17, 2021

Roasbeef self-requested a review February 17, 2021 19:37

Roasbeef requested changes Feb 27, 2021

View reviewed changes

halseth force-pushed the breacharbiter-justice-splitting branch from de91e70 to 3990efa Compare March 3, 2021 12:21

Roasbeef reviewed Mar 10, 2021

View reviewed changes

Roasbeef requested changes Mar 10, 2021

View reviewed changes

breacharbiter.go Show resolved Hide resolved

cfromknecht reviewed Mar 16, 2021

View reviewed changes

Roasbeef requested review from cfromknecht and Roasbeef April 14, 2021 19:43

halseth force-pushed the breacharbiter-justice-splitting branch 3 times, most recently from 8b1bc9d to 9818b6e Compare April 20, 2021 19:39

halseth commented Apr 20, 2021

View reviewed changes

breacharbiter.go Outdated Show resolved Hide resolved

halseth force-pushed the breacharbiter-justice-splitting branch from 9818b6e to c2e9a7d Compare April 21, 2021 11:55

Roasbeef reviewed Apr 23, 2021

View reviewed changes

halseth force-pushed the breacharbiter-justice-splitting branch 2 times, most recently from 597d415 to a0d7fae Compare April 26, 2021 13:33

halseth mentioned this pull request Apr 26, 2021

Breacharbiter justice splitting itest #5242

Open

2 tasks

halseth requested a review from Roasbeef April 26, 2021 13:39

cfromknecht reviewed May 7, 2021

View reviewed changes

Roasbeef approved these changes May 7, 2021

View reviewed changes

breacharbiter.go Outdated Show resolved Hide resolved

breacharbiter.go Show resolved Hide resolved

halseth force-pushed the breacharbiter-justice-splitting branch 2 times, most recently from 86cf235 to 303ce6d Compare May 7, 2021 10:17

cfromknecht approved these changes May 11, 2021

View reviewed changes

halseth added 12 commits May 12, 2021 12:32

breacharbiter: extract countRevokedFunds

bca5839

breacharbiter: split waitForSpendEvent

a6724c1

We split the method waitForSpendEvent into two, such that we can reuse it in case the commitment is spent by various transactions.

lntest/mock: set input index on spend event

3aa5e65

breacharbiter_test: extract sweep tx creation into method

e7ee5ad

input: create IsHtlcSpendRevoke

0a0b5f8

breacharbiter: don't transition to second level if own spend

c3b2791

breacharbiter: replace justice tx conf check with spend check

3be9b74

Since we want to potentially broadcast multiple versions of the justice TX, instead of waiting for confirmation of a specific TXID, we instead wait for the breached outputs to be spent.

breacharbiter: remove justiceTx finalization

783d1f9

Now that we don't rely on the justice tx TXID anymore, we can remove finalization of it. Instead we'll recreate the transaction when needed from the persisted retribution info.

breacharbiter: broadcast "splitted" justice tx if spend all not

db0ec12

confirming In case 4 block passes without our justice tx confirming, we'll "split" it up, and separately sweep the commitment outs, and HTLC outs.

halseth force-pushed the breacharbiter-justice-splitting branch from 303ce6d to 02268b8 Compare May 12, 2021 10:32

Roasbeef merged commit c0acdd8 into lightningnetwork:master May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[breacharbiter] Split justice tx in case of delayed confirmation. #5036

[breacharbiter] Split justice tx in case of delayed confirmation. #5036

halseth commented Feb 17, 2021 •

edited

Loading

Roasbeef left a comment

Roasbeef Mar 10, 2021

cfromknecht left a comment

cfromknecht Mar 16, 2021

Roasbeef Apr 14, 2021

Roasbeef commented Apr 14, 2021

Roasbeef commented Apr 23, 2021

Roasbeef left a comment

Roasbeef Apr 23, 2021

Roasbeef Apr 23, 2021

Roasbeef Apr 23, 2021

halseth Apr 26, 2021

Roasbeef Apr 23, 2021

Roasbeef Apr 23, 2021

Roasbeef Apr 23, 2021

halseth commented Apr 26, 2021

halseth commented Apr 26, 2021

halseth commented May 5, 2021

Roasbeef commented May 6, 2021

cfromknecht left a comment

cfromknecht May 6, 2021

cfromknecht May 6, 2021

cfromknecht May 6, 2021

halseth May 7, 2021

cfromknecht May 6, 2021

halseth May 7, 2021

Roasbeef left a comment

Roasbeef commented May 7, 2021

halseth commented May 7, 2021

cfromknecht left a comment

[breacharbiter] Split justice tx in case of delayed confirmation. #5036

[breacharbiter] Split justice tx in case of delayed confirmation. #5036

Conversation

halseth commented Feb 17, 2021 • edited Loading

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Apr 14, 2021

Roasbeef commented Apr 23, 2021

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth commented Apr 26, 2021

halseth commented Apr 26, 2021

halseth commented May 5, 2021

Roasbeef commented May 6, 2021

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef commented May 7, 2021

halseth commented May 7, 2021

cfromknecht left a comment

Choose a reason for hiding this comment

halseth commented Feb 17, 2021 •

edited

Loading