Refactor/encapsulate chain globals into a CChain class

sipa commented at 9:10 pm on October 10, 2013: member

This removes several globals (pindexBest, pindexGenesisBlock, hashBestChain, nBestHeight, nBestChainWork, vBlockIndexByHeight), and replaces them by chainBlocks, an instance of a new CChain class. CChain actually only wraps what was previously stored in vBlockIndexByHeight - all other variables are redundant and can be computed from that one.

This is a preparation for a second iteration of headersfirst which I’m implementing, which will add a second instance of CChain (chainHeaders) in addition to chainActive. This pull request shouldn’t change any functionality.

Since pindexGenesisBlock, hashBestBlock and nBestChainWork are now computed when needed rather than stored, there are potential edge cases in which no genesis block exists yet, and chainActive.Tip() returns NULL. I have tested synchronizing from network and reindexing.

laanwj commented at 11:08 am on October 11, 2013: member

Encapsulating some of the global state is a great idea either way.

jgarzik commented at 1:14 pm on October 11, 2013: contributor

Seems OK except for some English that bugs me: “tip” does not necessarily imply “best tip” to me.

sipa commented at 2:10 pm on October 11, 2013: member

If you’re talking about CChain, that’s intentional: it’s just a chain of blocks, and a chain has a tip. It has no concept of “best”.

One particular (and for now, only) instance of CChain, chainBlocks, represents the currently active (=synchronized) block chain, which indeed follows the notion of best chain (but during a reorganization for example, it’s not “best” either).

More generically, the word “best” is just ambiguous. In headers-first mode, another CChain instance is added (the headers chain), and it also follows its own notion of best. It’s better to just talk about the “currently active” chain (for what is currently called the best chain).

jgarzik commented at 2:21 pm on October 11, 2013: contributor

In the local context of the CChain class itself, “tip” makes sense.

In the wider context of the entire codebase, it makes the code less readable, because our blockchain can have multiple tips, only one of which is active/best.

sipa commented at 2:25 pm on October 11, 2013: member

I see what you mean. I think part of the problem is that many people use the term “block chain” to in fact refer to the entire block tree, which consists of many chains - while in this context it actually refers to just the active chain within it.

Suggestion: call it chainActive or chainSynchronized instead of chainBlocks? chainBest is a possibility as well, but equally ambiguous imho.

jgarzik commented at 2:32 pm on October 11, 2013: contributor

@sipa You’ve got it. Any of those suggestions is fine.

RayDillinger commented at 4:32 pm on October 11, 2013: none

On 10/10/2013 02:10 PM, Pieter Wuille wrote:

This removes several globals (pindexBest, hashBestChain, nBestHeight, nBestChainWork, vBlockIndexByHeight), and turns them into an instance of a CChain class, chainBlocks.

This is a preparation for a second iteration of headersfirst which I'm implementing, which will add a second instance (chainHeaders). This pull request shouldn't change any functionality. You can merge this Pull Request by running:

I’m starting to worry that the blockchain is too much bandwidth and computer power to keep up with.

A more distributed architecture is possible, where multiple “branches” can be adding blocks of transactions independently, and then the branches are joined to the blockchain (or to another branch) with a single transaction. Yes, I know there are “orphan block” problems between chains competing for transactions, if you do it the most obvious way; there are ways to avoid that, but this message is not about the strategies for avoiding that.

The savings is because people needn’t necessarily download and check the branches unless their own wallet has a transaction in that branch. In that case they can check to make sure that the stream of transactions is in fact represented by the “diff” applied to the main blockchain.

Anyway, all this is a lead-up to one question; how much of a pain in the tush do you think it would it be to adapt the class you’ve created here to work with a branching chain? I mean, is it roughly interface- compatible? Interface-compatible with some added methods? Or is it, including interface, all stuff you’d have to rip out and do again?

0            Bear

sipa commented at 4:39 pm on October 11, 2013: member

That is completely outside of the scope of this (or any single) pull request, and requires pretty much a full redesign of how Bitcoin’s consensus mechanism works.

If you don’t want to download or process the whole blockchain, run an SPV client (multibit, bitcoin wallet for android, …). There are proposals that would enable non-full nodes to contribute back to the network, but they are far more invasive than what you make it seem (conflicts between chains is a detail you seem to consider trivial here, but it is ultimately the only reason why we need a chain in the first place: to reconsile conflicts). In any case, this is a discussion for the mailing list and not this refactoring-only pull request in a single client.

sipa commented at 4:55 pm on October 11, 2013: member

@jgarzik Updated the variable name to chainActive.

jgarzik commented at 4:59 pm on October 11, 2013: contributor

ACK

Refactor/encapsulate chain globals into a CChain class 4c6d41b8b6

sipa commented at 1:05 pm on October 12, 2013: member

(moving some extra commits to another pullreq)

BitcoinPullTester commented at 1:17 pm on October 12, 2013: none

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

jgarzik commented at 2:30 am on October 15, 2013: contributor

re-ACK

gavinandresen commented at 3:38 am on October 15, 2013: contributor

ACK

gavinandresen referenced this in commit b9beea6e9d on Oct 15, 2013

gavinandresen merged this on Oct 15, 2013

gavinandresen closed this on Oct 15, 2013

in src/main.cpp: in 4c6d41b8b6

167@@ -173,14 +168,22 @@ void static ResendWalletTransactions()
168 // Registration of network node signals.
169 //
170 
171+int static GetHeight()

rebroad commented at 11:19 am on September 6, 2014:

When should this be used in preference to chainActive.Height() please?

laanwj commented at 12:09 pm on September 6, 2014:

Well it’s a signal handler for nodeSignals.GetHeight - you aren’t supposed to use this function directly.

sipa commented at 12:12 pm on September 6, 2014:

chainActive is an internal data structure in main (which is, for now, more widely accessible than it should be). GetHeight() is a way to expose part of the information contained in it in a safe way.

laanwj commented at 12:50 pm on September 6, 2014:

@sipa Isn’t nodeSignals only supposed to be used by net?

rebroad commented at 8:34 am on September 7, 2014:

@sipa ah, ok, thanks. …Safer in what way?

Bushstar referenced this in commit d26b6a84cc on Apr 8, 2020

DrahtBot locked this on Sep 8, 2021

Refactor/encapsulate chain globals into a CChain class #3077