Veil-Project / veil

Veil-Project
MIT License
117 stars 91 forks source link

Staking effectively disabled if no block for 60 minutes #957

Open CaveSpectre11 opened 3 years ago

CaveSpectre11 commented 3 years ago

The problem code is in miner.cpp:

            if (!gArgs.GetBoolArg("-genoverride", false) && (GetAdjustedTime() - nTimeLastBlock > 60*60 || IsInitialBlockDownload() || !g_connman->GetNodeCount(CConnman::NumConnections::CONNECTIONS_ALL) || nHeight < Params().HeightPoSStart())) {
                MilliSleep(5000);
                continue;
            }

In commit 5b7a6a67770444640b79476cd9d5b9187ce2520a things started to go down this path; that results in the staking effectively being turned off if there's no block in 3600 seconds. The intent of genoverride here is to allow another mined block to unstick the chain.

genoverride is a developer tool; but the basic thing is that if there's a significant drop in staking power, and we hit the limit on mined blocks in a row; we should still wait for a stake, for however long it takes, without needing a gen-overridden mined block to come.

Zannick commented 3 years ago

Would there be any negative effect from just removing this time check? GetAdjustedTime() - nTimeLastBlock > 60*60

CaveSpectre11 commented 3 years ago

Would there be any negative effect from just removing this time check? GetAdjustedTime() - nTimeLastBlock > 60*60

The part that just logically throws me for a loop is that what it -should- allow is: If there's been too many consecutive mining blocks so the next block -has- to be a staking block, but there's not been a block for 6 minutes, it re-allows mining for another block.

Zannick commented 3 years ago

The part that just logically throws me for a loop is that what it -should- allow is: If there's been too many consecutive mining blocks so the next block -has- to be a staking block, but there's not been a block for 6 minutes, it re-allows mining for another block.

That's going to be a consensus change, because we're not turning off mining but rejecting the pow block: https://github.com/Veil-Project/veil/blob/27d0b70c2371e339082ebc99303674ef72424ef1/src/validation.cpp#L4667-L4671

The intent of genoverride here is to allow another mined block to unstick the chain.

I don't think that works, then, because this is only in staking code: the effect is only to allow the user of genoverride to create a stake after 3600 seconds. This would only be a change on the staking side, unless I missed a consensus check somewhere where we reject stake blocks after 3600 seconds (I don't think we do or we would have failed to revive testnet as much as we have).

Also, oops, 3600 seconds = 60 minutes, I don't know how I misread that before. That's a lot less worrisome than I initially thought. :)

seanPhill commented 3 years ago

Aha. 60 minutes gels with our experience of testnets getting stalled. I don't really mind so long as we remember to include some PoW when getting things running.