multi: implement new safu commitment format #3365

Roasbeef · 2019-08-01T03:31:40Z

In this PR, we implement the current working draft of the new safu commitment format (has no tweak for the remote party's output in your commitment transaction). In short, once we roll this out, once a channel is closed by the remote party, the funds will go directly into an output that you control, so there's no need for any additional action. This essentially replaces the existing DLP protocol (we still need to understand it for old nodes though), as we no longer need the remote party's last unrevoked commitment point.

This is still labeled as WIP, as the current spec draft is missing some necessary signalling cut outs. Beyond that, I still need to update the integration tests to cover the new format (and still the old), and possibly a debug build flag guarded RPC to force the old format.

On the topic of how this affects our current SCB scheme, the short answer is that it only kinda does. With the change in this PR, the existing SCB files in the wild can be used with new channels, with the caveat that we'll still sweep the funds into our wallet, although we don't need to. In order to cut down on this extra transaction, we can instead add the commitment key (as an address) into a sort of look aside while we undergo our regular scan for keys on chain during the recovery process. I say "look aside" as we don't generate keys from the key family we use to generate commitment keys in the normal process, and it isn't expected that they'll be used in order. Therefore, we'll provide these addresses as hints, distinct from the normal look ahead process. This allows us to pick up these funds during the normal recovery process.

At a later time, we can then update the regular on-chain recovery process to also scan for these commitment keys as normal. However, it's likely that it won't locate all the funds since we can't really expect them to be used in order (you don't FIFO close your channels, it's more arbitrary).

NOTE: the PR as a whole mostly calls this new format "tweakless" commitments, but upon reflection I find that it isn't very descriptive since it requires you to know low level details w.r.t how the scripts are constructed. IMO "safu commitments" is gets the point across a bit better, and it's more meme-y to boot, which may promote users to update sooner to this new version: "dude, you're not using safu commitments?! wot??".

Roasbeef · 2019-08-01T03:43:10Z

Also the commits were crafted knowing that some of them don't build in isolation, as otherwise I would've had to commit the bulk of the PR in a single commit. IMO this is undesirable, as there're portions that need to be reviewed in isolation, and having them be distinct commits creates a guide for the reviewer.

Also the bulk of this diff is updating the existing tests, the final diff will also do a similar update for our set of integration tests.

joostjager

Not so easy to review for me, as I am not very familiar with the funding flow and dlp. Just some low level comments, but the changes look straight forward.

One thing I was wondering: what would be the consequences if we'd not negotiate the new format in the open channel flow, but add the commitment format to commitment_signed? so that existing channels can change over to the new format when they signal the global feature bit. For the next next version of the commitment txes (with the anchor point?), it could upgrade in the same way.

input/script_utils.go

lnwallet/transactions_test.go

breacharbiter_test.go

lnwallet/channel_test.go

lnwallet/channel.go

joostjager · 2019-08-01T09:13:29Z

lnwallet/channel.go

+
+		// If this is a tweakless commitment, then we can safely blank
+		// out the SingleTweak value as it isn't needed.
+		if tweaklessCommit {


I think not setting it above and only setting the tweak in case of !tweaklessCommit is clearer. Then the legacy flow is the exception.

cfromknecht

nice work @Roasbeef! diff is pretty straight forward and contained. only other area i can think of that needs changing is in the wtclient where we sign the justice transactions. no changes are needed on the tower side, since it doesn't care whether the input was tweaked or not, only that the signature is valid.

lnwallet/channel.go

cfromknecht · 2019-08-02T01:42:56Z

we may also need a way of signaling to the client whether the channel is tweakless or not, since we assume that it's tweaked here

lnd/watchtower/wtclient/backup_task.go

Line 90 in a533232

input.CommitmentNoDelay,

Roasbeef · 2019-08-08T03:50:26Z

Responded to the initial set of comments, and added commits to update the brar logic, as well as the tower logic. Still need to update the itests (I have a format I used in #3362 that I think will work well), and then also add a new funding flag to allow negotiation at funding flow time.

lnwire/features.go

channeldb/channel.go

halseth · 2019-09-16T09:13:22Z

lnwallet/channel_test.go

@@ -3593,7 +3605,7 @@ func TestChanSyncFailure(t *testing.T) {
 	// Create a test channel which will be used for the duration of this
 	// unittest. The channel will be funded evenly with Alice having 5 BTC,
 	// and Bob having 5 BTC.
-	aliceChannel, bobChannel, cleanUp, err := CreateTestChannels()
+	aliceChannel, bobChannel, cleanUp, err := CreateTestChannels(false)


What's the rationale behind doing some tweakless here, some not?

I only do tweakless where it would materially affect the test. Alternatively, everything can just be wrapped into one giant for loop and always test both behavior. I'm not sure if this is worth it though since we test the basic channel open an update functionality for both, while many tests just repeat that logic then asserts some db transitions that are unrelated to the commitment format.

Interesting. It would be useful to do it at least once do, to make sure it all passes.

I'm not opposed to wrapping them all in a for loop, it seems like free test coverage of the tweakless channels without much effort.

Would also let use smoothly transition to remove the tweaked format later, as all tests would be running for the new format.

When we get commit format no. 3, it may also be easier to have it all in a for loop.

rpcserver.go

input/script_utils.go

fundingmanager.go

lncfg/protocol_legacy_on.go

lnpeer/peer.go

joostjager · 2019-09-24T09:17:41Z

and ensuring every commit builds and ideally also passes tests

This can be difficult as a simple change like adding an extra arg to some spending utils, or updating the funding flow for awareness touch so many related sub-systems, that the entire PR would end up just being a handful of much larger commits. There's a tradeoff between build-ability and test properly executing (if the OP desires that) and commit size.

I don't think that it requires much to get the stack of commits in this PR building and I also don't think you need to end up with fewer commits. I looked at a few failing ones and it is mainly just a missing parameter. Having all the commits passing build allows to do git bisect run. I quite often use this to efficiently reduce the change set in which to look for a problem. If you'd reorganize the commits so that opening and accepting staticremotekey channels is added in one of the last, every commit would even still be a fully functioning version on its own. First all the preparatory steps are taken to commit by commit support the new tx format in all sub systems and finally it is enabled.

As for the next planned update, we split things off so we could ship things more quickly to cover our users, ideally in this release. As far as future ground work, we have the current lncfg based build flags for testing infrastructure. We'll also add a new channel type (which already exists), which similar to this PR will be checked to ensure we construct the commitment properly. Rather than modify an existing output, the new change will add new outputs, so much of the code will be touching weight estimates, and commitment transaction construction/verification. This also won't affect breach logic, DLP logic, or tower logic like this PR did.

I had this idea where there would be an object that contains everything that is specific to a particular commitment tx type. This object would have an interface through which it is used by the rest of the system. Adding new commitment formats would require the creation of a new type that implements this interface.

lnwire/features.go

breacharbiter.go

htlcswitch/link.go

lnwallet/channel.go

…e outputs

…d new witness type In this commit, we update the `CommitSpendNoDelay` method to be aware of the alternate spending mechanism for commitments that don't have a tweak for the remote party's non-delay output. We also add a new witness type so callers can convey their expected signing path.

In this commit, we define a new channel type: SingleFunderTweakless. We'll use this channel type to denote channels with commitments that don't tweak the remote party's key in their non-delay output.

In this commit, we update the channel state machine to be aware of tweakless commits. In several areas, we'll now check the channel's type to see if it's `SingleFunderTweakless`. If so, then we'll opt to use the remote party's non-delay based point directly in the script, skipping any additional cryptographic operations. Along the way we move the `validateCommitmentSanity` method to be defined _before_ it's used as is cutomary within the codebase. Notably, within the `NewUnilateralCloseSummary` method, we'll now _blank out_ the `SingleTweak` value if the commitment is tweakless. This indicates to callers the witness type they should map to, as the value isn't needed at all any longer when sweeping a non-delay output. We also update the signing+verification tests to also test that we're able to properly generate a valid witness for the new tweakless commitment format.

In this commit, we update the funding workflow to be aware of the new channel type that doesn't tweak the remote party's output within the non-delay script on their commitment transaction. To do this, we now allow the caller of `InnitChannelReservation` to signal if they want the old or new (tweakless) commitment style. The funding tests are also updated to test both funding variants, as we'll still need to understand the legacy format for older nodes.

…s commits In this commit, we update the `commitSweepResolver` to be aware of tweakless commitments. We'll now use the new behavior of the uni close summary (leaving out the single tweak) to detect if we're dealing with a new, or modern commitment. Depending on the commitment type, we'll then set the witness type accordingly so we can generate the proper signature within the sweeper.

…ote close In this commit, we update the logic in the `chainWatcher` to no longer wait until the DLP point has been populated in the database before we dispatch the force close summary to any registered clients. Instead, we can sweep immediately, as we have all the information we need to sweep the funds (just our key).

…mmitments In this commit, we update the tower+link logic to tag a commitment as the new (tweakless) format if it applies. In order to do this, the BackupTask method has gained an additional parameter to indicate the type of commitment that we're attempting to upload. This new tweakless bool is then threaded through all the way to back up task creation to ensure that we make the proper input.Input. Finally, we've added a new test case for each existing test case to test each case w/ and w/o the tweakless modifier.

…ment format In this commit, we update the brar logic in the channel state machine, and also the brar itself to be aware of the new commitment format. Similar to the unilateral close summary, we'll now blank out the SingleTweak field in `NewBreachRetribution` if it's a tweakless commitment. The brar will then use this to properly identify the commitment type, to ensure we use the proper witness generation function when we're handling our own breach.

…er interface

…feature bits In this commit, we add a new legacy protocol command line flag: `committweak`. When set, this forces the node to NOT signal usage of the new commitment format. This allows us to test that we're able to properly establish channels with legacy nodes. Within the server, we'll now gate our signalling of this new feature based on the legacy protocol config. Finally, when accepting/initiating a new channel funding, we'll now check both the local and remote global feature bits, only using the new commitment format if both signal the global feature bit.

…ment cases

…hain watcher In this commit, we consolidate the number of areas where we derive our commitment keys. Before this commit, the `isOurCommitment` function in the chain watcher used a custom routine to derive the expected scripts/keys for our commitment at that height. With the recent changes, we now have additional logic in `DeriveCommitmentKeys` that wasn't copied over to this area. As a result, the prior logic would erroneously detect if it was our commitment that had hit the chain or not. In this commit, we remove the old custom code, and use `DeriveCommitmentKeys` wihtin the chain watcher as well. This ensures that we only need to maintain the key derivation code in a single place, preventing future bugs of this nature.

In this commit, we create a new Single version for channels that use the tweakless commitment scheme. When recovering from an SCB into an open channel shell, we'll now check this field and use it to determine the proper channel type. Otherwise, we may attempt to sweep the on chain funds using the commitment point, when it goes directly to our key, or the other way around.

Roasbeef · 2019-09-26T01:27:32Z

Pushed up new version w/ slightly diff commitment structure.

Roasbeef requested review from cfromknecht, halseth and joostjager as code owners August 1, 2019 03:31

joostjager reviewed Aug 1, 2019

View reviewed changes

cfromknecht suggested changes Aug 2, 2019

View reviewed changes

lnwallet/channel.go Show resolved Hide resolved

lnwallet/channel.go Outdated Show resolved Hide resolved

Roasbeef force-pushed the safu-commitments branch from 0821d25 to e428333 Compare September 11, 2019 12:50

Roasbeef requested a review from wpaulino as a code owner September 11, 2019 12:50

Roasbeef changed the title ~~[WIP] multi: implement new safu commitment format~~ multi: implement new safu commitment format Sep 11, 2019

wpaulino removed their request for review September 11, 2019 12:56

Roasbeef force-pushed the safu-commitments branch from e428333 to 853edca Compare September 14, 2019 13:10

halseth reviewed Sep 16, 2019

View reviewed changes

Roasbeef force-pushed the safu-commitments branch 3 times, most recently from 27e9752 to d988acc Compare September 17, 2019 03:52