smite: add commitment_signed message codec implementation

[kartikangiras]

Add support for commitment-signed codec for BOLT 2 message.

[kartikangiras]

Hi @morehouse , Could you PTAL into this PR, it is the part of milestone 2 adding support for commitment-signed codec (channel-ready is implemented in other PR by another contributor). Thanks.

[morehouse]

Thanks for working on this. Please make sure CI passes before requesting another review. You should be able to run all the CI commands locally (e.g., cargo test, cargo fmt, cargo clippy --all-targets --all-features -- -D warnings)

[morehouse]

In this file we have been keeping the messages sorted by type number. Please reorder accordingly.

[morehouse]

You still haven't addressed the message order here or below.

[morehouse]

We should panic if this cast would overflow, similar to the other codecs. I'd suggesting adding an assert to the beginning of encode:

assert!(self.htlc_signatures.len() <= u16::MAX as usize, "too many HTLC signatures");

[ekzyis]

@morehouse I was wondering if making WireFormat for Vec<u8> generic would be useful. This passes all existing tests:

patch

diff --git a/smite/src/bolt/wire.rs b/smite/src/bolt/wire.rs
index ebfef88..513389e 100644
--- a/smite/src/bolt/wire.rs
+++ b/smite/src/bolt/wire.rs
@@ -146,7 +146,7 @@ impl WireFormat for BigSize {
     }
 }

-impl WireFormat for Vec<u8> {
+impl<T: WireFormat> WireFormat for Vec<T> {
     /// Reads a `[u16:len][len*byte]` variable-length field, advancing past both.
     ///
     /// # Errors
@@ -160,16 +160,20 @@ impl WireFormat for Vec<u8> {
                 actual: data.len(),
             });
         }
-        let bytes = data[..len].to_vec();
-        *data = &data[len..];
-        Ok(bytes)
+        let mut vec = Vec::with_capacity(len);
+        for _ in 0..len {
+            vec.push(T::read(data)?);
+        }
+        Ok(vec)
     }

     /// Writes a `[u16:len][len*byte]` variable-length field.
     #[allow(clippy::cast_possible_truncation)]
     fn write(&self, out: &mut Vec<u8>) {
         (self.len() as u16).write(out);
-        out.extend_from_slice(self);
+        for item in self {
+            item.write(out);
+        }
     }
 }

However, I searched for num_ in bolt02 and only found witnesses in tx_signatures as another case where this could be useful.

So I think the generic interface would be overkill. Writing tests for it also feels arbitrary because of the decision for which types to write tests. I wrote some for Vec<Signature>:

tests

diff --git a/smite/src/bolt/wire.rs b/smite/src/bolt/wire.rs
index 513389e..a3dabf9 100644
--- a/smite/src/bolt/wire.rs
+++ b/smite/src/bolt/wire.rs
@@ -542,6 +542,34 @@ mod tests {
         assert!(cursor.is_empty());
     }

+    fn sample_signatures() -> Vec<Signature> {
+        let secp = Secp256k1::new();
+        let sk = SecretKey::from_byte_array([0x12; 32]).unwrap();
+        vec![
+            secp.sign_ecdsa(secp256k1::Message::from_digest([0x00; 32]), &sk),
+            secp.sign_ecdsa(secp256k1::Message::from_digest([0xAA; 32]), &sk),
+        ]
+    }
+
+    #[test]
+    fn vec_signature_read_empty_field() {
+        let mut data: &[u8] = &[0x00, 0x00];
+        let result = Vec::<Signature>::read(&mut data).unwrap();
+        assert_eq!(result, Vec::<Signature>::new());
+        assert!(data.is_empty());
+    }
+
+    #[test]
+    fn vec_signature_write_roundtrip() {
+        let signatures = sample_signatures();
+        let mut buf = Vec::new();
+        signatures.write(&mut buf);
+        let mut data: &[u8] = &buf;
+        let result = Vec::<Signature>::read(&mut data).unwrap();
+        assert_eq!(result, signatures);
+        assert!(data.is_empty());
+    }
+
     // Test vectors from BOLT 1 Appendix A
     // https://github.com/lightning/bolts/blob/master/01-messaging.md#appendix-a-bigsize-test-vectors

What do you think? Not needed?

[morehouse]

I'm afraid this might make the Vec<u8> read/write much slower, since they would be copying one byte at a time unless rustc can somehow optimize for this case.

I think for now it probably makes sense to just handle these cases manually. If we start getting lots of repetitive code we can do a refactoring later.

[morehouse]

Please squash into one commit.

[morehouse]

The field comment is missing here.

Suggested change

	pub tlvs: CommitmentSignedTlvs,
	/// Optional TLV extensions
	pub tlvs: CommitmentSignedTlvs,

[morehouse]

A no-op TLV implementation isn't useful. We should implement the funding_txid TLV from the spec.

[morehouse]

I'm afraid this might make the Vec<u8> read/write much slower, since they would be copying one byte at a time unless rustc can somehow optimize for this case.

I think for now it probably makes sense to just handle these cases manually. If we start getting lots of repetitive code we can do a refactoring later.

[morehouse]

You still haven't addressed the message order here or below.

[morehouse]

Suggested change

	/// The channel ID
	/// The channel ID derived from the funding outpoint

[morehouse]

Also can return InvalidSignature and TLV errors.

[morehouse]

Suggested change

	#[allow(clippy::cast_possible_truncation)]
	#[allow(clippy::cast_possible_truncation)] // HTLC signature count checked above

[morehouse]

We should add a decode_unknown_odd_tlv_ignored test.