v1.4.4 doesn't handle orphans correctly

[dooglus]

Lots of people have been complaining that their client is getting 'stuck' on certain blocks.

When it's your turn to stake an output, it's possible to create two different conflicting blocks staking the same output at the same time. Each peer will accept whichever of the two blocks it sees first, and reject the other.

Before v1.4.4 this was OK, because if the next block was staked on top of the block that had been rejected, the client would treat the new block as an orphan and request its unknown parent. The previously rejected block would be re-downloaded and since it was now the parent of an orphan block it would be accepted. The 3rd condition in the check for duplicate PoS would fail, since mapOrphanBlocksByPrev would now contain an entry for the previously rejected parent:

if (pblock->IsProofOfStake() &&
    setStakeSeen.count(pblock->GetProofOfStake()) &&
    !mapOrphanBlocksByPrev.count(hash) && // mapOrphanBlocksByPrev has at least one entry for 'hash'
    !Checkpoints::WantedByPendingSyncCheckpoint(hash))
    return error("ProcessBlock() : duplicate proof-of-stake (%s, %d) for block %s", pblock->GetProofOfStake().first.ToString(), pblock->GetProofOfStake().second, hash.ToString());

In v1.4.4 however, commit 6649357 added the following code near the top of ProcessBlock():

map<uint256, CBlockIndex*>::iterator mi = mapBlockIndex.find(pblock->hashPrevBlock);
if (mi == mapBlockIndex.end())
    return false; 

That says that if we receive a block with a parent we don't already know, reject it. Don't add it as an orphan, just reject it. We never get to re-request the previously rejected block, and never get back onto the correct fork.

I don't understand enough of that commit to know what the correct fix is, but at least now I understand why the problem is happening.

The new code seems to have been added so that we can be sure of finding the parent block's hash, which is needed when calling CheckProofOfStake(). But until now we never called CheckProofOfStake() directly from ProcessBlock(). It is called from AcceptBlock(), which ProcessBlock() calls. So it looks to me as if the recent commit has added an extra call to CheckProofOfStake() which may well not be necessary.

[dooglus]

I'm not sure commit 6649357 is doing the right thing in running CheckProofOfStake() an extra time, but f8677c0 seems to at least prevent clients from getting stuck due to the bug it introduced.