Optimized binary-to-hex converter (ToHex) #562

Part of JoelKatz's 4diff patch.

If the "cache getwork" patch is applied, does this matter?

The original "cache getwork" patch used this. I split them up for code cleanliness. If this one is merged first, I can rebase the FastGetWork code and bring it back to its original state from JoelKatz.

I appreciate that you're trying to optimize bitcoin, but please don't add any fixed-sized buffers! I rather have somewhat slower code than potential security holes.

Fixed-size buffers are not a security hole, only misuse of them. That being said, I have not checked over this patch for security checks either.

I think we all know that already Luke. The thing is, people make mistakes, and they are usually not all found on the first time around code inspection. It's playing with fire, in a way, and should be restricted imo to direct interaction with hw, OS-level libraries and such stuff. I'd prefer to err on the safe side in financial sw.

Sure, but that ignores the fact that this code has had a lot of real-world production testing in probably 90% of pools (and even required by PSJ)

I don't understand the concern here. If you look in the context of this patch, there's plenty of fixed buffer usage already.

What would make sense is to instead make the buffer size a result of a constexpr calculation, but in this case that really seems like overkill.

That's exactly my concern. I'd like to reduce fixed buffer use not increase it. C++ has introduced all kinds of stuff exactly to not have to muck around with them.

Also, this does not completely replace the old HexStr function, so this means that the source will now be littered with not one but two hex conversion utility functions.

I would prefer if he just optimized the current HexStr function, which is very possible in a similar way, as it seems to be the strprintf and string concatenation that make it slow. Writing to a fixed buffer instead of returning a std::string is likely a unnecessary micro-optimization.

I have a few problems with this code, starting with it doesn't follow the bitcoin coding conventions (e.g. nLen instead of len). And I agree with laanwj RE: optimizing HexStr instead.

But I'll go back to my original question: does this have any measurable effect on performance if the bypass-JSON-and-return-a-cached-response-to-getwork patch is applied? What is the performance of getwork with/without: Just this patch Just the bypass-JSON-cache-getwork patch Both this AND the cache-getwork patch

I think it is a bad idea to add another (potentially unsafe if you use it wrong!) way of converting to hex, unless it gives a really significant (on the order of "you can serve twice as many getwork requests with one bitcoind") performance advantage.

Pull #565 includes performance testing for this.