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

Already on GitHub? Sign in to your account

Documentation omission: compactSize encodings must be canonical #1365

Closed
daira opened this Issue Sep 14, 2016 · 2 comments

Comments

Projects
None yet
2 participants
Contributor

daira commented Sep 14, 2016 edited

The documentation of a compactSize field in the Bitcoin Developer Reference describes four possible encodings based on length. The ranges of integers expressible in each encoding overlap. Therefore it is possible in principle to represent a compactSize with a non-canonical encoding that uses more than the minimum number of bytes.

The actual implementation of ReadCompactSize excludes this possibility. This is also tested. However, it is not documented. Since it affects consensus, it should be documented.

This issue was filed first at bitcoin/bitcoin#8721 and moved here. I believe the assertion by @TheBlueMatt on that issue that this is only a p2p network rule rather than a consensus rule is inaccurate. For example, the Raw Transaction Format section says:

Bitcoin transactions are broadcast between peers in a serialized byte format, called raw format. It is this form of a transaction which is SHA256(SHA256()) hashed to create the TXID and, ultimately, the merkle root of a block containing the transaction—making the transaction format part of the consensus rules.

and this does not make clear that the uses of the serialized form in consensus checks are of a reserialization, not the serialization that was "broadcast between peers". But this is in some sense academic since it should be documented either way.

Ref: zcash/zcash#842 , which is a potential security bug that the Zcash fork of Bitcoin would have had if canonicity were not enforced.

@wbnns wbnns self-assigned this Dec 9, 2016

@wbnns wbnns added the Dev Docs label Dec 16, 2016

@wbnns wbnns added the Under Review label Jan 31, 2017

Contributor

wbnns commented Feb 8, 2017

Thanks for mentioning, @daira - would you be able to submit a PR to document this?

@wbnns wbnns added Changes Requested and removed Under Review labels Feb 9, 2017

@wbnns wbnns added Bounty Bounty and removed Bounty labels May 21, 2017

Contributor

wbnns commented May 21, 2017

Bounty: 5,000 bits / ~$10 USD

@wbnns wbnns closed this May 26, 2017

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