Move maximum timewarp attack threshold back to 600s from 7200s #30647
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
Update the BIP first? |
|
I'm confused why is this touching ancient stuff related to every net? |
|
Should be setting |
|
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. |
In 6bfa260 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.
cd2a79e to
16e95bd
Compare
The BIP was originally 600, and then was updated to follow the code, so I figured I'd do the same :)
Cause I was distracted and just blindly tracing constants back without thinking 😅 |
|
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? |
|
I have opened a PR to revert my change to the BIP here: bitcoin/bips#1660 |
|
tACK 16e95bd I was able to sync with this as well (as of yesterday). |
99eeb51 [doc] mention bip94 support (glozow) Pull request description: Followup to #29775, noticed while looking at #30604 and #30647. See [release process](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-every-major-and-minor-release). ACKs for top commit: maflcko: ACK 99eeb51 fjahr: ACK 99eeb51 tdb3: ACK 99eeb51 Tree-SHA512: 95838d3ace7e5d7b1a2481f2d7bd82902081713e6e89dbf21e0dad16d1cf5295e0c1cfda1f03af72304a5844743d24769f5fa04d4dc9f02f36462ef0ae82a552
|
is this too hard to make a boundary test? |
|
Concept ACK, but afaik our own miner code needs to be updated to take this rule into account. Lines 31 to 38 in 2f7d9ae I tested syncing a fresh node with 16e95bd, 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. |
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. |
|
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. |
Possible ideas to speed it up could be to use the "real" testnet4 blocks for the first interval, similar to |
|
@maflcko for this test I set @TheBlueMatt feel free to take all or some of that code. Otherwise I'll open a followup. |
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 |
|
@maflcko added |
Concept ACK |
|
ACK 16e95bd |
|
See #30681. |
… regtest, lower nPowTargetTimespan to 144 and add test 59ff17e miner: adjust clock to timewarp rule (Sjors Provoost) e929054 Add timewarp attack mitigation test (Sjors Provoost) e85f386 consensus: enable BIP94 on regtest (Sjors Provoost) dd154b0 consensus: lower regtest nPowTargetTimespan to 144 (Sjors Provoost) Pull request description: Because #30647 reduced the timewarp attack threshold from 7200s to 600s, our miner code will fail to propose a block template (on testnet4) if the last block of the previous period has a timestamp two hours in the future. This PR fixes that and also adds a test. The non-test changes in the last commit should be in v28, otherwise miners have to patch it themselves. If necessary I can split that out into a separate PR, but I prefer to get the tests in as well. In order to add the test, we activate BIP94 on regtest. In order for the test to run faster, we reduce its difficulty retarget period to 144, the same number that's already used for softfork activation logic. Regtest does not actually adjust its difficulty, so this change has no effect (except for `getnetworkhashps`, see commit). An alternative approach would be to run this test on testnet4, by hardcoding its first 2015 in the test suite. But since the timewarp mitigation is a serious candidate for a future mainnet softfork, it seems better to just deploy it on regtest. The next commits add a test and fix the miner code. The `MAX_TIMEWARP` constant is moved to `consensus.h` so both validation and miner code have access to it. ACKs for top commit: achow101: ACK 59ff17e fjahr: ACK 59ff17e glozow: ACK 59ff17e Tree-SHA512: 50af9fdcba9b0d5c57e1efd5feffd870bd11b5318f1f8b0aabf684657f2d33ab108d5f00b1475fe0d38e8e0badc97249ef8dda20c7f47fcc1698bc1008798830
In 6bfa260 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.