Remove LOCKTIME_MEDIAN_TIME_PAST constant#24565
Hidden character warning
Conversation
src/validation.cpp
Outdated
There was a problem hiding this comment.
Perhaps
// Cannot get MTP for parent of genesis block, so only potentially enforce post genesis
const bool enforce_locktime_median_time_past = (pindexPrev != nullptr) && DeploymentActiveAfter(..);(Not a hardfork, since it only affects the validity of the genesis block...)
(alternatively const bool enforce = DeploymentActiveAfter(..); and assert(!(enforce && pindexPrev == nullptr));)
There was a problem hiding this comment.
I think the proper solution would be to not call this function with a nullptr. However, my goal is to remove the constant, so I'll leave logic changes to future pull requests.
|
Edit: In light of #24567, this makes sense. |
|
I think this change can be evaluated on it's own, ignoring other stuff that builds on top of it. I don't see how passing in a flag to a function that never reads it could possibly make any sense. This is only asking for more bugs like the ones mentioned in #24080 (comment) to appear in the future. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
maflcko
force-pushed
the
2203-remConstant-
branch
from
fade3d0 to
faaa2d8
Compare
maflcko
force-pushed
the
2203-remConstant-
branch
from
faaa2d8 to
fa1fe2e
Compare
|
utACK fa1fe2e Some extra context: the constant It was used in This was two (?) years before BIP 113 activated and enforced it for blocks. When BIP 113 was buried in #16060 1c93b9b we still had miner code using the constant. But that was removed in #23637. |
maflcko
force-pushed
the
2203-remConstant-
branch
from
fa1fe2e to
faf6101
Compare
maflcko
changed the title
maflcko
force-pushed
the
2203-remConstant-
branch
from
faf6101 to
fa8b61e
Compare
maflcko
changed the title
maflcko
force-pushed
the
2203-remConstant-
branch
from
fa8b61e to
fa1fe2e
Compare
|
A kind offline reviewer asked if the other flag can be turned into a bool, so I did that in commit fa8b61e19f. While this can be done, I think it should be done in a follow-up to keep this pull request focussed on un-exposing stuff in policy that has nothing to do with policy and isn't used in policy at all. |
fanquake
requested review from
ajtowns and
instagibbs
and removed request for
ajtowns
fa1fe2e Remove LOCKTIME_MEDIAN_TIME_PAST constant (MarcoFalke) Pull request description: The constant is exposed in policy code, which doesn't make sense: * Wallet and mempool need to assume the flag to be always active to function properly. * Setting (or unsetting) the flag has no effect on policy code. The constant is only used in `ContextualCheckBlock` (consensus code) to set a flag and then read the flag again. I think this can be better achieved by using a `bool`. If there is a need to use a flag in the future, it will be trivial to do so then. (The previous use for the constant was removed in df562d6) ACKs for top commit: Sjors: utACK fa1fe2e glozow: code review ACK fa1fe2e, AFAICT this is safe and makes sense as `SequenceLocks` doesn't use it, wallet/ATMP no longer need it since bitcoin#24080, and `ContextualCheckBlock` effectively uses it as a roundabout boolean. instagibbs: utACK fa1fe2e Tree-SHA512: de1972498c545d608a09630d77d8c7e38ed50a6ec40d6c0d720310a1647ed5b48b4ace0078c80db10e7f97aacc552fffae251fe3256e9a19a908b933ba2dc552
9 participants
The constant is exposed in policy code, which doesn't make sense:
The constant is only used in
ContextualCheckBlock(consensus code) to set a flag and then read the flag again. I think this can be better achieved by using abool. If there is a need to use a flag in the future, it will be trivial to do so then.(The previous use for the constant was removed in df562d6)