Skip to content

Additional tests for BIP 174#702

Merged
luke-jr merged 2 commits intobitcoin:masterfrom
achow101:bip174-tests
Aug 9, 2018
Merged

Additional tests for BIP 174#702
luke-jr merged 2 commits intobitcoin:masterfrom
achow101:bip174-tests

Conversation

@achow101
Copy link
Copy Markdown
Member

No description provided.

@jb55
Copy link
Copy Markdown

jb55 commented Jul 20, 2018

I was meaning to mention this on the previous PRs. Is there any point to including the base64 encoding as well? The translation from hex is pretty straightforward. Just thinking it would cut down on the verbosity a bit.

@achow101
Copy link
Copy Markdown
Member Author

Is there any point to including the base64 encoding as well? The translation from hex is pretty straightforward.

The point was to effectively provide the two forms that a PSBT would be found in: as a binary file or as a Base64 string. It helps those who are writing tests to know exactly what to expect especially since there are (apparently) multiple ways of doing Base 64.

@jb55
Copy link
Copy Markdown

jb55 commented Jul 20, 2018

there are (apparently) multiple ways of doing Base 64

https://en.wikipedia.org/wiki/Base64#Variants_summary_table

so it seems... seems reasonable to include it in that case, or alternatively an explicit mention of which variant is used.

@achow101
Copy link
Copy Markdown
Member Author

@luke-jr This is ready.

@achow101
Copy link
Copy Markdown
Member Author

achow101 commented Aug 8, 2018

I updated this with some parts that specify what signers should look out for and an algorithm for a simple signer. Also more tests for those.


For a Signer to only produce valid signatures for what it expects to sign, it must check that the following conditions are true:

* If a non-witness UTXO is provided, it's hash must match the hash specified in the prevout
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: "its".

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


for input,i in enumerate(psbt.inputs):
if non_witness_utxo.exists:
assert(sha256d(non_witness_utxo) == prevhash)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

write prevhash as psbt.tx.input[i].prevout.hash?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if non_witness_utxo.exists:
assert(sha256d(non_witness_utxo) == prevhash)
if redeemScript.exists:
assert(non_witness_utxo.scriptPubKey == P2SH(redeemScript))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write as non_witness_utxo.vout[psbt.tx.input[i].prevout.n].scriptPubKey ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

For a Signer to only produce valid signatures for what it expects to sign, it must check that the following conditions are true:

* If a non-witness UTXO is provided, it's hash must match the hash specified in the prevout
* If a witness UTXO is provided, a witness signature is being produced
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps write this as "If a witness UTXO is provided, no non-witness signature may be created"? The current language sounds like a signature must be created.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

sign_non_witness(non_witness_utxo.scriptPubKey)
else if witness_utxo.exists:
if redeemScript.exists:
assert(witness_utxos.scriptPubKey == P2SH(redeemScript))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "witness_utxos".

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

else:
script = witness_utxo.scriptPubKey
if IsP2WPKH(script):
sign_witness(P2PKH(script))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't really convert a script to P2PKH. What about P2PKH(script[2:])?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Test cases for what signers should check for
@luke-jr luke-jr merged commit 68587cf into bitcoin:master Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants