Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bip174: Add test vector for global xpub #810

Merged
merged 1 commit into from Sep 20, 2019

Conversation

@NicolasDorier
Copy link
Member

commented Jul 25, 2019

The global xpub by @achow101 is missing test vectors. This has already created a mismatch of implementation between NBitcoin (and thus BTCPay) and Coldcard.

One of our implementation is buggy, this test vector comes from NBitcoin.

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

@peter-conalgo

This comment has been minimized.

Copy link

commented Jul 25, 2019

Coldcard doesn't like the inputs' UTXO's in this file. They don't look like legit transactions.

I would prefer a vector that was based on the other examples already in the BIP. Perhaps add the XPUB data to one of the multisig examples.

Also, pet peeve: there is no way to refer to the various vectors. They need section numbers, or symbolic names or something.

@achow101

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

I will write tests for this and implement serialization into Core.

@achow101

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Bitcoin Core does not like the psbt provided in this PR. Even when using the master branch (which should still be able to deserialize this), it is unable to deserialize it.

edit: You have 0 input txs as UTXOs, that's not allowed.

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

will make a better PSBT (the one that failed on coldcard was a proper mainnet PSBT)

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:bips/global-xpub-test-vector branch from 7b3254e to ca0bcf8 Jul 25, 2019

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

I updated the test vector, it should be correct now. (with bitcoind decoding it correctly)

@@ -587,6 +587,10 @@ must create this PSBT:
* Bytes in Hex: <pre>70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd750000000000ffffffff838d0427d0ec650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d0100000000ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb815e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df5546e8742d1d87008f000000000000000000</pre>
* Base64 String: <pre>cHNidP8BAJoCAAAAAljoeiG1ba8MI76OcHBFbDNvfLqlyHV5JPVFiHuyq911AAAAAAD/////g40EJ9DsZQpoqka7CwmK6kQiwHGyyng1Kgd5WdB86h0BAAAAAP////8CcKrwCAAAAAAWABTYXCtx0AYLCcmIauuBXlCZHdoSTQDh9QUAAAAAFgAUAK6pouXw+HaliN9VRuh0LR2HAI8AAAAAAAAAAAA=</pre>
If global `PSBT_GLOBAL_XPUB` is supported, it might create this PSBT:
* Bytes in Hex: <pre>70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd750000000000ffffffff838d0427d0ec650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d0100000000ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb815e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df5546e8742d1d87008f000000004f01043587cf0000000000000000001b7b7ddcd9b3a9dc2589f063b560519c5ac7624a0c766ae2486d6cef83e267c203dfee30349d7c7cf78c0ec4517bd893028a6c2a880341fc4b98be8a823d3bc52704d90c6a4f0000000000</pre>
* Base64 String: <pre>cHNidP8BAJoCAAAAAljoeiG1ba8MI76OcHBFbDNvfLqlyHV5JPVFiHuyq911AAAAAAD/////g40EJ9DsZQpoqka7CwmK6kQiwHGyyng1Kgd5WdB86h0BAAAAAP////8CcKrwCAAAAAAWABTYXCtx0AYLCcmIauuBXlCZHdoSTQDh9QUAAAAAFgAUAK6pouXw+HaliN9VRuh0LR2HAI8AAAAATwEENYfPAAAAAAAAAAAAG3t93NmzqdwlifBjtWBRnFrHYkoMdmriSG1s74PiZ8ID3+4wNJ18fPeMDsRRe9iTAopsKogDQfxLmL6Kgj07xScE2QxqTwAAAAAA</pre>

This comment has been minimized.

Copy link
@achow101

achow101 Jul 25, 2019

Member

This isn't correct. The xpub in this is useless since all of the keys have hardened steps. This would be better as a serialization test above in the "The following are valid PSBTs" section.

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Jul 26, 2019

Author Member

ok fixing it one moment. This was also not the problem when I tried on mainnet with coldcard.

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:bips/global-xpub-test-vector branch from ca0bcf8 to d832c51 Jul 26, 2019

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

@achow101 fixed, this is normally a correct one (except if my parsing is wrong)

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2019

Please can someone review this?

@luke-jr

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

Only @achow101's ACK/NACK matters here.

@achow101

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

ACK d832c51

@luke-jr luke-jr merged commit 7e9fab1 into bitcoin:master Sep 20, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.