[Minting] Overmint error when rewards increase

[Carbon-Zero]

Checking expected vs actual mint for the block can stop the chain on a block where rewards increase. The expected value is less than the new value and the error "reward pays too much (actual=%s vs limit=%s)" is logged because it is comparing expected mint and actual mint of different blocks.

[Warrows]

The problem is that we have been running with it since the beggining. So changing it now is going to be a problem.
I just tried this change on testnet. And it's failing while doing the initial sync.

2019-05-20 09:45:49 ERROR: ConnectBlock() : reward pays too much (actual=224.99998958 vs limit=45.00)
2019-05-20 09:45:49 InvalidChainFound: invalid block=1716124dff1c50895a39b71922e7431fd46aac74aebd9b07f6dcec797d7170f9  height=145001  log2_work=65.509062  date=2017-06-24 08:41:04
2019-05-20 09:45:49 InvalidChainFound:  current best=2bb7e851297244f40bc0f51d9eca3e52c7a728e14bd185c1f66e1eaf8998706d  height=145000  log2_work=65.509056  date=2017-06-24 08:40:56
2019-05-20 09:45:49 ERROR: ConnectTip() : ConnectBlock 1716124dff1c50895a39b71922e7431fd46aac74aebd9b07f6dcec797d7170f9 failed
2019-05-20 09:45:49 InvalidChainFound: invalid block=1716124dff1c50895a39b71922e7431fd46aac74aebd9b07f6dcec797d7170f9  height=145001  log2_work=65.509062  date=2017-06-24 08:41:04
2019-05-20 09:45:49 InvalidChainFound:  current best=2bb7e851297244f40bc0f51d9eca3e52c7a728e14bd185c1f66e1eaf8998706d  height=145000  log2_work=65.509056  date=2017-06-24 08:40:56

[Carbon-Zero]

That's the error I was getting prior to this change - not during initial sync but later if rewards change. I'll see if this can be fixed elsewhere without affecting initial sync.

[CaveSpectre11]

NACK

@Carbon-Zero ; the problem is that yes, IsBlockValueValid() is being called with expectations from a different block then the block being validated. However the change for pindex->nMint to be the previous block is actually a big error; since it will accept the block onto the chain based on the minting of the previous block, not the minting of the current block. (i.e. this would introduce a DoS vulnerability)

The more accurate fix is a few lines above:
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.

---

Now the above is all academic; because:

1. It's already happened
2. It's (currently) over with

Fixing it, as has already been realized; will break on syncing. So the "fixes" would have to be bound by a block height check, beyond the last change (e.g. nHeight > Params().Zerocoin_Block_V2_Start()). And since there is no more reward changes, it won't actually matter in the future.

Of course, those using PIVX as a reference coin will have the problem; but many will probably not even notice the block height check would need to be removed for their coin... in which case it doesn't even help there.

So, while, academically it's a bug:

• It won't be hit again
• As @Warrows says, and outlined above, the fix is more complicated because of old block synch (which is the challenge I am facing in my fixing this in an altcoin)
• The fix won't improve new implementations unless they remove the height check

So it may make the most sense to remain aware of it should PIVX change in the future to have another subsidy change.

[Carbon-Zero]

Thanks for the really long explanation. But a bug is a bug, and bugs should be fixed whether current plans intersect with the bug again or not. Not doing so is sloppy coding. It makes absolutely no sense to wait until it might be a problem again when nobody will remember it. It's a bug. It needs to be fixed. That's it. Not fixing it would be irresponsible. And why? Why not fix something that we can fix? For current use, having it corrected for testnet allows a testnet to be built. With the bug not fixed, you can't create a testnet. So this is *not* Academic. And you can say it's not their responsibility to keep up the code as a reference coin, would you say the same thing about bitcoin core? I seriously doubt it. As a reference coin you do have a responsibility to have all aspects of your code working properly.

[CaveSpectre11]

Thanks for the really long explanation. But a bug is a bug, and bugs should be fixed whether current plans intersect with the bug again or not.

In regards to this PR; the direction of the fix was in error; hence the NACK.

In my post, I specifically state that I have been working on a fix for this in an altcoin. An update on that development is that it has been successful.

However, the past is the past, and bug or not bug; it's immutable transactions and PIVX must account for the overminted blocks. The very nature of that becomes problematic for altcoins; as they have different genesis and timing; and a completely different model of which blocks were overminted. I could submit my fix, but it will require changes for PIVX, and require changes for the majority of coins.

Perhaps I can submit it as a proof of concept; and someone can continue it from there; to possibly put the switchover point as a spork, so that different coins can set it to whatever timestamp -they- fixed the problem. Otherwise you end up with a wonky coin specific tweak required in all clones.

   CAmount nExpectedMint = GetBlockValue(pindex->nHeight);

    if (210000 == pindex->nHeight) {
        // Account for that one wrong block
        LogPrintf("ConnectBlock(): Ignoring overmint at block %d\n", pindex->nHeight);
        nExpectedMint = GetBlockValue(pindex->pprev->nHeight);
    }

