validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime #27427
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. |
e6b570b to
3460721
Compare
src/kernel/chainparams.cpp
Outdated
| @@ -84,7 +84,8 @@ class CMainParams : public CChainParams { | |||
| consensus.BIP66Height = 363725; // 00000000000000000379eaa19dce8c9b722d46ae6a57c2f1a988119488b50931 | |||
| consensus.CSVHeight = 419328; // 000000000000000004a1b34462cb8aeebd5799177f7a29cf28f2d1961716b5b5 | |||
| consensus.SegwitHeight = 481824; // 0000000000000000001c8018d9cb3b742ef25114f27563e3fc4a1902167f9893 | |||
| consensus.MinBIP9WarningHeight = 483840; // segwit activation height + miner confirmation window | |||
| // Preferably to be updated with every release to avoid checking older blocks for warnings | |||
There was a problem hiding this comment.
If this is to be updated every release, there need to be clear rules as how to do that.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
(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)
There was a problem hiding this comment.
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 😄.
There was a problem hiding this comment.
It's more that MinBIP9WarningHeight might be lower, 0 in particular.
There was a problem hiding this comment.
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).
d477d40 to
8d39803
Compare
|
Went with the approach proposed by @ajtowns . |
8d39803 to
eea2952
Compare
|
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? |
eea2952 to
8fa7ece
Compare
Yes, was quite busy these past couple of months.
You are correct, it was initially implemented as |
8fa7ece to
31a810f
Compare
|
🐙 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. |
This PR addresses comment. Replaces
CChainParams::MinBIP9WarningHeightwithCChainParams::MinBIP9WarningStartTime. It is then used inWarningBitsConditionChecker::BeginTimeto only check block headers that are newer.This helps reduce the block headers that are being scanned for warnings.
cc @ajtowns