Modify GetNextWorkRequired to set Target Limit correctly

[CaveSpectre11]

GetNextWorkRequired() was not setting the work required properly on the first Proof of Stake block. The code section to cap the work at the bnTargetLimit wasn't called on that first block, but rather on the second Proof of Stake block.

The end result is that the first PoS block uses the difficulty from the PoW, rather than capping it at the target limit; with the second PoS block capping it at the target limit:

uint256 bnTargetLimit = (~uint256(0) >> 24);

Creating a difficulty problem should the final PoW difficulty be greater than bnTargetLimit.

---

This code has been tested on an altcoin; and I can provide debug output demonstrating the shift of the TargetLimit to 00000FFFF the block after the first PoS block should it be desired.

[Mrs-X]

Hmh, I have to check this more closely, but on a quick look LoadExternalBlockFile() would fail because ProcessNewBlock() would fail because CheckWork()would fail because GetNextWorkRequired() would return a different nBitsRequired, thus if (block.nBits != nBitsRequired) would apply for the first PoW block.

[CaveSpectre11]

Ok, my priority of getting a dev environment to test PIVX directly has increased.

Yes, that does appear to be the case on inspection (my initial issue was on the shift from PoW to PoS; but since this seems to be a potential reindex failure, it may not be innocuous to PIVX).

[CaveSpectre11]

Running the test overnight. Have a fix coded up that I will also validate before pushing.

[CaveSpectre11]

@Mrs-X you are correct. Please see with the change in main.cpp. By placing the check inside the failure condition; the extra check doesn't add any processing speed to the general block processing (since they won't enter that line of code. Nor will existing forks that port this code over. For existing forks that port this code over and end up with the same issue (on their historical first PoS block), the conditional inside the failure code will serve as a hook for their own custom modification to get past their block.

And, forks that have not hit their PoS stage, or new coins being developed from the Pivx reference code, will not have to worry about getting stuck on their first PoS block.

From my debugging build:

2019-06-21 13:00:56 GetNextWorkRequired - pure:
        bnNew:          000000000532ea8748d8748d8748d8748d8748d8748d8748d8748d8748d8748d
2019-06-21 13:00:56 GetNextWorkRequired:
        bnNew:          000000000532ea8748d8748d8748d8748d8748d8748d8748d8748d8748d8748d
        bnTargetLimit:  000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
2019-06-21 13:00:56 CheckWork(): nbits=470117804, required=470102762
2019-06-21 13:00:56 CheckWork(): Accepting special block
2019-06-21 13:00:56 UpdateTip: new best=1c9121bf9329a6234bfd1ea2d91515f19cd96990725265253f4b164283ade5dd  height=259201 version=3  log2_work=55.779856  tx=367159  date=2016-08-17 02:40:14 progress=0.077175  cache=6335
2019-06-21 13:00:56 ProcessNewBlock : ACCEPTED Block 259201 in 72 milliseconds with size=959

[random-zebra]

Concept ACK.
Might make sense to define variables somewhere to store those values for nTime and nBits, instead of using magic numbers.

[CaveSpectre11]

Concept ACK.
Might make sense to define variables somewhere to store those values for nTime and nBits, instead of using magic numbers.

I considered that; but I opted against it specifically because of how unique that check is. Originally I was just checking for the block; but by checking nTime and nBits; that code should not pass if it's ported somewhere other than PIVX. This way it's obscure, and requires whomever ports the fix into their code base to understand exactly what they're doing, and exactly where they need to do it. If they have more than one block; it won't be obscured. If I was to define a variable, I would be compelled to define a vector and have it loop through the vector so it's a more portable).

But that's MHO; and I don't feel strongly about the opinion. I will pull it out into chainparams and define them if you wish.

[random-zebra]

I think those are valid arguments so, as far as I am concerned, this can remain as is.
Let's wait for other reviewers.

[Warrows]

I don't understand why you'd need a vector 🤔 In chainparams you could define two constants like NEXTWORK_BUG_BLOCKTIME = 1471401614 and NEXTWORK_BUG_NBITS = 0x1c056dac and use them in the test. This way you explicit what those numbers are without making the code much more complicated.
I think that's the best way to go about it.

[CaveSpectre11]

I don't understand why you'd need a vector 🤔

I'm going to try my best to lay out my position without too many tangents; but I can't make any promises :)

