Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multi: implement new safu commitment format #3365

Merged
merged 15 commits into from Sep 26, 2019

Conversation

@Roasbeef
Copy link
Member

commented Aug 1, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

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.

Copy link
Collaborator

left a comment

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 Show resolved Hide resolved
lnwallet/transactions_test.go Show resolved Hide resolved
breacharbiter_test.go Show resolved Hide resolved
lnwallet/channel_test.go Outdated Show resolved Hide resolved
lnwallet/channel.go Show resolved Hide resolved

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

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 1, 2019

Collaborator

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

Copy link
Collaborator

left a comment

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 Show resolved Hide resolved
lnwallet/channel.go Outdated Show resolved Hide resolved
@cfromknecht

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2019

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

input.CommitmentNoDelay,

@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

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.

@Roasbeef Roasbeef force-pushed the Roasbeef:safu-commitments branch from 0821d25 to e428333 Sep 11, 2019
@Roasbeef Roasbeef requested a review from wpaulino as a code owner Sep 11, 2019
@Roasbeef Roasbeef changed the title [WIP] multi: implement new safu commitment format multi: implement new safu commitment format Sep 11, 2019
@wpaulino wpaulino removed their request for review Sep 11, 2019
@Roasbeef Roasbeef force-pushed the Roasbeef:safu-commitments branch from e428333 to 853edca Sep 14, 2019
lnwire/features.go Outdated Show resolved Hide resolved
channeldb/channel.go Outdated Show resolved Hide resolved
@@ -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)

This comment has been minimized.

Copy link
@halseth

halseth Sep 16, 2019

Collaborator

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

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Sep 17, 2019

Author Member

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.

This comment has been minimized.

Copy link
@halseth

halseth Sep 19, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@halseth

halseth Sep 19, 2019

Collaborator

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

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 23, 2019

Collaborator

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

rpcserver.go Outdated Show resolved Hide resolved
input/script_utils.go Show resolved Hide resolved
@Roasbeef Roasbeef force-pushed the Roasbeef:safu-commitments branch 3 times, most recently from 27e9752 to d988acc Sep 17, 2019
fundingmanager.go Outdated Show resolved Hide resolved
fundingmanager.go Outdated Show resolved Hide resolved
lnpeer/peer.go Outdated Show resolved Hide resolved
lnpeer/peer.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef force-pushed the Roasbeef:safu-commitments branch from d988acc to f9e440d Sep 18, 2019
@Roasbeef Roasbeef requested a review from halseth Sep 23, 2019
@Roasbeef Roasbeef force-pushed the Roasbeef:safu-commitments branch 2 times, most recently from 12d896e to 4764184 Sep 24, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

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 Show resolved Hide resolved
breacharbiter.go Show resolved Hide resolved
htlcswitch/link.go Show resolved Hide resolved
lnwallet/channel.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef force-pushed the Roasbeef:safu-commitments branch from 4764184 to af51121 Sep 25, 2019
@Roasbeef Roasbeef requested a review from halseth Sep 25, 2019
lnwallet/channel.go Outdated Show resolved Hide resolved
lnwallet/channel.go Outdated Show resolved Hide resolved
@wpaulino wpaulino removed the incomplete label Sep 25, 2019
@wpaulino wpaulino added this to the 0.8.0 milestone Sep 25, 2019
@Roasbeef Roasbeef force-pushed the Roasbeef:safu-commitments branch from af51121 to 9fd80ab Sep 26, 2019
Roasbeef added 15 commits Aug 1, 2019
…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 Roasbeef force-pushed the Roasbeef:safu-commitments branch from 9fd80ab to c6ee42d Sep 26, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2019

Pushed up new version w/ slightly diff commitment structure.

@Roasbeef Roasbeef merged commit b1b4aab into lightningnetwork:master Sep 26, 2019
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.02%) to 63.226%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.