Add commit_upfront_shutdown_pubkey to ChannelHandshakeConfig #1270

matchacactus · 2022-01-21T02:53:02Z

First step for #216

codecov-commenter · 2022-01-21T03:22:25Z

Codecov Report

Merging #1270 (1366c2c) into main (89cbb6d) will increase coverage by 0.07%.
The diff coverage is 92.64%.

❗ Current head 1366c2c differs from pull request most recent head b41c2fb. Consider uploading reports for the commit b41c2fb to get more accurate results

@@            Coverage Diff             @@
##             main    #1270      +/-   ##
==========================================
+ Coverage   90.41%   90.49%   +0.07%     
==========================================
  Files          69       71       +2     
  Lines       38002    39053    +1051     
==========================================
+ Hits        34360    35341     +981     
- Misses       3642     3712      +70

Impacted Files	Coverage Δ
lightning/src/lib.rs	`100.00% <ø> (ø)`
lightning/src/ln/features.rs	`98.28% <ø> (ø)`
lightning/src/ln/peer_handler.rs	`48.41% <0.00%> (-0.21%)`	⬇️
lightning/src/routing/router.rs	`91.79% <ø> (-0.26%)`	⬇️
lightning/src/util/config.rs	`46.80% <0.00%> (+1.15%)`	⬆️
lightning/src/util/test_utils.rs	`82.26% <83.33%> (+0.04%)`	⬆️
lightning-invoice/src/lib.rs	`87.42% <84.61%> (-0.08%)`	⬇️
lightning-background-processor/src/lib.rs	`93.04% <85.71%> (+0.06%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.23% <86.36%> (-0.06%)`	⬇️
lightning/src/ln/channel.rs	`89.23% <86.48%> (-0.15%)`	⬇️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89cbb6d...b41c2fb. Read the comment docs.

TheBlueMatt · 2022-01-21T16:45:15Z

lightning/src/util/config.rs

-	/// Default value: true.
-	pub commit_upfront_shutdown_pubkey: bool,
+	/// This value is moved to ChannelHandshakeConfig, optional here for old serialiization
+	pub commit_upfront_shutdown_pubkey: Option<bool>,


Lets make this pub(crate) and default to None? That way its really only used internally and we'll start defaulting to using the new one instead by seeing its None.

TheBlueMatt · 2022-01-21T16:49:36Z

lightning/src/ln/channel.rs

@@ -5460,6 +5461,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
 			// Read the 8 bytes of backwards-compatibility ChannelConfig data.
 			let mut _val: u64 = Readable::read(reader)?;
 		}
+
+		handshake_config.as_mut().unwrap().commit_upfront_shutdown_pubkey = Readable::read(reader)?;


We can't just add a new field to be read in the middle of the object - its not there in old serialized data! Instead, we should, I think, just store the commit_upfront_shutdown_pubkey field explicitly in a new boolean and load it explicitly.

TheBlueMatt · 2022-02-01T20:38:21Z

Any update on this @matchacactus? No particular rush, but I may make some changes in this area soon that would conflict.

matchacactus · 2022-02-02T01:45:29Z

Any update on this @matchacactus? No particular rush, but I may make some changes in this area soon that would conflict.

im struggling with testing the writer/reader and decided to understand rust a bit more - feel free to land changes, i can rebase.

TheBlueMatt · 2022-02-02T14:32:05Z

im struggling with testing the writer/reader and decided to understand rust a bit more - feel free to land changes, i can rebase.

Okay, no rush. We're also happy to answer questions, we have a #rust-101 room on slack/discord :)

TheBlueMatt

Yea, okay, I can't blame you for getting confused here, our serialization stuff is (a) relatively poorly documented (b) not made clear what the requirements are for developers anywhere. I pushed something I believe to probably be correct at https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=shortlog;h=refs/heads/2022-02-1270-ser-backwards-compat

TheBlueMatt · 2022-02-04T17:24:21Z

lightning/src/ln/channel.rs

@@ -5191,7 +5193,8 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 		self.config.forwarding_fee_proportional_millionths.write(writer)?;
 		self.config.cltv_expiry_delta.write(writer)?;
 		self.config.announced_channel.write(writer)?;
-		self.config.commit_upfront_shutdown_pubkey.write(writer)?;
+		// self.config.commit_upfront_shutdown_pubkey.write(writer)?;


Yep, this line should just be removed - we can't change the bytes written here for backwards/forwards compatibility - we currently write one byte to indicate the status of commit_upfront_shutdown_pubkey, old versions expect to read exactly one byte for it, and current versions need to read exactly one byte for it to read content written by old versions.

TheBlueMatt · 2022-02-04T17:26:09Z

