Rebroadcast channel_announcements when we broadcast a node_announce #913

TheBlueMatt · 2021-05-06T01:47:55Z

Also take this chance to add a forward-compatible serialization step.

codecov · 2021-05-06T02:09:10Z

Codecov Report

Merging #913 (5bac875) into main (ec3739b) will decrease coverage by 0.00%.
The diff coverage is 85.87%.

❗ Current head 5bac875 differs from pull request most recent head eace819. Consider uploading reports for the commit eace819 to get more accurate results

@@            Coverage Diff             @@
##             main     #913      +/-   ##
==========================================
- Coverage   90.46%   90.46%   -0.01%     
==========================================
  Files          59       59              
  Lines       29896    30022     +126     
==========================================
+ Hits        27046    27159     +113     
- Misses       2850     2863      +13

Impacted Files	Coverage Δ
lightning/src/util/enforcing_trait_impls.rs	`90.38% <ø> (ø)`
lightning/src/routing/router.rs	`96.05% <50.00%> (-0.12%)`	⬇️
lightning/src/util/ser_macros.rs	`91.11% <62.06%> (-5.63%)`	⬇️
lightning/src/ln/channel.rs	`87.21% <81.81%> (-0.04%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`94.90% <88.88%> (-0.17%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.64% <95.83%> (+0.09%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.87% <97.43%> (+0.06%)`	⬆️
lightning/src/chain/channelmonitor.rs	`95.52% <100.00%> (+0.06%)`	⬆️
lightning/src/chain/keysinterface.rs	`94.96% <100.00%> (+0.04%)`	⬆️
lightning/src/ln/msgs.rs	`88.20% <100.00%> (-0.02%)`	⬇️
... and 7 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 c7e198e...eace819. Read the comment docs.

valentinewallace

Looks pretty reasonable to me. Still somewhat reasoning through how upgrading and serialization are going to work.

I see you just pushed more commits so will review those later!

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/chain/keysinterface.rs

jkczyz · 2021-05-10T16:53:00Z

lightning/src/chain/keysinterface.rs

+		writer.write_all(&[SERIALIZATION_VERSION; 1])?;
+		writer.write_all(&[MIN_SERIALIZATION_VERSION; 1])?;


Given this will be repeated across each implementation, would it make sense to make these part of the trait with a provided method for writing these? Similarly for Readable. Or is this only needed for "top-level" structs?

Thinking mainly about consolidating documentation around forward and backwards compatibility.

That's a conversation probably worth having in a meeting/on slack - I'm certainly open to moving it into the serialization traits, presumably then writing versions into ~every non-p2p struct. Alternatively we could have some helper that handles it for us, ie SerializationVersionTag{}.write(writer)??

jkczyz · 2021-05-10T16:58:08Z

lightning/src/chain/keysinterface.rs

 		Ok(())
 	}
 }

 impl Readable for InMemorySigner {
 	fn read<R: ::std::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+		let _ver: u8 = Readable::read(reader)?;


In what scenario would implementor need to examine the read version? Reading optional fields?

We'd need it to read old versions - eg "oh, we added a new field or gave a new field some different meaning, we need to check which version object was written so we can interpret it properly".

jkczyz · 2021-05-10T17:11:45Z

lightning/src/util/ser_macros.rs

@@ -8,21 +8,54 @@
 // licenses.

 macro_rules! encode_tlv {
-	($stream: expr, {$(($type: expr, $field: expr)),*}) => { {
+	($stream: expr, {$(($type: expr, $field: expr)),*}, {$(($optional_type: expr, $optional_field: expr)),*}) => { {


Not sure I fully understand the need for this. Aren't optional types defined by having an odd type number?

In this case optional_type means "thing which may or may not exist at compile time, and is actually stored in an Option at runtime, only optionally writing it.

TheBlueMatt · 2021-05-12T20:27:28Z

I macro-ized the new code, which allows for a good place to place documentation about the various versioning schemes in play, and also unifies the code to handle it somewhat.

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channel.rs

TheBlueMatt · 2021-05-20T20:12:39Z

Rebased on latest usptream, only changes were the comments Val left above.

lightning/src/util/ser_macros.rs

lightning/src/ln/channel.rs

valentinewallace · 2021-05-20T21:26:36Z

lightning/src/ln/channelmanager.rs

@@ -4431,8 +4424,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable f
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
 		let _consistency_lock = self.total_consistency_lock.write().unwrap();

-		writer.write_all(&[SERIALIZATION_VERSION; 1])?;
-		writer.write_all(&[MIN_SERIALIZATION_VERSION; 1])?;
+		write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);


Some other structs I'm not sure if it'd be worth adding versioning to, while we're at it:
Route
NetworkGraph
EnforcingSigner
Features
?

Route and NetworkGraph: yea, good catch, added.
EnforcingSigner is currently test-only (and may very well never be exposed as lightning-signer basically does what we'd want it to be), but I added a comment about it,
Features is a on-the-wire BOLT struct so we can't differ from the prescribed serialization format there.

Question: so for keysend, I think I'm going to be adding/modifying fields in PendingHTLCInfo . Would this mean that versioning needs to be added to this struct (I believe this field is persisted in Channel so it could have potential compatibility issues)? Or does it mean I'll need to bump the serialization version in Channel? I guess I'm not sure of the exact procedure for changing struct fields going forward.

Right, so for enums its easy - we usually have 0-N already defined and we can just add N+1... For something like PendingHTLCInfo we could maybe pass a version in to the read method, which would imply defining a new VersionedReadable trait. Of course that isn't exactly ideal because it means that even people who don't use keysend can't read new ChannelManagers with an old LDK version. If we want to be really broad about supporting old versions reading LDK structs from newer versions that don't use optional features, we should probably move the entire serialization of everything into TLVs (but in a separate PR).

lightning/src/ln/channel.rs

valentinewallace

Can't see any issues really, but I'd like to take a fresher look on Monday if that works

valentinewallace · 2021-05-20T23:22:24Z

lightning/src/ln/channelmanager.rs

+					msg,
+					update_msg: match self.get_channel_update(chan) {
+						Ok(msg) => msg,
+						Err(_) => continue,


Hm will this effectively continue if the channel isn't fully open yet? Could maybe use a comment

This should only hit if the funding transaction hasn't been confirmed at all (ie doesn't have a short channel ID yet), in which case we shouldn't have gotten a result from get_signed_channel_announcement, either.

in which case we shouldn't have gotten a result from get_signed_channel_announcement, either.

If that's the case, could this be an assertion / panic?

I figured this is the correct behavior if the logic in get_channel_update ever changes (or I'm wrong about the logic now) - its not a case where we're at risk of losing funds or any serious issues, so best not to panic.

lightning/src/ln/channel.rs

TheBlueMatt · 2021-05-24T17:52:33Z

Squashed without changes + rebased, which should fix the check_commits CI test.

jkczyz

Took a pass through each commit. Largely looks good.

lightning/src/util/ser_macros.rs

lightning/src/ln/channel.rs

CONTRIBUTING.md

lightning/src/ln/channelmanager.rs

jkczyz · 2021-05-24T20:04:09Z

lightning/src/ln/channelmanager.rs

+					msg,
+					update_msg: match self.get_channel_update(chan) {
+						Ok(msg) => msg,
+						Err(_) => continue,


in which case we shouldn't have gotten a result from get_signed_channel_announcement, either.

If that's the case, could this be an assertion / panic?

Currently our serialization is very compact, and contains version numbers to indicate which versions the code can read a given serialized struct. However, if you want to add a new field without needlessly breaking the ability of previous versions of the code to read the struct, there is not a good way to do so. This adds dummy, currently empty, TLVs to the major structs we serialize out for users, providing an easy place to put new optional fields without breaking previous versions.

Previously we handled most of the logic of announcement_signatures in ChannelManager, rather than Channel. This is somewhat unique as far as our message processing goes, but it also avoided having to pass the node_secret in to the Channel. Eventually, we'll move the node_secret behind the signer anyway, so there isn't much reason for this, and storing the announcement_signatures-provided signatures in the Channel allows us to recreate the channel_announcement later for rebroadcast, which may be useful.

valentinewallace

One question about changing struct fields going forward, but this LGTM 👍

valentinewallace · 2021-05-25T17:26:23Z

lightning/src/ln/channelmanager.rs

@@ -4431,8 +4424,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable f
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
 		let _consistency_lock = self.total_consistency_lock.write().unwrap();

-		writer.write_all(&[SERIALIZATION_VERSION; 1])?;
-		writer.write_all(&[MIN_SERIALIZATION_VERSION; 1])?;
+		write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);


Question: so for keysend, I think I'm going to be adding/modifying fields in PendingHTLCInfo . Would this mean that versioning needs to be added to this struct (I believe this field is persisted in Channel so it could have potential compatibility issues)? Or does it mean I'll need to bump the serialization version in Channel? I guess I'm not sure of the exact procedure for changing struct fields going forward.

TheBlueMatt · 2021-05-25T19:37:55Z

Agreement on Slack indicated (a) merge this, (b) do a followup which TLV's most internal structs.

TheBlueMatt force-pushed the 2021-05-tlv-ser branch 2 times, most recently from 0ab8ac2 to f2fb0f8 Compare May 7, 2021 22:38

valentinewallace reviewed May 7, 2021

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

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

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

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

jkczyz reviewed May 10, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-05-tlv-ser branch from ce3b66b to 2bdeae3 Compare May 12, 2021 20:27

TheBlueMatt force-pushed the 2021-05-tlv-ser branch from 2bdeae3 to 78f5be7 Compare May 12, 2021 20:29

valentinewallace reviewed May 20, 2021

View reviewed changes

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

lightning/src/ln/channel.rs Show resolved Hide resolved

TheBlueMatt added this to the 0.0.15 milestone May 20, 2021

TheBlueMatt force-pushed the 2021-05-tlv-ser branch from 78f5be7 to c3a4e54 Compare May 20, 2021 20:12

valentinewallace reviewed May 20, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-05-tlv-ser branch 2 times, most recently from e0e0246 to 9735461 Compare May 21, 2021 16:58

valentinewallace reviewed May 21, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-05-tlv-ser branch 3 times, most recently from 5bac875 to eace819 Compare May 24, 2021 17:52

jkczyz reviewed May 24, 2021

View reviewed changes

TheBlueMatt added 9 commits May 24, 2021 22:05

f clean up docs as jeff suggested

729bf76

Support serializing TLV fields which may or may not be present

6397919

Add version and TLV suffix for more user-facing "major" structs

ea1924d

Update net graph copy used in tests and bench for new format

30c1a7c

f clean up err wording?

aabc3a1

Rebroadcast channel_announcements when we broadcast a node_announce

635a9b2

f fixup docs

a987385

TheBlueMatt added 2 commits May 24, 2021 22:25

Add note in CONTRIBUTING.md that some structs need prefix/suffixes

9664be9

f clean up contributing wording

3c7bee3

TheBlueMatt force-pushed the 2021-05-tlv-ser branch from eace819 to 3c7bee3 Compare May 24, 2021 22:25

jkczyz approved these changes May 25, 2021

View reviewed changes

valentinewallace approved these changes May 25, 2021

View reviewed changes

TheBlueMatt merged commit 55538db into lightningdevkit:main May 25, 2021

		writer.write_all(&[SERIALIZATION_VERSION; 1])?;
		writer.write_all(&[MIN_SERIALIZATION_VERSION; 1])?;

Rebroadcast channel_announcements when we broadcast a node_announce #913

Rebroadcast channel_announcements when we broadcast a node_announce #913

Uh oh!

Conversation

TheBlueMatt commented May 6, 2021

Uh oh!

codecov bot commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented May 12, 2021

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented May 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented May 24, 2021

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

codecov bot commented May 6, 2021 •

edited

Loading

valentinewallace May 20, 2021 •

edited

Loading