Migrate some inner structs to TLVs #928

TheBlueMatt · 2021-05-26T03:18:23Z

This adds more TLVs to various serialized structs to provide more forward/backward compatibility.

I need to update the routing graph that is used in benchmarks to support this. Based on #642.

ariard

Code Review ACK dcd8561

codecov · 2021-05-26T03:50:15Z

Codecov Report

Merging #928 (d0fc7a3) into main (8e7b590) will increase coverage by 1.25%.
The diff coverage is 87.61%.

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

@@            Coverage Diff             @@
##             main     #928      +/-   ##
==========================================
+ Coverage   90.46%   91.71%   +1.25%     
==========================================
  Files          60       60              
  Lines       30592    39136    +8544     
==========================================
+ Hits        27674    35894    +8220     
- Misses       2918     3242     +324

Impacted Files	Coverage Δ
lightning/src/ln/msgs.rs	`87.87% <0.00%> (-0.34%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.45% <89.74%> (+2.30%)`	⬆️
lightning/src/util/ser.rs	`90.67% <91.66%> (-2.51%)`	⬇️
lightning/src/chain/package.rs	`92.40% <100.00%> (+0.08%)`	⬆️
lightning/src/chain/transaction.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/chan_utils.rs	`97.98% <100.00%> (+0.64%)`	⬆️
lightning/src/routing/network_graph.rs	`94.83% <100.00%> (+2.94%)`	⬆️
lightning/src/routing/router.rs	`96.65% <100.00%> (+0.59%)`	⬆️
lightning/src/util/ser_macros.rs	`97.95% <100.00%> (+6.84%)`	⬆️
lightning/src/util/errors.rs	`64.51% <0.00%> (-6.92%)`	⬇️
... and 33 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 8e7b590...ad20bc7. Read the comment docs.

TheBlueMatt · 2021-05-26T17:08:12Z

Rebased after #642 merge and added a commit to migrate sub-fields of PackageTemplate to TLVs.

TheBlueMatt · 2021-05-27T01:15:04Z

Actually, I'm gonna add a ton more structs here.

TheBlueMatt · 2021-05-27T02:39:33Z

OK, fewer than I thought, but needed to tweak the macros, this should be good, let me know if I missed any.

TheBlueMatt · 2021-05-27T02:40:15Z

Tagging 0.0.15 as this is likely important for stabilizing serialization formats.

jkczyz · 2021-05-27T01:51:36Z

lightning/src/ln/chan_utils.rs

