LNP-BP / LNPBPs

LNP/BP standards for bitcoin layer 2 & 3 protocols
https://standards.lnp-bp.org
202 stars 39 forks source link

Procol doc nits and framing issues for LNPBP-2 #41

Closed rajarshimaitra closed 3 years ago

rajarshimaitra commented 4 years ago

The following are some few nits and questions I came up with while reviewing LNPBP-2.

Nits:

Maybe adding the Hmac equation here HMAC_SHA256(SHA256("LNPBP2") || SHA256(<protocol-specific-tag>) || msg, S) can be more expressive.

-

on the forth step of the protocol the tweaking-factor based point F is added to the P (a single original public key), resulting in key T to -> on the fourth step of the protocol the tweaking-factor based point F is added to the P (a single original public key), resulting in key T, i.e. T = P + G * HMAC_SHA256(SHA256("LNPBP2") || SHA256(<protocol-specific-tag>) || msg, S)

-

Constructs and stores an extra-transaction proof (ETP), which structure depends on the generated scriptPubkey type: to -> Constructs and stores an extra-transaction proof (ETP), which is a structure that depends on the generated scriptPubkey type and consists of: a).... b)...

There is also a minor markup formatting issue here, making the resulting paragraph structure bit confusing.

-

The revel protocol is usually run between the committing and verifying parties; however it may be used by the committing party to publicaly revel the proofs of the commitment. These proofs include: to -> The revel protocol is usually run between the committing and verifying parties; however, it may be used by the committing party to publicly revel the proofs of the commitment. These proofs include:

-

The proposed cryptographic commitment scheme is fully compatible with any other LNPBP1-based commitments for the case of P2PK, P2PKH and P2WPH transaction outputs, since they always contain only a single public key and the original to -> The proposed cryptographic commitment scheme is fully compatible with any other LNPBP1-based commitments for the case of P2PK, P2PKH and P2WPKH transaction outputs, since they always contain only a single public key and the original

-

The author does not aware of any P2(W)SH or non-OP_RETURN P2S cryptographic commitment schemes existing before this to -> The author is not aware of any P2(W)SH or non-OP_RETURN P2S cryptographic commitment schemes existing before this

-

Reference implementation

https://github.com/LNP-BP/rust-lnpbp/blob/master/src/cmt/txout.rs

It would be better to give https://github.com/LNP-BP/rust-lnpbp/blob/master/src/bp/dbc/lockscript.rs & https://github.com/LNP-BP/rust-lnpbp/blob/master/src/bp/dbc/keyset.rs as the reference implementation link as this is where the majority of the LNPBP-02 logics are lying.

Questions:

Constructs necessary scripts and generates scriptPubkey of the required type. If OP_RETURN scriptPubkey format is used, it MUST be serialized according to the following rules:

  • only a single OP_RETURN code MUST be present in the scriptPubkey and it MUST be the first byte of it;
    • it must be followed by 32-byte push of the public key value P from the step 2 of the algorithm, serialized according to from [15]; if the resulting public key containing the commitment is non-square, a new public key MUST be picked and procedure from step 4 MUST BE repeated once more.

It would be nice to explain the rationale of requiring squared public key specifically for OP_RETURN type outputs as it's not enforced in any other part of the protocol. In the implementation here https://github.com/LNP-BP/rust-lnpbp/blob/2f6fee732417aad5c71fcd0120ed0db4b1e61061/src/bp/dbc/scriptpubkey.rs#L239 this behavior is not reproduced as the pubkey serialization output is in compressed form.

-

TODO: Schnorr compatibility

If I understand correctly in order to be schnorr compatible we need to enforce all the pubkeys to be squared. do we need any other consideration also?

Code Review

The composition assignment for P2PK and P2PKH are reversed here https://github.com/LNP-BP/rust-lnpbp/blob/2f6fee732417aad5c71fcd0120ed0db4b1e61061/src/bp/dbc/scriptpubkey.rs#L111-L112

-

if the number of original public keys defined at step 1 exceeds one, LNPBP2 tag MUST BE used instead of LNPBP1

This behaviour is not reproduced in the code here https://github.com/LNP-BP/rust-lnpbp/blob/2f6fee732417aad5c71fcd0120ed0db4b1e61061/src/bp/dbc/keyset.rs#L122 The tag is kept as LNPBP-1.

So these are some minor issues I observed in the specification and the implementation and they provide some scope of improvement. If consensus on the suggested changes is achieved I can start making small PRs to fix them.

dr-orlovsky commented 4 years ago

@rajarshimaitra the last two points are very good catch! Fixed in https://github.com/LNP-BP/rust-lnpbp/pull/125/commits/2cad793b7992c114a304df15d08f74b56b51af8f

The rest is fixed with #68