[random-zebra]

In #967.
Closing.

[CaveSpectre11]

In #967.
Closing.

@random-zebra; this actually is a different issue. #967 gets everything lined up; so that all parts are off by one block. Technically this issue remains... but after #967, the only part that is a problem is that block value changes occur one block after they're "supposed" to.

[random-zebra]

Yes, still the fix proposed here was in error and no further change has been done in several months.
What's in 967 is good enough for PIVX right now. A separate issue can be open for tracking purpose of the last problem.

[CaveSpectre11]

True on all points. And yes, I had been working on it (low priority given the issues), so it would be coming in as a new PR anyway.

[Carbon-Zero]

Unfortunately, I don't understand this reply. Seems like everyone is saying it's a problem that's not a problem. I give up. Maybe I'm an idiot.

…

On Mon, Jun 10, 2019, 8:18 AM CaveSpectre11 ***@***.***> wrote: Thanks for the really long explanation. But a bug is a bug, and bugs should be fixed whether current plans intersect with the bug again or not. In regards to this PR; the direction of the fix was in error; hence the NACK. In my post, I specifically state that I have been working on a fix for this in an altcoin. An update on that development is that it has been successful. However, the past is the past, and bug or not bug; it's immutable transactions and PIVX must account for the overminted blocks. The very nature of that becomes problematic for altcoins; as they have different genesis and timing. I could submit my fix, but it will require changes for PIVX, and require changes for the majority of coins. Perhaps I can submit it as a proof of concept; and someone can continue it from there; to possibly put the switchover point as a spork, so that different coins can set it to whatever timestamp -they- fixed the problem. Otherwise you end up with a wonky coin specific tweak required in all clones. CAmount nExpectedMint = GetBlockValue(pindex->nHeight); if (210000 == pindex->nHeight) { // Account for that one wrong block LogPrintf("ConnectBlock(): Ignoring overmint at block %d\n", pindex->nHeight); nExpectedMint = GetBlockValue(pindex->pprev->nHeight); } Not doing so is sloppy coding. It makes absolutely no sense to wait until it might be a problem again when nobody will remember it. It's a bug. It needs to be fixed. That's it. Not fixing it would be irresponsible. And why? Why not fix something that we can fix? For current use, having it corrected for testnet allows a testnet to be built. With the bug not fixed, you can't create a testnet. So this is *not* Academic. And you can say it's not their responsibility to keep up the code as a reference coin, would you say the same thing about bitcoin core? I seriously doubt it. As a reference coin you do have a responsibility to have all aspects of your code working properly. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#814?email_source=notifications&email_token=AKFOO5B4H2XWQPN5VE6NTHDPZZBBRA5CNFSM4GZD7OWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXJW52I#issuecomment-500395753>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKFOO5E5B24GFIUNGA5ZJITPZZBBRANCNFSM4GZD7OWA> .

[random-zebra]

@Carbon-Zero No one said that "it's not a problem".
The issue has not high priority, of course, but still it was (partially) addressed with the change in 967 and CaveSpectre already stated he's working on the last bit.
This PR is closed because the proposed "fix" was in error and there was no follow up in over two months.

[CaveSpectre11]

@Carbon-Zero; not sure where the miscommunication is; but since I'm in the middle of it... I apologize if I'm the source.

Yes, this is a problem. However as I outlined originally; this PR was not the best solution; as it was going to introduce a potential DoS attack. I outlined a better way of doing it. random-zebra is correct in closing this one; since the fork branch is gone, and the fix needs to be reworked. It can (will) be submitted as another PR.

I will log it as an Issue so it's not forgotten; if someone doesn't beat me to that.

Speaking to the fix... Since it's historical and there's not a looming block reward change, it's not a pressing issue. There are two challenges to the fix; both fairly minor but significant enough that it's lower on my priority. The first is that we need to go through explorer and find all the blocks that need to be added as exceptions to the protocol change, pulling out enough unique characteristics to prevent mistaken exceptions on other implementations. The second is the time it will take me to reload the whole chain in test, to make sure no blocks were missed. That's the one that's daunting, since it's going to tie up my system for quite a while.

It will get done, by me if no one else picks it up and runs with it.

[barrystyle]

I bumped into this recent, after thinking that this was a tall tale. It seems like the additional amount is from the masternode payment vout.

2019-12-06 03:50:40 ERROR: ConnectBlock() : reward pays too much (actual=8.00 vs limit=5.00)
2019-12-06 03:50:40 CreateNewBlock() : TestBlockValidity failed
2019-12-06 03:50:40 CBlock(hash=e7877ee4f476c6da64dd5e5a7760bb7a1e5d285042d83b22c0e1d1e68fb255af, ver=4, hashPrevBlock=000000137651cf574dd24cab595119427989ae3403428da00149b709578e8a4f, hashMerkleRoot=0000000000000000000000000000000000000000000000000000000000000000, nTime=1575604240, nBits=1d27ab5d, nNonce=0, vtx=1)
  CTransaction(hash=e505e15ba4, ver=1, vin.size=1, vout.size=2, nLockTime=0)
    CTxIn(COutPoint(0000000000000000000000000000000000000000000000000000000000000000, 4294967295), coinbase 02c70000)
    CTxOut(nValue=5.00000000, scriptPubKey=037787c39547c93112fb1afd47de16)
    CTxOut(nValue=3.00000000, scriptPubKey=OP_DUP OP_HASH160 00e4906e47e1)