lightning/src/ln/channel.rs

 		} else {
 			// Read the 8 bytes of backwards-compatibility ChannelConfig data.
 			let mut _val: u64 = Readable::read(reader)?;
 		}
-
-		let channel_id = Readable::read(reader)?;
+		let handshake_config = Some(ChannelHandshakeConfig::default());		let channel_id = Readable::read(reader)?;


Note here that you're creating a fresh handshake_config with default values, never reading any data to update it, then later setting commit_upfront_shutdown_pubkey with the value from the fresh object.

TheBlueMatt · 2022-02-04T17:41:09Z

lightning/src/ln/channel.rs

@@ -5455,13 +5458,12 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
 			config.as_mut().unwrap().forwarding_fee_proportional_millionths = Readable::read(reader)?;
 			config.as_mut().unwrap().cltv_expiry_delta = Readable::read(reader)?;
 			config.as_mut().unwrap().announced_channel = Readable::read(reader)?;
-			config.as_mut().unwrap().commit_upfront_shutdown_pubkey = Readable::read(reader)?;
+			// config.as_mut().unwrap().commit_upfront_shutdown_pubkey = Readable::read(reader)?;


Note that because the type of commit_upfront_shutdown_pubkey has changed from a bool (represented as a single byte) to an Option, we have to change how we read it here - otherwise we try to read a byte indicating if the value is Some or None and then try to read a second byte for the value.

matchacactus · 2022-02-05T20:36:02Z

Yea, okay, I can't blame you for getting confused here, our serialization stuff is (a) relatively poorly documented (b) not made clear what the requirements are for developers anywhere. I pushed something I believe to probably be correct at https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=shortlog;h=refs/heads/2022-02-1270-ser-backwards-compat

Thank you so much for taking a look. I guess I completely missed understanding howwrite_tlv_fields and read_tlv_fields works. I updated the commit, PTAL.

ariard

Overall sounds good direction.

ariard · 2022-02-07T02:01:35Z

lightning/src/util/config.rs

+	/// lightning payments, so we never require that our counterparties support this option.
+	///
+	/// This cannot be changed after a channel has been initialized.
+	///


You can add "The upfront key committed is provided from [KeysInterface::get_shutdown_pubkey]".

TheBlueMatt

One substantive comment, and two minor nitpicks. Can you rebase?

TheBlueMatt · 2022-02-07T21:39:15Z

