Miner: never create a template which exploits the timewarp bug #31376

1. 
   darosior commented at 6:05 pm on November 26, 2024: member

   This check was introduced in #30681 but only enabled for testnet4. To avoid potentially creating an invalid block template if a soft fork to fix the timewarp attack were to activate in the future, we should have this check on all networks. It also seems wise for our miner to not support it whether or not a soft fork activates to fix it at the consensus level.
2. miner: never create a template which exploits the timewarp bug 733fa0b0a1
3. 
   DrahtBot commented at 6:05 pm on November 26, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31376.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	TheCharlatan

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. DrahtBot added the label Mining on Nov 26, 2024
5. 
   TheCharlatan commented at 6:21 pm on November 26, 2024: contributor

   Concept ACK
6. 
   Sjors commented at 12:30 pm on November 28, 2024: member

   I’m not sure. Maybe it’s better to wait with this change until a timewarp fix soft fork is further along the proposal process (though not necessarily deployed, or even in a signalling phase). E.g. I’d rather not have to think about different Bitcoin Core versions having a different value for MAX_TIMEWARP.
7. 
   darosior commented at 5:13 pm on November 30, 2024: member

   Why? As mentioned in OP it would only delay such a soft fork if there is a concern that miners could produce an invalid block template, and we can choose our software to not support a timewarp attack without considering whether it might be fixed at the consensus level at all.

   I’d rather not have to think about different Bitcoin Core versions having a different value for MAX_TIMEWARP.

   I don’t understand. MAX_TIMEWARP would never come into play in any normal circumstance. What is your concern?

8. 
   Sjors commented at 10:37 am on December 2, 2024: member

   in any normal circumstance

   By normal circumstance you mean when nobody else on the network is attempting a timewarp attack? But what if there is one?

   Let’s say the soft fork is deployed in the future with MAX_TIMEWARP set to 300. But honest miner Alice is still running the v29 release with this PR it, using 600. Mean miner Bob now wants to produce a block at the end of the retarget period such that others accidentally produce an invalid block. They would set it 600 seconds in the future, which means Alice will produce an invalid block if she finds one within 300 seconds.

   The other way around there’s no problem. E.g. if the eventual softfork uses MAX_TIMEWARP = 1200, Bob can set the timestamp 1200 seconds in the future, Alice will needlessly bump her block 600 seconds to the future, but she’ll be fine.

   Maybe this isn’t a big deal, since in order to be safe miners have to upgrade to a version with the correct MAX_TIMEWARP. But then maybe we should just use 0 for the miner code.

9. 
   darosior commented at 3:06 pm on December 2, 2024: member

   By normal circumstance you mean when nobody else on the network is attempting a timewarp attack? But what if there is one?

   Sure, a timewarp attack is not normal circumstances. But even then this change would not be harmful. It would merely not be supporting attackers. Which as i argued in OP i see as a benefit.

   Let’s say the soft fork is deployed in the future with MAX_TIMEWARP set to 300. But honest miner Alice is still running the v29 release with this PR it, using 600. Mean miner Bob now wants to produce a block at the end of the retarget period such that others accidentally produce an invalid block. They would set it 600 seconds in the future, which means Alice will produce an invalid block if she finds one within 300 seconds.

   I’m confused. This is exactly the concern that this PR intends to address. That in the event of a soft fork invalidating timewarp attacks, a miner might be tricked into creating an invalid block. But this PR does not make any block invalid. This is therefore not a concern with this PR but merely with a future one implementing a soft fork, addressed by this very PR you are objecting to!

   With regard to “but what if we decide to make MAX_TIMEWARP 300 seconds instead?”. First of all i don’t see any reason for reducing it further. 600 seconds makes sense. If anything we might have to increase it. In the highly unlikely scenario that a soft fork with a lower value is considered, then we would have to adapt it in the miner code first as done here. And we would have to do it whether or not we first set this value to 600 here. I don’t see how this is an objection to this PR.

10. in src/node/miner.cpp:39 in 733fa0b0a1

    40-            nNewTime = std::max<int64_t>(nNewTime, pindexPrev->GetBlockTime() - MAX_TIMEWARP);
    41-        }
    42+    // Height of block to be mined.
    43+    const int height{pindexPrev->nHeight + 1};
    44+    if (height % consensusParams.DifficultyAdjustmentInterval() == 0) {
    45+        nNewTime = std::max<int64_t>(nNewTime, pindexPrev->GetBlockTime() - MAX_TIMEWARP);
    

    ---

    ---

    

    Sjors commented at 8:24 am on December 3, 2024:

    So why not drop - MAX_TIMEWARP and just make sure that we never propose a block with an earlier timestamp?

    Or go beyond that and never allow time to go backwards on any block?

    ---

    

    darosior commented at 4:36 pm on December 3, 2024:

    Because it is unnecessary?

    ---

    

    Sjors commented at 6:32 am on December 17, 2024:

    I finally wrapped my head around the purpose of - MAX_TIMEWARP. It’s in fact very important that we use it here and also that we don’t reduce it.

    The original Great Consensus Cleanup soft fork proposal by @TheBlueMatt says the following:

    • Sadly, some deployed mining hardware relies on the ability to roll nTime forward by up to 600 seconds[3]. Thus, only requiring that the nTime field move forward during difficulty adjustment would allow a malicious miner to prevent some competitors from mining the next block by setting their timestamp to two hours in the future. Thus, we allow nTime to go backwards by 600 seconds, ensuring that even a block with a timestamp two hours in the future allows for 600 seconds of nTime rolling on the next block.

    The only way to protect this specific mining hardware is by us setting nTime no higher than 2 hours minus 10 minutes in the future. We should add a comment to that effect here.

    I also think we should increase this number (can’t do that here of course, it’s testnet4 consensus): https://delvingbitcoin.org/t/timewarp-attack-600-second-grace-period/1326

    ---

    

    darosior commented at 2:27 pm on December 17, 2024:

    Yes. So i say let’s merge this and if we end up wanting to use a larger MAX_TIMEWARP as part of a mainnet soft fork, we’ll be covered with the tighter value here.

    Should i just add the comment as you suggested and we’re good here?

    ---

    

    Sjors commented at 3:03 am on December 18, 2024:

    I initially thought yes, but no. It reduces the tolerance for bad clocks from 2 hours to 10 minutes, if an adversarial miner pushes the timestamp of the last block to the limit. Such a miner would take a risk themselves, but still.
11. 
    Sjors commented at 8:26 am on December 3, 2024: member

    But this PR does not make any block invalid. This is therefore not a concern with this PR

    No, but it’s adding complexity that might not match the actual soft fork. And then we have to touch those lines again, which is never great, even if it’s probably fine. See my inline comment for a more generic approach (but I’m not sure if it’s safe).

Contributors
darosior DrahtBot TheCharlatan Sjors

Labels
Mining