In my instance, I was able to work around it by getting the masternode function to return zero; however I will point out that it occurred at a seemingly random blockheight - perhaps a conditional which is using an uninitialized variable?

 int64_t GetMasternodePayment(int nHeight, int64_t blockValue)
 {
+    if (nHeight <= Params().LAST_POW_BLOCK()) return 0;
     return blockValue * 0.6;
 }

[CaveSpectre11]

@barrystyle if you are working on a PIVX fork, look at the referenced issue above (#973) which details the changes needed. It's a lot simpler for forks as they likely have little to no value transitions to account for in their existing chain.

[Carbon-Zero]

I think the proper goal is to fix it for PIVX, even though it's unlikely to run into the issue again and go from there. I know PIVX doesn't support forks, but it makes a whole lot more sense to have it fixed in the code other projects will be forking from. Also, it really should work on PIVX testnet since the functionality still remains in the software. My opinion would probably be different if the feature were removed entirely in the PIVX code (not what I'm recommending at all).

…

On Fri, Dec 6, 2019, 8:15 AM CaveSpectre11 ***@***.***> wrote: @barrystyle <https://github.com/barrystyle> if you are working on a PIVX fork, look at the referenced issue above (#973 <#973>) which details the changes needed. It's a lot simpler for forks as they likely have little to no value transitions to account for in their existing chain. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#814?email_source=notifications&email_token=AKFOO5GCQU66U5ONUXS6WWDQXJF6RA5CNFSM4GZD7OWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGEBONY#issuecomment-562566967>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFOO5CUW337HKBELCCU6V3QXJF6RANCNFSM4GZD7OWA> .

[Carbon-Zero]

This may work as a workaround, but it doesn't solve the problem. You are just defeating one of the checks that was intentionally put in place.

…

On Fri, Dec 6, 2019, 3:33 AM barrystyle ***@***.***> wrote: I bumped into this recent, after thinking that this was a tall tale. It seems like the additional amount is from the masternode payment vout. 2019-12-06 03:50:40 ERROR: ConnectBlock() : reward pays too much (actual=8.00 vs limit=5.00) 2019-12-06 03:50:40 CreateNewBlock() : TestBlockValidity failed 2019-12-06 03:50:40 CBlock(hash=e7877ee4f476c6da64dd5e5a7760bb7a1e5d285042d83b22c0e1d1e68fb255af, ver=4, hashPrevBlock=000000137651cf574dd24cab595119427989ae3403428da00149b709578e8a4f, hashMerkleRoot=0000000000000000000000000000000000000000000000000000000000000000, nTime=1575604240, nBits=1d27ab5d, nNonce=0, vtx=1) CTransaction(hash=e505e15ba4, ver=1, vin.size=1, vout.size=2, nLockTime=0) CTxIn(COutPoint(0000000000000000000000000000000000000000000000000000000000000000, 4294967295), coinbase 02c70000) CTxOut(nValue=5.00000000, scriptPubKey=037787c39547c93112fb1afd47de16) CTxOut(nValue=3.00000000, scriptPubKey=OP_DUP OP_HASH160 00e4906e47e1) In my instance, I was able to work around it by getting the masternode function to return zero; however I will point out that it occurred at a seemingly random blockheight - perhaps a conditional which is using an uninitialized variable? int64_t GetMasternodePayment(int nHeight, int64_t blockValue) { + if (nHeight <= Params().LAST_POW_BLOCK()) return 0; return blockValue * 0.6; } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#814?email_source=notifications&email_token=AKFOO5BYAZYTXE6MQGH2I33QXIE7DA5CNFSM4GZD7OWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGDMYMA#issuecomment-562482224>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFOO5FFPNJY7C2WIHLL473QXIE7DANCNFSM4GZD7OWA> .

[ComputerCraftr]

I have dealt with this bug on many PIVX and legacy Crave forks (Crave, for some reason, calculated PoW reward with current height and PoS reward with prev height) and the simplest solution in my opinion is correcting the values passed to GetBlockValue and GetMasternodePayment and then adding the following code to the beginning of GetBlockValue.

int64_t GetBlockValue(int nHeight)
{
    // account for the fact that rewards were previously calculated from prev block height rather than current height
    if (nHeight < Params().RewardSwitchoverBlock())
        nHeight--;

No changes should have to be made to GetMasternodePayment because IsBlockPayeeValid is only called on newly created blocks and not those received while syncing.

Since PIVX's block rewards have not changed in a long time, the reward switchover height could theoretically be set to any arbitrary block height in the past or future so long as it is after the last rewards change, and this would provide forks an easy method for dealing with the issue on their own chains.