option_data_loss_protect-on-channel_reestablishment #244

SWvheerden · 2018-11-07T13:43:39Z

No description provided.

SWvheerden · 2018-11-07T13:47:53Z

adresses #238

ariard · 2018-11-08T03:12:13Z

Hmm I'm a little bit skeptical of data_loss_protect has it's laid on other peer benevolence, I mean how do we manage "MUST NOT broadcast its commitment transaction and SHOULD fail the channel" ?
If other peer is malevolent it can force us to let channel endlessly open right ?

To answer to your IRC question, compressed pubkey are 33-bytes length.

SWvheerden · 2018-11-08T05:43:35Z

The spec is very vague on it. Best info I found was looking at the LND and Elements implementation's source code.

As far as I have it the idea behind it is that it will protect you from publishing old commitment transactions to the chain so you wont loose the funds in the channel. We should only not publish when we get a valid "your_last_per_commitment_secret" that is newer than the one we have.

As the size, yeah I realised it this morning.

SWvheerden · 2018-11-08T16:06:06Z

I looked at channel error needs to be changed probably as well for that return.

TheBlueMatt · 2018-11-09T03:19:26Z

A chunk of the reason for data_loss_protect is so that you at least have the per_commitment_point and can claim your to_remote_output when the remote side broadcasts the latest commitment transaction, which we need, but its unclear where things are going with the 1.1 spec, which we may end up only supporting anyway.

That's not to say we shouldn't support this - ensuring we can claim to_remote_output now is pretty important.

TheBlueMatt · 2018-11-14T00:14:06Z

