Skip to content
This repository was archived by the owner on Apr 3, 2019. It is now read-only.

Transaction and block version are signed integers#106

Merged
gabegattis merged 1 commit intobitpay:masterfrom
afk11:tx-block-ver-signed
Feb 16, 2017
Merged

Transaction and block version are signed integers#106
gabegattis merged 1 commit intobitpay:masterfrom
afk11:tx-block-ver-signed

Conversation

@afk11
Copy link
Copy Markdown
Contributor

@afk11 afk11 commented Oct 12, 2016

https://en.bitcoin.it/wiki/Protocol_documentation#Block_Headers
https://en.bitcoin.it/wiki/Protocol_documentation#tx

The transaction and block version fields are both int32's (little endian encoded), so technically the range is -2^31 to 2^31 (not zero to 2^32)

@gabegattis
Copy link
Copy Markdown
Contributor

@gabegattis gabegattis added this to the 1.0.0 milestone Oct 12, 2016
@afk11
Copy link
Copy Markdown
Contributor Author

afk11 commented Feb 7, 2017

Merge please?

@afk11 afk11 mentioned this pull request Feb 7, 2017
@kleetus
Copy link
Copy Markdown
Contributor

kleetus commented Feb 15, 2017

The Bitcoin wiki isn't the Bitcoin protocol's specification. Nonetheless, @afk11 is correct based on Bitcoin Core's source to date. Please see:

https://github.com/bitcoin/bitcoin/blob/master/src/primitives/block.h#L24
https://github.com/bitcoin/bitcoin/blob/master/src/primitives/transaction.h#L319

'int32_t' is type defined to 'int' in coda.h in the Linux kernel header files. I think it is safe to assume that the other supported platforms that Bitcoin actively supports with also yield int32_t to a 4 byte length and handle the signed magnitude encoding correctly given the compiler used.

This is probably a breaking api change, but if you have a counter argument as to why it is not, please reply.

@kleetus kleetus self-assigned this Feb 15, 2017
@gabegattis
Copy link
Copy Markdown
Contributor

gabegattis commented Feb 15, 2017

I'm going to pull down the code and do a proper review later today. I think we can get probably get this pulled in by tomorrow.

@kleetus
Copy link
Copy Markdown
Contributor

kleetus commented Feb 15, 2017

Removing the api change label. @gabegattis and I don't think this will break the api in any way.

@gabegattis gabegattis merged commit 2bfb4ce into bitpay:master Feb 16, 2017
@gabegattis
Copy link
Copy Markdown
Contributor

👍

@gabegattis
Copy link
Copy Markdown
Contributor

Thanks @afk11

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants