Slight cleanup of signet commitment description #976

instagibbs commented at 8:23 PM on August 21, 2020: member

"either" and "Signet scriptSig header" seem to make no sense here, if other touchups are disagreeable I can drop them.

in bip-0325.mediawiki:29 in 9d2a575548 outdated

  25 | @@ -26,10 +26,10 @@ A new type of test network would be more suitable for integration testing by org
  26 |  
  27 |  A new type of network ("signet"), which takes an additional consensus parameter called the challenge (scriptPubKey). The challenge can be a simple pubkey (P2PKH style), or a k-of-n multisig, or any other script you would want.
  28 |  
  29 | -The witness commitment of the coinbase transaction is extended to include a secondary commitment (the signature/solution) of either:
  30 | +The witness commitment scriptPubKey of the coinbase transaction is extended to include an additional commitment in the optional data portion of the signature/solution for the block:

maflcko commented at 3:05 PM on August 22, 2020:

I think it could make sense to clarify that this is the BIP 141 commitment, and not some other random witness commitment. Also, it could mention that the BIP 141 commitment is required. So I suggest the following replacement

Signet requires all blocks to have a BIP 141 commitment in the coinbase transaction. The BIP 141 commitment's optional data must include an additional commitment of the signature/solution for the block:

instagibbs commented at 5:43 PM on August 22, 2020:

great idea, adding more hooks into bip141 makes this section clearer

maflcko commented at 5:36 PM on August 22, 2020: member

I think the either refers to the fact that one of (scriptSig, scriptWitness) can be empty, but not both.

maflcko commented at 6:03 AM on August 25, 2020: member

Could mention the BIP number in the pull subject line?

Also, cc @kallewoof

kallewoof commented at 6:09 AM on August 25, 2020: contributor

ACK. Thanks for making that more understandable!

Ping @luke-jr.

in bip-0325.mediawiki:29 in eedc08810d outdated

  25 | @@ -26,10 +26,10 @@ A new type of test network would be more suitable for integration testing by org
  26 |  
  27 |  A new type of network ("signet"), which takes an additional consensus parameter called the challenge (scriptPubKey). The challenge can be a simple pubkey (P2PKH style), or a k-of-n multisig, or any other script you would want.
  28 |  
  29 | -The witness commitment of the coinbase transaction is extended to include a secondary commitment (the signature/solution) of either:
  30 | +Signet requires all blocks to have a BIP 141 commitment in the coinbase transaction. The BIP 141 commitment's optional data must include an additional commitment of the signature/solution for the block:

ajtowns commented at 6:41 AM on September 3, 2020:

This seems like it contradicts the text from #983: "There is one other acceptable special case: if a block's challenge is e.g. OP_TRUE (0x51), where an empty solution would result in success, the block is also considered valid if the signet commitment is absent"

kallewoof commented at 7:26 AM on September 3, 2020:

Yeah, good point. @instagibbs there is an exception now for the case where if the challenge is enough to satisfy (e.g. OP_TRUE); in this case, the signet commitment may be omitted entirely.

maflcko commented at 7:46 AM on September 3, 2020:

In which case the BIP 141 commitment isn't required either

ajtowns commented at 3:12 PM on September 8, 2020:

How about:

Signet requires all blocks to have a BIP 141 commitment in the coinbase transaction. In order to provide a solution to the block challenge, the BIP 141 commitment's optional data must include an additional commitment of the solution for the block: [...]. In the special case where an empty solution is valid (ie scriptSig and scriptWitness are both empty) this additional commitment can optionally be left out. This special case is to allow non-signet-aware block generation code to be used to test a custom signet chain where the challenge is OP_TRUE.

instagibbs commented at 3:39 PM on September 8, 2020:

Took a modified version of what you posted, noting that bip141 commitment is only required with non-empty solutions, and that any "trivial" challenge can be used to avoid requiring fancy mining software for non-signature tests.

