Breach Arbiter Sweep 2nd layer HTLCs #264

cfromknecht · 2017-08-24T23:38:51Z

This PR builds upon the initial work of #241 by enabling the breach arbiter to sweep 2nd layer HTLCs. In it's current state, this PR:

sweeps 2nd layer HTLCs from revoked commitment states.
uses proper fee estimation when generating justice and commit sweep transactions.
uses updated BOLT 5 penalty txn fee calculation for justice txn fee.
adds a SpendableOutput interface that helps decouple transaction creation from the types of outputs being spent.
refactors txn generation logic to be more flexible w/ regard to spending these various types of outputs.
delays and memoizes the computation of the witness generation function needed to spend an output of interest.
add config for breach arbiter to enable more granular test cases and interleaving of different subsystems in the future.

Looking into a fix for issue #256 as this seems like the appropriate time to resolve this. A fix has been implemented, though it will be added in a separate PR to keep this one from spreading further.

This PR builds upon #308 which adds HodlHTLC mode. The modified integration test (testRevokedCloseRetributionRemoteHodl) requires the remote node to be started with the --htlchodl flag in order to properly test that we are sweeping extended-but-unsettled HTLCs.

mention-bot · 2017-08-24T23:38:53Z

@cfromknecht, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Roasbeef, @bryanvu and @cjamthagen to be potential reviewers.

Roasbeef

Nice work! Most of my comments are pretty minor: mostly style related, constant usage, etc.

I'll wait until you've finished up the remainder of the TODO items (tests, the slight decoupling) to start the next review cycle.

Roasbeef · 2017-08-28T22:23:40Z

breacharbiter.go

+	return nil
+}
+
+func (b *breachArbiter) waitForPendingCloseConfs(currentHeight int32) error {


Missing a godoc comment on this new method. I dig the code move though, makes the Start method much more digestible.

Roasbeef · 2017-08-28T22:30:39Z

breacharbiter.go

 		}

+		// TODO(conner): move responsibility of channel closure into


As is now, it seems the this is unnecessary as the goroutine within the LightningChannel struct should block until it's received a signal that the breachArbiter has written the retribution info to disk.

Actually, scratch that. I remember now that we started to draft up this approach, but abandoned it at the might for the sake of minimizing that prior PR.

Can you create an issue describing this case and what needs to be done to address it? Thanks!

Finally got around to this 🐢🐢🐢, peep le issue #327

Roasbeef · 2017-08-28T22:34:43Z

breacharbiter.go

+	witnessFunc lnwallet.WitnessGenerator
+}
+
+// newBreachedOutput assembles new breachedOutput that can be used by the breach


assembles new -> assembles a new

Roasbeef · 2017-08-28T22:45:33Z

breacharbiter.go

+	// deterministically generate a valid witness for each output. This will
+	// allow the breach arbiter to recover from failures, in the event that
+	// it must sign and broadcast the justice transaction.
+	var htlcOutputs = make([]*breachedOutput, nHtlcs)


Any particular reason for this initialization style? Could instead just be:

htlcOutputs := make([]*breachedOutput, nHtlcs)

Typically I use var to alert the reader to the fact that the variable is mutable, which helps to logically categorize them (especially in debugging+review) from static references, e.g. :=. Most often, the scope in which a variable is modified is limited to the following stanza, which permits the reader to assume that following stanza modifies or constructs the mutable variable being introduced. Even if the variable is modified much later in the function, the reader is at least alerted to which variables are intended to be altered dynamically. Counterintuitively, this in fact gives greater confidence as to which variables can be assumed to be static, which reduces working complexity and cognitive load on behalf of the reader. Otherwise, one more or less has to assume that any variable assigned with := could be mutable. There are certainly exceptions, but in general I've found it to be a very helpful form of self-documentation. Happy to change if we prefer the brevity of define+assign.

Roasbeef · 2017-08-28T22:48:45Z

breacharbiter.go

+		)
+	}
+
+	// TODO(conner) remove dependency on channel snapshot after decoupling


Minor nit on the TODO's added in this commit/PR: customarily in the project the format we use is TODO(nick): (notice the colon there). If we all use the same format, then it'll be easy for us to grep through later as we only have a single pattern to look for.

Plus, my current vimconfig detects this pattern and high-lights it distinctly ^_^

Noted. 😂

Roasbeef · 2017-08-28T23:02:37Z

breacharbiter.go

+	// witness_header_size.
+	var txWeight uint64 = 4*53 + 2
+	// Add receiver script witness and tx input
+	txWeight += 325 + 4*41


Similar comment here to that above: line spacing and magic numbers.

Roasbeef · 2017-08-28T23:04:08Z

breacharbiter.go

