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

Incorrect handling of leading 0s in Bitcoin addresses. #8

Closed
dougal opened this issue Jan 5, 2018 · 4 comments · Fixed by #9
Closed

Incorrect handling of leading 0s in Bitcoin addresses. #8

dougal opened this issue Jan 5, 2018 · 4 comments · Fixed by #9

Comments

@dougal
Copy link
Owner

dougal commented Jan 5, 2018

@keirfb commenting on #5:

A second reason for not using this base58 library for producing bitcoin addresses is that it doesn't deal with leading zeroes in the manner that bitcoin requires. They get dropped when you convert your string of hex digits to an integer (and there are often many leading zeroes as the network bytes for the main bitcoin network are 00).

Bitcoin docs: https://en.bitcoin.it/wiki/Base58Check_encoding

@joelnordell
Copy link
Contributor

@dougal I think I'll take a look at this one -- it could be handled for binary strings with different implementations of binary_to_base58 and base58_to_binary. Probably can't be solved for integer inputs, since there would be no way to know how many leading zeroes there are.

@dougal
Copy link
Owner Author

dougal commented Jan 5, 2018

I've not looked closely at it, but I would assume there is some maximum number of bitcoin blocks or wallet addresses, to which any block identifier or wallet address is padded.

Perhaps more than just the alphabet should change when :bitcoin is selected, but the behaviour also.

@joelnordell
Copy link
Contributor

The more general issue, I think, is that cryptographic hashes such as SHA256 may contain leading zeroes -- for example:

Digest::SHA256.hexdigest('{"last_name":"Smith","first_name":"John","count":46467}')

...produces "00000a81278d83391ba8d9b4efb02973acdce02173a8db4d4552c25c1b7d2c70".

The way I see it, the problem is not so much that Bitcoin wants things padded to a certain size, but rather that many applications of base58 encoding would want to encode arbitrary byte arrays (such as the output of a hash function) which may start with zeroes in the first bytes.

@joelnordell
Copy link
Contributor

joelnordell commented Jan 5, 2018

Hmm, looking at bitcoin's base58.cpp implementation, it is actually not quite as I thought, and seems a little bit "non-standard" (not that base58 was a standard to begin with). It actually prepends the leading zeroes to the string, in a separate operation from the encoding, based on the number of leading zero bytes in the input. Unlike the actual base58 encoding, where one byte of input does not equate to one character in the output, one leading zero byte actually equals one zero character in the output.

https://github.com/bitcoin/bitcoin/blob/master/src/base58.cpp

That said, I could imagine bitcoin might not be the only application that wants behavior like this, though it should probably be optional (but enabled by default, as it seems to be typically desired due to everyone following bitcoin's example), and this reinforces to me that it only makes any sense when the input is a byte string rather than an integer.

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

Successfully merging a pull request may close this issue.

2 participants