LNPBP-1 lacks standardization of public key serialization format

[yojoe]

The reference implementation code of LNPBP-1 serializes the aggregated public key S in the commit function, however the standard doesn't mention/define which serialization format must be used.

...
/// Function performs commitment procedure according to LNPBP-1.
pub fn commit(
...
let mut hmac_engine =
        HmacEngine::<sha256::Hash>::new(&pubkey_sum.serialize());
...

[dr-orlovsky]

No, this is not serialization (and just a bad API name for Secp256k1 function). This is initialization of HMAC engine, which is language-specific (since engine itself is independent from key representation)

[yojoe]

I'm not sure. Looking at https://github.com/rust-bitcoin/bitcoin_hashes/blob/c7882c0f75cdaf6c30df0fd1f26f14f1d6b76479/src/hmac.rs#L69 the argument key - to initialize the HmacEngine - is NOT an EC key, but just an arbitrary byte array:

impl<T: HashTrait> HmacEngine<T> {
    /// Construct a new keyed HMAC with the given key. We only support underlying hashes
    /// whose block sizes are ≤ 128 bytes; larger hashes will result in panics.
    pub fn new(key: &[u8]) -> HmacEngine<T> {

So in my opinion it is important which serialization format was used in the commit phase, because you have to use the exact same format in the verify phase.

And in the case of the current LNPBP-1 reference source code, the compressed format is used right now, because secp256k1 has to methods: serialize() and serialize_uncompressed(): https://docs.rs/secp256k1/0.20.1/secp256k1/key/struct.PublicKey.html#method.serialize https://docs.rs/secp256k1/0.20.1/secp256k1/key/struct.PublicKey.html#method.serialize_uncompressed

Therefore, I argue that LNPBP-1 should specify in text (not only in code) that the aggregated public key S must be provided as a byte array in compressed form, according to the rust-secp256k1 implementation of the serialize() function. Else it could result in verification failure if another rgb implementation than the reference implementation is used. In case of the reference implementation this isn't an issue, because verify calls commit, so both use the same serialization. But that doesn't mean other implementations do it the same. To guarantee compatibility, it would be better if the LNPBP-1 spec was clear about which serialization format (e.g. compressed unit8 array) must be used.

[dr-orlovsky]

This is unrelated to serialization of pubic key and is about initializing HMAC engine. Of course constructor requires data for it in some serialized form - but this form is specific for each HMAC engine implementation.

[yojoe]

So you are saying it has no influence on the tweaking_factor whether pubkey_sum.serialize() or pubkey_sum.serialize_uncompressed() is used?

[dr-orlovsky]

You can create HMAC engine implementation in rust which will use unserialized keys.

You are mixing standards / mathematics with rust-specific implementation details. We should not let rust code implementation specifics leak into standard.

Digged into HMAC engine maths. You are right, the questions seems more complex. HMAC engine does not have a concept of elliptic curve keys and operates only hashes of data, and key there is not an EC-key, but a block of data, which size is important. While HMAC key can be of any size, if it is larger than HMAC block it will be simply hashed to fit into block size; if it is smaller - less entropy will be used https://en.wikipedia.org/wiki/HMAC#Definition:

K is the secret key K' is a block-sized key derived from the secret key, K; either by padding to the right with 0s up to the block size, or by hashing down to less than or equal to the block size first and then padding to the right with zeros

So first, we use HMAC-SHA256, which has block size of 64 bytes, thus we do not fill the block with compressed key data and should use uncompressed data instead, so the code should be fixed. Second, as you proposed originally, we need to specify that HMAC engine should be initialized with 64-byte public key representation.

I propose to do that as a separate PR from #107 here, and also another PR to the LNP/BP Core library. Would you like to do (both) of them? If yes, you are welcome, otherwise I will work on them myself.

[dr-orlovsky]

Closed with #109