From 07b674ecb1b92028ba830e8de056d669e50f3752 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 29 Aug 2021 05:26:39 +0000 Subject: [PATCH 1/4] Drop writer size hinting/message vec preallocation In order to avoid significant malloc traffic, messages previously explicitly stated their serialized length allowing for Vec preallocation during the message serialization pipeline. This added some amount of complexity in the serialization code, but did avoid some realloc() calls. Instead, here, we drop all the complexity in favor of a fixed 2KiB buffer for all message serialization. This should not only be simpler with a similar reduction in realloc() traffic, but also may reduce heap fragmentation by allocating identically-sized buffers more often. --- fuzz/src/chanmon_consistency.rs | 3 -- fuzz/src/chanmon_deser.rs | 3 -- fuzz/src/msg_targets/utils.rs | 8 +-- lightning/src/chain/transaction.rs | 2 +- lightning/src/ln/features.rs | 10 +--- lightning/src/ln/msgs.rs | 83 +++++++----------------------- lightning/src/ln/peer_handler.rs | 2 +- lightning/src/util/ser.rs | 17 +----- lightning/src/util/ser_macros.rs | 82 ++--------------------------- lightning/src/util/test_utils.rs | 3 -- 10 files changed, 33 insertions(+), 180 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 88826d65570..835061feb37 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -94,9 +94,6 @@ impl Writer for VecWriter { self.0.extend_from_slice(buf); Ok(()) } - fn size_hint(&mut self, size: usize) { - self.0.reserve_exact(size); - } } struct TestChainMonitor { diff --git a/fuzz/src/chanmon_deser.rs b/fuzz/src/chanmon_deser.rs index 933930cf6d1..c31930342f4 100644 --- a/fuzz/src/chanmon_deser.rs +++ b/fuzz/src/chanmon_deser.rs @@ -18,9 +18,6 @@ impl Writer for VecWriter { self.0.extend_from_slice(buf); Ok(()) } - fn size_hint(&mut self, size: usize) { - self.0.reserve_exact(size); - } } #[inline] diff --git a/fuzz/src/msg_targets/utils.rs b/fuzz/src/msg_targets/utils.rs index 82fa739fbcf..6325f3bf10d 100644 --- a/fuzz/src/msg_targets/utils.rs +++ b/fuzz/src/msg_targets/utils.rs @@ -13,13 +13,9 @@ use lightning::util::ser::Writer; pub struct VecWriter(pub Vec); impl Writer for VecWriter { fn write_all(&mut self, buf: &[u8]) -> Result<(), ::std::io::Error> { - assert!(self.0.capacity() >= self.0.len() + buf.len()); self.0.extend_from_slice(buf); Ok(()) } - fn size_hint(&mut self, size: usize) { - self.0.reserve_exact(size); - } } // We attempt to test the strictest behavior we can for a given message, however, some messages @@ -43,6 +39,7 @@ macro_rules! test_msg { msg.write(&mut w).unwrap(); assert_eq!(w.0.len(), p); + assert_eq!(msg.serialized_length(), p); assert_eq!(&r.into_inner()[..p], &w.0[..p]); } } @@ -60,6 +57,7 @@ macro_rules! test_msg_simple { if let Ok(msg) = <$MsgType as Readable>::read(&mut r) { let mut w = VecWriter(Vec::new()); msg.write(&mut w).unwrap(); + assert_eq!(msg.serialized_length(), w.0.len()); let msg = <$MsgType as Readable>::read(&mut ::std::io::Cursor::new(&w.0)).unwrap(); let mut w_two = VecWriter(Vec::new()); @@ -82,6 +80,7 @@ macro_rules! test_msg_exact { let mut w = VecWriter(Vec::new()); msg.write(&mut w).unwrap(); assert_eq!(&r.into_inner()[..], &w.0[..]); + assert_eq!(msg.serialized_length(), w.0.len()); } } } @@ -99,6 +98,7 @@ macro_rules! test_msg_hole { let mut w = VecWriter(Vec::new()); msg.write(&mut w).unwrap(); let p = w.0.len() as usize; + assert_eq!(msg.serialized_length(), p); assert_eq!(w.0.len(), p); assert_eq!(&r.get_ref()[..$hole], &w.0[..$hole]); diff --git a/lightning/src/chain/transaction.rs b/lightning/src/chain/transaction.rs index 502eb895b26..0219ebbe8ca 100644 --- a/lightning/src/chain/transaction.rs +++ b/lightning/src/chain/transaction.rs @@ -75,7 +75,7 @@ impl OutPoint { } } -impl_writeable!(OutPoint, 0, { txid, index }); +impl_writeable!(OutPoint, { txid, index }); #[cfg(test)] mod tests { diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs index d1f6b89db4f..8f251d8478c 100644 --- a/lightning/src/ln/features.rs +++ b/lightning/src/ln/features.rs @@ -392,7 +392,6 @@ impl InitFeatures { /// Writes all features present up to, and including, 13. pub(crate) fn write_up_to_13(&self, w: &mut W) -> Result<(), io::Error> { let len = cmp::min(2, self.flags.len()); - w.size_hint(len + 2); (len as u16).write(w)?; for i in (0..len).rev() { if i == 0 { @@ -584,12 +583,6 @@ impl Features { (byte & unknown_features) != 0 }) } - - /// The number of bytes required to represent the feature flags present. This does not include - /// the length bytes which are included in the serialized form. - pub(crate) fn byte_count(&self) -> usize { - self.flags.len() - } } impl Features { @@ -702,7 +695,6 @@ impl Features { impl Writeable for Features { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(self.flags.len() + 2); (self.flags.len() as u16).write(w)?; for f in self.flags.iter().rev() { // Swap back to big-endian f.write(w)?; @@ -835,7 +827,7 @@ mod tests { #[test] fn convert_to_context_with_unknown_flags() { // Ensure the `from` context has fewer known feature bytes than the `to` context. - assert!(InvoiceFeatures::known().byte_count() < NodeFeatures::known().byte_count()); + assert!(InvoiceFeatures::known().flags.len() < NodeFeatures::known().flags.len()); let invoice_features = InvoiceFeatures::known().set_unknown_feature_optional(); assert!(invoice_features.supports_unknown_bits()); let node_features: NodeFeatures = invoice_features.to_context(); diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 5b49f43b118..34ccc8adf7a 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -1053,10 +1053,7 @@ impl Readable for OptionalField { } -impl_writeable_len_match!(AcceptChannel, { - {AcceptChannel{ shutdown_scriptpubkey: OptionalField::Present(ref script), .. }, 270 + 2 + script.len()}, - {_, 270} - }, { +impl_writeable!(AcceptChannel, { temporary_channel_id, dust_limit_satoshis, max_htlc_value_in_flight_msat, @@ -1074,7 +1071,7 @@ impl_writeable_len_match!(AcceptChannel, { shutdown_scriptpubkey }); -impl_writeable!(AnnouncementSignatures, 32+8+64*2, { +impl_writeable!(AnnouncementSignatures, { channel_id, short_channel_id, node_signature, @@ -1083,7 +1080,6 @@ impl_writeable!(AnnouncementSignatures, 32+8+64*2, { impl Writeable for ChannelReestablish { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(if let OptionalField::Present(..) = self.data_loss_protect { 32+2*8+33+32 } else { 32+2*8 }); self.channel_id.write(w)?; self.next_local_commitment_number.write(w)?; self.next_remote_commitment_number.write(w)?; @@ -1121,7 +1117,6 @@ impl Readable for ChannelReestablish{ impl Writeable for ClosingSigned { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(32 + 8 + 64 + if self.fee_range.is_some() { 1+1+ 2*8 } else { 0 }); self.channel_id.write(w)?; self.fee_satoshis.write(w)?; self.signature.write(w)?; @@ -1145,40 +1140,36 @@ impl Readable for ClosingSigned { } } -impl_writeable!(ClosingSignedFeeRange, 2*8, { +impl_writeable!(ClosingSignedFeeRange, { min_fee_satoshis, max_fee_satoshis }); -impl_writeable_len_match!(CommitmentSigned, { - { CommitmentSigned { ref htlc_signatures, .. }, 32+64+2+htlc_signatures.len()*64 } - }, { +impl_writeable!(CommitmentSigned, { channel_id, signature, htlc_signatures }); -impl_writeable_len_match!(DecodedOnionErrorPacket, { - { DecodedOnionErrorPacket { ref failuremsg, ref pad, .. }, 32 + 4 + failuremsg.len() + pad.len() } - }, { +impl_writeable!(DecodedOnionErrorPacket, { hmac, failuremsg, pad }); -impl_writeable!(FundingCreated, 32+32+2+64, { +impl_writeable!(FundingCreated, { temporary_channel_id, funding_txid, funding_output_index, signature }); -impl_writeable!(FundingSigned, 32+64, { +impl_writeable!(FundingSigned, { channel_id, signature }); -impl_writeable!(FundingLocked, 32+33, { +impl_writeable!(FundingLocked, { channel_id, next_per_commitment_point }); @@ -1202,10 +1193,7 @@ impl Readable for Init { } } -impl_writeable_len_match!(OpenChannel, { - { OpenChannel { shutdown_scriptpubkey: OptionalField::Present(ref script), .. }, 319 + 2 + script.len() }, - { _, 319 } - }, { +impl_writeable!(OpenChannel, { chain_hash, temporary_channel_id, funding_satoshis, @@ -1227,54 +1215,47 @@ impl_writeable_len_match!(OpenChannel, { shutdown_scriptpubkey }); -impl_writeable!(RevokeAndACK, 32+32+33, { +impl_writeable!(RevokeAndACK, { channel_id, per_commitment_secret, next_per_commitment_point }); -impl_writeable_len_match!(Shutdown, { - { Shutdown { ref scriptpubkey, .. }, 32 + 2 + scriptpubkey.len() } - }, { +impl_writeable!(Shutdown, { channel_id, scriptpubkey }); -impl_writeable_len_match!(UpdateFailHTLC, { - { UpdateFailHTLC { ref reason, .. }, 32 + 10 + reason.data.len() } - }, { +impl_writeable!(UpdateFailHTLC, { channel_id, htlc_id, reason }); -impl_writeable!(UpdateFailMalformedHTLC, 32+8+32+2, { +impl_writeable!(UpdateFailMalformedHTLC, { channel_id, htlc_id, sha256_of_onion, failure_code }); -impl_writeable!(UpdateFee, 32+4, { +impl_writeable!(UpdateFee, { channel_id, feerate_per_kw }); -impl_writeable!(UpdateFulfillHTLC, 32+8+32, { +impl_writeable!(UpdateFulfillHTLC, { channel_id, htlc_id, payment_preimage }); -impl_writeable_len_match!(OnionErrorPacket, { - { OnionErrorPacket { ref data, .. }, 2 + data.len() } - }, { +impl_writeable!(OnionErrorPacket, { data }); impl Writeable for OnionPacket { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(1 + 33 + 20*65 + 32); self.version.write(w)?; match self.public_key { Ok(pubkey) => pubkey.write(w)?, @@ -1301,7 +1282,7 @@ impl Readable for OnionPacket { } } -impl_writeable!(UpdateAddHTLC, 32+8+8+32+4+1366, { +impl_writeable!(UpdateAddHTLC, { channel_id, htlc_id, amount_msat, @@ -1312,7 +1293,6 @@ impl_writeable!(UpdateAddHTLC, 32+8+8+32+4+1366, { impl Writeable for FinalOnionHopData { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(32 + 8 - (self.total_msat.leading_zeros()/8) as usize); self.payment_secret.0.write(w)?; HighZeroBytesDroppedVarInt(self.total_msat).write(w) } @@ -1328,7 +1308,6 @@ impl Readable for FinalOnionHopData { impl Writeable for OnionHopData { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(33); // Note that this should never be reachable if Rust-Lightning generated the message, as we // check values are sane long before we get here, though its possible in the future // user-generated messages may hit this. @@ -1429,7 +1408,6 @@ impl Readable for OnionHopData { impl Writeable for Ping { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(self.byteslen as usize + 4); self.ponglen.write(w)?; vec![0u8; self.byteslen as usize].write(w)?; // size-unchecked write Ok(()) @@ -1451,7 +1429,6 @@ impl Readable for Ping { impl Writeable for Pong { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(self.byteslen as usize + 2); vec![0u8; self.byteslen as usize].write(w)?; // size-unchecked write Ok(()) } @@ -1471,7 +1448,6 @@ impl Readable for Pong { impl Writeable for UnsignedChannelAnnouncement { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(2 + 32 + 8 + 4*33 + self.features.byte_count() + self.excess_data.len()); self.features.write(w)?; self.chain_hash.write(w)?; self.short_channel_id.write(w)?; @@ -1499,10 +1475,7 @@ impl Readable for UnsignedChannelAnnouncement { } } -impl_writeable_len_match!(ChannelAnnouncement, { - { ChannelAnnouncement { contents: UnsignedChannelAnnouncement {ref features, ref excess_data, ..}, .. }, - 2 + 32 + 8 + 4*33 + features.byte_count() + excess_data.len() + 4*64 } - }, { +impl_writeable!(ChannelAnnouncement, { node_signature_1, node_signature_2, bitcoin_signature_1, @@ -1512,13 +1485,10 @@ impl_writeable_len_match!(ChannelAnnouncement, { impl Writeable for UnsignedChannelUpdate { fn write(&self, w: &mut W) -> Result<(), io::Error> { - let mut size = 64 + self.excess_data.len(); let mut message_flags: u8 = 0; if let OptionalField::Present(_) = self.htlc_maximum_msat { - size += 8; message_flags = 1; } - w.size_hint(size); self.chain_hash.write(w)?; self.short_channel_id.write(w)?; self.timestamp.write(w)?; @@ -1557,17 +1527,13 @@ impl Readable for UnsignedChannelUpdate { } } -impl_writeable_len_match!(ChannelUpdate, { - { ChannelUpdate { contents: UnsignedChannelUpdate {ref excess_data, ref htlc_maximum_msat, ..}, .. }, - 64 + 64 + excess_data.len() + if let OptionalField::Present(_) = htlc_maximum_msat { 8 } else { 0 } } - }, { +impl_writeable!(ChannelUpdate, { signature, contents }); impl Writeable for ErrorMessage { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(32 + 2 + self.data.len()); self.channel_id.write(w)?; (self.data.len() as u16).write(w)?; w.write_all(self.data.as_bytes())?; @@ -1594,7 +1560,6 @@ impl Readable for ErrorMessage { impl Writeable for UnsignedNodeAnnouncement { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(76 + self.features.byte_count() + self.addresses.len()*38 + self.excess_address_data.len() + self.excess_data.len()); self.features.write(w)?; self.timestamp.write(w)?; self.node_id.write(w)?; @@ -1677,10 +1642,7 @@ impl Readable for UnsignedNodeAnnouncement { } } -impl_writeable_len_match!(NodeAnnouncement, <=, { - { NodeAnnouncement { contents: UnsignedNodeAnnouncement { ref features, ref addresses, ref excess_address_data, ref excess_data, ..}, .. }, - 64 + 76 + features.byte_count() + addresses.len()*(NetAddress::MAX_LEN as usize + 1) + excess_address_data.len() + excess_data.len() } - }, { +impl_writeable!(NodeAnnouncement, { signature, contents }); @@ -1724,7 +1686,6 @@ impl Writeable for QueryShortChannelIds { // Calculated from 1-byte encoding_type plus 8-bytes per short_channel_id let encoding_len: u16 = 1 + self.short_channel_ids.len() as u16 * 8; - w.size_hint(32 + 2 + encoding_len as usize); self.chain_hash.write(w)?; encoding_len.write(w)?; @@ -1752,7 +1713,6 @@ impl Readable for ReplyShortChannelIdsEnd { impl Writeable for ReplyShortChannelIdsEnd { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(32 + 1); self.chain_hash.write(w)?; self.full_information.write(w)?; Ok(()) @@ -1787,7 +1747,6 @@ impl Readable for QueryChannelRange { impl Writeable for QueryChannelRange { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(32 + 4 + 4); self.chain_hash.write(w)?; self.first_blocknum.write(w)?; self.number_of_blocks.write(w)?; @@ -1838,7 +1797,6 @@ impl Readable for ReplyChannelRange { impl Writeable for ReplyChannelRange { fn write(&self, w: &mut W) -> Result<(), io::Error> { let encoding_len: u16 = 1 + self.short_channel_ids.len() as u16 * 8; - w.size_hint(32 + 4 + 4 + 1 + 2 + encoding_len as usize); self.chain_hash.write(w)?; self.first_blocknum.write(w)?; self.number_of_blocks.write(w)?; @@ -1869,7 +1827,6 @@ impl Readable for GossipTimestampFilter { impl Writeable for GossipTimestampFilter { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(32 + 4 + 4); self.chain_hash.write(w)?; self.first_timestamp.write(w)?; self.timestamp_range.write(w)?; diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 34bcda3a009..e6a06b56e95 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -721,7 +721,7 @@ impl P /// Append a message to a peer's pending outbound/write buffer, and update the map of peers needing sends accordingly. fn enqueue_message(&self, peer: &mut Peer, message: &M) { - let mut buffer = VecWriter(Vec::new()); + let mut buffer = VecWriter(Vec::with_capacity(2048)); wire::write(message, &mut buffer).unwrap(); // crash if the write failed let encoded_message = buffer.0; diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index 0b5036bd749..73f3b4dca52 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -35,18 +35,13 @@ use util::byte_utils::{be48_to_array, slice_to_be48}; /// serialization buffer size pub const MAX_BUF_SIZE: usize = 64 * 1024; -/// A trait that is similar to std::io::Write but has one extra function which can be used to size -/// buffers being written into. -/// An impl is provided for any type that also impls std::io::Write which simply ignores size -/// hints. +/// A simplified version of std::io::Write that exists largely for backwards compatibility. +/// An impl is provided for any type that also impls std::io::Write. /// /// (C-not exported) as we only export serialization to/from byte arrays instead pub trait Writer { /// Writes the given buf out. See std::io::Write::write_all for more fn write_all(&mut self, buf: &[u8]) -> Result<(), io::Error>; - /// Hints that data of the given size is about the be written. This may not always be called - /// prior to data being written and may be safely ignored. - fn size_hint(&mut self, size: usize); } impl Writer for W { @@ -54,8 +49,6 @@ impl Writer for W { fn write_all(&mut self, buf: &[u8]) -> Result<(), io::Error> { ::write_all(self, buf) } - #[inline] - fn size_hint(&mut self, _size: usize) { } } pub(crate) struct WriterWriteAdaptor<'a, W: Writer + 'a>(pub &'a mut W); @@ -82,10 +75,6 @@ impl Writer for VecWriter { self.0.extend_from_slice(buf); Ok(()) } - #[inline] - fn size_hint(&mut self, size: usize) { - self.0.reserve_exact(size); - } } /// Writer that only tracks the amount of data written - useful if you need to calculate the length @@ -97,8 +86,6 @@ impl Writer for LengthCalculatingWriter { self.0 += buf.len(); Ok(()) } - #[inline] - fn size_hint(&mut self, _size: usize) {} } /// Essentially std::io::Take but a bit simpler and with a method to walk the underlying stream diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 5178732c745..b229383b283 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -231,39 +231,18 @@ macro_rules! decode_tlv_stream { } macro_rules! impl_writeable { - ($st:ident, $len: expr, {$($field:ident),*}) => { + ($st:ident, {$($field:ident),*}) => { impl ::util::ser::Writeable for $st { fn write(&self, w: &mut W) -> Result<(), $crate::io::Error> { - if $len != 0 { - w.size_hint($len); - } - #[cfg(any(test, feature = "fuzztarget"))] - { - // In tests, assert that the hard-coded length matches the actual one - if $len != 0 { - let mut len_calc = ::util::ser::LengthCalculatingWriter(0); - $( self.$field.write(&mut len_calc).expect("No in-memory data may fail to serialize"); )* - assert_eq!(len_calc.0, $len); - assert_eq!(self.serialized_length(), $len); - } - } $( self.$field.write(w)?; )* Ok(()) } #[inline] fn serialized_length(&self) -> usize { - if $len == 0 || cfg!(any(test, feature = "fuzztarget")) { - let mut len_calc = 0; - $( len_calc += self.$field.serialized_length(); )* - if $len != 0 { - // In tests, assert that the hard-coded length matches the actual one - assert_eq!(len_calc, $len); - } else { - return len_calc; - } - } - $len + let mut len_calc = 0; + $( len_calc += self.$field.serialized_length(); )* + return len_calc; } } @@ -276,59 +255,6 @@ macro_rules! impl_writeable { } } } -macro_rules! impl_writeable_len_match { - ($struct: ident, $cmp: tt, ($calc_len: expr), {$({$match: pat, $length: expr}),*}, {$($field:ident),*}) => { - impl Writeable for $struct { - fn write(&self, w: &mut W) -> Result<(), $crate::io::Error> { - let len = match *self { - $($match => $length,)* - }; - w.size_hint(len); - #[cfg(any(test, feature = "fuzztarget"))] - { - // In tests, assert that the hard-coded length matches the actual one - let mut len_calc = ::util::ser::LengthCalculatingWriter(0); - $( self.$field.write(&mut len_calc).expect("No in-memory data may fail to serialize"); )* - assert!(len_calc.0 $cmp len); - assert_eq!(len_calc.0, self.serialized_length()); - } - $( self.$field.write(w)?; )* - Ok(()) - } - - #[inline] - fn serialized_length(&self) -> usize { - if $calc_len || cfg!(any(test, feature = "fuzztarget")) { - let mut len_calc = 0; - $( len_calc += self.$field.serialized_length(); )* - if !$calc_len { - assert_eq!(len_calc, match *self { - $($match => $length,)* - }); - } - return len_calc - } - match *self { - $($match => $length,)* - } - } - } - - impl ::util::ser::Readable for $struct { - fn read(r: &mut R) -> Result { - Ok(Self { - $($field: Readable::read(r)?),* - }) - } - } - }; - ($struct: ident, $cmp: tt, {$({$match: pat, $length: expr}),*}, {$($field:ident),*}) => { - impl_writeable_len_match!($struct, $cmp, (true), { $({ $match, $length }),* }, { $($field),* }); - }; - ($struct: ident, {$({$match: pat, $length: expr}),*}, {$($field:ident),*}) => { - impl_writeable_len_match!($struct, ==, (false), { $({ $match, $length }),* }, { $($field),* }); - } -} /// Write out two bytes to indicate the version of an object. /// $this_version represents a unique version of a type. Incremented whenever the type's diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 64b88acb008..379f72a87c6 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -52,9 +52,6 @@ impl Writer for TestVecWriter { self.0.extend_from_slice(buf); Ok(()) } - fn size_hint(&mut self, size: usize) { - self.0.reserve_exact(size); - } } pub struct TestFeeEstimator { From 7f9eef553c89bef4e5aaf369ad02cfff4e0f9048 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 4 Sep 2021 06:37:36 +0000 Subject: [PATCH 2/4] [fuzz] Swap mode on most messages to account for TLV suffix --- fuzz/src/msg_targets/gen_target.sh | 46 +++++++++---------- fuzz/src/msg_targets/mod.rs | 22 ++++----- fuzz/src/msg_targets/msg_accept_channel.rs | 4 +- .../msg_announcement_signatures.rs | 4 +- fuzz/src/msg_targets/msg_commitment_signed.rs | 4 +- fuzz/src/msg_targets/msg_funding_created.rs | 4 +- fuzz/src/msg_targets/msg_funding_locked.rs | 4 +- fuzz/src/msg_targets/msg_funding_signed.rs | 4 +- .../msg_gossip_timestamp_filter.rs | 4 +- fuzz/src/msg_targets/msg_open_channel.rs | 4 +- .../msg_targets/msg_query_channel_range.rs | 4 +- .../msg_reply_short_channel_ids_end.rs | 4 +- fuzz/src/msg_targets/msg_revoke_and_ack.rs | 4 +- fuzz/src/msg_targets/msg_shutdown.rs | 4 +- fuzz/src/msg_targets/msg_update_add_htlc.rs | 4 +- fuzz/src/msg_targets/msg_update_fail_htlc.rs | 4 +- .../msg_update_fail_malformed_htlc.rs | 4 +- fuzz/src/msg_targets/msg_update_fee.rs | 4 +- .../msg_targets/msg_update_fulfill_htlc.rs | 4 +- 19 files changed, 68 insertions(+), 68 deletions(-) diff --git a/fuzz/src/msg_targets/gen_target.sh b/fuzz/src/msg_targets/gen_target.sh index 0c1d061a74d..2e133484138 100755 --- a/fuzz/src/msg_targets/gen_target.sh +++ b/fuzz/src/msg_targets/gen_target.sh @@ -11,36 +11,36 @@ echo "mod utils;" > mod.rs # Note when adding new targets here you should add a similar line in src/bin/gen_target.sh -GEN_TEST AcceptChannel test_msg "" -GEN_TEST AnnouncementSignatures test_msg "" +GEN_TEST AcceptChannel test_msg_simple "" +GEN_TEST AnnouncementSignatures test_msg_simple "" +GEN_TEST ClosingSigned test_msg_simple "" +GEN_TEST CommitmentSigned test_msg_simple "" +GEN_TEST FundingCreated test_msg_simple "" +GEN_TEST FundingLocked test_msg_simple "" +GEN_TEST FundingSigned test_msg_simple "" +GEN_TEST GossipTimestampFilter test_msg_simple "" +GEN_TEST Init test_msg_simple "" +GEN_TEST OnionHopData test_msg_simple "" +GEN_TEST OpenChannel test_msg_simple "" +GEN_TEST Ping test_msg_simple "" +GEN_TEST Pong test_msg_simple "" +GEN_TEST QueryChannelRange test_msg_simple "" +GEN_TEST ReplyShortChannelIdsEnd test_msg_simple "" +GEN_TEST RevokeAndACK test_msg_simple "" +GEN_TEST Shutdown test_msg_simple "" +GEN_TEST UpdateAddHTLC test_msg_simple "" +GEN_TEST UpdateFailHTLC test_msg_simple "" +GEN_TEST UpdateFailMalformedHTLC test_msg_simple "" +GEN_TEST UpdateFee test_msg_simple "" +GEN_TEST UpdateFulfillHTLC test_msg_simple "" + GEN_TEST ChannelReestablish test_msg "" -GEN_TEST CommitmentSigned test_msg "" GEN_TEST DecodedOnionErrorPacket test_msg "" -GEN_TEST FundingCreated test_msg "" -GEN_TEST FundingLocked test_msg "" -GEN_TEST FundingSigned test_msg "" -GEN_TEST OpenChannel test_msg "" -GEN_TEST RevokeAndACK test_msg "" -GEN_TEST Shutdown test_msg "" -GEN_TEST UpdateFailHTLC test_msg "" -GEN_TEST UpdateFailMalformedHTLC test_msg "" -GEN_TEST UpdateFee test_msg "" -GEN_TEST UpdateFulfillHTLC test_msg "" GEN_TEST ChannelAnnouncement test_msg_exact "" GEN_TEST NodeAnnouncement test_msg_exact "" GEN_TEST QueryShortChannelIds test_msg "" -GEN_TEST ReplyShortChannelIdsEnd test_msg "" -GEN_TEST QueryChannelRange test_msg "" GEN_TEST ReplyChannelRange test_msg "" -GEN_TEST GossipTimestampFilter test_msg "" -GEN_TEST UpdateAddHTLC test_msg_hole ", 85, 33" GEN_TEST ErrorMessage test_msg_hole ", 32, 2" GEN_TEST ChannelUpdate test_msg_hole ", 108, 1" - -GEN_TEST ClosingSigned test_msg_simple "" -GEN_TEST Init test_msg_simple "" -GEN_TEST OnionHopData test_msg_simple "" -GEN_TEST Ping test_msg_simple "" -GEN_TEST Pong test_msg_simple "" diff --git a/fuzz/src/msg_targets/mod.rs b/fuzz/src/msg_targets/mod.rs index 0f273cb7606..af70c58e30e 100644 --- a/fuzz/src/msg_targets/mod.rs +++ b/fuzz/src/msg_targets/mod.rs @@ -1,31 +1,31 @@ mod utils; pub mod msg_accept_channel; pub mod msg_announcement_signatures; -pub mod msg_channel_reestablish; +pub mod msg_closing_signed; pub mod msg_commitment_signed; -pub mod msg_decoded_onion_error_packet; pub mod msg_funding_created; pub mod msg_funding_locked; pub mod msg_funding_signed; +pub mod msg_gossip_timestamp_filter; +pub mod msg_init; +pub mod msg_onion_hop_data; pub mod msg_open_channel; +pub mod msg_ping; +pub mod msg_pong; +pub mod msg_query_channel_range; +pub mod msg_reply_short_channel_ids_end; pub mod msg_revoke_and_ack; pub mod msg_shutdown; +pub mod msg_update_add_htlc; pub mod msg_update_fail_htlc; pub mod msg_update_fail_malformed_htlc; pub mod msg_update_fee; pub mod msg_update_fulfill_htlc; +pub mod msg_channel_reestablish; +pub mod msg_decoded_onion_error_packet; pub mod msg_channel_announcement; pub mod msg_node_announcement; pub mod msg_query_short_channel_ids; -pub mod msg_reply_short_channel_ids_end; -pub mod msg_query_channel_range; pub mod msg_reply_channel_range; -pub mod msg_gossip_timestamp_filter; -pub mod msg_update_add_htlc; pub mod msg_error_message; pub mod msg_channel_update; -pub mod msg_closing_signed; -pub mod msg_init; -pub mod msg_onion_hop_data; -pub mod msg_ping; -pub mod msg_pong; diff --git a/fuzz/src/msg_targets/msg_accept_channel.rs b/fuzz/src/msg_targets/msg_accept_channel.rs index 095556d57d9..a8ec438784b 100644 --- a/fuzz/src/msg_targets/msg_accept_channel.rs +++ b/fuzz/src/msg_targets/msg_accept_channel.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_accept_channel_test(data: &[u8], _out: Out) { - test_msg!(msgs::AcceptChannel, data); + test_msg_simple!(msgs::AcceptChannel, data); } #[no_mangle] pub extern "C" fn msg_accept_channel_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::AcceptChannel, data); + test_msg_simple!(msgs::AcceptChannel, data); } diff --git a/fuzz/src/msg_targets/msg_announcement_signatures.rs b/fuzz/src/msg_targets/msg_announcement_signatures.rs index 138ab5dbf19..0fc40fcb69b 100644 --- a/fuzz/src/msg_targets/msg_announcement_signatures.rs +++ b/fuzz/src/msg_targets/msg_announcement_signatures.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_announcement_signatures_test(data: &[u8], _out: Out) { - test_msg!(msgs::AnnouncementSignatures, data); + test_msg_simple!(msgs::AnnouncementSignatures, data); } #[no_mangle] pub extern "C" fn msg_announcement_signatures_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::AnnouncementSignatures, data); + test_msg_simple!(msgs::AnnouncementSignatures, data); } diff --git a/fuzz/src/msg_targets/msg_commitment_signed.rs b/fuzz/src/msg_targets/msg_commitment_signed.rs index 9ac846f6f10..163ce744606 100644 --- a/fuzz/src/msg_targets/msg_commitment_signed.rs +++ b/fuzz/src/msg_targets/msg_commitment_signed.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_commitment_signed_test(data: &[u8], _out: Out) { - test_msg!(msgs::CommitmentSigned, data); + test_msg_simple!(msgs::CommitmentSigned, data); } #[no_mangle] pub extern "C" fn msg_commitment_signed_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::CommitmentSigned, data); + test_msg_simple!(msgs::CommitmentSigned, data); } diff --git a/fuzz/src/msg_targets/msg_funding_created.rs b/fuzz/src/msg_targets/msg_funding_created.rs index 3b61a64bf6a..e0005cb082a 100644 --- a/fuzz/src/msg_targets/msg_funding_created.rs +++ b/fuzz/src/msg_targets/msg_funding_created.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_funding_created_test(data: &[u8], _out: Out) { - test_msg!(msgs::FundingCreated, data); + test_msg_simple!(msgs::FundingCreated, data); } #[no_mangle] pub extern "C" fn msg_funding_created_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::FundingCreated, data); + test_msg_simple!(msgs::FundingCreated, data); } diff --git a/fuzz/src/msg_targets/msg_funding_locked.rs b/fuzz/src/msg_targets/msg_funding_locked.rs index 677af731895..2c6ad63c963 100644 --- a/fuzz/src/msg_targets/msg_funding_locked.rs +++ b/fuzz/src/msg_targets/msg_funding_locked.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_funding_locked_test(data: &[u8], _out: Out) { - test_msg!(msgs::FundingLocked, data); + test_msg_simple!(msgs::FundingLocked, data); } #[no_mangle] pub extern "C" fn msg_funding_locked_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::FundingLocked, data); + test_msg_simple!(msgs::FundingLocked, data); } diff --git a/fuzz/src/msg_targets/msg_funding_signed.rs b/fuzz/src/msg_targets/msg_funding_signed.rs index 388738ff2d4..f0586e7b294 100644 --- a/fuzz/src/msg_targets/msg_funding_signed.rs +++ b/fuzz/src/msg_targets/msg_funding_signed.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_funding_signed_test(data: &[u8], _out: Out) { - test_msg!(msgs::FundingSigned, data); + test_msg_simple!(msgs::FundingSigned, data); } #[no_mangle] pub extern "C" fn msg_funding_signed_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::FundingSigned, data); + test_msg_simple!(msgs::FundingSigned, data); } diff --git a/fuzz/src/msg_targets/msg_gossip_timestamp_filter.rs b/fuzz/src/msg_targets/msg_gossip_timestamp_filter.rs index 73999cd3a86..448aaffc90c 100644 --- a/fuzz/src/msg_targets/msg_gossip_timestamp_filter.rs +++ b/fuzz/src/msg_targets/msg_gossip_timestamp_filter.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_gossip_timestamp_filter_test(data: &[u8], _out: Out) { - test_msg!(msgs::GossipTimestampFilter, data); + test_msg_simple!(msgs::GossipTimestampFilter, data); } #[no_mangle] pub extern "C" fn msg_gossip_timestamp_filter_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::GossipTimestampFilter, data); + test_msg_simple!(msgs::GossipTimestampFilter, data); } diff --git a/fuzz/src/msg_targets/msg_open_channel.rs b/fuzz/src/msg_targets/msg_open_channel.rs index b0e96734e4c..ce637c167ef 100644 --- a/fuzz/src/msg_targets/msg_open_channel.rs +++ b/fuzz/src/msg_targets/msg_open_channel.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_open_channel_test(data: &[u8], _out: Out) { - test_msg!(msgs::OpenChannel, data); + test_msg_simple!(msgs::OpenChannel, data); } #[no_mangle] pub extern "C" fn msg_open_channel_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::OpenChannel, data); + test_msg_simple!(msgs::OpenChannel, data); } diff --git a/fuzz/src/msg_targets/msg_query_channel_range.rs b/fuzz/src/msg_targets/msg_query_channel_range.rs index bd16147f641..4b3de6aa895 100644 --- a/fuzz/src/msg_targets/msg_query_channel_range.rs +++ b/fuzz/src/msg_targets/msg_query_channel_range.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_query_channel_range_test(data: &[u8], _out: Out) { - test_msg!(msgs::QueryChannelRange, data); + test_msg_simple!(msgs::QueryChannelRange, data); } #[no_mangle] pub extern "C" fn msg_query_channel_range_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::QueryChannelRange, data); + test_msg_simple!(msgs::QueryChannelRange, data); } diff --git a/fuzz/src/msg_targets/msg_reply_short_channel_ids_end.rs b/fuzz/src/msg_targets/msg_reply_short_channel_ids_end.rs index fcd13154370..7634329a435 100644 --- a/fuzz/src/msg_targets/msg_reply_short_channel_ids_end.rs +++ b/fuzz/src/msg_targets/msg_reply_short_channel_ids_end.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_reply_short_channel_ids_end_test(data: &[u8], _out: Out) { - test_msg!(msgs::ReplyShortChannelIdsEnd, data); + test_msg_simple!(msgs::ReplyShortChannelIdsEnd, data); } #[no_mangle] pub extern "C" fn msg_reply_short_channel_ids_end_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::ReplyShortChannelIdsEnd, data); + test_msg_simple!(msgs::ReplyShortChannelIdsEnd, data); } diff --git a/fuzz/src/msg_targets/msg_revoke_and_ack.rs b/fuzz/src/msg_targets/msg_revoke_and_ack.rs index 41f40becdfd..873939ca7c9 100644 --- a/fuzz/src/msg_targets/msg_revoke_and_ack.rs +++ b/fuzz/src/msg_targets/msg_revoke_and_ack.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_revoke_and_ack_test(data: &[u8], _out: Out) { - test_msg!(msgs::RevokeAndACK, data); + test_msg_simple!(msgs::RevokeAndACK, data); } #[no_mangle] pub extern "C" fn msg_revoke_and_ack_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::RevokeAndACK, data); + test_msg_simple!(msgs::RevokeAndACK, data); } diff --git a/fuzz/src/msg_targets/msg_shutdown.rs b/fuzz/src/msg_targets/msg_shutdown.rs index 3e5358205cd..e6e74cc661a 100644 --- a/fuzz/src/msg_targets/msg_shutdown.rs +++ b/fuzz/src/msg_targets/msg_shutdown.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_shutdown_test(data: &[u8], _out: Out) { - test_msg!(msgs::Shutdown, data); + test_msg_simple!(msgs::Shutdown, data); } #[no_mangle] pub extern "C" fn msg_shutdown_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::Shutdown, data); + test_msg_simple!(msgs::Shutdown, data); } diff --git a/fuzz/src/msg_targets/msg_update_add_htlc.rs b/fuzz/src/msg_targets/msg_update_add_htlc.rs index d45eeadb194..409f0fac8df 100644 --- a/fuzz/src/msg_targets/msg_update_add_htlc.rs +++ b/fuzz/src/msg_targets/msg_update_add_htlc.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_update_add_htlc_test(data: &[u8], _out: Out) { - test_msg_hole!(msgs::UpdateAddHTLC, data, 85, 33); + test_msg_simple!(msgs::UpdateAddHTLC, data); } #[no_mangle] pub extern "C" fn msg_update_add_htlc_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg_hole!(msgs::UpdateAddHTLC, data, 85, 33); + test_msg_simple!(msgs::UpdateAddHTLC, data); } diff --git a/fuzz/src/msg_targets/msg_update_fail_htlc.rs b/fuzz/src/msg_targets/msg_update_fail_htlc.rs index 8fadde21030..12d3f1c3fc3 100644 --- a/fuzz/src/msg_targets/msg_update_fail_htlc.rs +++ b/fuzz/src/msg_targets/msg_update_fail_htlc.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_update_fail_htlc_test(data: &[u8], _out: Out) { - test_msg!(msgs::UpdateFailHTLC, data); + test_msg_simple!(msgs::UpdateFailHTLC, data); } #[no_mangle] pub extern "C" fn msg_update_fail_htlc_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::UpdateFailHTLC, data); + test_msg_simple!(msgs::UpdateFailHTLC, data); } diff --git a/fuzz/src/msg_targets/msg_update_fail_malformed_htlc.rs b/fuzz/src/msg_targets/msg_update_fail_malformed_htlc.rs index 9d0481481bc..0b36070d1e5 100644 --- a/fuzz/src/msg_targets/msg_update_fail_malformed_htlc.rs +++ b/fuzz/src/msg_targets/msg_update_fail_malformed_htlc.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_update_fail_malformed_htlc_test(data: &[u8], _out: Out) { - test_msg!(msgs::UpdateFailMalformedHTLC, data); + test_msg_simple!(msgs::UpdateFailMalformedHTLC, data); } #[no_mangle] pub extern "C" fn msg_update_fail_malformed_htlc_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::UpdateFailMalformedHTLC, data); + test_msg_simple!(msgs::UpdateFailMalformedHTLC, data); } diff --git a/fuzz/src/msg_targets/msg_update_fee.rs b/fuzz/src/msg_targets/msg_update_fee.rs index 318f5b089eb..3c23a902f57 100644 --- a/fuzz/src/msg_targets/msg_update_fee.rs +++ b/fuzz/src/msg_targets/msg_update_fee.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_update_fee_test(data: &[u8], _out: Out) { - test_msg!(msgs::UpdateFee, data); + test_msg_simple!(msgs::UpdateFee, data); } #[no_mangle] pub extern "C" fn msg_update_fee_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::UpdateFee, data); + test_msg_simple!(msgs::UpdateFee, data); } diff --git a/fuzz/src/msg_targets/msg_update_fulfill_htlc.rs b/fuzz/src/msg_targets/msg_update_fulfill_htlc.rs index 692cb972f78..460ff0e1678 100644 --- a/fuzz/src/msg_targets/msg_update_fulfill_htlc.rs +++ b/fuzz/src/msg_targets/msg_update_fulfill_htlc.rs @@ -17,11 +17,11 @@ use utils::test_logger; #[inline] pub fn msg_update_fulfill_htlc_test(data: &[u8], _out: Out) { - test_msg!(msgs::UpdateFulfillHTLC, data); + test_msg_simple!(msgs::UpdateFulfillHTLC, data); } #[no_mangle] pub extern "C" fn msg_update_fulfill_htlc_run(data: *const u8, datalen: usize) { let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg!(msgs::UpdateFulfillHTLC, data); + test_msg_simple!(msgs::UpdateFulfillHTLC, data); } From f60da31e56cab7713119be1658d4204b054a30d2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 29 Aug 2021 02:55:39 +0000 Subject: [PATCH 3/4] Add forward-compat due serialization variants of HTLCFailureMsg Going forward, all lightning messages have a TLV stream suffix, allowing new fields to be added as needed. In the P2P protocol, messages have an explicit length, so there is no implied length in the TLV stream itself. HTLCFailureMsg enum variants have messages in them, but without a size prefix or any explicit end. Thus, if a HTLCFailureMsg is read as a part of a ChannelManager, with a TLV stream at the end, there is no way to differentiate between the end of the message and the next field(s) in the ChannelManager. Here we add two new variant values for HTLCFailureMsg variants in the read path, allowing us to switch to the new values if/when we add new TLV fields in UpdateFailHTLC or UpdateFailMalformedHTLC so that older versions can still read the new TLV fields. --- lightning/src/ln/channelmanager.rs | 74 ++++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index ac2297f86ae..6b9b20f5006 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -55,7 +55,7 @@ use chain::keysinterface::{Sign, KeysInterface, KeysManager, InMemorySigner}; use util::config::UserConfig; use util::events::{EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider}; use util::{byte_utils, events}; -use util::ser::{Readable, ReadableArgs, MaybeReadable, Writeable, Writer}; +use util::ser::{BigSize, FixedLengthReader, Readable, ReadableArgs, MaybeReadable, Writeable, Writer}; use util::chacha20::{ChaCha20, ChaChaReader}; use util::logger::{Logger, Level}; use util::errors::APIError; @@ -4841,10 +4841,74 @@ impl_writeable_tlv_based!(PendingHTLCInfo, { (8, outgoing_cltv_value, required) }); -impl_writeable_tlv_based_enum!(HTLCFailureMsg, ; - (0, Relay), - (1, Malformed), -); + +impl Writeable for HTLCFailureMsg { + fn write(&self, writer: &mut W) -> Result<(), io::Error> { + match self { + HTLCFailureMsg::Relay(msgs::UpdateFailHTLC { channel_id, htlc_id, reason }) => { + 0u8.write(writer)?; + channel_id.write(writer)?; + htlc_id.write(writer)?; + reason.write(writer)?; + }, + HTLCFailureMsg::Malformed(msgs::UpdateFailMalformedHTLC { + channel_id, htlc_id, sha256_of_onion, failure_code + }) => { + 1u8.write(writer)?; + channel_id.write(writer)?; + htlc_id.write(writer)?; + sha256_of_onion.write(writer)?; + failure_code.write(writer)?; + }, + } + Ok(()) + } +} + +impl Readable for HTLCFailureMsg { + fn read(reader: &mut R) -> Result { + let id: u8 = Readable::read(reader)?; + match id { + 0 => { + Ok(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC { + channel_id: Readable::read(reader)?, + htlc_id: Readable::read(reader)?, + reason: Readable::read(reader)?, + })) + }, + 1 => { + Ok(HTLCFailureMsg::Malformed(msgs::UpdateFailMalformedHTLC { + channel_id: Readable::read(reader)?, + htlc_id: Readable::read(reader)?, + sha256_of_onion: Readable::read(reader)?, + failure_code: Readable::read(reader)?, + })) + }, + // In versions prior to 0.0.101, HTLCFailureMsg objects were written with type 0 or 1 but + // weren't length-prefixed and thus didn't support reading the TLV stream suffix of the network + // messages contained in the variants. + // In version 0.0.101, support for reading the variants with these types was added, and + // we should migrate to writing these variants when UpdateFailHTLC or + // UpdateFailMalformedHTLC get TLV fields. + 2 => { + let length: BigSize = Readable::read(reader)?; + let mut s = FixedLengthReader::new(reader, length.0); + let res = Readable::read(&mut s)?; + s.eat_remaining()?; // Return ShortRead if there's actually not enough bytes + Ok(HTLCFailureMsg::Relay(res)) + }, + 3 => { + let length: BigSize = Readable::read(reader)?; + let mut s = FixedLengthReader::new(reader, length.0); + let res = Readable::read(&mut s)?; + s.eat_remaining()?; // Return ShortRead if there's actually not enough bytes + Ok(HTLCFailureMsg::Malformed(res)) + }, + _ => Err(DecodeError::UnknownRequiredFeature), + } + } +} + impl_writeable_tlv_based_enum!(PendingHTLCStatus, ; (0, Forward), (1, Fail), From 997dc5f5f30924c53c5238c256a4d4df22dbae6d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 29 Aug 2021 06:03:41 +0000 Subject: [PATCH 4/4] Convert most P2P msg serialization to a new macro with TLV suffixes The network serialization format for all messages was changed some time ago to include a TLV suffix for all messages, however we never bothered to implement it as there isn't a lot of use validating a TLV stream with nothing to do with it. However, messages are increasingly utilizing the TLV suffix feature, and there are some compatibility concerns with messages written as a part of other structs having their format changed (see previous commit). Thus, here we go ahead and convert most message serialization to a new macro which includes a TLV suffix after a series of fields, simplifying several serialization implementations in the process. --- lightning/src/ln/msgs.rs | 167 ++++++++++--------------------- lightning/src/util/ser_macros.rs | 23 +++++ 2 files changed, 76 insertions(+), 114 deletions(-) diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 34ccc8adf7a..4517afb2230 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -1053,7 +1053,7 @@ impl Readable for OptionalField { } -impl_writeable!(AcceptChannel, { +impl_writeable_msg!(AcceptChannel, { temporary_channel_id, dust_limit_satoshis, max_htlc_value_in_flight_msat, @@ -1069,14 +1069,14 @@ impl_writeable!(AcceptChannel, { htlc_basepoint, first_per_commitment_point, shutdown_scriptpubkey -}); +}, {}); -impl_writeable!(AnnouncementSignatures, { +impl_writeable_msg!(AnnouncementSignatures, { channel_id, short_channel_id, node_signature, bitcoin_signature -}); +}, {}); impl Writeable for ChannelReestablish { fn write(&self, w: &mut W) -> Result<(), io::Error> { @@ -1115,41 +1115,21 @@ impl Readable for ChannelReestablish{ } } -impl Writeable for ClosingSigned { - fn write(&self, w: &mut W) -> Result<(), io::Error> { - self.channel_id.write(w)?; - self.fee_satoshis.write(w)?; - self.signature.write(w)?; - encode_tlv_stream!(w, { - (1, self.fee_range, option), - }); - Ok(()) - } -} - -impl Readable for ClosingSigned { - fn read(r: &mut R) -> Result { - let channel_id = Readable::read(r)?; - let fee_satoshis = Readable::read(r)?; - let signature = Readable::read(r)?; - let mut fee_range = None; - decode_tlv_stream!(r, { - (1, fee_range, option), - }); - Ok(Self { channel_id, fee_satoshis, signature, fee_range }) - } -} +impl_writeable_msg!(ClosingSigned, + { channel_id, fee_satoshis, signature }, + { (1, fee_range, option) } +); impl_writeable!(ClosingSignedFeeRange, { min_fee_satoshis, max_fee_satoshis }); -impl_writeable!(CommitmentSigned, { +impl_writeable_msg!(CommitmentSigned, { channel_id, signature, htlc_signatures -}); +}, {}); impl_writeable!(DecodedOnionErrorPacket, { hmac, @@ -1157,22 +1137,22 @@ impl_writeable!(DecodedOnionErrorPacket, { pad }); -impl_writeable!(FundingCreated, { +impl_writeable_msg!(FundingCreated, { temporary_channel_id, funding_txid, funding_output_index, signature -}); +}, {}); -impl_writeable!(FundingSigned, { +impl_writeable_msg!(FundingSigned, { channel_id, signature -}); +}, {}); -impl_writeable!(FundingLocked, { +impl_writeable_msg!(FundingLocked, { channel_id, - next_per_commitment_point -}); + next_per_commitment_point, +}, {}); impl Writeable for Init { fn write(&self, w: &mut W) -> Result<(), io::Error> { @@ -1193,7 +1173,7 @@ impl Readable for Init { } } -impl_writeable!(OpenChannel, { +impl_writeable_msg!(OpenChannel, { chain_hash, temporary_channel_id, funding_satoshis, @@ -1213,47 +1193,53 @@ impl_writeable!(OpenChannel, { first_per_commitment_point, channel_flags, shutdown_scriptpubkey -}); +}, {}); -impl_writeable!(RevokeAndACK, { +impl_writeable_msg!(RevokeAndACK, { channel_id, per_commitment_secret, next_per_commitment_point -}); +}, {}); -impl_writeable!(Shutdown, { +impl_writeable_msg!(Shutdown, { channel_id, scriptpubkey -}); +}, {}); -impl_writeable!(UpdateFailHTLC, { +impl_writeable_msg!(UpdateFailHTLC, { channel_id, htlc_id, reason -}); +}, {}); -impl_writeable!(UpdateFailMalformedHTLC, { +impl_writeable_msg!(UpdateFailMalformedHTLC, { channel_id, htlc_id, sha256_of_onion, failure_code -}); +}, {}); -impl_writeable!(UpdateFee, { +impl_writeable_msg!(UpdateFee, { channel_id, feerate_per_kw -}); +}, {}); -impl_writeable!(UpdateFulfillHTLC, { +impl_writeable_msg!(UpdateFulfillHTLC, { channel_id, htlc_id, payment_preimage -}); +}, {}); +// Note that this is written as a part of ChannelManager objects, and thus cannot change its +// serialization format in a way which assumes we know the total serialized length/message end +// position. impl_writeable!(OnionErrorPacket, { data }); +// Note that this is written as a part of ChannelManager objects, and thus cannot change its +// serialization format in a way which assumes we know the total serialized length/message end +// position. impl Writeable for OnionPacket { fn write(&self, w: &mut W) -> Result<(), io::Error> { self.version.write(w)?; @@ -1282,14 +1268,14 @@ impl Readable for OnionPacket { } } -impl_writeable!(UpdateAddHTLC, { +impl_writeable_msg!(UpdateAddHTLC, { channel_id, htlc_id, amount_msat, payment_hash, cltv_expiry, onion_routing_packet -}); +}, {}); impl Writeable for FinalOnionHopData { fn write(&self, w: &mut W) -> Result<(), io::Error> { @@ -1700,24 +1686,10 @@ impl Writeable for QueryShortChannelIds { } } -impl Readable for ReplyShortChannelIdsEnd { - fn read(r: &mut R) -> Result { - let chain_hash: BlockHash = Readable::read(r)?; - let full_information: bool = Readable::read(r)?; - Ok(ReplyShortChannelIdsEnd { - chain_hash, - full_information, - }) - } -} - -impl Writeable for ReplyShortChannelIdsEnd { - fn write(&self, w: &mut W) -> Result<(), io::Error> { - self.chain_hash.write(w)?; - self.full_information.write(w)?; - Ok(()) - } -} +impl_writeable_msg!(ReplyShortChannelIdsEnd, { + chain_hash, + full_information, +}, {}); impl QueryChannelRange { /** @@ -1732,27 +1704,11 @@ impl QueryChannelRange { } } -impl Readable for QueryChannelRange { - fn read(r: &mut R) -> Result { - let chain_hash: BlockHash = Readable::read(r)?; - let first_blocknum: u32 = Readable::read(r)?; - let number_of_blocks: u32 = Readable::read(r)?; - Ok(QueryChannelRange { - chain_hash, - first_blocknum, - number_of_blocks - }) - } -} - -impl Writeable for QueryChannelRange { - fn write(&self, w: &mut W) -> Result<(), io::Error> { - self.chain_hash.write(w)?; - self.first_blocknum.write(w)?; - self.number_of_blocks.write(w)?; - Ok(()) - } -} +impl_writeable_msg!(QueryChannelRange, { + chain_hash, + first_blocknum, + number_of_blocks +}, {}); impl Readable for ReplyChannelRange { fn read(r: &mut R) -> Result { @@ -1812,28 +1768,11 @@ impl Writeable for ReplyChannelRange { } } -impl Readable for GossipTimestampFilter { - fn read(r: &mut R) -> Result { - let chain_hash: BlockHash = Readable::read(r)?; - let first_timestamp: u32 = Readable::read(r)?; - let timestamp_range: u32 = Readable::read(r)?; - Ok(GossipTimestampFilter { - chain_hash, - first_timestamp, - timestamp_range, - }) - } -} - -impl Writeable for GossipTimestampFilter { - fn write(&self, w: &mut W) -> Result<(), io::Error> { - self.chain_hash.write(w)?; - self.first_timestamp.write(w)?; - self.timestamp_range.write(w)?; - Ok(()) - } -} - +impl_writeable_msg!(GossipTimestampFilter, { + chain_hash, + first_timestamp, + timestamp_range, +}, {}); #[cfg(test)] mod tests { diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index b229383b283..1ddb7e017eb 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -230,6 +230,29 @@ macro_rules! decode_tlv_stream { } } } +macro_rules! impl_writeable_msg { + ($st:ident, {$($field:ident),* $(,)*}, {$(($type: expr, $tlvfield: ident, $fieldty: tt)),* $(,)*}) => { + impl ::util::ser::Writeable for $st { + fn write(&self, w: &mut W) -> Result<(), $crate::io::Error> { + $( self.$field.write(w)?; )* + encode_tlv_stream!(w, {$(($type, self.$tlvfield, $fieldty)),*}); + Ok(()) + } + } + impl ::util::ser::Readable for $st { + fn read(r: &mut R) -> Result { + $(let $field = ::util::ser::Readable::read(r)?;)* + $(init_tlv_field_var!($tlvfield, $fieldty);)* + decode_tlv_stream!(r, {$(($type, $tlvfield, $fieldty)),*}); + Ok(Self { + $($field),*, + $($tlvfield),* + }) + } + } + } +} + macro_rules! impl_writeable { ($st:ident, {$($field:ident),*}) => { impl ::util::ser::Writeable for $st {