+// sweepSpendableOutputsTxn creates a signed transaction from a sequence of
+// spendable outputs by sweeping the funds into a single p2wkh output.
+func (b *breachArbiter) sweepSpendableOutputsTxn(
+	txWeight uint64,


Minor style nit: I think the first param can safely lie on the first line w/o going over the character limit.

Roasbeef · 2017-08-28T23:05:23Z

breacharbiter.go

+// sweepSpendableOutputsTxn creates a signed transaction from a sequence of
+// spendable outputs by sweeping the funds into a single p2wkh output.
+func (b *breachArbiter) sweepSpendableOutputsTxn(
+	txWeight uint64,


It seems that this function accounts for the weight of the inputs added (with their witnesses), but not the weight of the added outputs.

To remedy this, we can instead modify the param to be the totalWitnessWeight alone. Then within this function, we can craft the sweep transaction and compute the total weight of that. The final weight we'll use for fee estimation is the sum of both of these computed weights.

Roasbeef · 2017-08-28T23:08:28Z

breacharbiter.go


-	return sweepTx, nil
+	return txn, nil


Before returning transaction for singing, we should ensure that the transaction itself is sane. We can use blockchain.CheckTransactionSanity for this purpose. This'll catch things like a negative output due to a very high estimated fee.

In general whenever crafting transactions like this, we'll need to ensure that we don't try to create negative valued outputs, the CheckTransactionSanity function can do this job for us.

Didn't realize this was available, makes a lot of sense! :)

Roasbeef · 2017-08-28T23:09:07Z

breacharbiter.go

+
+	// With the fee calculated, we can now create the transaction using the
+	// information gathered above and the provided retribution information.
+	var txn = wire.NewMsgTx(2)


tnx := wire.NewMsgTx(2)

halseth · 2017-08-31T12:49:52Z

breacharbiter.go

+
+// waitForPendingCloseConfs dispatches confirmation notification subscribers
+// that mark any pending channels as fully closed when signaled.
+func (b *breachArbiter) waitForPendingCloseConfs(currentHeight int32) error {


The name wiatFor.. indicates that the method is synchronous, maybe call it watchForPendingCloseConfs

halseth · 2017-08-31T13:15:02Z

breacharbiter.go

+	// First, record the breach information and witness type for the local
+	// channel point.  This will allow us to completely generate a valid
+	// witness in the event of failures, as it will be persisted in the
+	// retribution store.  Here we use CommitmentNoDelay since


halseth · 2017-08-31T13:22:42Z

breacharbiter.go

@@ -1207,8 +1338,7 @@ func (bo *breachedOutput) Encode(w io.Writer) error {
 		return err
 	}

-	if err := lnwallet.WriteSignDescriptor(
-		w, &bo.signDescriptor); err != nil {
+	if err := lnwallet.WriteSignDescriptor(w, bo.signDesc); err != nil {


if line is too long, then maybe

err := lnwallet.WriteSignDescriptor(w, bo.signDesc) if err != nil {

is preferred?

halseth · 2017-08-31T13:26:08Z

breacharbiter_test.go

@@ -1,3 +1,5 @@
+// +build !rpctest


I thought this flag was for integration tests?

Correct! This one is saying to not include the unit tests in this file when compiling the integration tests, note the exclaimation point :)

ah, right XD

Roasbeef · 2017-09-19T18:40:09Z

breacharbiter.go

+	Store RetributionStore
+
+	// GenSweepScript generates the receiving scripts for swept outputs.
+	GenSweepScript func(lnwallet.WalletController) ([]byte, error)


Don't think this actually needs to take a WalletController as an argument. The function itself can simply close over the WalletController and take no arguments.

Roasbeef · 2017-09-19T18:40:58Z

breacharbiter.go

+	// HtlcSwitch gives the breach arbiter access to shutdown any channel
+	// links for which it detects a breach, ensuring no further activity
+	// will continue across the link.
+	HtlcSwitch *htlcswitch.Switch


We don't need the entire switch. Instead, we can simply take something with identical arguments to CloseLink.

Roasbeef · 2017-09-19T18:43:29Z

breacharbiter.go

+
+	// Wallet facilitates sweep transaction publication and further
+	// facilitates the generation of receiver scripts when sweeping funds.
+	Wallet *lnwallet.LightningWallet


The Wallet itself can similarly be omitted. From the PoV of the breach arb, the only functionality it needs from the wallet is `PublishTransaction.

This commit alters the existing retribution post breach conf test case with the intention of testing the correct response in event that the remote party broadcasts a prior state while HTLCs have been extended. This serves as a preliminary integration for an expansion of the breach arbiter integration tests. The primary change involves using the new htlchodl mode for debugging, which causes the remote peer to ignore any intent to settle incoming HTLCs. The result is that any payments sent to the remote party are held in limbo, allowing us to test for these conditions more accurately. Currently the test case only tests that the justice transaction is mined. After we have fully integrated the breach arbiter to sweep 2nd layer HTLCs, this test will be altered to check for spends from the appropriate inputs.

This commit also adds a BreachConfig to abstract the instantiation of the breach arbiter, as well as various formatting improvements.

This commit also adds an incoming flag to HtlcRetribution struct to allow the breach arbiter to generate the appropriate witness based on the htlc's directionality. It also ensures that the size of the htlc retribution slice is now determined by the size of the number of htlcs present in the revoked snapshot, which fixes a minor bug that could lead to nil pointer deferences.

…derivation

Roasbeef

LGTM 🎉

cfromknecht mentioned this pull request Aug 24, 2017

Bug in breach arbiter #256

Closed

cfromknecht force-pushed the sweep-2nd-layer-htlcs branch 4 times, most recently from f06fa4d to d80a41f Compare August 28, 2017 19:17

Roasbeef requested changes Aug 29, 2017

View reviewed changes

cfromknecht force-pushed the sweep-2nd-layer-htlcs branch 8 times, most recently from b4a163e to 2351449 Compare August 30, 2017 23:42

halseth reviewed Aug 31, 2017

View reviewed changes

cfromknecht force-pushed the sweep-2nd-layer-htlcs branch 5 times, most recently from 061f5b7 to 026ddd6 Compare September 13, 2017 11:11

Roasbeef requested changes Sep 19, 2017

View reviewed changes

cfromknecht force-pushed the sweep-2nd-layer-htlcs branch 3 times, most recently from 5fad2d9 to 4449a0e Compare September 20, 2017 00:32

cfromknecht changed the title ~~WIP: Breach Arbiter Sweep 2nd layer HTLCs~~ Breach Arbiter Sweep 2nd layer HTLCs Sep 20, 2017

cfromknecht force-pushed the sweep-2nd-layer-htlcs branch 2 times, most recently from a8f7ad5 to dd56302 Compare September 20, 2017 02:08

cfromknecht added 2 commits September 19, 2017 19:18

breacharbiter: unifies ba sweep txn creation

d888041

breacharbiter_test: modify test vectors to support new structs

9cc9977

cfromknecht added 13 commits September 19, 2017 19:18

lnwallet/witnessgen: remove use of pointer to Signer iface

da250cd

utxonursery: remove use of pointer to lnwallet.Signer iface

7f93aec

breacharbiter: use BOLT 5 constants

f0ac991

lnwallet/size: updated with recent BOLT 5 constants

b3a80f7

breacharbiter: sweep incoming + outgoing htlcs

a8d667b

This commit also adds a BreachConfig to abstract the instantiation of the breach arbiter, as well as various formatting improvements.

server: instantiate breacharbiter using BreachConfig

e32d302

breacharbiter_test: properly assigns SignDescriptor w/o ptr

7ee7bf8

lnwallet/witnessgen: adds htlc offered+accepted revoke witness type

84abc96

lnwallet/script_utils: adds public receiver+sender htlc revoke w/key …

de10632

…derivation

lnwallet/signdesc: increases max witness parsing length

4735649

ba+utxno: final pass for formatting

ec288dd

cfromknecht force-pushed the sweep-2nd-layer-htlcs branch from dd56302 to ec288dd Compare September 20, 2017 02:18

cfromknecht mentioned this pull request Sep 20, 2017

Breach Arbiter Ignore Dust Commitment Outputs #341

Merged

Roasbeef approved these changes Sep 21, 2017

View reviewed changes

Roasbeef merged commit 94b54f0 into lightningnetwork:master Sep 21, 2017

cfromknecht deleted the sweep-2nd-layer-htlcs branch December 19, 2019 11:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breach Arbiter Sweep 2nd layer HTLCs #264

Breach Arbiter Sweep 2nd layer HTLCs #264

cfromknecht commented Aug 24, 2017 •

edited

mention-bot commented Aug 24, 2017

Roasbeef left a comment

Roasbeef Aug 28, 2017

Roasbeef Aug 28, 2017

Roasbeef Aug 28, 2017

cfromknecht Sep 13, 2017

Roasbeef Aug 28, 2017

Roasbeef Aug 28, 2017

cfromknecht Aug 29, 2017

Roasbeef Aug 28, 2017

cfromknecht Aug 29, 2017

Roasbeef Aug 28, 2017

Roasbeef Aug 28, 2017

Roasbeef Aug 28, 2017

Roasbeef Aug 28, 2017

Roasbeef Aug 28, 2017

cfromknecht Aug 29, 2017

Roasbeef Aug 28, 2017

halseth Aug 31, 2017

halseth Aug 31, 2017

halseth Aug 31, 2017

halseth Aug 31, 2017

cfromknecht Aug 31, 2017

halseth Aug 31, 2017

Roasbeef Sep 19, 2017

Roasbeef Sep 19, 2017

Roasbeef Sep 19, 2017

Roasbeef left a comment

		}

		// TODO(conner): move responsibility of channel closure into

Breach Arbiter Sweep 2nd layer HTLCs #264

Breach Arbiter Sweep 2nd layer HTLCs #264

Conversation

cfromknecht commented Aug 24, 2017 • edited

mention-bot commented Aug 24, 2017

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

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

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

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

cfromknecht commented Aug 24, 2017 •

edited