Fix backwards compat for blocked_monitor_updates and finally kill vec_type
#2400
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In 1ce2beb, `Channel::blocked_monitor_updates` was moved to an even TLV to ensure downgrades with vec entries are forbidden. However, the serialized type remained `vec_type`, which is always written. Instead, `optional_vec` must be used.
Historically, we used `vec_type` for all TLV Vec reads/writes, but it is asymmetric and thus somewhat confusing - on the write side it always writes a TLV entry, even if there are zero elements. On the read side, it happily accepts a missing TLV, providing a zero-length vector. In 85b573d a new `optional_vec` TLV format was added which was symmetric, but only supports optional vecs. This adds the corresponding required form, always writing a TLV and ensuring it is present.
This converts some required TLVs to `required_vec` which are, in fact, required (and have been written forever). * `HTLCFailReason` hasn't changed since many structs were converted to TLVs in 66784e3. * `NodeInfo::channels` has been written since `NetworkGraph` structs were converted to TLVs in 321b19c. * Several test-only TLV writes were converted.
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2400 +/- ##
========================================
Coverage 90.31% 90.31%
========================================
Files 106 106
Lines 54965 55166 +201
Branches 54965 55166 +201
========================================
+ Hits 49639 49822 +183
- Misses 5326 5344 +18
☔ View full report in Codecov by Sentry. |
alecchendev
reviewed
Jul 9, 2023
| (4, path.hops, vec_type), | ||
| (4, path.hops, required_vec), |
There was a problem hiding this comment.
Just curious, how did you find all the commits you referenced when checking if certain fields had been written as required for a while?
There was a problem hiding this comment.
git blame and some elbow grease :)
| @@ -6781,7 +6781,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for Channel<Signer> { | |||
| (5, self.context.config, required), | |||
| (6, serialized_holder_htlc_max_in_flight, option), | |||
| (7, self.context.shutdown_scriptpubkey, option), | |||
| (8, self.context.blocked_monitor_updates, vec_type), | |||
| (8, self.context.blocked_monitor_updates, optional_vec), | |||
There was a problem hiding this comment.
Oops :( Good thing we're getting rid of vec_type.
TheBlueMatt
force-pushed
the
2023-07-kill-vec_type
branch
from
6e0f0d6 to
28fa187
Compare
|
Added some further documentation on the TLV read/write types. |
Historically, we used `vec_type` for all TLV Vec reads/writes, but it is asymmetric and thus somewhat confusing - on the write side it always writes a TLV entry, even if there are zero elements. On the read side, it happily accepts a missing TLV, providing a zero-length vector. In 85b573d a new `optional_vec` TLV format was added which was symmetric, but only supports optional vecs. Now that we've migrated entirely to the new `required_vec` TLV type, we can entirely remove the awkward `vec_type`.
While we don't want to publicly document these and support them for downstream crates, documenting them internally is useful.
TheBlueMatt
force-pushed
the
2023-07-kill-vec_type
branch
from
28fa187 to
d83390c
Compare
|
Fixed a typo and squashed - $ git diff-tree -U1 28fa1872 d83390c6
diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs
index 302cb6c31..710085e2b 100644
--- a/lightning/src/util/ser_macros.rs
+++ b/lightning/src/util/ser_macros.rs
@@ -20,3 +20,3 @@
// Some of the other types include:
-// * (default_value, $default) - reads optionally, writing $default if no TLV is present
+// * (default_value, $default) - reads optionally, reading $default if no TLV is present
// * (static_value, $value) - ignores any TLVs, always using $value
$ |
wpaulino
approved these changes
Jul 11, 2023
valentinewallace
approved these changes
Jul 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In 1ce2beb,
Channel::blocked_monitor_updateswas moved to an even TLV toensure downgrades with vec entries are forbidden. However, the
serialized type remained
vec_type, which is always written.Instead,
optional_vecmust be used. This is fixed in the first commit.Historically, we used
vec_typefor all TLV Vec reads/writes, butit is asymmetric and thus somewhat confusing - on the write side it
always writes a TLV entry, even if there are zero elements. On the
read side, it happily accepts a missing TLV, providing a
zero-length vector.
In 85b573d a new
optional_vecTLV format was added which was symmetric, but only supports
optional vecs.
We then add a
required_vecTLV format, and move all the oldvec_typeformats to eitherrequired_vecor
optional_vec, removingvec_typeat the end to avoid these kinds of mistakes in the future.In practice only the first commit needs to land for 0.0.116, but if we can go ahead and land the whole thing that'd be cool too.