Move maximum timewarp attack threshold back to 600s from 7200s #30647

pull TheBlueMatt wants to merge 1 commits into bitcoin:master from TheBlueMatt:2024-08-conervative-timewarp changing 1 files +1 −9
  1. TheBlueMatt commented at 4:41 pm on August 13, 2024: contributor

    In 6bfa26048dbafb91e9ca63ea8d3960271e798098 the testnet4 timewarp attack fix block time variation was increased from the Great Consensus Cleanup value of 600s to 7200s on the thesis that this allows miners to always create blocks with the current time. Sadly, doing so does allow for some nonzero inflation, even if not a huge amount.

    While it could be that some hardware ignores the timestamp provided to it over Stratum and forces the block header timestamp to the current time, I’m not aware of any such hardware, and it would also likely suffer from random invalid blocks due to relying on NTP anyway, making its existence highly unlikely.

    This leaves the only concern being pools, but most of those rely on work generated by Bitcoin Core (in one way or another, though when spy mining possibly not), and it seems likely that they will also not suffer any lost work. While its possible that a pool does generate invalid work due to spy mining or otherwise custom logic, it seems unlikely that a substantial portion of hashrate would do so, making the difference somewhat academic (any pool that screws this up will only do so once and the network would come out just fine).

    Further, while we may end up deciding these assumptions were invalid and we should instead use 7200s, it seems prudent to try with the value we “want” on testnet4, giving us the ability to learn if the compatibility concerns are an issue before we go to mainnet.

  2. DrahtBot commented at 4:41 pm on August 13, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK fjahr, murchandamus, achow101
    Concept ACK Sjors, jonatack

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

  3. achow101 commented at 5:16 pm on August 13, 2024: member
    Update the BIP first?
  4. achow101 added this to the milestone 28.0 on Aug 13, 2024
  5. instagibbs commented at 5:24 pm on August 13, 2024: member
    I’m confused why is this touching ancient stuff related to every net?
  6. fjahr commented at 5:29 pm on August 13, 2024: contributor
    Should be setting MAX_TIMEWARP in validation.cpp to 600 not MAX_FUTURE_BLOCK_TIME.
  7. fjahr commented at 5:34 pm on August 13, 2024: contributor

    Concept ACK

    I was going with a more conservative approach because the timewarp attack didn’t seem highest on the wishlist of what people wanted out of Testnet4 but the argument to rather break things on testnet than later on mainnet convinced me.

  8. DrahtBot added the label CI failed on Aug 13, 2024
  9. Move maximum timewarp attack threshold back to 600s from 7200s
    In 6bfa26048dbafb91e9ca63ea8d3960271e798098 the testnet4 timewarp
    attack fix block time variation was increased from the Great
    Consensus Cleanup value of 600s to 7200s on the thesis that this
    allows miners to always create blocks with the current time. Sadly,
    doing so does allow for some nonzero inflation, even if not a huge
    amount.
    
    While it could be that some hardware ignores the timestamp provided
    to it over Stratum and forces the block header timestamp to the
    current time, I'm not aware of any such hardware, and it would also
    likely suffer from random invalid blocks due to relying on NTP
    anyway, making its existence highly unlikely.
    
    This leaves the only concern being pools, but most of those rely on
    work generated by Bitcoin Core (in one way or another, though when
    spy mining possibly not), and it seems likely that they will also
    not suffer any lost work. While its possible that a pool does
    generate invalid work due to spy mining or otherwise custom logic,
    it seems unlikely that a substantial portion of hashrate would do
    so, making the difference somewhat academic (any pool that screws
    this up will only do so once and the network would come out just
    fine).
    
    Further, while we may end up deciding these assumptions were
    invalid and we should instead use 7200s, it seems prudent to try
    with the value we "want" on testnet4, giving us the ability to
    learn if the compatibility concerns are an issue before we go to
    mainnet.
    16e95bda86
  10. TheBlueMatt force-pushed on Aug 13, 2024
  11. TheBlueMatt commented at 6:21 pm on August 13, 2024: contributor

    Update the BIP first?

    The BIP was originally 600, and then was updated to follow the code, so I figured I’d do the same :)

    I’m confused why is this touching ancient stuff related to every net?

    Cause I was distracted and just blindly tracing constants back without thinking :sweat_smile:

  12. DrahtBot removed the label CI failed on Aug 13, 2024
  13. maflcko added the label Tests on Aug 14, 2024
  14. glozow commented at 2:53 pm on August 14, 2024: member
    Makes sense to me, sanity checked you can sync with this. Is https://github.com/TheBlueMatt/bips/blob/7f9670b643b7c943a0cc6d2197d3eabe661050c2/bip-XXXX.mediawiki still the right place to see great consensus cleanup spec?
  15. fjahr commented at 9:09 am on August 15, 2024: contributor
    I have opened a PR to revert my change to the BIP here: https://github.com/bitcoin/bips/pull/1660
  16. fjahr commented at 9:11 am on August 15, 2024: contributor

    tACK 16e95bda86302af20cfb314a2c0252256d01f750

    I was able to sync with this as well (as of yesterday).

  17. glozow referenced this in commit 2f7d9aec4d on Aug 15, 2024
  18. instagibbs commented at 2:08 pm on August 15, 2024: member
    is this too hard to make a boundary test?
  19. Sjors commented at 2:45 pm on August 15, 2024: member

    Concept ACK, but afaik our own miner code needs to be updated to take this rule into account.

    https://github.com/bitcoin/bitcoin/blob/2f7d9aec4d049701fccfc029f44934d187467432/src/node/miner.cpp#L31-L38

    I tested syncing a fresh node with 16e95bda86302af20cfb314a2c0252256d01f750, which didn’t cause a fork. I did not check if any historical blocks would have been invalid if we had a time machine. That would require some old log file to get the wall clock arrival times.

  20. TheBlueMatt commented at 3:45 pm on August 15, 2024: contributor

    Makes sense to me, sanity checked you can sync with this. Is https://github.com/TheBlueMatt/bips/blob/7f9670b643b7c943a0cc6d2197d3eabe661050c2/bip-XXXX.mediawiki still the right place to see great consensus cleanup spec?

    Uhh, I mean that’s the right place for the original version, but I believe the new version(s?) live elsewhere. Dunno if there’s a formal spec for it yet but haven’t been following it super closely.

  21. Sjors commented at 5:07 pm on August 15, 2024: member

    Here’s a branch on top of this PR that add a test for the timewarp attack, and also fixes the miner.

    Maybe for this PR we should only take the fix and leave the test for later, because it’s quite slow.

    https://github.com/bitcoin/bitcoin/compare/master...Sjors:bitcoin:2024/08/conervative-timewarp

  22. maflcko commented at 5:28 pm on August 15, 2024: member

    because it’s quite slow

    Possible ideas to speed it up could be to use the “real” testnet4 blocks for the first interval, similar to ./test/functional/data/blockheader_testnet3.hex. If they are too large, one could “pre-mine” the first interval and recreate it deterministically in the test on each run, which should also be fast.

  23. murchandamus commented at 5:30 pm on August 15, 2024: contributor
    crACK 16e95bda86302af20cfb314a2c0252256d01f750
  24. DrahtBot requested review from Sjors on Aug 15, 2024
  25. Sjors commented at 7:25 am on August 16, 2024: member
    @maflcko for this test I set enforce_BIP94 = true for regtest. I just updated the branch to set nPowTargetTimespan to 144. @TheBlueMatt feel free to take all or some of that code. Otherwise I’ll open a followup.
  26. maflcko commented at 7:45 am on August 16, 2024: member

    enforce_BIP94 = true for regtest

    I guess it is fine to do, but if this change is taken, it would be good to add a short release note snippet with a Tests section about this change. Otherwise people may wonder why their fancy regtest chain isn’t working anymore due to a testnet4 rule (time-timewarp-attack). (Also, the code comment could be updated to say testnet4/regtest)

  27. Sjors commented at 8:03 am on August 16, 2024: member
    @maflcko added
  28. jonatack commented at 2:21 pm on August 17, 2024: member

    it seems prudent to try with the value we “want” on testnet4, giving us the ability to learn if the compatibility concerns are an issue before we go to mainnet.

    Concept ACK

  29. achow101 commented at 6:39 pm on August 19, 2024: member
    ACK 16e95bda86302af20cfb314a2c0252256d01f750
  30. DrahtBot requested review from jonatack on Aug 19, 2024
  31. achow101 merged this on Aug 19, 2024
  32. achow101 closed this on Aug 19, 2024

  33. Sjors commented at 8:47 am on August 20, 2024: member
    See #30681.

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-11-21 12:12 UTC

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