From 5d0ee867ea6c9670ebfe6004614e9ebaa7502c64 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sun, 19 Feb 2023 16:52:22 -0500 Subject: [PATCH] Fix upgradable_required fields to actually be required in lower level macros When using lower level macros such as read_tlv_stream, upgradable_required fields have been treated as regular options. This is incorrect, they should either be upgradable_options or treated as required fields. --- lightning/src/chain/channelmonitor.rs | 10 ++--- lightning/src/chain/onchaintx.rs | 10 ++--- lightning/src/routing/gossip.rs | 6 +-- lightning/src/util/events.rs | 31 ++++++--------- lightning/src/util/ser.rs | 12 ++++++ lightning/src/util/ser_macros.rs | 54 ++++++++++++++++++++++++--- 6 files changed, 81 insertions(+), 42 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 44e1d8c5..045ed10b 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -49,7 +49,7 @@ use crate::chain::onchaintx::OnchainTxHandler; use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageSolvingData, PackageTemplate, RevokedOutput, RevokedHTLCOutput}; use crate::chain::Filter; use crate::util::logger::Logger; -use crate::util::ser::{Readable, ReadableArgs, RequiredWrapper, MaybeReadable, Writer, Writeable, U48}; +use crate::util::ser::{Readable, ReadableArgs, RequiredWrapper, MaybeReadable, UpgradableRequired, Writer, Writeable, U48}; use crate::util::byte_utils; use crate::util::events::Event; #[cfg(anchors)] @@ -454,7 +454,7 @@ impl MaybeReadable for OnchainEventEntry { let mut transaction = None; let mut block_hash = None; let mut height = 0; - let mut event = None; + let mut event = UpgradableRequired(None); read_tlv_fields!(reader, { (0, txid, required), (1, transaction, option), @@ -462,11 +462,7 @@ impl MaybeReadable for OnchainEventEntry { (3, block_hash, option), (4, event, upgradable_required), }); - if let Some(ev) = event { - Ok(Some(Self { txid, transaction, height, block_hash, event: ev })) - } else { - Ok(None) - } + Ok(Some(Self { txid, transaction, height, block_hash, event: _init_tlv_based_struct_field!(event, upgradable_required) })) } } diff --git a/lightning/src/chain/onchaintx.rs b/lightning/src/chain/onchaintx.rs index 23140068..d3ca02ca 100644 --- a/lightning/src/chain/onchaintx.rs +++ b/lightning/src/chain/onchaintx.rs @@ -36,7 +36,7 @@ use crate::chain::keysinterface::WriteableEcdsaChannelSigner; use crate::chain::package::PackageSolvingData; use crate::chain::package::PackageTemplate; use crate::util::logger::Logger; -use crate::util::ser::{Readable, ReadableArgs, MaybeReadable, Writer, Writeable, VecWriter}; +use crate::util::ser::{Readable, ReadableArgs, MaybeReadable, UpgradableRequired, Writer, Writeable, VecWriter}; use crate::io; use crate::prelude::*; @@ -106,18 +106,14 @@ impl MaybeReadable for OnchainEventEntry { let mut txid = Txid::all_zeros(); let mut height = 0; let mut block_hash = None; - let mut event = None; + let mut event = UpgradableRequired(None); read_tlv_fields!(reader, { (0, txid, required), (1, block_hash, option), (2, height, required), (4, event, upgradable_required), }); - if let Some(ev) = event { - Ok(Some(Self { txid, height, block_hash, event: ev })) - } else { - Ok(None) - } + Ok(Some(Self { txid, height, block_hash, event: _init_tlv_based_struct_field!(event, upgradable_required) })) } } diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 957be295..167f79fb 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -883,9 +883,9 @@ impl Readable for ChannelInfo { (0, features, required), (1, announcement_received_time, (default_value, 0)), (2, node_one, required), - (4, one_to_two_wrap, upgradable_required), + (4, one_to_two_wrap, upgradable_option), (6, node_two, required), - (8, two_to_one_wrap, upgradable_required), + (8, two_to_one_wrap, upgradable_option), (10, capacity_sats, required), (12, announcement_message, required), }); @@ -1161,7 +1161,7 @@ impl Readable for NodeInfo { read_tlv_fields!(reader, { (0, _lowest_inbound_channel_fees, option), - (2, announcement_info_wrap, upgradable_required), + (2, announcement_info_wrap, upgradable_option), (4, channels, vec_type), }); diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 6e8dd506..e083036b 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -23,7 +23,7 @@ use crate::ln::features::ChannelTypeFeatures; use crate::ln::msgs; use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret}; use crate::routing::gossip::NetworkUpdate; -use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, WithoutLength}; +use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, UpgradableRequired, WithoutLength}; use crate::routing::router::{RouteHop, RouteParameters}; use bitcoin::{PackedLockTime, Transaction}; @@ -1199,7 +1199,7 @@ impl MaybeReadable for Event { let mut payment_id = None; read_tlv_fields!(reader, { (0, payment_hash, required), - (1, network_update, upgradable_required), + (1, network_update, upgradable_option), (2, payment_failed_permanently, required), (5, path, vec_type), (7, short_channel_id, option), @@ -1276,7 +1276,7 @@ impl MaybeReadable for Event { 9u8 => { let f = || { let mut channel_id = [0; 32]; - let mut reason = None; + let mut reason = UpgradableRequired(None); let mut user_channel_id_low_opt: Option = None; let mut user_channel_id_high_opt: Option = None; read_tlv_fields!(reader, { @@ -1285,7 +1285,6 @@ impl MaybeReadable for Event { (2, reason, upgradable_required), (3, user_channel_id_high_opt, option), }); - if reason.is_none() { return Ok(None); } // `user_channel_id` used to be a single u64 value. In order to remain // backwards compatible with versions prior to 0.0.113, the u128 is serialized @@ -1293,7 +1292,7 @@ impl MaybeReadable for Event { let user_channel_id = (user_channel_id_low_opt.unwrap_or(0) as u128) + ((user_channel_id_high_opt.unwrap_or(0) as u128) << 64); - Ok(Some(Event::ChannelClosed { channel_id, user_channel_id, reason: reason.unwrap() })) + Ok(Some(Event::ChannelClosed { channel_id, user_channel_id, reason: _init_tlv_based_struct_field!(reason, upgradable_required) })) }; f() }, @@ -1349,7 +1348,7 @@ impl MaybeReadable for Event { 19u8 => { let f = || { let mut payment_hash = PaymentHash([0; 32]); - let mut purpose = None; + let mut purpose = UpgradableRequired(None); let mut amount_msat = 0; let mut receiver_node_id = None; read_tlv_fields!(reader, { @@ -1358,11 +1357,10 @@ impl MaybeReadable for Event { (2, purpose, upgradable_required), (4, amount_msat, required), }); - if purpose.is_none() { return Ok(None); } Ok(Some(Event::PaymentClaimed { receiver_node_id, payment_hash, - purpose: purpose.unwrap(), + purpose: _init_tlv_based_struct_field!(purpose, upgradable_required), amount_msat, })) }; @@ -1410,22 +1408,15 @@ impl MaybeReadable for Event { 25u8 => { let f = || { let mut prev_channel_id = [0; 32]; - let mut failed_next_destination_opt = None; + let mut failed_next_destination_opt = UpgradableRequired(None); read_tlv_fields!(reader, { (0, prev_channel_id, required), (2, failed_next_destination_opt, upgradable_required), }); - if let Some(failed_next_destination) = failed_next_destination_opt { - Ok(Some(Event::HTLCHandlingFailed { - prev_channel_id, - failed_next_destination, - })) - } else { - // If we fail to read a `failed_next_destination` assume it's because - // `MaybeReadable::read` returned `Ok(None)`, though it's also possible we - // were simply missing the field. - Ok(None) - } + Ok(Some(Event::HTLCHandlingFailed { + prev_channel_id, + failed_next_destination: _init_tlv_based_struct_field!(failed_next_destination_opt, upgradable_required), + })) }; f() }, diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index 36b64b63..5adf5758 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -303,6 +303,18 @@ impl From for RequiredWrapper { fn from(t: T) -> RequiredWrapper { RequiredWrapper(Some(t)) } } +/// Wrapper to read a required (non-optional) TLV record that may have been upgraded without +/// backwards compat. +pub struct UpgradableRequired(pub Option); +impl MaybeReadable for UpgradableRequired { + #[inline] + fn read(reader: &mut R) -> Result, DecodeError> { + let tlv = MaybeReadable::read(reader)?; + if let Some(tlv) = tlv { return Ok(Some(Self(Some(tlv)))) } + Ok(None) + } +} + pub(crate) struct U48(pub u64); impl Writeable for U48 { #[inline] diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 2aa3846a..1f617de4 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -217,7 +217,7 @@ macro_rules! _check_decoded_tlv_order { // no-op }}; ($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, upgradable_required) => {{ - // no-op + _check_decoded_tlv_order!($last_seen_type, $typ, $type, $field, required) }}; ($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, upgradable_option) => {{ // no-op @@ -259,7 +259,7 @@ macro_rules! _check_missing_tlv { // no-op }}; ($last_seen_type: expr, $type: expr, $field: ident, upgradable_required) => {{ - // no-op + _check_missing_tlv!($last_seen_type, $type, $field, required) }}; ($last_seen_type: expr, $type: expr, $field: ident, upgradable_option) => {{ // no-op @@ -293,7 +293,10 @@ macro_rules! _decode_tlv { $field = Some($crate::util::ser::Readable::read(&mut $reader)?); }}; ($reader: expr, $field: ident, upgradable_required) => {{ - $field = $crate::util::ser::MaybeReadable::read(&mut $reader)?; + $field = match $crate::util::ser::MaybeReadable::read(&mut $reader)? { + Some(res) => res, + _ => return Ok(None) + }; }}; ($reader: expr, $field: ident, upgradable_option) => {{ $field = $crate::util::ser::MaybeReadable::read(&mut $reader)?; @@ -636,7 +639,7 @@ macro_rules! _init_tlv_based_struct_field { $field }; ($field: ident, upgradable_required) => { - if $field.is_none() { return Ok(None); } else { $field.unwrap() } + $field.0.unwrap() }; ($field: ident, upgradable_option) => { $field @@ -671,7 +674,7 @@ macro_rules! _init_tlv_field_var { let mut $field = None; }; ($field: ident, upgradable_required) => { - let mut $field = None; + let mut $field = $crate::util::ser::UpgradableRequired(None); }; ($field: ident, upgradable_option) => { let mut $field = None; @@ -1050,6 +1053,47 @@ mod tests { (0xdeadbeef1badbeef, 0x1bad1dea, Some(0x01020304))); } + #[derive(Debug, PartialEq)] + struct TestUpgradable { + a: u32, + b: u32, + c: Option, + } + + fn upgradable_tlv_reader(s: &[u8]) -> Result, DecodeError> { + let mut s = Cursor::new(s); + let mut a = 0; + let mut b = 0; + let mut c: Option = None; + decode_tlv_stream!(&mut s, {(2, a, upgradable_required), (3, b, upgradable_required), (4, c, upgradable_option)}); + Ok(Some(TestUpgradable { a, b, c, })) + } + + #[test] + fn upgradable_tlv_simple_good_cases() { + assert_eq!(upgradable_tlv_reader(&::hex::decode( + concat!("0204deadbeef", "03041bad1dea", "0404deadbeef") + ).unwrap()[..]).unwrap(), + Some(TestUpgradable { a: 0xdeadbeef, b: 0x1bad1dea, c: Some(0xdeadbeef) })); + + assert_eq!(upgradable_tlv_reader(&::hex::decode( + concat!("0204deadbeef", "03041bad1dea") + ).unwrap()[..]).unwrap(), + Some(TestUpgradable { a: 0xdeadbeef, b: 0x1bad1dea, c: None})); + } + + #[test] + fn missing_required_upgradable() { + if let Err(DecodeError::InvalidValue) = upgradable_tlv_reader(&::hex::decode( + concat!("0100", "0204deadbeef") + ).unwrap()[..]) { + } else { panic!(); } + if let Err(DecodeError::InvalidValue) = upgradable_tlv_reader(&::hex::decode( + concat!("0100", "03041bad1dea") + ).unwrap()[..]) { + } else { panic!(); } + } + // BOLT TLV test cases fn tlv_reader_n1(s: &[u8]) -> Result<(Option>, Option, Option<(PublicKey, u64, u64)>, Option), DecodeError> { let mut s = Cursor::new(s); -- 2.30.2