util: add default_value_vec for defaults without LengthReadable#4507
util: add default_value_vec for defaults without LengthReadable#4507carlaKC wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4507 +/- ##
==========================================
+ Coverage 86.18% 87.11% +0.93%
==========================================
Files 160 163 +3
Lines 107537 108765 +1228
Branches 107537 108765 +1228
==========================================
+ Hits 92681 94755 +2074
+ Misses 12229 11526 -703
+ Partials 2627 2484 -143
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lightning/src/util/ser_macros.rs
Outdated
| items: Vec<u32>, | ||
| } | ||
| impl_writeable_tlv_based!(DefaultValueVecStruct, { | ||
| (1, items, (default_value_vec, Vec::new())), |
There was a problem hiding this comment.
nit: Might make sense to use a dummy default value here rather than the empty Vec to check this is actually what's used rather than Vec::default() in general?
Hmm, I guess we could use I'm not a huge fan of yet more write variants, ideally we'd drop some, really, but I do see why this is useful, and its not much of an extension on what we already have, so if |
Yeah we can, it's just pretty ugly, worth having the option IMO. |
carlaKC
force-pushed
the
default-vec-macro
branch
from
bcde6b5 to
1a24f8f
Compare
Oof, yea, that's not great. Happy to see this move forward. |
|
I've performed a thorough re-review of this PR, tracing through all 7 macro dispatch points ( No issues found. Specifically verified:
|
lightning/src/util/ser_macros.rs
Outdated
| let mut $field = $crate::util::ser::RequiredWrapper(None); | ||
| }; | ||
| ($field: ident, (default_value_vec, $default: expr)) => { | ||
| let mut $field: Option<Vec<_>> = None; |
There was a problem hiding this comment.
If we use RequiredWrapper then we can re-use other patterns in all but one place.
diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs
index 52ee09505..751bf8422 100644
--- a/lightning/src/util/ser_macros.rs
+++ b/lightning/src/util/ser_macros.rs
@@ -308,13 +308,7 @@ macro_rules! _check_decoded_tlv_order {
}
}};
($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, (default_value_vec, $default: expr)) => {{
- // Note that $type may be 0 making the second comparison always false
- #[allow(unused_comparisons)]
- let invalid_order =
- ($last_seen_type.is_none() || $last_seen_type.unwrap() < $type) && $typ.0 > $type;
- if invalid_order {
- $field = Some($default);
- }
+ _check_decoded_tlv_order!($last_seen_type, $typ, $type, $field, (default_value, $default));
}};
($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, (static_value, $value: expr)) => {};
($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, required) => {{
@@ -388,12 +382,7 @@ macro_rules! _check_missing_tlv {
}
}};
($last_seen_type: expr, $type: expr, $field: ident, (default_value_vec, $default: expr)) => {{
- // Note that $type may be 0 making the second comparison always false
- #[allow(unused_comparisons)]
- let missing_req_type = $last_seen_type.is_none() || $last_seen_type.unwrap() < $type;
- if missing_req_type {
- $field = Some($default);
- }
+ _check_missing_tlv!($last_seen_type, $type, $field, (default_value, $default));
}};
($last_seen_type: expr, $type: expr, $field: expr, (static_value, $value: expr)) => {
$field = $value;
@@ -465,7 +454,7 @@ macro_rules! _decode_tlv {
}};
($outer_reader: expr, $reader: expr, $field: ident, (default_value_vec, $default: expr)) => {{
let f: $crate::util::ser::WithoutLength<Vec<_>> = $crate::util::ser::LengthReadable::read_from_fixed_length_buffer(&mut $reader)?;
- $field = Some(f.0);
+ $field = $crate::util::ser::RequiredWrapper(Some(f.0));
}};
($outer_reader: expr, $reader: expr, $field: ident, (static_value, $value: expr)) => {{
}};
@@ -882,7 +871,7 @@ macro_rules! _init_tlv_based_struct_field {
$field.0.unwrap()
};
($field: ident, (default_value_vec, $default: expr)) => {
- $field.unwrap()
+ $crate::_init_tlv_based_struct_field!($field, (default_value, $default))
};
($field: ident, (static_value, $value: expr)) => {
$field
@@ -936,7 +925,7 @@ macro_rules! _init_tlv_field_var {
let mut $field = $crate::util::ser::RequiredWrapper(None);
};
($field: ident, (default_value_vec, $default: expr)) => {
- let mut $field: Option<Vec<_>> = None;
+ $crate::_init_tlv_field_var!($field, (default_value, $default));
};
($field: ident, (static_value, $value: expr)) => {
let $field;
|
Put this on the 0.3 milestone as it would be if this makes it into the release and we can address the follow-up in LDK Node. |
|
Sorry for delay - was OOO last week - will get to this today! |
Right now, use of `default_value` requires that the struct implements `LengthReadable` itself. When trying to use `default_value` outside of LDK for `Vec<T>`, your code will run into the orphan rule because it does not own the trait `LengthReadable` or the type `Vec`. There are various ugly workarounds for this (like using `custom`), but wanting to persist a vec with a default value seems like a common enough use case to justify the change.
carlaKC
force-pushed
the
default-vec-macro
branch
from
1a24f8f to
5330f9f
Compare
|
Pushed suggested changes to use |
| // Old format: only the legacy type-0 field is present, falls back via default expression. | ||
| let old_encoded = <Vec<u8>>::from_hex("0600040000002a").unwrap(); // TLV len 6, type 0, len 4, value 42u32 |
There was a problem hiding this comment.
nit: can make another struct for this.
6 participants
In lightningdevkit/ldk-node#825, I ran into the orphan rule when trying to migrate from a single incoming/outgoing HTLC to a vec of
HTLCLocatortypes to allow expressing multiple in/out HTLCs. This issue stems from the following:HTLCLocatortype in LDK-Node, so that we can have niceties like a typeduser_channel_id.impl_writeable_tlv_based_enum!macro for LDK-Node'sEventtype.default_valueto fill the old fields when the new ones aren't present.However:
default_valueusesrequiredunder the hood, which expects the field to beLengthReadable.LengthReadableforVec<HTLCLocator>in LDK-Node due to the orphan rule (we don't ownVec<T>, even ifTis our type).impl_for_vec!to implement this.This change introduces a new
default_value_vecto our macro which usesrequired_vec(and thusWithoutLength) under the hood, so that we don't needLengthReadable. There are a few ugly workarounds (like usingcustom), but wanting to write aVecwith a default seems like a common enough one to justify a change to our macros.