There was repeated hashing happening

[pol081]

One case is during the bootstrap ProcessBlock -> AcceptBlock calls each did a GetHash. Changed GetHash to only compute on the first call to GetHash and store in memory only the hash (If ever loaded it would have to hash again) Using mutable on those objects to accomplish this so not sure if I am breaking code practices.

Tested by looking at LoadExternalBlockFile and every 500 blocks printed (total elapsed time/ nloaded) in ms

nLoaded	Avg Before	Avg After
500	4.98ms	2.54ms
20000	5.96ms	3.38ms
100000	7.97ms	5.26ms

[Fuzzbawls]

i'll run this through the gauntlet of tests this week, thanks

[pol081]

cool
The hash itself doesn't take much time, but looking deeper through everything it was called at least 5 times per bootstrapped block and recalculating each time.
Trying to track down the true performance killer for block loading now.

[shane-kerr]

I know this PR was submitted ages ago, but I'm trying to update the MintCoin wallet a bit and so looking through these old PR.

Anyway, I kind of like the approach here, which I take to be a kind of memoization.

There are a couple of problems with the specific implementation though.

The first problem is that the scrypt_hash() call uses sizeof(block_header) - meaning that it includes all of the block_header object used. However, we have added two members to the class, which at the very least means that we are hashing over an extra 257 or so bytes. It will also change the hash values from the expected results; this may not be a problem if all comparisons are internal, but if the expectation is for hash values to be the same in the new code as the old code then this will break.

The second problem is that the members of this class are public, and so can be changed. If that happens, then the resulting saved hash is no longer the same. If we return the saved hash, then it will be wrong.

It is possible that this never happens with the way the code is today, but we have no easy way to know (I am too lazy to read all of the code to check for it, and would not trust that I had not missed it). Also, a future change to the code might unintentionally break things; there are reasons that we have public/private distinctions. 😉

I have two possible approaches that can resolve this problem:

1. The first approach is to keep a copy of the CBlockHeader contents at the time when bComputedHash is computed, and verify this is the same before returning hashThisBlock. This will be slower that just returning the values, but should still be much faster than re-computing the scrypt hash again.

2. The second approach (probably the "correct" one) is to change the CBlockHeader class so that the members are private, and provide accessor methods to get/set the values, like:

   ...
    inline unsigned int get_nTime() { return nTime; }
    inline void set_nTime(unsigned int new_nTime) { if (nTime != new_nTime) { bComputedHash = false; nTime = new_nTime; } }
   ...

With this second approach we can invalidate the hash if any of the members of an instance change, insuring that we only use the saved version if it is still valid. It is definitely more work and would require a lot of changes (although honestly the code should have been designed this way in the first place).

I don't know if you're still interested in this work or not. I am happy to work with you or even make the changes myself. I appreciate the effort you have taken!