Fix Base58 Decoding

[mloit]

Fix Base58 decoding bug where a stream of all 1's would result in a negative length
-- fixes #424

Minor optimization to the decoder

• allocate a single buffer and trim it instead of allocating 2 buffers and performing a trim and then a copy and append

basic lint cleanup of the file

• trailing whitespace
• vertical whitespace
• commented code

[JeneaVranceanu]

@mloit Since we provide Base58 encoding/decoding could you please add tests for it?
Tests can based on results from https://www.rfctools.com/base58-encoder/ and https://www.rfctools.com/base58-decoder/

[mloit]

@mloit Since we provide Base58 encoding/decoding could you please add tests for it? Tests can based on results from https://www.rfctools.com/base58-encoder/ and https://www.rfctools.com/base58-decoder/

We totally should. I'll have to look into doing that. I have not delved into the test code yet. Thanks for the reference links.

There are probably a lot of things we are not currently testing, but should. Be a good PR for down the road, not sure I have time for all that.

The intent of this PR was to address #424 as a string of all 1's is a possible scenario for and encoded value of 0

[mloit]

Interestingly the referenced "RFC" encoder/decoder may not be valid. [Though might be a variant encoding in some spaces] That endcoder is encoding leading 0 bytes as ASCII "0", problem is ASCII "0" is not a valid Base58 encoded character. So not a good reference here.

Here is a good reference doc, and it also includes test vectors
https://tools.ietf.org/id/draft-msporny-base58-01.html

[JeneaVranceanu]

Here is a good reference doc, and it also includes test vectors
https://tools.ietf.org/id/draft-msporny-base58-01.html

I wasn't sure if it's valid source becaause the standard is in draft mode. Overall, PR look okay. I'll test and compare it some more with what I'll be able to find online and will add a few test cases to the project.

[mloit]

Tests added, I modified the one comment to hopefully make it more clear

[mloit]

Here is a good reference doc, and it also includes test vectors
https://tools.ietf.org/id/draft-msporny-base58-01.html

I wasn't sure if it's valid source becaause the standard is in draft mode.

Typical for the IETF! But the important thing is that we follow the Satoshi implementation, as that is what is used in the crypto space. (there is one error in that document for the test vectors, but I fixed it... it's a visibly evident error)

[mloit]

Interestingly the 1's thing may be a common implementation error, that has become the "standard". It results in a bit of compression, as you get 8 bits out for each token in, instead of ~5.4. My fix re-implemented what was there, just correcting the initial buffer size length calculation. And then I spotted a simple optimization with the buffer management, so I added that as well. I did confirm this with a couple of other implementations (including the core Bitcoin code), and it matches.

As far as the implementation error, it is mostly safe on the decode side, worst case is it results in a buffer a few bytes bigger than the original data, filled with 0's. The error is also relatively safe on the encode side as worst case you transmit a few less bits. The receiving decoder should pad out with 0 bits anyway.

here is a decent online tool for encode/decode
https://appdevtools.com/base58-encoder-decoder

here is the reference Bitcoin implementation: (also contains the implementation error/anomaly)
https://github.com/bitcoin/bitcoin/blob/master/src/base58.cpp