Remove LOCKTIME_MEDIAN_TIME_PAST constant #24565

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2203-remConstant-🕴 changing 4 files +8 −11
  1. MarcoFalke commented at 5:09 pm on March 14, 2022: member

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

  2. MarcoFalke added the label Refactoring on Mar 14, 2022
  3. MarcoFalke added the label Consensus on Mar 14, 2022
  4. in src/test/miner_tests.cpp:373 in fade3d0f62 outdated 
    408@@ -409,8 +409,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
409     }
410 
411     // non-final txs in mempool
412-    SetMockTime(m_node.chainman->ActiveChain().Tip()->GetMedianTimePast()+1);
413-    const int flags{LOCKTIME_VERIFY_SEQUENCE | LOCKTIME_MEDIAN_TIME_PAST};
414+    SetMockTime(m_node.chainman->ActiveChain().Tip()->GetMedianTimePast() + 1);
415+    const int flags{LOCKTIME_VERIFY_SEQUENCE};
    ajtowns commented at 5:11 pm on March 14, 2022:
    Shouldn’t this be STANDARD_LOCKTIME_VERIFY_FLAGS instead?
    MarcoFalke commented at 8:29 am on March 15, 2022:
    No idea. STANDARD_LOCKTIME_VERIFY_FLAGS wasn’t picked for this test when this was added in commit c6c2f0fd782ccf607027414012f45c8f48561a30 , so I’ll leave this as-is for now. Unless there is a reason to change.
  5. in src/validation.cpp:3497 in fade3d0f62 outdated 
    3417@@ -3418,15 +3418,15 @@ static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& stat
3418     const int nHeight = pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1;
3419 
3420     // Enforce BIP113 (Median Time Past).
3421-    int nLockTimeFlags = 0;
3422+    bool enforce_locktime_median_time_past{false};
3423     if (DeploymentActiveAfter(pindexPrev, consensusParams, Consensus::DEPLOYMENT_CSV)) {
3424         assert(pindexPrev != nullptr);
3425-        nLockTimeFlags |= LOCKTIME_MEDIAN_TIME_PAST;
3426+        enforce_locktime_median_time_past = true;
    ajtowns commented at 5:13 pm on March 14, 2022:

    Perhaps

    0// Cannot get MTP for parent of genesis block, so only potentially enforce post genesis
1const 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));)

    MarcoFalke commented at 8:31 am on March 15, 2022:
    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.
  6. ajtowns commented at 5:28 pm on March 14, 2022: member
    Concept ACK
  7. luke-jr commented at 5:31 pm on March 18, 2022: member

    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.

  8. MarcoFalke commented at 9:06 am on March 19, 2022: member

    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#issue-1105012849 to appear in the future.

  9. DrahtBot commented at 3:31 pm on March 19, 2022: member

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

    Conflicts

    No conflicts as of last run.

  10. w0xlt approved
  11. w0xlt commented at 6:52 pm on March 23, 2022: contributor
    Code Review ACK fade3d0
  12. DrahtBot added the label Needs rebase on May 13, 2022
  13. MarcoFalke force-pushed on May 13, 2022
  14. DrahtBot removed the label Needs rebase on May 13, 2022
  15. fanquake requested review from ajtowns on Jun 10, 2022
  16. DrahtBot added the label Needs rebase on Jun 20, 2022
  17. Remove LOCKTIME_MEDIAN_TIME_PAST constant fa1fe2e500
  18. MarcoFalke force-pushed on Jun 22, 2022
  19. DrahtBot removed the label Needs rebase on Jun 22, 2022
  20. Sjors commented at 4:53 pm on June 22, 2022: member

    utACK fa1fe2e5004a6bacded464ed9778ff196e05859c

    Some extra context: the constant LOCKTIME_MEDIAN_TIME_PAST was restored in e4e5334ef8d923993ef9cf704ea8f3d0b5801350, 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 1c93b9b31c2ab7358f9d55f52dd46340397c906d we still had miner code using the constant. But that was removed in #23637.

  21. fanquake requested review from glozow on Jun 22, 2022
  22. MarcoFalke force-pushed on Jun 27, 2022
  23. MarcoFalke renamed this:
    Remove LOCKTIME_MEDIAN_TIME_PAST constant
    Remove LOCKTIME_* constants/flags
    on Jun 27, 2022
  24. MarcoFalke force-pushed on Jun 27, 2022
  25. MarcoFalke renamed this:
    Remove LOCKTIME_* constants/flags
    Remove LOCKTIME_MEDIAN_TIME_PAST constant
    on Jun 27, 2022
  26. MarcoFalke force-pushed on Jun 27, 2022
  27. MarcoFalke commented at 12:05 pm on June 27, 2022: member
    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.
  28. glozow commented at 3:20 pm on June 27, 2022: member
    code review ACK fa1fe2e5004a6bacded464ed9778ff196e05859c, 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.
  29. fanquake removed review request from ajtowns on Jun 27, 2022
  30. fanquake requested review from ajtowns on Jun 27, 2022
  31. fanquake requested review from instagibbs on Jun 27, 2022
  32. instagibbs approved
  33. instagibbs commented at 5:04 pm on June 28, 2022: member
    utACK fa1fe2e5004a6bacded464ed9778ff196e05859c
  34. fanquake merged this on Jun 28, 2022
  35. fanquake closed this on Jun 28, 2022
  36. sidhujag referenced this in commit 57a86429a3 on Jun 28, 2022
  37. MarcoFalke deleted the branch on Jun 29, 2022
  38. DrahtBot locked this on Jun 29, 2023


MarcoFalke ajtowns luke-jr DrahtBot w0xlt Sjors glozow instagibbs


ajtowns

Labels
Refactoring Consensus

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me