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

pull darosior wants to merge 1 commits into bitcoin:master from darosior:2411_miner_never_timewarp changing 1 files +4 −6
  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?
  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).


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me