BIP-322: add clarifications and more test vectors#2136
BIP-322: add clarifications and more test vectors#2136guggero wants to merge 3 commits intobitcoin:masterfrom
Conversation
guggero
force-pushed
the
bip-0322-clarifications
branch
2 times, most recently
from
44335b3 to
6e607be
Compare
murchandamus
left a comment
murchandamus
left a comment
There was a problem hiding this comment.
cc: @kallewoof for sign-off
murchandamus
added
Proposed BIP modification
Pending acceptance
guggero
force-pushed
the
bip-0322-clarifications
branch
2 times, most recently
from
1131532 to
2b051e2
Compare
|
I think we might be able to change the test vectors to just a single signature if we get clarity on bitcoin/bitcoin#24058 (comment). I suspect that |
kallewoof
left a comment
kallewoof
left a comment
There was a problem hiding this comment.
LGTM. Two nits, feel free to disregard.
bip-0322.mediawiki
Outdated
| ! Signature format | ||
| |- | ||
| | Legacy | ||
| | <code>P2PKH, P2SH-P2WPKH, P2WPKH</code> |
There was a problem hiding this comment.
This should only list P2PKH, I think. See "Legacy" section below in which it states
The legacy format MAY be used, but must be restricted to the legacy P2PKH invoice address format.
There was a problem hiding this comment.
Yes, I see what you mean. My intention was to show what's possible with the different variants. But we should discourage using the legacy format, I agree. I added a clarification hint, I hope that makes it clearer and also more readable.
bip-0322/generated-test-vectors.json
Outdated
| @@ -0,0 +1,181 @@ | |||
| { | |||
| "tx_hashes": null, | |||
There was a problem hiding this comment.
Can this be removed since it's null? Or is this here to demonstrate that the tx hash is not forgotten but intentionally not set?
There was a problem hiding this comment.
Just an artifact of the JSON serialization. Removed.
guggero
force-pushed
the
bip-0322-clarifications
branch
from
2b051e2 to
df702f7
Compare
This commit adds a table that clarifies what script types are compatible with what signing variant and also makes more clear what the exact format for the signatures of the different variants are.
Before this commit it was not clear that non-native SegWit outputs (e.g. P2PKH or P2SH-P2WPKH) only work if the correct scriptSig is provided. This then also makes it more clear why P2SH-P2WPKH outputs are NOT supported by the "simple" variant.
This commit turns the existing test vectors into a JSON and then adds more test cases covering the most common script types.
guggero
force-pushed
the
bip-0322-clarifications
branch
from
df702f7 to
3ab70c9
Compare
|
I added some negative test cases as well. |
|
I didn't look at the test cases, but LGTM otherwise. |
3 participants
While implementing a Golang implementation of BIP-322, I noticed a couple of things that weren't super clear.
I also added more test vectors for both the "simple" and "full" variants.