Inconsistencies in encoding of LN messages

[dr-orlovsky]

Lightning encoding was a trait introduced to simplify implementation for encoding different values. However, because of internal inconsistencies in how BOLTs specify encoding of the same data in different cases, this is doesn't work as intended. This is a tracking issue for all problems caused by this inconsistency. The goal is to try to "save" lightning encoding trait and derive macros, but solve all possible inconsistencies ad-hoc on a higher type level.

• features flag constituting channel_types in open_channel message TLV are encoded using BigSize TLV length field, while the same feature flags in the global/local features of init message are encoded with u16 length value (fixed in Fix ChannelType and Script decodings in TLVs #30)
• scripts in TLVs are encoded without size prefix (fixed in Fix ChannelType and Script decodings in TLVs #30)

CC @crisdut

[crisdut]

Hi @dr-orlovsky,

I found another inconsistency in lightning_enconding.

The error occurs when we try to encode/decode the commit_sig message. The reason is we used usize instead of u16 to store the size of the vector (see the num_htlcs field in the test below), as documented in the bolt here.

You can use the test below to check the error:

#[test]
fn real_cln_commit_sig_message() {
    // Real commit_sig message sent by clightning
    let msg_recv = [
        // 1. type: 132 (commitment_signed)
        0, 132, 
        // 2. data
        // [channel_id:channel_id]
        144, 126, 57, 252, 212, 105, 195, 245, 167, 53, 85, 84, 78, 225, 18,
        39, 66, 215, 204, 172, 103, 191, 57, 179, 110, 95, 0, 103, 121,
        183, 137, 229,
        // [signature:signature]
        45, 20, 246, 116, 50, 191, 19, 49, 213, 193, 96,
        112, 87, 119, 25, 144, 108, 227, 41, 9, 170, 112, 127, 122, 35,
        251, 79, 49, 39, 185, 230, 77, 55, 70, 213, 71, 199, 35, 196, 15,
        62, 241, 71, 186, 102, 233, 220, 160, 174, 227, 46, 111, 51, 96,
        161, 252, 178, 87, 108, 131, 219, 44, 17, 207,
        // [u16:num_htlcs]
        0, 1,
        // [num_htlcs*signature:htlc_signature]
        29, 140, 31, 126, 37, 240, 28, 159, 160, 6, 50, 83, 231, 11, 215,
        187, 15, 49, 69, 199, 108, 193, 192, 5, 53, 73, 209, 207, 189, 133,
        244, 150, 44, 56, 239, 131, 248, 117, 42, 0, 211, 250, 33, 3, 102,
        156, 250, 137, 97, 142, 135, 174, 39, 31, 114, 193, 18, 91, 209,
        139, 130, 62, 246, 230,
    ];

    Messages::lightning_deserialize(msg_recv).unwrap();
}

[dr-orlovsky]

Random use of CompactSize integers and u16 in BOLTs will one day drive me crazy. Thank you for spotting this out.

In v0.10 this all will be fixed with the new confined array types from the new amplify crate (the way we already did it with strict encoding v2.0 in RGB), but I need to think about the best strategy for v0.9 here, since I expect this will not be the last case of this problem.

[crisdut]

Random use of CompactSize integers and u16 in BOLTs will one day drive me crazy. Thank you for spotting this out.

Yes, I understand.

In v0.10 this all will be fixed with the new confined array types from the new amplify crate (the way we already did it with strict encoding v2.0 in RGB), but I need to think about the best strategy for v0.9 here, since I expect this will not be the last case of this problem.

For now, we can handle case by case basis (similar to the ChannelType case).

Also, I started publishing the small PRs related to interop with LN implementations. My goal is to complete the basic operations of the BOLTs (open, pay, forward, and close) and fix and/or map related issues. I think is good idea add taproot support still in version v0.9.x.

What do you think?

BTW, after we finish the review and fix the issues of the new RGB version, we can start re-writing LNP-Node with rust-netservices, bump dependencies amplify and others.