Add test coverage for cc78b77c715d6ef62693d4c1bc7190da990ec0fa 2024-04-test-ser-bug
authorValentine Wallace <vwallace@protonmail.com>
Fri, 29 Mar 2024 21:48:26 +0000 (17:48 -0400)
committerMatt Corallo <git@bluematt.me>
Tue, 9 Apr 2024 15:43:57 +0000 (15:43 +0000)
cc78b77c715d6ef62693d4c1bc7190da990ec0fa fixed an important
downgrade bug, but neglected to add test coverage. Here we recitfy
that by adding a few simple tests of common cases.

Tests heavily tweaked by Matt Corallo <git@bluematt.me>.

lightning/src/util/ser_macros.rs

index df030d0b01eb4452ccf8e7b391e8c738bb9af453..740b7c12561ce8466d1a2e30f56faacc797961ac 100644 (file)
@@ -1148,7 +1148,7 @@ mod tests {
 
        use crate::io::{self, Cursor};
        use crate::ln::msgs::DecodeError;
-       use crate::util::ser::{Writeable, HighZeroBytesDroppedBigSize, VecWriter};
+       use crate::util::ser::{MaybeReadable, Readable, Writeable, HighZeroBytesDroppedBigSize, VecWriter};
        use bitcoin::hashes::hex::FromHex;
        use bitcoin::secp256k1::PublicKey;
 
@@ -1258,6 +1258,131 @@ mod tests {
                } else { panic!(); }
        }
 
+       /// A "V1" enum with only one variant
+       enum InnerEnumV1 {
+               StructVariantA {
+                       field: u32,
+               },
+       }
+
+       impl_writeable_tlv_based_enum_upgradable!(InnerEnumV1,
+               (0, StructVariantA) => {
+                       (0, field, required),
+               },
+       );
+
+       struct OuterStructOptionalEnumV1 {
+               inner_enum: Option<InnerEnumV1>,
+               other_field: u32,
+       }
+
+       impl_writeable_tlv_based!(OuterStructOptionalEnumV1, {
+               (0, inner_enum, upgradable_option),
+               (2, other_field, required),
+       });
+
+       /// An upgraded version of [`InnerEnumV1`] that added a second variant
+       enum InnerEnumV2 {
+               StructVariantA {
+                       field: u32,
+               },
+               StructVariantB {
+                       field2: u64,
+               }
+       }
+
+       impl_writeable_tlv_based_enum_upgradable!(InnerEnumV2,
+               (0, StructVariantA) => {
+                       (0, field, required),
+               },
+               (1, StructVariantB) => {
+                       (0, field2, required),
+               },
+       );
+
+       struct OuterStructOptionalEnumV2 {
+               inner_enum: Option<InnerEnumV2>,
+               other_field: u32,
+       }
+
+       impl_writeable_tlv_based!(OuterStructOptionalEnumV2, {
+               (0, inner_enum, upgradable_option),
+               (2, other_field, required),
+       });
+
+       #[test]
+       fn upgradable_enum_option() {
+               // Test downgrading from `OuterStructOptionalEnumV2` to `OuterStructOptionalEnumV1` and
+               // ensure we still read the `other_field` just fine.
+               let serialized_bytes = OuterStructOptionalEnumV2 {
+                       inner_enum: Some(InnerEnumV2::StructVariantB { field2: 64 }),
+                       other_field: 0x1bad1dea,
+               }.encode();
+               let mut s = Cursor::new(serialized_bytes);
+
+               let outer_struct: OuterStructOptionalEnumV1 = Readable::read(&mut s).unwrap();
+               assert!(outer_struct.inner_enum.is_none());
+               assert_eq!(outer_struct.other_field, 0x1bad1dea);
+       }
+
+       /// A struct that is read with an [`InnerEnumV1`] but is written with an [`InnerEnumV2`].
+       struct OuterStructRequiredEnum {
+               #[allow(unused)]
+               inner_enum: InnerEnumV1,
+       }
+
+       impl MaybeReadable for OuterStructRequiredEnum {
+               fn read<R: io::Read>(reader: &mut R) -> Result<Option<Self>, DecodeError> {
+                       let mut inner_enum = crate::util::ser::UpgradableRequired(None);
+                       read_tlv_fields!(reader, {
+                               (0, inner_enum, upgradable_required),
+                       });
+                       Ok(Some(Self {
+                               inner_enum: inner_enum.0.unwrap(),
+                       }))
+               }
+       }
+
+       impl Writeable for OuterStructRequiredEnum {
+               fn write<W: crate::util::ser::Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
+                       write_tlv_fields!(writer, {
+                               (0, InnerEnumV2::StructVariantB { field2: 0xdeadbeef }, required),
+                       });
+                       Ok(())
+               }
+       }
+
+       struct OuterOuterStruct {
+               outer_struct: Option<OuterStructRequiredEnum>,
+               other_field: u32,
+       }
+
+       impl_writeable_tlv_based!(OuterOuterStruct, {
+               (0, outer_struct, upgradable_option),
+               (2, other_field, required),
+       });
+
+
+       #[test]
+       fn upgradable_enum_required() {
+               // Test downgrading from an `OuterOuterStruct` (i.e. test downgrading an
+               // `upgradable_required` `InnerEnumV2` to an `InnerEnumV1`).
+               //
+               // Note that `OuterStructRequiredEnum` has a split write/read implementation that writes an
+               // `InnerEnumV2::StructVariantB` irrespective of the value of `inner_enum`.
+
+               let dummy_inner_enum = InnerEnumV1::StructVariantA { field: 42 };
+               let serialized_bytes = OuterOuterStruct {
+                       outer_struct: Some(OuterStructRequiredEnum { inner_enum: dummy_inner_enum }),
+                       other_field: 0x1bad1dea,
+               }.encode();
+               let mut s = Cursor::new(serialized_bytes);
+
+               let outer_outer_struct: OuterOuterStruct = Readable::read(&mut s).unwrap();
+               assert!(outer_outer_struct.outer_struct.is_none());
+               assert_eq!(outer_outer_struct.other_field, 0x1bad1dea);
+       }
+
        // BOLT TLV test cases
        fn tlv_reader_n1(s: &[u8]) -> Result<(Option<HighZeroBytesDroppedBigSize<u64>>, Option<u64>, Option<(PublicKey, u64, u64)>, Option<u16>), DecodeError> {
                let mut s = Cursor::new(s);