Decode ChannelType causes DataIntegrityError

[crisdut]

Hi @dr-orlovsky,

I tried to decode a ChannelType using lightning_deserialize from message open channel by clightning, but it occurred an error:

Err(DataIntegrityError("can't unmarshall bolt LNP2P message. Details: IO error"))

According to the BOLT-2 spec, this occurs because some situations are not necessary channel_type information in the message.

You can test this by adding the bellow test in bolt2.rs, in lnp-core repo:

#[cfg(test)]
mod test {
    use lightning_encoding::LightningDecode;

    use crate::bolt::Messages;

    #[test]
    fn real_clightning_open_channel() {
        // Real open_channel message sent by clightning
        let msg_recv = [
            0, 32, 6, 34, 110, 70, 17, 26, 11, 89, 202, 175, 18, 96, 67, 235,
            91, 191, 40, 195, 79, 58, 94, 51, 42, 31, 199, 178, 183, 60, 241,
            136, 145, 15, 22, 145, 1, 71, 149, 179, 87, 248, 190, 135, 14, 128,
            6, 231, 127, 76, 45, 137, 175, 179, 41, 39, 181, 150, 135, 218, 40,
            68, 235, 9, 140, 89, 0, 0, 0, 0, 0, 1, 134, 160, 0, 0, 0, 0, 0, 0,
            0, 0, 0, 0, 0, 0, 0, 0, 2, 34, 255, 255, 255, 255, 255, 255, 255,
            255, 0, 0, 0, 0, 0, 0, 3, 232, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
            253, 0, 6, 1, 227, 2, 163, 76, 91, 209, 179, 37, 241, 217, 227,
            190, 56, 5, 236, 7, 160, 138, 88, 114, 175, 48, 21, 46, 14, 153,
            146, 158, 126, 225, 12, 111, 131, 10, 2, 225, 239, 131, 243, 222,
            240, 239, 98, 190, 230, 192, 188, 131, 126, 244, 155, 76, 222, 5,
            101, 185, 160, 228, 83, 23, 84, 249, 194, 79, 163, 177, 186, 3,
            204, 49, 206, 253, 45, 131, 197, 237, 230, 119, 210, 47, 23, 109,
            85, 213, 115, 0, 250, 124, 98, 66, 91, 111, 57, 112, 126, 135, 216,
            176, 85, 129, 3, 187, 13, 129, 200, 13, 124, 92, 181, 198, 96, 147,
            178, 18, 171, 20, 138, 192, 51, 37, 199, 166, 220, 71, 26, 233,
            232, 54, 78, 2, 245, 15, 101, 3, 156, 36, 195, 109, 14, 233, 15,
            49, 144, 81, 57, 24, 18, 235, 59, 209, 131, 185, 165, 48, 1, 223,
            28, 207, 119, 153, 246, 147, 12, 2, 72, 208, 2, 68, 86, 229, 216,
            34, 4, 234, 220, 238, 164, 21, 1, 132, 103, 87, 228, 53, 93, 84,
            217, 197, 134, 141, 149, 32, 34, 25, 136, 118, 190, 172, 126, 1, 0,
            0, 1, 2, 16, 0,
        ];
        let message = Messages::lightning_deserialize(&msg_recv);
        println!("{:?}", message);
        assert_eq!(true, message.is_ok());
    }
}

Expected Result:

Ok(OpenChannel(OpenChannel { chain_hash: Slice32(06226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f), temporary_channel_id: TempChannelId(Slice32(1691014795b357f8be870e8006e77f4c2d89afb32927b59687da2844eb098c59)), funding_satoshis: 100000, push_msat: 0, dust_limit_satoshis: 546, max_htlc_value_in_flight_msat: 18446744073709551615, channel_reserve_satoshis: 1000, htlc_minimum_msat: 0, feerate_per_kw: 253, to_self_delay: 6, max_accepted_htlcs: 483, funding_pubkey: PublicKey(0a836f0ce17e9e92990e2e1530af72588aa007ec0538bee3d9f125b3d15b4ca322a82c74f042f684a14c20d465a1ae310c0fce6ade479d472354d4509500efd6), revocation_basepoint: PublicKey(bab1a34fc2f9541753e4a0b96505de4c9bf47e83bcc0e6be62eff0def383efe1dc2bfcba80ce52711b4fc49dacfed03588975784846285ad4ee75b75868f05d3), payment_point: PublicKey(8155b0d8877e70396f5b42627cfa0073d5556d172fd277e6edc5832dfdce31ccdfb17ff537910b3363e522b120b2595ca29babee106b4d9ae4e0c80440a05529), delayed_payment_basepoint: PublicKey(650ff5024e36e8e91a47dca6c72533c08a14ab12b29360c6b55c7c0dc8810dbb29ac75d2fcb3583059ee0267743df95e4b53ecea2437501990fc65713dd56ddf), htlc_basepoint: PublicKey(d048020c93f69977cf1cdf0130a5b983d13beb1218395190310fe90e6dc3249c5152ba613988416e44fdbcddc4988ce1472422e26f889026dfb87038c28949bf), first_per_commitment_point: PublicKey(7eacbe7688192220958d86c5d9545d35e45767840115a4eedcea0422d8e55644ca3e53ec75512a6b9b1fd5d0e33beaee1ea8ae65485789a25876b372c5ec9620), channel_flags: 1, shutdown_scriptpubkey: Some(PubkeyScript(Script())), channel_type: Some(Basic), unknown_tlvs: Stream({}) }))

PS: I bypassed this issue changing this code to:

impl LightningDecode for usize {
    fn lightning_decode<D: Read>(mut d: D) -> Result<Self, Error> {
        let mut buf = [0u8; 2];
        let _ = d.read_exact(&mut buf);
        Ok(u16::from_be_bytes(buf) as usize)
    }
}

And made other change in ChannelType (lnp-core repo):

I known this not correct approach, but I couldn't solve this problem.

I hope this help.

Thanks

[dr-orlovsky]

So basically the issue is the fact that FlagsVec can be absent and that is not indicated anyhow, right?

[crisdut]

So basically the issue is the fact that FlagsVec can be absent and that is not indicated anyhow, right?

So so, the situation occurs in others parts too.

I was thinking of modifying the ln encode/decode so that it detects when the buffer EOF and ends the operation instead of return a Error. But I'm not sure if this is correct approach.

What's your opnion?

[dr-orlovsky]

For me it looks like the whole Lightning encode traits need a redesign. But I need a bit more time to figure out the best way forward - maybe we can avoid such dramatic changes.

[crisdut]

For me it looks like the whole Lightning encode traits need a redesign. But I need a bit more time to figure out the best way forward - maybe we can avoid such dramatic changes.

Sure.

For now, I will open a PR with some "workarounds" and minor issues.

I have a goal to finish the LN review in end of mounth.

[dr-orlovsky]

Ok, I figured out the source of the issue and a fix: #5

The issue was caused by a TLV with 0 bytes, which must be handled as a default FlagVec. The approach you proposed in the comments is invalid leading to failing parsing length values in all lightning-encoded data inproperly.