So this also (obviously, as you point out) needs a mechanism to indicate "fail channel, but don't broadcast latest local tx" in ChannelError and ChannelManager needs to keep enough info around to broadcast local txn after some time has passed (assuming the remote doesn't broadcast after we send them an error message). Also, we need to be able to pass the new per_commitment_point to the ChannelMonitor so that we can reconstruct the to_remote_output private key if they broadcast their latest commitment transaction. Finally, needs to set the local_flags bit in our init message.

TheBlueMatt · 2018-11-14T00:59:58Z

Note that the bit about ChannelMonitor will largely go away with oprion_simplified_commitment, see https://github.com/lightningnetwork/lightning-rfc/wiki/Lightning-Specification-1.1-Proposal-States

…

On November 14, 2018 12:14:06 AM UTC, Matt Corallo ***@***.***> wrote: So this also (obviously, as you point out) needs a mechanism to indicate "fail channel, but don't broadcast latest local tx" in ChannelError and ChannelManager needs to keep enough info around to broadcast local txn after some time has passed (assuming the remote doesn't broadcast after we send them an error message). Also, we need to be able to pass the new per_commitment_point to the ChannelMonitor so that we can reconstruct the to_remote_output private key if they broadcast their latest commitment transaction. Finally, needs to set the local_flags bit in our init message. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #244 (comment)

SWvheerden · 2018-11-14T06:10:10Z

When is 1.1 going live?

TheBlueMatt · 2018-11-14T06:11:56Z

When everyone implements it :p. I presume it'll be soon enough that we can just rely on it as we're still a ways from being considered stable enough to put real funds in (and it means we have less fee crap to implement =D).

…

On November 14, 2018 6:10:10 AM UTC, SW van Heerden ***@***.***> wrote: When is 1.1 going live? -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #244 (comment)

TheBlueMatt · 2018-11-14T06:12:40Z

Note that 1.1 will be a series of feature bits, not a wholesale protocol rev.

…

On November 14, 2018 6:10:10 AM UTC, SW van Heerden ***@***.***> wrote: When is 1.1 going live? -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #244 (comment)

TheBlueMatt · 2019-01-14T22:56:02Z

src/ln/channelmanager.rs

@@ -386,7 +395,7 @@ macro_rules! handle_error {
 			Ok(msg) => Ok(msg),
 			Err(MsgHandleErrInternal { err, shutdown_finish }) => {
 				if let Some((shutdown_res, update_option)) = shutdown_finish {
-					$self.finish_force_close_channel(shutdown_res);
+					$self.finish_force_close_channel(shutdown_res,true);


Why not put the flag in ShutdownResult so you dont have to keep track of it everywhere?

I will push a version that is not like it. But that version will require a send a bool back from channel.force_shutdown() or I have to send it in like it was up onto now?

Another option is perhaps is till remove the new error type, and we only save a flag in the channel indicating whenever that error occurred or not.

src/ln/channelmonitor.rs

TheBlueMatt

Definitely needs tests, but looking good.

src/ln/channelmanager.rs

src/ln/channelmonitor.rs

TheBlueMatt · 2019-01-23T16:37:30Z

src/ln/functional_tests.rs

@@ -83,12 +83,12 @@ impl Drop for Node {
 	}
 }

-fn create_chan_between_nodes(node_a: &Node, node_b: &Node) -> (msgs::ChannelAnnouncement, msgs::ChannelUpdate, msgs::ChannelUpdate, [u8; 32], Transaction) {
-	create_chan_between_nodes_with_value(node_a, node_b, 100000, 10001)
+fn create_chan_between_nodes(node_a: &Node, node_b: &Node, local_features: &Option<msgs::LocalFeatures>) -> (msgs::ChannelAnnouncement, msgs::ChannelUpdate, msgs::ChannelUpdate, [u8; 32], Transaction) {


It may be easier diff/rebase-wise if you just create a new intermediary wrapper function and leave the existing functions as-is. Just create a create_chan_between_nodes_with_features and make the other functions call it.

You mean like rename the original, and let the original calls go towards towards a wrapper that puts in None?

Yea, that should reduce the diff and make rebase much easier.

I stopped doping this halfway, its going to make the code a bit of a mess according to me. We are going to add about 10 extra duplicate functions.

I would rather take on the complicated task of rebasing then doing that.

TheBlueMatt · 2019-03-07T16:49:20Z

Hey, sorry for the delay in taking this back up. This is looking pretty good, though I dont think we want to track our peer's feature flags in Channel - if the peer upgrades we may want to start using a different set of features (at least wrt data_loss_protect) than we started with. I think you should be able to just pass a &LocalFeatureFlags into the relevant handle_* messages.

TheBlueMatt · 2019-10-05T13:31:05Z

Closing since #349 got merged.

SWvheerden changed the title ~~{WIPInitial WIP push~~ [WIP] option_data_loss_protect-on-channel_reestablishment Nov 7, 2018

This was referenced Nov 7, 2018

Check Bolt 2 spec #240

Closed

BOLT 2 : SHOULD fail an HTLC which has timed out. #215

Closed

SWvheerden force-pushed the SW_option_data_loss_protect-on-channel_reestablishment- branch from dcd5d1c to 240384c Compare November 20, 2018 06:25

SWvheerden force-pushed the SW_option_data_loss_protect-on-channel_reestablishment- branch 2 times, most recently from c52644e to a468c35 Compare November 29, 2018 12:45

SWvheerden force-pushed the SW_option_data_loss_protect-on-channel_reestablishment- branch 4 times, most recently from 3f15958 to efee362 Compare January 9, 2019 13:18

TheBlueMatt reviewed Jan 14, 2019

View reviewed changes

SWvheerden changed the title ~~[WIP] option_data_loss_protect-on-channel_reestablishment~~ option_data_loss_protect-on-channel_reestablishment Jan 15, 2019

TheBlueMatt reviewed Jan 17, 2019

View reviewed changes

src/ln/channelmanager.rs Outdated Show resolved Hide resolved

src/ln/channelmanager.rs Outdated Show resolved Hide resolved

src/ln/channelmonitor.rs Outdated Show resolved Hide resolved

SWvheerden force-pushed the SW_option_data_loss_protect-on-channel_reestablishment- branch from e03a27b to 95d485f Compare January 18, 2019 05:47

Add data_loss_protect support

f4995cf

SWvheerden force-pushed the SW_option_data_loss_protect-on-channel_reestablishment- branch from 95d485f to f4995cf Compare January 18, 2019 05:49

add local_feature flags to tests

152d4c2

TheBlueMatt reviewed Jan 23, 2019

View reviewed changes

SWvheerden added 2 commits January 24, 2019 14:15

wip new test

4f79ebe

fix unit test

2c9c81f

SWvheerden force-pushed the SW_option_data_loss_protect-on-channel_reestablishment- branch from 482c941 to 2c9c81f Compare January 25, 2019 12:27

SWvheerden added 3 commits January 28, 2019 15:05

unit test two for testing claiming of peer after time

faf7909

change period

44f564c

unit test add block limit

b0b22d8

TheBlueMatt closed this Oct 5, 2019

option_data_loss_protect-on-channel_reestablishment #244

option_data_loss_protect-on-channel_reestablishment #244

Uh oh!

Conversation

SWvheerden commented Nov 7, 2018

Uh oh!

SWvheerden commented Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard commented Nov 8, 2018

Uh oh!

SWvheerden commented Nov 8, 2018

Uh oh!

SWvheerden commented Nov 8, 2018

Uh oh!

TheBlueMatt commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Nov 14, 2018

Uh oh!

TheBlueMatt commented Nov 14, 2018 via email

Uh oh!

SWvheerden commented Nov 14, 2018

Uh oh!

TheBlueMatt commented Nov 14, 2018 via email

Uh oh!

TheBlueMatt commented Nov 14, 2018 via email

Uh oh!

TheBlueMatt Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

SWvheerden Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

SWvheerden Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

SWvheerden Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

SWvheerden Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Mar 7, 2019

Uh oh!

TheBlueMatt commented Oct 5, 2019

Uh oh!

Uh oh!

SWvheerden commented Nov 7, 2018 •

edited

Loading

TheBlueMatt commented Nov 9, 2018 •

edited

Loading