I'd rather keep mapBlockIndex in main, as it's a data structure protected by its cs_main lock. Idea ACK on moving the definitions out though.

It needs rebase so maybe I incorporate the @gmaxwell idea of including a typedef for map<uint256, CBlockIndex*> (more flexibility if we want to change/test another structure), but yeah, I'll keep the critical sections and the couple of related functions I thought moving here (CBlockIndex * InsertBlockIndex(uint256 hash) and bool AddToBlockIndex(CBlock& block, CValidationState& state, const CDiskBlockPos& pos)) in main. I think this 2 could be easily unified, but I failed my first lazy attempt and just left them on main, just mentioning.

Closing until corrected and rebased

Rebased leaving mapBlockIndex in main.

Verified 13d6bf187b5d969a4c661733a3e0502190f1f0e4 as code-movement only.

Thank you @theuni and I'm sorry but it's the second time I'm going to have to rebase after you verify the diff. Apparently last time with the script PR wasn't really necessary and it was just a little lie from github (@sipa told me it was still a clean rebase).
But somehow I didn't noticed locally so I did the rebase and the squash, so that was my fault.
I'll rebase soon.

@jtimon As you had to rebase anyway, I hope #4838 doesn't make things too much harder.

@jtimon Thanks for the heads up. I'll stash/pull/diff again.

@theuni cool thanks, I hope this one is the last one, script doesn't have as much activity as main has.
@sipa no problem, I'll reabase this tomorrow.
A fast glance at #4838 reminds me @gmaxwell's suggestion of having a typedef for mapBlockIndex, I'm thinking a class with at least cs_nBlockSequenceId in it. I don't like these globals, maybe a singleton factory for mapBlockIndexClass instead with all its CCriticalSection inside?
Of course that would be another PR, but it's kind of related (remember I was moving mapBlockIndex to chain at first) and I keep thinking about it...

The globals all have to go if we ever realistically want to use this code as a library... but eliminating them requires a lot of plumbing.

(Not that you can't have globals in a library— but you can't both have globals in a library and show your face in public afterwards :P)

I mean just make it an attribute of the class for the map, and the function
that uses it becomes a method. I understand that other critical sections
are not going to be so easy to hide...
On Sep 5, 2014 1:15 AM, "Gregory Maxwell" [email protected] wrote:

The globals all have to go if we ever realistically want to use this code
as a library... but eliminating them requires a lot of plumbing.

—
Reply to this email directly or view it on GitHub
#4796 (comment).

I don't like these globals, maybe a singleton factory for mapBlockIndexClass instead with all its CCriticalSection inside

Moving away from global state is a sensible goal. But I'm heavily opposed to anything involving singletons. That just moves from one way of having side effects and hiding dependencies between modules to another one. It looks 'object oriented' and allows some more flexibility in initialization/destruction order but in the end it is the same thing. For that reason, the singleton pattern has become quite controversial.

Whatever, let's do what we do with the global variable that Params() returns, almost indistinguishable from a singleton factory function. Or just keep the variable name of the global as it is. The main point encapsulate the type in a class and move the functions and critical sections that make sense inside (maybe only cs_nBlockSequenceId and a couple of methods).

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4796_e8b5f0d549b1b76611c7374bed9ceec7d09fa847/ 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.

ACK. Verified move-only (e8b5f0d).

ut ACK, seems like a good unit to move out of main.