ajtowns commented at 4:09 PM on September 8, 2020:

I believe the current code requires a bip141 commitment even with empty solutions. Should this be changed?

instagibbs commented at 4:12 PM on September 8, 2020:

you are correct: https://github.com/bitcoin/bitcoin/pull/18267/commits/d0f364c2ddf6b7e7a1d89af18c60de8248ce5ff3#diff-4bdd7c44636dae0429a4d61909eeed17R90

I think for simplicity it's fine to require it. Any objections?

kallewoof commented at 4:26 AM on September 9, 2020:

Simplicity is good. Let's require

in bip-0325.mediawiki:31 in eedc08810d outdated

  25 | @@ -26,10 +26,10 @@ A new type of test network would be more suitable for integration testing by org
  26 |  
  27 |  A new type of network ("signet"), which takes an additional consensus parameter called the challenge (scriptPubKey). The challenge can be a simple pubkey (P2PKH style), or a k-of-n multisig, or any other script you would want.
  28 |  
  29 | -The witness commitment of the coinbase transaction is extended to include a secondary commitment (the signature/solution) of either:
  30 | +Signet requires all blocks to have a BIP 141 commitment in the coinbase transaction. The BIP 141 commitment's optional data must include an additional commitment of the signature/solution for the block:
  31 |  
  32 |      1-4 bytes - Push the following (4 + x + y) bytes

ajtowns commented at 6:44 AM on September 3, 2020:

I believe this should be 1-5 bytes (1 byte if 4+x+y <= 75, 2 bytes for PUSHDATA1 if 4+x+y < 256, 3 bytes for PUSHDATA2 if 4+x+y < 65536, and 5 bytes for PUSHDATA4).

kallewoof commented at 7:34 AM on September 3, 2020:

https://github.com/bitcoin/bips/pull/986

maflcko commented at 5:46 AM on September 8, 2020: member

@instagibbs needs rebase

instagibbs force-pushed on Sep 8, 2020

instagibbs commented at 2:13 PM on September 8, 2020: member

rebased/squashed

Slight cleanup of signet commitment description c8b47a8d48

instagibbs force-pushed on Sep 8, 2020

kallewoof commented at 4:27 AM on September 9, 2020: contributor

ACK, please merge at your convenience, @luke-jr.

in bip-0325.mediawiki:29 in c8b47a8d48 outdated

  25 | @@ -26,13 +26,15 @@ A new type of test network would be more suitable for integration testing by org
  26 |  
  27 |  A new type of network ("signet"), which takes an additional consensus parameter called the challenge (scriptPubKey). The challenge can be a simple pubkey (P2PKH style), or a k-of-n multisig, or any other script you would want.
  28 |  
  29 | -The witness commitment of the coinbase transaction is extended to include a secondary commitment (the signature/solution) of either:
  30 | +In order to provide a non-empty solution to the block challenge the block's BIP 141 commitment's optional data must include an additional commitment of the signature/solution for the block:

maflcko commented at 7:12 AM on September 9, 2020:

Signet requires all blocks to have a BIP 141 commitment in the coinbase transaction. In order to provide a non-empty solution to the block challenge the block's BIP 141 commitment's optional data must include an additional commitment of the signature/solution for the block:

Could keep this for clarification?

instagibbs commented at 1:39 PM on September 9, 2020:

yes it's more correct, so re-adding this. Sorry for back and forth!

maflcko approved

maflcko commented at 7:12 AM on September 9, 2020: member

Better with or without my suggested change

bip141 commitment is required for signet blocks

Co-authored-by: MarcoFalke <falkemarco@gmail.com>

6d7d3011e8

kallewoof commented at 2:04 PM on September 9, 2020: contributor

ACK

maflcko commented at 2:15 PM on September 9, 2020: member

@luke-jr ;)

luke-jr merged this on Oct 5, 2020

luke-jr closed this on Oct 5, 2020