I looked at the diff file you proposed very thoroughly. And I must conclude that your fix is nothing more but a bullshit. The only thing your fix does - it protects PastRateActualSeconds varibale in a way that it is always >= 1 (second). Old code assumed it was always >= 0. You probably cared about PastRateAdjustmentRatio variable which is equal to 1 if PastRateActualSeconds happens to be 0. But the point is that it never happens. KGW takes at least PastBlocksMin and at most PastBlocksMax blocks into the calculation. You will never have PastRateActualSeconds == 0 except for the case if your blockchain has only one block.
You have misunderstood the fix. There are 2 fixes and the one you are referring fixes another attack vector.
The fix to usual TW attack (which BCX was planning was to use) was
to use LatestBlockTime instead of BlockLastSolved->GetBlockTime() to count the timespan. Without this one can timetravel back without diff rise, with this the benefit attacker gets by travellin past is lost.
The another fix (preventing PastRateActualSeconds to go to 0) takes care of another attack vector. Here is a short explanation of the attack:
1. generate a block 2 weeks to the future. You cannot publish it, it is not on current time window.
2. Start generating blocks with the same timestamp (ie the moment 2 weeks in the future)
See what would happen: after there is PastBlocksMax blocks in the private chain, *the diff would not change* at all!
That would mean you have 2 weeks to generate blocks with 0 difficulty. With decent hashrate, you easily get 1 block in a second. In 2 weeks you get 1209600 blocks.
When that 2 weeks has passed, what would happen to the blockchain, if you suddenly publish 1209600 perfectly valid blocks? The whole network would be doing nothing but checking those 1209600 blocks... and finding nothing wrong with them. That would be the end of the coin.
You will never have PastRateActualSeconds == 0 except for the case if your blockchain has only one block.
Thats not true. You can generate blocks with the same timestamp. Or is there something that would prevent it (I have not read all the code, it might be prevented somewhere) ? If there is, then this attack vector was already closed and this part was not necessary.
EDIT: Actually, it *is* prevented somewhere else. One can generate only 5 blocks with the same timestamp. So this #2 fix is not necessary to prevent that attack vector, it is already closed elsewhere. However that means the whole if clauses are never true, ie they are itself worthless. But leaving them as they were would keep there an unecessary dependancy between the code blocks, so it is nevertheless better to change it. Also, the main fix is #1, which has been confirmed to work.
// Check timestamp against prev
if (GetBlockTime() <= pindexPrev->GetMedianTimePast())
return error("AcceptBlock() : block's timestamp is too early");
