Blockchain: potential _headHeaderHash inconsistencies / bugs

[holgerd77]

This issue is NOT related to the latest stuff I wrote in the chat but some separate finding, I'll extract this as an issue since I wouldn't want to merge into #1068 and I would like to have some clear place for confirmation and discussion here before changing anything.

So the thing is that I have the impression that it is forgotten a couple of times to update the _headHeaderHash value along some update of the _headBlockHash value.

My current assumption here is that _headHeaderHash should always be updated if _headBlockHash is updated, let me know if this is for some reason is not the case and I understand something wrong here.

So following occurrences:

• At the end of _putBlockOrHeader here, missing
• In the else part of _putBlockOrHeader section from above as well here, missing _headHeaderHash update
• In _deleteCanonicalChainReferences here, missing _headHeaderHash update
• At the end of _rebuildCanonical()here, missing _headHeaderHash update
• I then wonder if the comment from here on the header fields should also be expanded to _headBlockHash

//cc @jochem-brouwer

[holgerd77]

Side note: if my whole base assumption that these two fields should be in sync is not correct it would need some better explanation what the different purposes of these fields are. In case the assumption is correct, we should improve here and e.g. encapsulate the updates on both fields in a helper function to make this less error prone to update just one of the fields (code somewhat like here as an example of several code parts where both of the fields are updated).

[jochem-brouwer]

Yeah this is a good point. When I refactored blockchain I also noticed this, but I think I figured out what the reason was (it might still not be used consistently though). Sadly I didn't write down what the reason was, so I will take a new look soon what's the reason for this. It indeed looks incorrect or at least confusing.

[holgerd77]

This would still be a very useful refactoring. Needs some serious digging into the Blockchain library.

[holgerd77]

I guess this is still an issue, even after all the (VM) v6 Blockchain refactoring work.

[holgerd77]

Would still think this is an issue.

[g11tech]

yea seems like atleast at one place its still missing, can pick it up

[jochem-brouwer]

I think a gotcha here is that the _headHeaderHash and _headBlockHash do not necessarily need to be updated at all times, and they neither have to be equal. The reason is, for instance, that in light clients you only download headers, so _headBlockHash does not get updated at any point. Also, if you are a full client, the _headHeaderHash might be at a higher header than the _headBlockHash:

You might first download headers over devp2p, and then once you get the headers you start downloading the blocks (over devp2p also, downloading block bodies). So, in this case your _headBlockHash will likely trail behind _headHeaderHash.

I think this is something to keep in mind. This nevertheless needs a good look, so we are consistent everywhere, and maybe indeed first lay down the "rules". For instance, what might work is that for any update of any of these values, check if the other counterpart is at a lower hash (i.e. lower block/header number), and if thats the case then also update the other value.