lightning/src/ln/channel.rs

 		let mut secp_ctx = Secp256k1::new();
 		secp_ctx.seeded_randomize(&keys_source.get_secure_random_bytes());

 		Ok(Channel {
 			user_id,
-
+			


nit: please drop the excess whitespace here.

TheBlueMatt · 2022-02-07T21:39:55Z

lightning/src/ln/channel.rs

 		let mut preimages_opt: Option<Vec<Option<PaymentPreimage>>> = None;

 		// If we read an old Channel, for simplicity we just treat it as "we never sent an
 		// AnnouncementSignatures" which implies we'll re-send it on reconnect, but that's fine.
 		let mut announcement_sigs_state = Some(AnnouncementSigsState::NotSent);
-
+		let mut commit_upfront_shutdown_pubkey = None;
+


nit: looks like there's spare whitespace on this line.

TheBlueMatt · 2022-02-07T21:53:58Z

lightning/src/ln/channel.rs

@@ -5939,12 +5943,14 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
 		// only, so we default to that if none was written.
 		let mut channel_type = Some(ChannelTypeFeatures::only_static_remote_key());
 		let mut channel_creation_height = Some(serialized_height);
+


I can't leave a comment there, but on L 5717 we change the bytes read, which will break backwards compat.

config.as_mut().unwrap().commit_upfront_shutdown_pubkey = Readable::read(reader)?; is no longer reading a single bool, but instead an Option<bool> now, which is written as 2 bytes instead of 1. We should instead set it to Some(Readable::read(reader)?);

TheBlueMatt · 2022-02-12T22:32:47Z

lightning/src/util/config.rs

@@ -269,7 +274,7 @@ impl_writeable_tlv_based!(ChannelConfig, {
 	(2, cltv_expiry_delta, required),
 	(3, force_close_avoidance_max_fee_satoshis, (default_value, 1000)),
 	(4, announced_channel, required),
-	(6, commit_upfront_shutdown_pubkey, required),
+	(6, commit_upfront_shutdown_pubkey, option),


Actually, this presents a backwards-compatibility concern - one thing we explicitly support in LDK is for users to downgrade (at least a version or two) and still be able to open objects serialized with the newest version. Here, if a user did that, they'd fail to read the ChannelConfig because the value went from always present (and required) to defaulting to None. We can paper over this, though, by always defaulting to creating commit_upfront_shutdown_pubkey and setting it to Some(...) but then setting the "correct" value on the write side of channel to ensure old versions read the current/new value.

TheBlueMatt

Needs rebase, sorry about that.

TheBlueMatt · 2022-03-09T19:59:41Z

Are you still interested in working on this, @matchacactus?

matchacactus · 2022-03-12T02:34:03Z

Unfortunately I will not be able to work on this at this moment. Will pick something else up in the future! Sorry for the churn.

…

On Wed, Mar 9, 2022 at 12:00 PM Matt Corallo ***@***.***> wrote: Are you still interested in working on this, @matchacactus <https://github.com/matchacactus>? — Reply to this email directly, view it on GitHub <#1270 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACLAI7KD2MVPBFCZX337NWLU7D7MFANCNFSM5MOMLAUQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

TheBlueMatt · 2022-03-15T19:49:05Z

No problem! Gonna close this for now.

TheBlueMatt · 2022-06-13T11:10:06Z

Eventually landed rebased as #1529.

matchacactus mentioned this pull request Jan 21, 2022

Move forwarding_fee_proportional_millionths, forwarding_fee_base_msat to ChannelHandshakeConfig #1247

Closed

TheBlueMatt reviewed Jan 21, 2022

View reviewed changes

TheBlueMatt reviewed Feb 4, 2022

View reviewed changes

matchacactus force-pushed the handshake-1 branch 5 times, most recently from f1366d8 to 4a2cbf5 Compare February 5, 2022 20:30

matchacactus force-pushed the handshake-1 branch from 1070c6d to 0de25dd Compare February 5, 2022 20:44

ariard reviewed Feb 7, 2022

View reviewed changes

TheBlueMatt reviewed Feb 7, 2022

View reviewed changes

matchacactus force-pushed the handshake-1 branch 2 times, most recently from 1366c2c to bd3f694 Compare February 12, 2022 04:24

Add commit_upfront_shutdown_pubkey to ChannelHandshakeConfig

b41c2fb

matchacactus force-pushed the handshake-1 branch from bd3f694 to b41c2fb Compare February 12, 2022 04:28

TheBlueMatt reviewed Feb 12, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Mar 11, 2022

Implement the SCIDAlias Channel Type and provide SCID Privacy #1351

Merged

TheBlueMatt closed this Mar 15, 2022

TheBlueMatt mentioned this pull request Nov 21, 2023

Bump MSRV to rustc 1.63.0 #2681

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commit_upfront_shutdown_pubkey to ChannelHandshakeConfig #1270

Add commit_upfront_shutdown_pubkey to ChannelHandshakeConfig #1270

matchacactus commented Jan 21, 2022 •

edited

Loading

codecov-commenter commented Jan 21, 2022 •

edited

Loading

TheBlueMatt Jan 21, 2022

TheBlueMatt Jan 21, 2022

TheBlueMatt commented Feb 1, 2022

matchacactus commented Feb 2, 2022

TheBlueMatt commented Feb 2, 2022

TheBlueMatt left a comment

TheBlueMatt Feb 4, 2022

TheBlueMatt Feb 4, 2022

TheBlueMatt Feb 4, 2022

matchacactus commented Feb 5, 2022

ariard left a comment •

edited

Loading

ariard Feb 7, 2022

TheBlueMatt left a comment

TheBlueMatt Feb 7, 2022

TheBlueMatt Feb 7, 2022

TheBlueMatt Feb 7, 2022

TheBlueMatt Feb 12, 2022

TheBlueMatt left a comment

TheBlueMatt commented Mar 9, 2022

matchacactus commented Mar 12, 2022 via email

TheBlueMatt commented Mar 15, 2022

TheBlueMatt commented Jun 13, 2022

Add commit_upfront_shutdown_pubkey to ChannelHandshakeConfig #1270

Add commit_upfront_shutdown_pubkey to ChannelHandshakeConfig #1270

Conversation

matchacactus commented Jan 21, 2022 • edited Loading

codecov-commenter commented Jan 21, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 1, 2022

matchacactus commented Feb 2, 2022

TheBlueMatt commented Feb 2, 2022

TheBlueMatt 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

matchacactus commented Feb 5, 2022

ariard left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt 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

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 9, 2022

matchacactus commented Mar 12, 2022 via email

TheBlueMatt commented Mar 15, 2022

TheBlueMatt commented Jun 13, 2022

matchacactus commented Jan 21, 2022 •

edited

Loading

codecov-commenter commented Jan 21, 2022 •

edited

Loading

ariard left a comment •

edited

Loading