👍

@gmaxwell we can be sure about that. DecodeBase58 is only called from two places in base58.h itself:

• DecodeBase58(std::string) -> passed string.c_str() which is always zero-terminated
• DecodeBase58Check(const char*) -> passes through its input which has always been a zero-terminated string

In general I too prefer APIs that explicitly pass the length of strings (or that use a string type with embedded length field like std::string), but it doesn't look like there is reason to change the API here.

:) Yea the "will" was meant to include all future callers. Communication is hard.

Right. It's especially useful to pass a length - or an begin* and end* pointer - if we want to be able to error out on NULs before the real end of the string (whose presence has triggered many browser bugs). It would also allow using std::string.data() and std::string.size() instead of c_str(), which is theoretically slighty more efficient.

Good to keep in mind, but let's not change the API and implementation at the same time, it would interfere with testing :)

The code looks straightforward and correct to me. It passes the tests and we have a fairly good testing suite in place for the Base58 functions.

Why reimplement DecodeBase58 and contribute to the current de-modularisation? Seems like a better idea to move libblkmaker's base58.c to its own library and add an encoder...

A library for 74 lines of trivial code? It's kind of an odd duck, in that I don't know what else I'd put in a library with it except 'generic bitcoin stuff'.

We definitely should move the Base58 stuff to a library when we library-ize Bitcoin Core. It's one of the useful functions for other projects. Removing the dependency on OpenSSL helps there, too.

However I certainly don't want to add a dependency on an external library for this (trivial) functionality.

@gmaxwell Everything needed to create a raw transaction spending to an address? :)

@laanwj: I agree that the interface is dangerous (and inconsistent too... mixing c++ style and c style strings/iteration). I'd like to replace that, but let's do that separately.

I've addresses your comments too.

ACK

Needs rebase after #4014 (should be easy as it only affected comments)

Rebased.

Sorry for the late review, I was pinged from one of my PRs so I figured I'd give it a quick look :)

Pushed an update to address @theuni's comments.

Thought I ACKed this already.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b58be132c994b6f9b25cb4a702186ef96104953f for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.