Move CBlockIndex, CChain and related code out of main

jtimon commented at 8:27 pm on August 30, 2014: contributor

After these changes main.h has only 603 lines, and main.cpp only 4344 lines. One step at a time…

jtimon force-pushed on Aug 31, 2014

sipa commented at 11:37 pm on September 1, 2014: member

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.

jtimon commented at 1:03 am on September 2, 2014: contributor

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.

jtimon commented at 10:41 am on September 2, 2014: contributor

Closing until corrected and rebased

jtimon closed this on Sep 2, 2014

jtimon reopened this on Sep 3, 2014

jtimon force-pushed on Sep 3, 2014

jtimon commented at 9:06 am on September 3, 2014: contributor

Rebased leaving mapBlockIndex in main.

laanwj added the label Improvement on Sep 3, 2014

theuni commented at 4:39 pm on September 4, 2014: member

Verified 13d6bf187b5d969a4c661733a3e0502190f1f0e4 as code-movement only.

jtimon commented at 8:01 pm on September 4, 2014: contributor

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.

sipa commented at 10:06 pm on September 4, 2014: member

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

theuni commented at 10:13 pm on September 4, 2014: member

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

jtimon commented at 11:08 pm on September 4, 2014: contributor

@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…

gmaxwell commented at 11:14 pm on September 4, 2014: contributor

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)

jtimon commented at 6:54 am on September 5, 2014: contributor

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” notifications@github.com 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).

laanwj commented at 2:09 am on September 6, 2014: member

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.

jtimon commented at 5:30 pm on September 6, 2014: contributor

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).

Decouple CChain from mapBlockIndex 6db83db3eb

Move CBlockIndex, CChain and related code out of main e8b5f0d549

jtimon force-pushed on Sep 8, 2014

BitcoinPullTester commented at 8:32 pm on September 8, 2014: none

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.

sipa commented at 0:05 am on September 10, 2014: member

ACK. Verified move-only (e8b5f0d549b1b76611c7374bed9ceec7d09fa847).

laanwj commented at 8:49 am on September 16, 2014: member

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

sipa merged this on Sep 29, 2014

sipa closed this on Sep 29, 2014

sipa referenced this in commit bf3a5dd7f0 on Sep 29, 2014

jtimon deleted the branch on Sep 29, 2014

DrahtBot locked this on Sep 8, 2021

Move CBlockIndex, CChain and related code out of main #4796