Testnet4 including PoW difficulty adjustment fix #29775
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. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK |
|
When resetting a test chain, it is also important to consider the script interpreter coverage of the current chain. Test chains are (usually) the first place to go to, to test new script primitives and protocols, as well as consensus deployments. The existing chain thus serves as a test for consensus implementations, apart from basic unit test vectors. It would be good to think how to preserve the test vectors in the chain. See also #11739 (comment) . Or if it is not needed, it would be good to say so. Maybe https://github.com/bitcoin-core/qa-assets/blob/main/unit_test_data/script_assets_test.json already covers a good portion? Moreover, testnet is the only public chain where anyone can submit a nonstandard transaction from their laptop. Recall that policy is enforced on all networks equally (see commit e1dc15d), so getting a non-mempool transaction into a block is only possible for a miner, or by cooperating with a miner. So if the difficulty hack is removed completely, anyone wishing to submit a transaction would have to go purchase and set up mining hardware, or find a miner willing to accept the transaction. Not saying what is the best approach here, just saying that the effects should be considered and a change be done intentionally. |
|
Probably we should support tracking both testnet3 and the new testnet4 for some time. Making the new code conditional on a different chain param that's only set for testnet4 would probably be the easiest way of accomplishing that? |
fjahr
force-pushed
the
2024-04-testnet-4-fix
branch
from
61e51d1
to
7fbd4c6
Compare
|
Pushed some improvements and addressed some feedback. I am experimenting with some of the proposals from the mailing list and so I added Andres Poelstra's suggested difficulty adjustment with 6h/1M from here: https://groups.google.com/g/bitcoindev/c/9bL00vRj7OU/m/kFPaQCzmBwAJ
Updated the code to introduce T4 ( I am using the Genesis block hash to distinguish between the two testnets. There may be cleaner solutions but I think this is ok since it would be only temporary until T3 is removed.
Is it really realistic that someone with just their CPU would be able to mine a block with their non-standard tx on the current testnet? If the bug isn't active currently they would need to wait for it to become active and that could take weeks, right? And when it becomes active I would imagine the miner who found the first block in the difficulty=1 series just blasts the network and the CPU miner still has no chance to get a block in between. We could revert #28354 for testnet4 if this is a feature that matters to users. Is it too much to ask that people use
Interesting thought. I think once there is consensus to do T4 we will find a creative solution for this. Cool would be to convert this coverage to fuzzing coverage somehow but I am not sure if that's realistic or worth the effort. Otherwise, we could write a program that looks at all the different scripts that exist on T3 and replays them on T4 or if we can compress them somehow like by filtering everything that doesn't add coverage, then we turn it into a unit test that replays the interesting scripts. |
fjahr
force-pushed
the
2024-04-testnet-4-fix
branch
from
7fbd4c6
to
aaed0d3
Compare
|
Since some people consider the blockstorms an interesting feature of Testnet3, it might be interesting to only raise the difficulty of the delayed block exception to 100,000 instead of 1,000,000. This would allow the network to return to the organic difficulty in fewer difficulty periods and slow down the blockstorms but not remove the feature altogether. My understanding is that this would correspond roughly a tenth of one S9 mining on the network, so if no one had mined for a while, a single S9 could restart the network with ~60 s blocks, but wouldn’t churn out thousands of blocks per second. Only allowing lower difficulty blocks after 6 hours could easily make testnet useless for extended periods, if someone put several ASICs on testnet for a while, it might prevent other users from getting confirmations for up to 6 hours. I could see an increase from the twenty minute rule to maybe an hour, but more seems counter to why the rule was introduced in the first place. |
Yes, I am not sure what would be the problem. All you have to do is to set the time +20min and mine a block on your laptop. If you don't want to try it yourself, you can come by to watch it on my laptop.
After a quick chat with @murchandamus, an alternative fix would be to require the pre-retarget block to have the "correct" difficulty, so that all retarget periods are organic. The +20min hack would remain to allow a CPU to mine a few blocks, if needed, however, a block storm would be naturally limited by the +120h cut-off rule. This would limit the block storms to small block "gusts", which seems good enough to make everyone happy? |
|
I spun up When running with If anyone wants to deploy a faucet, let me know and I'll send some coins... unless someone reorgs me. |
|
This seems too complicated for a testnet exception IMO. And it breaks the use case of someone testing being able to mine a block on-demand without actual mining hardware. Shouldn't it be enough to just fix the timewarp bug? |
I doubt many people do that. You can still set |
Two people raised the concern in this thread, so why would you doubt it? |
I missed that response, so if this is possible at any time with or without a block storm happening, I am not sure how the change here is making a difference? I will give it a try. |
"Many" is very relative but I think we probably would not see a market for trading testnet coins against bitcoin if that is something everyone can do as easily as setting a bitcore core node for example. |
If it depends on the difficulty being 1 rather 1 million, that would make a difference. The two people who brought it up can definitely recompile, but maybe there's a better solution - maybe just a startup flag to override the minimum difficulty? |
I don't think consensus rules of remote nodes can be affected by a local startup flag (or re-compilation). If someone wanted to create a block locally only, they could use regtest. |
|
Tested ACK, commit 06c2c71 It works, my current tip is UPDATE: Everything works fine, also Thank you! |
|
TACK 06c2c71 |
With zero changes to Core, signet could serve the same need to get non standard transactions into test blocks. Signet could have a website where you submit nonstandard transactions for the miner to include. This would directly parallel how out-of-band transactions are added by miners on mainnet. |
|
tACK - 06c2c71 Have been mining and stress testing (reorgs) and functions as expected. Nuanced testing and full review have not been completed. |
|
tACK 06c2c71 |
|
tACK 06c2c71 I just tested mining with CPUs and a S9 ASIC. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
| m_assumed_blockchain_size = 0; | ||
| m_assumed_chain_state_size = 0; | ||
|
|
||
| const char* testnet4_genesis_msg = "03/May/2024 000000000000000000001ebd58c244970b3aa9d783bb001011fbe8ea8e98e00e"; |
There was a problem hiding this comment.
Wouldn't it be better to defer the creation of the genesis block until a much farther date, so commit to changing during the rc phase, until the final release. We'd then include a similar mainnet block height hash or w/e as well. This way people aren't mining the chain for months before it's included in a real release.
There was a problem hiding this comment.
Wouldn't it be better to defer the creation of the genesis block until a much farther date, so commit to changing during the rc phase, until the final release. We'd then include a similar mainnet block height hash or w/e as well.
We could still reset it before the release if reviewers really think it's necessary (I said so early in the discussion here as well) but we couldn't have tested that the PR works without a Genesis block. And think it's good we discussed what's to be included in the message early and didn't bikeshed that part last minute before the release.
This way people aren't mining the chain for months before it's included in a real release.
It's not clear to me why that is a problem though.
There was a problem hiding this comment.
It could be a problem if we want test vectors early in the chain. But that can be fixed by a reset once those test vectors are available.
There was a problem hiding this comment.
I don't think the fact the chain has been mined on has any large impact and may even reduce the value proposition later on since there would be 'large' swaths of coins that the early devs have.
Disclaimer: I mined 800K+ testnet4 coins with 700Th/s. I also don't mind burning those coins with OP_RETURN if desired by the community.
There was a problem hiding this comment.
@russeree the use case for these test vectors is for regression testing, so you'd spin up a testnet4 node, sync a few thousand blocks and delete it again. Having such a small number of blocks might allow you to store the actual blocks on the CI server - so it doesn't need to make network requests.
|
tACK 06c2c71 |
There was a problem hiding this comment.
tACK 06c2c71
There's a bunch of things here and in earlier comments that warrant followup, but not worth losing ACKs.
We currently have 24729 blocks and the difficulty is 16,777,216. That makes sense: we've had 12 retarget periods, and if each time the difficulty maximally increased, you get exactly 4^12.
In order to get test vectors in early, we'd have to reset with a new genesis block and immediately publish the transactions.
It would be nice to set the genesis block difficulty to whatever the network difficulty is when we reset it. But that's pure cosmetics.
I reviewed the timewarp commit, because ideally we get this right the first time. Then a mainnet soft fork only needs to change the if (consensusParams.hashGenesisBlock... check with DeploymentActiveAfter.
| @@ -4028,6 +4028,16 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio | |||
| if (block.GetBlockTime() <= pindexPrev->GetMedianTimePast()) | |||
| return state.Invalid(BlockValidationResult::BLOCK_INVALID_HEADER, "time-too-old", "block's timestamp is too early"); | |||
|
|
|||
| // Testnet4 only: Check timestamp against prev for difficulty-adjustment | |||
| // blocks to prevent timewarp attacks (see https://github.com/bitcoin/bitcoin/pull/15482). | |||
| if (consensusParams.hashGenesisBlock == uint256S("0x00000000da84f2bafbbc53dee25a72ae507ff4914b867c565be350b0da8bf043")) { | |||
There was a problem hiding this comment.
06c2c71: we have to remember to update this hardcoded value in three different places if we reset testnet4 (before the release). I verified that it currently matches the actual genesis block (getblockhash 0).
It's not dangerous to mainnet if we forget, it would only break testnet4. But I would prefer to introduce an enum class Network so we can do consensusParams.network == Network::Testnet4. It can wait for a followup.
If you're worried about growing struct Params you can remove fPowNoRetargeting and replace its use with consensusParams.network == Network::Regtest`. Basically anything that's only used by a single test network might as well use this more direct approach.
| // Testnet4 only: Check timestamp against prev for difficulty-adjustment | ||
| // blocks to prevent timewarp attacks (see https://github.com/bitcoin/bitcoin/pull/15482). | ||
| if (consensusParams.hashGenesisBlock == uint256S("0x00000000da84f2bafbbc53dee25a72ae507ff4914b867c565be350b0da8bf043")) { | ||
| if (pindexPrev->nHeight % consensusParams.DifficultyAdjustmentInterval() == consensusParams.DifficultyAdjustmentInterval() - 1) { |
There was a problem hiding this comment.
06c2c71: Suggested comment:
// For the first block of each difficulty adjustment interval,
// except the genesis block.| @@ -4028,6 +4028,16 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio | |||
| if (block.GetBlockTime() <= pindexPrev->GetMedianTimePast()) | |||
| return state.Invalid(BlockValidationResult::BLOCK_INVALID_HEADER, "time-too-old", "block's timestamp is too early"); | |||
|
|
|||
| // Testnet4 only: Check timestamp against prev for difficulty-adjustment | |||
There was a problem hiding this comment.
06c2c71: I initially thought that this should be moved to ConnectBlock, somewhere in the "Sanity checks", maybe right before bool fScriptChecks = true;. But it's fine here.
It doesn't need to be here, because we don't use the system clock for this check. However it's more readable to have it right next to the time-too-old check. So is that safe?
ContextualCheckBlockHeader is only called when we receive a header. It's called by AcceptBlockHeader (and by the miner), which in turn is called by ProcessNewBlockHeaders (header message or compact block) and AcceptBlock (block message, to store on disk). Notably it's not called during a reorg or when doing a reindex. However in both these cases the headers will have been previously checked for this issue.
| bnNew.SetCompact(pindexLast->nBits); | ||
|
|
||
| // Special difficulty rule for Testnet4 | ||
| if (params.fPowAllowMinDifficultyBlocks && params.hashGenesisBlock == uint256S("0x00000000da84f2bafbbc53dee25a72ae507ff4914b867c565be350b0da8bf043")) { |
There was a problem hiding this comment.
e172a96: params.fPowAllowMinDifficultyBlocks && is redundant. Though you could an assert.
| if (params.fPowAllowMinDifficultyBlocks && params.hashGenesisBlock == uint256S("0x00000000da84f2bafbbc53dee25a72ae507ff4914b867c565be350b0da8bf043")) { | ||
| // Use the last non-special-min-difficulty-rules-block | ||
| const CBlockIndex* pindex = pindexLast; | ||
| const unsigned int pow_min{bnPowLimit.GetCompact()}; |
There was a problem hiding this comment.
e172a96: const uint32_t? (matching return type of GetCompact())
| // Use the last non-special-min-difficulty-rules-block | ||
| const CBlockIndex* pindex = pindexLast; | ||
| const unsigned int pow_min{bnPowLimit.GetCompact()}; | ||
| while (pindex->pprev && pindex->nHeight % params.DifficultyAdjustmentInterval() != 0 && pindex->nBits == pow_min) |
There was a problem hiding this comment.
e172a96: this had me confused again, so maybe a comment is helpful.
If miners go away in the middle of difficulty adjustment period A, this rule will find the last "real" nBits value and apply it to the first block of period B. If nothing changes, then for period C this seeks back to the first block of period B, finds the real value and applies it.
But then how do the first blocks of B and C ever mined?
Well, remember that GetNextWorkRequired ignores the nBit value if enough time went by.
A test would be nice, but unfortunately it seems the only way to do that is to introduce a new regtest with the same rule, but at much lower difficulty.
There was a problem hiding this comment.
Could one possibly test with -noconnect, i.e. without connecting to the network and getting any other than genesis testnet4 block?
Yes, I'd be happy to set up a Testnet4 faucet similar to Alt Signet Faucet. Please send me some testnet coins to |
|
If someone has coins they don't need, they can also donate them to the donate address of the faucet: https://coinfaucet.eu/en/btc-testnet4/ tb1qn9rvr53m7qvrpysx48svuxsgahs88xfsskx367 |
Podelil som sa s tebou, svet je maly 😃 |
Thank you! One new testnet4 faucet running at https://testnet4.anyone.eu.org/ |
|
testnet4 faucet up at https://mempool.space/testnet4/faucet |
To supplement the ongoing conceptual discussion about a testnet reset I have drafted a move to v4 including a fix to the difficulty adjustment mechanism, which was part of the motivation that started the discussion.
Conceptual considerations:
CalculateNextWorkRequiredfunction and uses the same logic used inGetNextWorkRequiredto find the last previous block that was not mined with difficulty 1 under the exceptionf. An alternative fix briefly mentioned on the mailing list by Jameson Lopp would be to "restrict the special testnet minimum difficulty rule so that it can't be triggered on the block right before a difficulty retarget". That would also fix the issue but I find my suggestion here a bit more elegant.