-	transaction_output_index
-});
+impl HTLCOutputInCommitment {
+	pub(crate) fn deserialization_dummy() -> Self {


For consistency elsewhere, could you name this null?

I ended up rewriting the macro stuff that required this so is now gone.

lightning/src/util/ser_macros.rs

jkczyz · 2021-05-27T02:03:56Z

lightning/src/chain/transaction.rs

@@ -73,6 +73,14 @@ impl OutPoint {
 			vout: self.index as u32,
 		}
 	}
+
+	/// Creates a dummy BitcoinOutPoint, useful for serializing into.


s/serializing/deserializing

jkczyz · 2021-05-27T02:42:33Z

lightning/src/chain/transaction.rs

+	pub(crate) fn null() -> Self {
+		Self {
+			txid: Default::default(),
+			index: 0
+		}
+	}


Should we consider simply implementing Default? I guess it exposes it outside the crate. Would be nice if traits could be implemented internally only.

Yea, so initially I went down a rabbit hole on creating a whole trait for this stuff but I eventually just gave up and created a wrapper that deserializes into Options. This one is left because its used outside of the impl_writeable_tlv_based macro, so its kinda a one-off.

If a one-off, consider inlining.

I found it to be a bit cleaner, but I'm happy to inline in channelmanager.rs if you prefer?

lightning/src/routing/network_graph.rs

jkczyz · 2021-05-27T03:03:51Z

lightning/src/chain/package.rs

-	amount
-});
+impl_writeable_tlv_based!(HolderHTLCOutput, {
+	(0, amount, 0),


I don't quite understand when a default value is needed.

It isn't anymore :).

jkczyz

Looks good other than some minor comments.

jkczyz · 2021-05-27T19:26:37Z

lightning/src/chain/transaction.rs

+	pub(crate) fn null() -> Self {
+		Self {
+			txid: Default::default(),
+			index: 0
+		}
+	}


If a one-off, consider inlining.

lightning/src/routing/network_graph.rs

ariard · 2021-05-27T21:40:21Z

Code Review ACK d0fc7a3

This also includes a `VecWriteWrapper` and `VecReadWrapper` which implements serialization for any `Readable`/`Writeable` type that is in a Vec. We do this instead of implementing `Readable`/`Writeable` directly as there isn't always a univerally-defined way to serialize a Vec and this makes things more explicit. Finally, this tweaks existing macros (and in the new macros) to support a trailing `,` after a list, eg `write_tlv_fields!(stream, {(0, a),}, {});` whereas previously the trailing `,` after the `(0, a)` would be a compile-error.

Note that enums are left alone as we can use the type byte already present for future compatibility.

TheBlueMatt · 2021-05-27T21:47:48Z

Squashed fixup commits and regenerated the graph for the new serialization:

$ git diff-tree -U0 d0fc7a3c ad20bc7f
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 30e10b192..61e961690 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -146,2 +146,2 @@ jobs:
-          path: lightning/net_graph-2021-05-26.bin
-          key: ldk-net_graph-2138c80ccaa5-2021-05-26.bin
+          path: lightning/net_graph-2021-05-27.bin
+          key: ldk-net_graph-45d86ead641d-2021-05-27.bin
@@ -151,2 +151,2 @@ jobs:
-          wget -O lightning/net_graph-2021-05-26.bin https://bitcoin.ninja/ldk-net_graph-2138c80ccaa5-2021-05-26.bin
-          if [ "$(sha256sum lightning/net_graph-2021-05-26.bin | awk '{ print $1 }')" != "71ca4fa8eb1561b6d544cd9f834292a948528e333bd3cb24f5f9a8ad1d1faad7" ]; then
+          wget -O lightning/net_graph-2021-05-27.bin https://bitcoin.ninja/ldk-net_graph-45d86ead641d-2021-05-27.bin
+          if [ "$(sha256sum lightning/net_graph-2021-05-27.bin | awk '{ print $1 }')" != "3d6261187cfa583255d978efb908b51c2f4dc4ad9a7160cd2c5263c9a4830121" ]; then
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index f6da5a73a..7688e70b3 100644
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -3860,2 +3860,2 @@ mod tests {
-		let res = File::open("net_graph-2021-05-26.bin") // By default we're run in RL/lightning
-			.or_else(|_| File::open("lightning/net_graph-2021-05-26.bin")) // We may be run manually in RL/
+		let res = File::open("net_graph-2021-05-27.bin") // By default we're run in RL/lightning
+			.or_else(|_| File::open("lightning/net_graph-2021-05-27.bin")) // We may be run manually in RL/
@@ -3870 +3870 @@ mod tests {
-				path.push("net_graph-2021-05-26.bin");
+				path.push("net_graph-2021-05-27.bin");
@@ -3893 +3893 @@ mod tests {
-				eprintln!("Please fetch https://bitcoin.ninja/ldk-net_graph-2138c80ccaa5-2021-05-26.bin and place it at lightning/net_graph-2021-05-26.bin");
+				eprintln!("Please fetch https://bitcoin.ninja/ldk-net_graph-45d86ead641d-2021-05-27.bin and place it at lightning/net_graph-2021-05-27.bin");
@@ -3920 +3920 @@ mod tests {
-				eprintln!("Please fetch https://bitcoin.ninja/ldk-net_graph-2138c80ccaa5-2021-05-26.bin and place it at lightning/net_graph-2021-05-26.bin");
+				eprintln!("Please fetch https://bitcoin.ninja/ldk-net_graph-45d86ead641d-2021-05-27.bin and place it at lightning/net_graph-2021-05-27.bin");
@@ -3958 +3958 @@ mod benches {
-			.expect("Please fetch https://bitcoin.ninja/ldk-net_graph-2138c80ccaa5-2021-05-26.bin and place it at lightning/net_graph-2021-05-26.bin");
+			.expect("Please fetch https://bitcoin.ninja/ldk-net_graph-45d86ead641d-2021-05-27.bin and place it at lightning/net_graph-2021-05-27.bin");
@@ -3990 +3990 @@ mod benches {
-			.expect("Please fetch https://bitcoin.ninja/ldk-net_graph-2138c80ccaa5-2021-05-26.bin and place it at lightning/net_graph-2021-05-26.bin");
+			.expect("Please fetch https://bitcoin.ninja/ldk-net_graph-45d86ead641d-2021-05-27.bin and place it at lightning/net_graph-2021-05-27.bin");
$

TheBlueMatt · 2021-05-27T22:06:32Z

Gonna merge post-CI, only diff was the URLs.

TheBlueMatt force-pushed the 2021-05-really-tlv-ser branch 2 times, most recently from 3de2cc5 to dcd8561 Compare May 26, 2021 03:28

ariard reviewed May 26, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-05-really-tlv-ser branch from dcd8561 to 3d2d3dc Compare May 26, 2021 17:07

TheBlueMatt force-pushed the 2021-05-really-tlv-ser branch 2 times, most recently from 91b18cc to 62495d0 Compare May 27, 2021 00:43

Fix compile warnings reading type-0 TLVs

c9f6a35

TheBlueMatt marked this pull request as draft May 27, 2021 01:15

TheBlueMatt force-pushed the 2021-05-really-tlv-ser branch from 62495d0 to 43ab6ee Compare May 27, 2021 02:39

TheBlueMatt marked this pull request as ready for review May 27, 2021 02:39

TheBlueMatt added this to the 0.0.15 milestone May 27, 2021

jkczyz reviewed May 27, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-05-really-tlv-ser branch from 43ab6ee to 78fdf48 Compare May 27, 2021 04:08

TheBlueMatt mentioned this pull request May 27, 2021

Broadcast transactions only after their timelock is up #932

Merged

TheBlueMatt force-pushed the 2021-05-really-tlv-ser branch from 78fdf48 to 2104d65 Compare May 27, 2021 19:21

jkczyz approved these changes May 27, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-05-really-tlv-ser branch from 2104d65 to d0fc7a3 Compare May 27, 2021 20:28

TheBlueMatt added 7 commits May 27, 2021 21:41

Add dummy OutPoint constructor to be used when reading it in a TLV

557aa81

Move ChannelManager-inner structs to TLV storage

9ec0f04

Note that enums are left alone as we can use the type byte already present for future compatibility.

Move NetworkGraph inner structs to TLV storage

321b19c

Add TLV suffix to PackageTemplate

a749422

Use TLV serialization for Commitment transaction structures

c8bc1b6

Migrate packages sub-fields to TLV serialization

45d86ea

Update net_graph used for benchmarks to use new ser format.

ad20bc7

TheBlueMatt force-pushed the 2021-05-really-tlv-ser branch from d0fc7a3 to ad20bc7 Compare May 27, 2021 21:46

TheBlueMatt merged commit df829a8 into lightningdevkit:main May 27, 2021

Migrate some inner structs to TLVs #928

Migrate some inner structs to TLVs #928

Uh oh!

Conversation

TheBlueMatt commented May 26, 2021

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt commented May 26, 2021

Uh oh!

TheBlueMatt commented May 27, 2021

Uh oh!

TheBlueMatt commented May 27, 2021

Uh oh!

TheBlueMatt commented May 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ariard commented May 27, 2021

Uh oh!

TheBlueMatt commented May 27, 2021

Uh oh!

TheBlueMatt commented May 27, 2021

Uh oh!

Uh oh!

codecov bot commented May 26, 2021 •

edited

Loading