add GetBlockFile() to main.cpp #1692

• this function converts an unsigned block-file number into a std::string containing the full path to the file
• make OpenBlockFile() use GetBlockFile()

Can be usefull for @sipa Ultraprune work, when we have many more block-files.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/e060928d322dd7d0425a53b65f7dce681684771d for binaries and test log.

Why is this cache needed? Is it showing up on profiles?

Remember, every static variable creates complications related to multi-threading.

I think the change is a nice small cleanup... without the static variable-cache stuff.

@jgarzik I can remove the cache, as I did not profile :). Will update, just a few minutes...

I don't see the point of creating an extra function here. Instead of the fairly simple expression (GetDataDir() / strprintf("blk%04d.dat", nFile)).string(), used in one place, you add a 20 line function that is only used in one place.

Also, please don't introduce any snprintf and fixed buffers. strprintf exists for a reason.

Guys that pull was open for 2 weeks now, I got no feedback, then used @jgarzik feedback to update and now you tell me that thing is useless...

I'm fine with it if you have a reason to expose the functionality outside main.cpp (ie, because of the sipa pull you mention) or plan to use it in multiple places, but you can reduce the function to 1 line.

@laanwj Sorry for my over-reacting, had a little trouble at work today. You are right, it can be even shorter and if you want I can use strprintf (although snprintf() has a buffer-length check afaik).

No hard feelings. We all have that kind of trouble sometimes.

snprintf checks the buffer length, that's right, but has some problems that can catch the unwary. Ie, the resulting buffer is not necessarily zero-terminated. And what if someone changes the filename and forgets to change the hardcoded '12'... For a project like bitcoin we should prefer clarity and robustness to low-level fiddling. Especially if it results in shorter code as well. No need to be clever here. Just use strprintf where it can be useful.

@laanwj You are absolutely right then, strprintf() is the function to take ... I'm just interested, was it deeply inspected for any faults / security flaws in it's implementation, as I find it rather strange to trust a hand-made function that much :) or even more than standard functions.

We use strprintf all over the codebase, so it is fairly well-tested. Personally I've looked at it many times and wasn't able to find issues with it. Internally it ofcourse simply calls (_v)snprintf, but handles all the edge cases as well. It is also the only place where we call the "evil" *snprintf directly, and I'd like to keep it that way.

If you don't trust strprintf please try to study it closely and fix the issues there (or write some testcases!). Using alternative functions in some parts of the program won't improve overall security.

It's good to have an eagle eye who catches usage of unsafe functions and I can benefit from your comments nearly all the time, so thanks. I updated that pull to use strprintf() now.

superceded in HEAD, closing

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/a236bd621e0e478f6817708271ecccf4e8a7e311 for test log.

This pull does not merge cleanly onto current master