Skip to content

Remove LOCKTIME_MEDIAN_TIME_PAST constant #24565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 28, 2022
Merged

Remove LOCKTIME_MEDIAN_TIME_PAST constant #24565

merged 1 commit into from
Jun 28, 2022

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Mar 14, 2022 •
edited
Loading

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)

ajtowns
ajtowns reviewed Mar 14, 2022
View reviewed changes
Copy link
Contributor

@ajtowns ajtowns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

src/validation.cpp
if (DeploymentActiveAfter(pindexPrev, consensusParams, Consensus::DEPLOYMENT_CSV)) {
assert(pindexPrev != nullptr);
nLockTimeFlags |= LOCKTIME_MEDIAN_TIME_PAST;
enforce_locktime_median_time_past = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@maflcko maflcko mentioned this pull request Mar 15, 2022
Closed
@luke-jr
Copy link
Member

luke-jr commented Mar 18, 2022 •
edited
Loading

Weak Concept NACK, I think it's cleaner how it currently is. (If it was the only flag, it'd be another matter.)

Edit: In light of #24567, this makes sense.

@maflcko
Copy link
Member Author

maflcko commented Mar 19, 2022

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.

@DrahtBot DrahtBot mentioned this pull request Mar 19, 2022
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 19, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

w0xlt
w0xlt approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review ACK fade3d0

@maflcko maflcko force-pushed the 2203-remConstant- branch from fade3d0 to faaa2d8 Compare May 13, 2022 11:17
@fanquake fanquake requested a review from ajtowns June 10, 2022 08:12
@DrahtBot DrahtBot mentioned this pull request Jun 16, 2022
Merged
@maflcko maflcko force-pushed the 2203-remConstant- branch from faaa2d8 to fa1fe2e Compare June 22, 2022 07:55
@Sjors
Copy link
Member

Sjors commented Jun 22, 2022

utACK fa1fe2e

Some extra context: the constant LOCKTIME_MEDIAN_TIME_PAST was restored in e4e5334, which was a reversion of #6928 which itself was a (mistaken, apparently) reversion of #6566. That last PR actually introduced it.

It was used in CheckFinalTx() to toggle between chainActive.Tip()->GetMedianTimePast() and the node clock GetAdjustedTime();. This applied only to the mempool (set by AcceptToMemoryPool()) and the miner code.

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.

@fanquake fanquake requested a review from glozow June 22, 2022 18:58
@maflcko maflcko force-pushed the 2203-remConstant- branch from fa1fe2e to faf6101 Compare June 27, 2022 11:34
@maflcko maflcko changed the title Remove LOCKTIME_MEDIAN_TIME_PAST constant Remove LOCKTIME_* constants/flags Jun 27, 2022
@maflcko maflcko force-pushed the 2203-remConstant- branch from faf6101 to fa8b61e Compare June 27, 2022 11:48
@maflcko maflcko changed the title Remove LOCKTIME_* constants/flags Remove LOCKTIME_MEDIAN_TIME_PAST constant Jun 27, 2022
@maflcko maflcko force-pushed the 2203-remConstant- branch from fa8b61e to fa1fe2e Compare June 27, 2022 12:03
@maflcko
Copy link
Member Author

maflcko commented Jun 27, 2022

A kind offline reviewer asked if the other flag can be turned into a bool, so I did that in commit fa8b61e. 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.

glozow
glozow reviewed Jun 27, 2022
View reviewed changes
Copy link
Member

@glozow glozow left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review ACK fa1fe2e, AFAICT this is safe and makes sense as SequenceLocks doesn't use it, wallet/ATMP no longer need it since #24080, and ContextualCheckBlock effectively uses it as a roundabout boolean.

@fanquake fanquake requested review from ajtowns and instagibbs and removed request for ajtowns June 27, 2022 17:22
instagibbs
instagibbs approved these changes Jun 28, 2022
View reviewed changes
Copy link
Member

@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fa1fe2e

@fanquake fanquake merged commit bace615 into bitcoin:master Jun 28, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 28, 2022
@fanquake
Merge bitcoin#24565: Remove LOCKTIME_MEDIAN_TIME_PAST constant
57a8642 
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
@maflcko maflcko deleted the 2203-remConstant-🕴 branch June 29, 2022 06:29
@bitcoin bitcoin locked and limited conversation to collaborators Jun 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@glozow glozow glozow left review comments

@ajtowns ajtowns Awaiting requested review from ajtowns

+2 more reviewers

@instagibbs instagibbs instagibbs approved these changes

@w0xlt w0xlt w0xlt approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Consensus Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@maflcko @luke-jr @DrahtBot @Sjors @ajtowns @instagibbs @glozow @w0xlt @fanquake