Hmm, seems like the following code will always pass no matter what.

                auto header = pindexNew->GetBlockHeader();
                if (header.GetHash() != pindexNew->GetBlockHash())

because earlier in the code:

                CBlockIndex* pindexNew = InsertBlockIndex(diskindex.GetBlockHash());
                ...
                // copy diskindex fields into pindexNew
                ..

where

CBlockIndex * InsertBlockIndex(uint256 hash) {
   ...
    mi = mapBlockIndex.insert(make_pair(hash, pindexNew)).first;
    pindexNew->phashBlock = &((*mi).first);
   ...
}

and

    uint256 GetBlockHash() const
    {
        return *phashBlock;
    }

That is, diskindex.GetBlockHash() is being checked against diskindex.GetBlockHash().

Not quite 😃 pindexNew->GetBlockHeader() copies the header properties from CBlockIndex to CBlockHeader, and then header.GetHash() reserializes. I'm intentionally comparing it to pindexNew->GetBlockHash() precisely because it is calculated from the original CDiskBlockIndex, and there's no point in recalculating that side.

Another way to achieve this would be:

CDiskBlockIndex reserialized(*pindexNew);
reserialized.GetBlockHash()

But I think this PR is more intuitive.

You mean pindexNew.GetBlockHeader() not pindexNew.GetHeader()?

Does this sound right?

1. There is a CDiskBlockIndex object called diskIndex.
2. diskindex.GetBlockHash() is invoked which calls GetHash() and involves serialization
3. CBlockIndex* pindexNew copies the member variables from diskIndex
4. pindexNew->GetBlockHeader() is invoked which returns a CBlockHeader object called header, and then header.GetHash() involves serialization

So when InsertBlockIndex is invoked, pindexNew->phashBlock is set to the value of diskindex.GetBlockHash() (call it V1) which is what pindexNew->GetBlockHash() will return.

Once the values from diskIndex are copied into pindexNew, reserializing seems redundant since it will be over the same values which were already hashed to generate V1, meaning the consistency check always passes

Am I missing something?

Once the values from diskIndex are copied into pindexNew, reserializing seems redundant since it will be over the same values which were already hashed to generate V1, meaning the consistency check always passes

"Once the values are copied" is the sticking point: this consistency check is intended to defend against bugs like #2931.

Imagine there is disk corruption. When zcashd wrote a block to disk, its hashReserved was all 0x00, as expected. However there has been some bit rot, and now whenver reading back from disk, it is non-zero junk data. (header.GetHash() != pindexNew->GetBlockHash()) won't catch this error, but CheckProofOfWork should. (That's pretty much how Zencash realized they had an issue on #2931).

Checking the block hash can help catch developer error of forgetting to copy a field (which is used as part of computing the block hash), but is there a way we can catch this type of error at build time?

This PR as it stands is fine for purpose of fixing the performance regression.

Non-blocking: Perhaps the log message "CheckEquihashSolution failed" should be changed to something like "GetBlockHash inconsistency detected, expected {} but got {}"

What I'm asking is what is the consistency check actually against?

It is protecting against us making similar mistakes as #2931 when making future changes to the block header format.

Checking the block hash can help catch developer error of forgetting to copy a field (which is used as part of computing the block hash), but is there a way we can catch this type of error at build time?

Not with the current architecture inherited from Bitcoin Core (this function is unchanged upstream). We'd need to largely rewrite this function at a minimum, which is well outside the scope of this PR.

This PR as it stands is fine for purpose of fixing the performance regression. Perhaps the log message "CheckEquihashSolution failed" should be changed to something like "GetBlockHash inconsistency detected, expected {} but got {}"

Oops, I change this message on one computer but forgot to do so on the one I actually made the commit on. Will fix.

The modified check is useful, but not equivalent to checking the Equihash solution. That said, it seems a pragmatic choice given the performance regression. The error message definitely needs to be changed as well though.

Force-pushed to update error message.

@zkbot r+

📌 Commit 704b763 has been approved by str4d

⌛ Testing commit 704b763 with merge f9dbd1e...

☀️ Test successful - pr-merge
Approved by: str4d
Pushing f9dbd1e to master...