Make CBlockIndex a subclass of CBlockHeader #4366

jtimon commented at 6:09 PM on June 18, 2014: contributor

Seems more in line with DRY and although I've been looking for a reason not to do it I've not been able to find it.

Also an assignment with no apparent effect (nVersion = this->nVersion) in CBlockHeader's IMPLEMENT_SERIALIZE is removed.

Note that if CBlockIndex's default constructor used sets CBlockHeader::CURRENT_VERSION as default value for its nVersion instead of 0, I could remove two more lines. If you know a reason why I cannot make that change, I will use your explanation to replace my TODO comment.

jtimon commented at 8:52 PM on June 18, 2014: contributor

I need to think more about this, I'm closing it for now.

jtimon closed this on Jun 18, 2014

sipa commented at 12:08 PM on June 19, 2014: member

I haven't looked at the code, but in general yes, CBlockIndex could be a subclass of CBlockHeader. The reason it is not yet the case is that CBlockHeader originally did not exist, and only CBlock existed. Now that the header is split off it could become a common ancestor to both.

However, do not remove nVersion = this->nVersion. The local variable nVersion is defined by the IMPLEMENT_SERIALIZE methods, and is the serialization version used. It may be necessary for future protocol upgrades that change serialization (though that would imply a hardfork).

jtimon reopened this on Jun 21, 2014

sipa commented at 12:22 PM on June 21, 2014: member

I'm not very keen on making core data structure methods virtual. It adds indirection overhead, and prevents inlining.

You can just override the method, no? You'll get the more efficient cached version when using a CBlockIndex reference, and the old on-the-fly-computed hash when using a CBlockHeader one.

jtimon commented at 11:46 PM on June 21, 2014: contributor

Yes, in this case is polymorphism is completely unnecessary, I just have to unify GetHash() and GetBlockHash() and remove that ugly TODO comment.

jtimon commented at 10:53 AM on June 25, 2014: contributor

The main objective of this PR is "don't repeat yourself" and sepcially get rid of those field-by-field ugly mappings. But it could be also solved with simple composition instead of inheritance. That would require more work since pindex->nTime would turn into pindex->header->nTime (the same for CBlocks), but the end result is probably more readable (since you don't have to take inheritance into account). Whatever it's done I think it should be the same for both CBlock and CBlockIndex.

Thoughts?

in src/main.h:None in ec00174471 outdated

 717 | @@ -724,33 +718,19 @@ class CBlockIndex
 718 |          nChainTx = 0;
 719 |          nStatus = 0;
 720 |          nSequenceId = 0;
 721 | +    }
 722 |  
 723 | +    CBlockIndex()
 724 | +    {
 725 | +        SetNull();
 726 | +        // TODO does it really need to be zero instead of CBlockHeader::CURRENT_VERSION?

sipa commented at 2:17 PM on June 28, 2014:

I do not believe that is necessary.

jtimon commented at 2:43 PM on June 28, 2014:

thanks, removing TODO

sipa commented at 2:20 PM on June 28, 2014: member

I only now realize that CBlockIndex did not store the hashPrev, but only a pointer to the previous CBlockIndex instead; changing that means adding 32 bytes to every CBlockIndex in memory, which means some 10MB extra RAM.

That seems like a high cost for a minor code cleanup.

jtimon commented at 3:03 PM on June 28, 2014: contributor

That's very interesting. Using composition wouldn't help either, unless the indexes share the headers with other classes, say CDiskBlockIndex or CBlock. I don't know which of these structures are maintained more permanently and which only temporarily in memory. What I'm proposing is to have a common container (say a map<block_hash, block_header>) and let the indexes and blocks have a pointer to their header. I know that would be a complex change so I'm thinking out loud to see if you think it would make any sense at all. How much header data duplication do you think it's out there?

A much simpler solution would be to create a new class CProtoHeader which lacks hashPrev and which CBlockHeader and CBlockIndex both extend. But that's not saving anything either and it's not particularly beautiful (specially considering that CDiskBlockIndex extends from CBlockIndex but also has hashPrev).

sipa commented at 3:59 PM on June 28, 2014: member

A CBlockIndex instance is permanently held in memory for every known block.

jtimon commented at 4:22 PM on June 28, 2014: contributor

I was asking if there was any other structure that is similarly contained in memory and contained a header too. Probably not.

Anyway, maybe there's another solution. Now that the index has hashPrev, it may be possible to delete uint256* phashBlock to take back the memory space...

sipa commented at 4:35 PM on June 28, 2014: member

Yes, you could, but at the cost of needing to look up hashPrev in mapBlockIndex on every access, and still only reclaiming 8 out 32 bytes. I don't think that is worth it.

Make CBlockIndex a subclass of CBlockHeader 52a96fdf6e

Unify CBlockHeader::hashPrevBlock and CDiskBlockIndex::hashPrev 6200fdfc24

Unify GetHash() and GetBlockHash() cd9659f2eb

Remove GetBlockHeader() e84533abc0

BitcoinPullTester commented at 11:22 PM on June 30, 2014: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p4366_e84533abc0a96ad4d6f48984b6ba537300b5a5b1/ for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

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.

jtimon closed this on Jul 2, 2014

MarcoFalke locked this on Sep 8, 2021