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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
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
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).
std::max(MinBIP9WarningHeight, nMinerConfirmationWindow) - nMinerConfirmationWindow in WarningBitsConditionChecker::BeginTime() ; then you’re just ignoring version bits prior to MinBIP9WarningHeight)
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:.
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).
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.
🐙 This pull request conflicts with the target branch and needs rebase.
