This PR addresses comment. Replaces CChainParams::MinBIP9WarningHeight with CChainParams::MinBIP9WarningStartTime. It is then used in WarningBitsConditionChecker::BeginTime to only check block headers that are newer.
This helps reduce the block headers that are being scanned for warnings.
cc @ajtowns
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
83 | @@ -84,7 +84,8 @@ class CMainParams : public CChainParams { 84 | consensus.BIP66Height = 363725; // 00000000000000000379eaa19dce8c9b722d46ae6a57c2f1a988119488b50931 85 | consensus.CSVHeight = 419328; // 000000000000000004a1b34462cb8aeebd5799177f7a29cf28f2d1961716b5b5 86 | consensus.SegwitHeight = 481824; // 0000000000000000001c8018d9cb3b742ef25114f27563e3fc4a1902167f9893 87 | - consensus.MinBIP9WarningHeight = 483840; // segwit activation height + miner confirmation window 88 | + // Preferably to be updated with every release to avoid checking older blocks for warnings
If this is to be updated every release, there need to be clear rules as how to do that.
IMO, it makes sense for it to to be updated to the day of the release with the assumption that the main chain does not contain any warning bits up until then. What are your thoughts?
Setting it to N will prevent warnings that would otherwise be triggered by version bits being set in blocks up to N+2015; so you pick a value where a reorg back to N+2015 or lower is sufficiently unlikely (for similar values of "sufficiently unlikely" as we'd use to bury successfully activated soft forks) and that getblockchaininfo from above that height doesn't already include an Unknown new rules activated warning. (If it does, it's obviously important to verify that the new rules that were signalled aren't something we should enforce first. Once that's figured out, it's then fine to ignore the signalling).
(Maybe it would be better to do std::max(MinBIP9WarningHeight, nMinerConfirmationWindow) - nMinerConfirmationWindow in WarningBitsConditionChecker::BeginTime() ; then you're just ignoring version bits prior to MinBIP9WarningHeight)
I think std::max(MinBIP9WarningHeight, nMinerConfirmationWindow) is unnecessary since I doubt that nMinerConfirmationWindow would be higher than MinBIP9WarningHeight. As for the rest, it makes sense since it removes the burden of having to keep in mind that a parameter would need adjusting in the future. I initially changed it to a timestamp due to your comment on the other PR :smile:.
It's more that MinBIP9WarningHeight might be lower, 0 in particular.
I'm not sure what I was trying to suggest in the comment above: having BeginTime() return a height doesn't make sense. Maybe I confused myself by using N as a block height when the PR at the time was already time based?
I think BeginTime() { return MinBIP9WarningTime > nPowTargetTimespan ? MinBIP9WarningTime - nPowTargetTimespan : MinBIP9WarningTime; } might make reasonable sense, with the idea being to check that there aren't any warning reported, then set MinBIP9WarningTime to a timestamp from a few months ago (and use the same timestamp for mainnet and testnet, with 0 for signet and regtest).
Went with the approach proposed by @ajtowns .
The current PR doesn't make sense to me. It's treating a height as a time? But no block would have a time before a height... So this just adds calculations that do nothing?
Are you still working on this?
Are you still working on this?
Yes, was quite busy these past couple of months.
The current PR doesn't make sense to me. It's treating a height as a time? But no block would have a time before a height... So this just adds calculations that do nothing?
You are correct, it was initially implemented as MinBIP9WarningTime but changed due to a suggestion by @ajtowns .
The implementation now uses time instead of height.
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
Concept ACK, but needs rebase and the title and description should be updated, since this is no longer replacing the height, but is instead adding logic to the start time of a period.
Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.