consensus: disable min difficulty blocks on testnet4 after height 151,200 - #34420
consensus: disable min difficulty blocks on testnet4 after height 151,200#34420batmanbytes wants to merge 10 commits into
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34420. ReviewsSee the guideline for information on the review process. LLM Linter (✨ experimental)Possible places where comparison-specific test macros should replace generic comparisons:
2026-04-09 15:10:18 |
886f755 to
2123e4d
Compare
|
It's a testnet. Any reason not to make it retroactive? |
Well, it is possible to remove 20 minutes rule entirely from the source code. But that would mean testnet3 and testnet4 in their current form, would be entirely removed, and then supported only by the old Bitcoin Core version. |
A few reasons from the top of my head:
In my opinion, the simplification from going retroactive is marginal and doesn't justify the disruption, especially with the fork height logic being only a few lines. |
In case of a hard-fork, old versions will not follow these changes anyway. If you want them, to end up in the same chain, then a different rule is needed, for example that blocks with more than 20 minutes delay, should be simply invalid (except difficulty adjustment, where you can use the real time). And then, if all ASIC blocks will be less than 20 minutes away, then they will be accepted by the old network, and will reorg CPU blocks, created in the meantime. |
fjahr
left a comment
fjahr
left a comment
There was a problem hiding this comment.
I don't have a strong opinion whether the issue needs to be fixed with a fork or not, that's something that should be judged by the users and I don't have a good picture if enough people are annoyed by this. The re-orgs have the nice effect of stress testing application robustness in this regard, something that was always promised for signet but never implemented afaik. If users would like to kick out the difficulty exception and find the PoW-powered Testnet more useful this way, then I would be happy to support it. In the development of Testnet4 early on I favored removing the exception but allowing non-standard tx relay but this was rejected by reviewers. I am not sure if there is renewed interest in changing relay along with this change but I doubt it.
To be fair, the idea of removing the exception was posted on the mailing list and bitcoin talk and I could not find any serious push-back in these discussions. To at least there appear to be no strong critique to removing the exception. Just typical bike shedding :) I have only skimmed it though for now. Maybe a delving post would be nice as well.
The non-fork fix to this is still available as re-orging the min-difficulty blocks, implemented in #31117 which I will refresh to help with discussion. I know some miners were running this code but I am not sure how large the share is. Per fork observer the re-orgs seem to have been reduced lately though. Additionally we could configure miners to always mine 2 hours in the future. This would prevent the exception from being usable typically. I preferred the above re-org variant only because it still allowed for the distribution non-standard transactions but if we are talking about removing the exception altogether with a hardfork I guess we ignore that aspect now.
Another tricky question seems on how this could be introduced with the least disruption. Since this has missed v31 and roughly the current activation height is around the v32 release, ideally I think activation should be pulled forward and if enough Testnet4 miners could be convinced to run the patch it would obviously be much easier to merge this PR into Core to have it in v32. If you are doing the outreach to miners part @batmanbytes I think that would be motivating to get more review here short term.
Last but not least you should also open an accompanying amendment pull to BIP94: https://github.com/bitcoin/bips/blob/master/bip-0094.mediawiki
Only skimmed the code superficially and left some initial comments.
| // On test networks return a minimum difficulty block after 20 minutes, | ||
| // but only if we haven't reached the fork height that disables this feature. | ||
| const auto& consensus = chainman.GetParams().GetConsensus(); | ||
| const int nNextHeight = chainman.ActiveChain().Tip()->nHeight + 1; |
There was a problem hiding this comment.
applies across the whole change: we don't use the hungarian style notation anymore for new variables
| @@ -16,10 +16,26 @@ unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHead | |||
| assert(pindexLast != nullptr); | |||
| unsigned int nProofOfWorkLimit = UintToArith256(params.powLimit).GetCompact(); | |||
|
|
|||
| // Check if min difficulty blocks are allowed at this height. | |||
There was a problem hiding this comment.
This code currently runs for every network every time. This should all be nexted within a if (params.fPowAllowMinDifficultyBlocks) block at least.
I would also like this generally extracted into a function so that it doesn't polute the rest of the code in this function and also doesn't need to be duplicated across PermittedDifficultyTransition. Ideally the whole min-difficulty handling could be extracted then.
| BOOST_CHECK(PermittedDifficultyTransition(consensus, non_retarget_height, some_difficulty, some_difficulty)); | ||
|
|
||
| // Further after fork: same strict rules | ||
| non_retarget_height = 152000; |
There was a problem hiding this comment.
below here you use a variable for the value you have hardcoded above many times. It would probably be the most readable if you have a constant "fork height" for the whole test file and then use fork height - 1 or fork height +1 etc. in the different scenarios.
| BOOST_CHECK(!PermittedDifficultyTransition(consensus, non_retarget_height, some_difficulty, nProofOfWorkLimit)); | ||
| BOOST_CHECK(PermittedDifficultyTransition(consensus, non_retarget_height, some_difficulty, some_difficulty)); | ||
|
|
||
| // Further after fork: same strict rules |
There was a problem hiding this comment.
Should also have a test that the next retargeting period is based on the 1m
| // We use 1,000,000 instead of 1 (powLimit) to avoid a block storm. | ||
| if (params.nMinDifficultyBlocksForkHeight != 0 && nNextHeight == params.nMinDifficultyBlocksForkHeight) { | ||
| arith_uint256 target = UintToArith256(params.powLimit); | ||
| target /= 1000000; |
There was a problem hiding this comment.
The 1m reset seems kind of random. It should be pretty easy to get a good approximation of what the real hashrate would be on the network without min-difficulty blocks and this way have a more calm transition.
There was a problem hiding this comment.
As I have written in the mailing list:
The number 1 million was selected arbitrarily to get the best of both worlds: (1) it won't result in a block storm (as it would if difficulty was reset to 1) and (2) it won't result in very long block intervals. If there weren't any difficulty reset, and difficulty would just be calculated as normal, it'd take around an hour for a block to be found, and this would mean epoch 75 alone would take around three months instead of two weeks.
The alternative would be to do something like new_difficulty = difficulty / 6. I just feel that this can also be a bit arbitrary, as there aren't always 6 CPU blocks for every ASIC block.
There was a problem hiding this comment.
Could be that the reason there are not always 6 CPU blocks is because some miners run the re-org code that I talked about. But why can't you calculate a hypothetical difficulty based on a chain that would be include only all the non-min difficulty blocks? Unless I am missing something the min-difficulty blocks are just "filler" within the "real" blocks. If the min-difficulty exception wouldn't exist, likely the same miners would have be running and found the same number of blocks they did now (potentially even a few more since they might lose a few seconds probably from reacting to the 6 min-difficulty blocks). So if you look at the chain without the min-difficulty blocks you should be able to get the "true" tip height. And looking at the hash rate that could be observed you should be able to calculate a "true" difficulty we should have had by now.
There was a problem hiding this comment.
And looking at the hash rate that could be observed you should be able to calculate a "true" difficulty we should have had by now.
As I wrote on bitcointalk, testnet4 produced around 30k more blocks, than it should. So, the difficulty can be just left as it is. For a while, we could have one block per hour, and then, things would naturally adjust to the proper level.
So, I think the difficulty should be left untouched. Then, the slowdown will compensate the speedup from the early days, when the difficulty climbed from 1 to the real level, and from CPU miners. And even then, I am not sure, that it would take 30k blocks, so we will still have some overproduction, even if the difficulty will be left as it is.
There was a problem hiding this comment.
@fjahr I had not thought of doing this. So, what you are proposing is to completely ignore the min-difficulty blocks at difficulty calculation in the difficulty adjustment of the 75h epoch. This is the most appropriate way to approach this, but code simplicity is also a priority.
I don't like @stwenhao's proposal, because I hate 1 hour block interval, but it is the simplest to implement.
There was a problem hiding this comment.
Another reason to leave the difficulty untouched, is to check, how fast the network will keep moving, when all blocks with CPU difficulty will be rejected. Because the whole reason, why this 20 minute rule was introduced in the first place, was to avoid the situation, when many ASICs joined the network, and then left it at some high difficulty. And then, the remaining testers were unable to move the chain forward.
So, if the difficulty will be bumped by someone (for example by some mainnet mining pool, when they will use testnet to test some things) and there will be no new blocks for a long time, then would you suggest making yet another hard-fork, to lower it again?
I hate 1 hour block interval
And you don't hate the fact, that testnet4 overproduced around 30k blocks?
genesis_time=1714777860
current_time=1773830000
time_difference=current_time-genesis_time=59052140
blocks=time_difference/600=98420
current_height=126440
overproduction=current_height-blocks=28020
adjustments=overproduction/2016=13.8
I think going from one block per hour to one block per 10 minutes will take less than 13 adjustments, which means, that if the difficulty will be left as it is, then we will still have some overproduction.
|
I have introduced AllowMinDifficultyBlocks() and GetMinDifficultyForkBits() to deduplicate min-difficulty fork handling between GetNextWorkRequired() and GetNextWorkRequired(). I have also renamed the variables from Hungarian notation to snake_case, replaced hardcoded fork heights in tests with consensus-driven values and added a test for retarget behavior after the fork. @fjahr you can review the changes. I have not changed the fork height (151200). Maybe I should change it to one year after 151200? (151200 + 2016 x 26 = 203616). I am not sure when it would be the ideal block. |
fjahr
left a comment
fjahr
left a comment
There was a problem hiding this comment.
I have not changed the fork height (151200). Maybe I should change it to one year after 151200? (151200 + 2016 x 26 = 203616). I am not sure when it would be the ideal block.
I have no idea, this is something that is probably best discussed on the mailing list and/or delving bitcoin alongside the a PR to the BIP. While I am giving you some practical feedback on the code here, I think should focus on making the BIP PR and asking these questions to a wider audience. There are other implementations and libraries that implement Testnet4 with it's difficulty exception, you will need to get them on board as well and you likely won't reach many of them here. If they don't engage right away you might need to be more proactive and open issues in their repos to get at least some acknowledgement of the project if you want to make progress quickly (but of course still be respectful).
| const auto& consensus = chainman.GetParams().GetConsensus(); | ||
| const int next_height = chainman.ActiveChain().Tip()->nHeight + 1; | ||
| if (!tip_changed && allow_min_difficulty && AllowMinDifficultyBlocks(consensus, next_height)) { |
There was a problem hiding this comment.
Even the consensus and next_height fetching should be skipped IMO
| const auto& consensus = chainman.GetParams().GetConsensus(); | |
| const int next_height = chainman.ActiveChain().Tip()->nHeight + 1; | |
| if (!tip_changed && allow_min_difficulty && AllowMinDifficultyBlocks(consensus, next_height)) { | |
| if (!tip_changed && allow_min_difficulty) { | |
| const auto& consensus = chainman.GetParams().GetConsensus(); | |
| const int next_height = chainman.ActiveChain().Tip()->nHeight + 1; | |
| if (AllowMinDifficultyBlocks(consensus, next_height)) { |
| const int next_height = pindexLast->nHeight + 1; | ||
|
|
||
| // At the fork height, reset difficulty to compensate for the | ||
| // artificially high difficulty caused by min-difficulty blocks in testnet4. | ||
| if (auto fork_bits = GetMinDifficultyForkBits(params, next_height)) { | ||
| return *fork_bits; | ||
| } | ||
|
|
||
| const bool allow_min_difficulty_blocks = AllowMinDifficultyBlocks(params, next_height); | ||
|
|
There was a problem hiding this comment.
This should all also move into the block that checks if (params.fPowAllowMinDifficultyBlocks), still still runs for every call of GetNextWorkRequired on every network and that is just not necessary.
| // At the fork height, only the reset to difficulty 1,000,000 is valid | ||
| if (auto fork_bits = GetMinDifficultyForkBits(params, height)) { | ||
| return new_nbits == *fork_bits; | ||
| } |
There was a problem hiding this comment.
This also still runs for every network on every call, same as in the other cases I commented.
| // We use 1,000,000 instead of 1 (powLimit) to avoid a block storm. | ||
| if (params.nMinDifficultyBlocksForkHeight != 0 && nNextHeight == params.nMinDifficultyBlocksForkHeight) { | ||
| arith_uint256 target = UintToArith256(params.powLimit); | ||
| target /= 1000000; |
There was a problem hiding this comment.
Could be that the reason there are not always 6 CPU blocks is because some miners run the re-org code that I talked about. But why can't you calculate a hypothetical difficulty based on a chain that would be include only all the non-min difficulty blocks? Unless I am missing something the min-difficulty blocks are just "filler" within the "real" blocks. If the min-difficulty exception wouldn't exist, likely the same miners would have be running and found the same number of blocks they did now (potentially even a few more since they might lose a few seconds probably from reacting to the 6 min-difficulty blocks). So if you look at the chain without the min-difficulty blocks you should be able to get the "true" tip height. And looking at the hash rate that could be observed you should be able to calculate a "true" difficulty we should have had by now.
Getting Testnet miners to mine the patch will not be an issue. Everyone wants this fixed, including the people doing it. Assuming the patch is fair and in line with Bitcoin. Just remove the funky difficulty stuff and let us know what to mine. It's a bad thing for the "quick" miner, but that's OK, and they are good folks who want to see Bitcoin thrive. Our company is stamped into some of the miners txid's from a good customer of ours, so you can be assured our customer friend has the blocks required to hit the numbers needed to get this all straightened out. You can verify my service is being mined into the blocks you discuss. Example: https://mempool.space/testnet4/block/0000000000808d8edc161db8811c943f817899005863a46ea724661a00b615d3 I've also spoken with Portland from MARA, and he definitely wants a fix. He expressed interest in helping if needed. I'm happy to arrange a conversation with our customer miner if interested, but ya, I don't think a talk is needed if we are just dropping the dif rule and catching >80% of the blocks for a while. (This doesn't mean the miner will for sure play ball, but they are good folks.) Thanks everyone. Respectfully, Steven Steiner |
batmanbytes
changed the title
|
Quoting myself from Bitcointalk
@fjahr what do you think? |
On the fork height increase I still can't say much other than what I already wrote above, that's something that you will need feedback from the wider community. But I think no difficulty adjustment is a good idea, less code to maintain and bikeshed over. Thanks for opening the delving thread. |
batmanbytes
changed the title
…s not a hard fork without difficulty adjustment to 1 million)
2866c62 to
0a6a819
Compare
|
After thorough discussion in Bitcointalk, we decided to make it a soft fork by simply invalidating any block with a timestamp that is greater than 20 minutes after its previous block timestamp, after fork height 151,200. This effectively fixes the issue, as min difficulty blocks can only be sent with a timestamp that is greater than 20 minutes than their previous blocks' timestamp. The problem with the current hard fork approach is that it results in a chain split, where non-upgraded clients will continue having a working chain, with just less hashrate, but no significant incentive to switch to the hard forked chain. As consequence, neither do the miners have a significant incentive to mine in a hard forked chain that may be forgotten after a few months, if Bitcoin Core never cares to merge it. With the soft fork approach, Bitcoin Core may never merge it, but miners can still force their stricter ruleset to the non-upgraded clients. I opened a new PR in: #35081. Further discussion can continue there. |
7 participants
Summary
Disable the min-difficulty block rule on testnet4 after block height 151,200 (epoch 75 boundary).
Motivation
Testnet4's min-difficulty rule (allowing difficulty-1 blocks after 20 minutes) is being exploited, causing ~85-90% of blocks to be CPU-mined min-difficulty blocks. This results in a race of CPU miners broadcasting blocks at exactly the second that it's acceptable. This race is so intense that CPU miners have figured out sending empty blocks, as they propagate faster than those that contain transactions, resulting in a network where transactions are confirmed at only the remaining ASIC blocks, that have ~1 hour average block times.
After the fork:
Changes
nMinDifficultyBlocksForkHeightconsensus parameter (default 0 = disabled)GetNextWorkRequired()to disable min-difficulty rule at fork height 151,200PermittedDifficultyTransition()to enforce the new rulesWaitAndCreateNewBlock()in miner to respect the forkTest Plan
./build/bin/test_bitcoin --run_test=pow_tests- all 21 tests passDiscussion
bitcoin-dev mailing list thread #1
bitcoin-dev mailing list thread #2
Bitcointalk thread