Describe the issue

As raised with PR #814; Block values are calculated on the current head when creating a block, not the next block; and on the previous block when validating a block; not the current block. This creates confusion comparing the block reward code and block reward schedules to the actual function of the code and consensus.

Expected behavior

Block creation should look at the block value for the next block, since that is the block that's being creating. Block validation should be calculated on the current block, not the previous block that was added to the chain.

Actual behavior

Block reward changes occur on the block after the expected block.

Any extra information that might be useful in the debugging process.

IsBlockValueValid() is being called with expectations from a different block then the block being validated.

CAmount nExpectedMint = GetBlockValue(pindex->pprev->nHeight);
should be
CAmount nExpectedMint = GetBlockValue(pindex->nHeight);

So that you are actually calculating the expected mint for the block height of the new block being checked. But the problem is a 'two wrongs make it work' problem. The block creation -also- uses the wrong height:

miner.cpp :: CreateNewBlock()

CAmount blockValue = nFees + GetBlockValue(pindexPrev->nHeight);

should be

CAmount blockValue = nFees + GetBlockValue(nHeight);

A problem also exists in masternode-payments.cpp; however it's very subtle:

masternode-payments.cpp

CAmount blockValue = GetBlockValue(pindexPrev->nHeight);
CAmount masternodePayment = GetMasternodePayment(pindexPrev->nHeight, blockValue, 0, fZPIVStake);

Should actually be

CAmount blockValue = GetBlockValue(pindexPrev->nHeight+1);
CAmount masternodePayment = GetMasternodePayment(pindexPrev->nHeight+1, blockValue, 0, fZPIVStake);

The above didn't get caught because the drift logic only cares if the minter is underpaying the masternode, it doesn't care if the minter is overpaying them. So while CMasternodeBlockPayees::IsTransactionValid() uses the correct block height, the required masternode payment (which was half of what was paid) was more than required, so it didn't disallow it.

Lastly, there needs to be backwards compatibility so that the current chain does not break.

   CAmount nExpectedMint = GetBlockValue(pindex->nHeight);

    if (pindex->nHeight < last-problem-block) {
        for (list of problem blocks)
             // Account for the wrong block
            LogPrintf("ConnectBlock(): Ignoring overmint at block %d\n", pindex->nHeight);
            nExpectedMint = GetBlockValue(pindex->pprev->nHeight);
    }