One thread that always runs in my coding style is reuse. It's everywhere in my current day job (basically 7 different forks of the code base I'm working on). This applies greatly to the whole crypto industry, as well. When I have something localized in a module, or a function, I'm more likely to opt for the quick route. When it extrapolates into more of a general or removed use, is where the reuse becomes more relevant. For example; 950 started with ValidateURL being local to the budget.cpp module. When it was suggested it be moved to utilities; the URL length became a relevant magic number; and thus I added it as a parameter to the function. So other callers from other uses may have different requirements for the URL length. Arguably, the hard coded "64" default in the header declaration could be made into a constant that's defined yet elsewhere; but going so far as to add the vectors to passable parameters in order for the URL content checks to be changed by the caller was a bit overkill. I did have that in mind while writing that code; choosing it such a way (the vector itself for example), so it would be an easy mod to pass in different search vectors into that routine ...should it be determined necessary for another use case.

So how this all applies to the subject at hand? I chose to use nbits and time, instead of block number; so that anyone forking pivx doesn't have a hole on that block; when they hit the block. It now has to hit the exact nbits at the exact time; which is already past. So the likelihood of another coins block being accepted is slim to none; and it's in the past as well so it shouldn't matter anyway since it's already deep in the accepted chain.

For Pivx; it was only one block affected. For others, it could be none, or many. That's when the "right" way to code that would be a vector that can cover 1 to many problem blocks. Yes, once again, a little overkill here. But I also argue that putting an exception to skip over a consensus problem into chainparams is too far disconnected from the problem to make it readable. If there was any reason that it would ever be relevant anywhere else in the code... then absolutely, a constant would be warranted.

Now, here's the parallel for Pivx specifically. For #814 to be handled properly, it needs to ignore the overmint in over a dozen blocks. Since it doesn't need to be anywhere else except ConnectBlock I would debate that it still belongs locally defined in main.cpp rather than chainparams. In the fork that I addressed the same issue as 814; it was only one effected block (and thus... coded with a magic number like this PR). For Pivx, however; and many forks, there is going to be a various number of blocks that need to be accounted for; and thus setting the framework of a vector that is populated for whatever a coin's specific instances are... becomes my recommended approach.

So with all that said... it's why I made the choices I did. I feel it's actually an appropriate use of magic numbers, and well contained in the location where it applies.

But, like most code style debates; I only casually date my code... I don't marry it, so if you guys agree "change it", your wish is my command.

[random-zebra]

For what is worth, I'm in line with these points:

But I also argue that putting an exception to skip over a consensus problem into chainparams is too far disconnected from the problem to make it readable.

I feel it's actually an appropriate use of magic numbers, and well contained in the location where it applies.

[Warrows]

Alright, I get your point and mostly agree. However, I still think magic numbers should be explicitly defined every time it's possible. So even if it's two lines above in main.cpp I'd like to see two constants defined for these numbers.

[CaveSpectre11]

I still think magic numbers should be explicitly defined every time it's possible. So even if it's two lines above in main.cpp I'd like to see two constants defined for these numbers.

Compromise accepted. All set.

[Mrs-X]

Not a big fan special processing for single blocks/transactions/whatever, but this seems to be the most reasonable way to do it.
utACK 59bff6f from my side.

[CaveSpectre11]

Not a big fan special processing for single blocks/transactions/whatever,

Agreed. If the error case wasn't there already, I would have considered letting it be; But this way there's only extra processing when the block is headed down the failure path.

[random-zebra]

Giving it some more thought... it's much better to have these constants in chainparams.

Because, right now, this only works for MainNet.
TestNet sync will obviously halt at block 201.

[CaveSpectre11]

TestNet sync will obviously halt at block 201.

Excellent point. Is there an explorer for regtest so I can get the info on block 251?

[Fuzzbawls]

Regtest is a non-persistent chain that is initialized from genesis on demand. Testnet has a BE though: https://testnet.pivx.link

[CaveSpectre11]

Regtest is a non-persistent chain that is initialized from genesis on demand. Testnet has a BE though: https://testnet.pivx.link

Ok; regtest won't apply. testnet I had already pulled.

[CaveSpectre11]

Set up for both testnet and mainnet to be covered; put stubs for regtest so regtest won't falter over the code if it ever ends up in it. I'll check travis and rebase in the morning.

[random-zebra]

ACK 2bef330

[furszy]

ACK 2bef330

[random-zebra]

Merging...