Stop UASF enforcement if SegWit has IsLockedIn state. #512

pull da2ce7 wants to merge 3 commits into bitcoin:master from da2ce7:bip148 changing 1 files +16 −7
  1. da2ce7 commented at 9:41 AM on April 4, 2017: contributor

    Includes basic refactor.

    No-need to calculate the GetMedianTimePast() value twice.

  2. shaolinfry commented at 9:56 AM on April 4, 2017: contributor

    More importantly, there is this question to be answered https://github.com/bitcoin/bips/commit/60d4b512b8ca9d57b479a5ee63c6825415ac0b20#commitcomment-21581456

  3. da2ce7 renamed this:
    Refactor Code-Example with Better Formatting
    Stop UASF enforcement if SegWit has IsLockedIn state.
    on Apr 4, 2017
  4. in bip-0148.mediawiki:51 in 1b9ecb1984 outdated 
      42 | -    pindex->GetMedianTimePast() <= 1510704000 && // Wed 15 Nov 2017 00:00:00 UTC
  43 | -    !IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus()))
  44 | +int64_t nMedianTimePast = pindex->GetMedianTimePast();
  45 | +if ( (nMedianTimePast >= 1506816000) &&   // Sun Oct 1 00:00:00 UTC 2017
  46 | +     (nMedianTimePast <= 1510704000) &&   // Sun Nov 15 00:00:00 UTC 2017
  47 | +     (!IsWitnessLockedIn(pindex->pprev, chainparams.GetConsensus()) &&  // Segwit is not locked in
    shaolinfry commented at 10:35 AM on April 4, 2017:

    This must be defined in the code example.

  5. in bip-0148.mediawiki:42 in 1b9ecb1984 outdated 
      37 | @@ -38,13 +38,15 @@ While this BIP is active, all blocks must set the nVersion header top 3 bits to
  38 |  
  39 |  <pre>
  40 |  // BIP148 mandatory segwit signalling.
  41 | -if (pindex->GetMedianTimePast() >= 1501545600 && // Tue 1 Aug 2017 00:00:00 UTC
  42 | -    pindex->GetMedianTimePast() <= 1510704000 && // Wed 15 Nov 2017 00:00:00 UTC
  43 | -    !IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus()))
  44 | +int64_t nMedianTimePast = pindex->GetMedianTimePast();
  45 | +if ( (nMedianTimePast >= 1506816000) &&   // Sun Oct 1 00:00:00 UTC 2017
    shaolinfry commented at 10:35 AM on April 4, 2017:

    Both timestamp and date are wrong;

  6. in bip-0148.mediawiki:43 in 1b9ecb1984 outdated 
      37 | @@ -38,13 +38,15 @@ While this BIP is active, all blocks must set the nVersion header top 3 bits to
  38 |  
  39 |  <pre>
  40 |  // BIP148 mandatory segwit signalling.
  41 | -if (pindex->GetMedianTimePast() >= 1501545600 && // Tue 1 Aug 2017 00:00:00 UTC
  42 | -    pindex->GetMedianTimePast() <= 1510704000 && // Wed 15 Nov 2017 00:00:00 UTC
  43 | -    !IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus()))
  44 | +int64_t nMedianTimePast = pindex->GetMedianTimePast();
  45 | +if ( (nMedianTimePast >= 1506816000) &&   // Sun Oct 1 00:00:00 UTC 2017
  46 | +     (nMedianTimePast <= 1510704000) &&   // Sun Nov 15 00:00:00 UTC 2017
    shaolinfry commented at 10:36 AM on April 4, 2017:

    Day is wrong, it's Weds not Sun

  7. shaolinfry changes_requested
  8. Refactor Code-Example with Better Formatting
    No-need to calculate the GetMedianTimePast() value twice.
    2a97af80db
  9. da2ce7 force-pushed on Apr 4, 2017
  10. da2ce7 commented at 10:49 AM on April 4, 2017: contributor

    @shaolinfry sorry about that: that was embarrassing. 😳

  11. shaolinfry referenced this in commit 60d4b512b8 on Apr 4, 2017
  12. in bip-0148.mediawiki:51 in b72afc554d outdated 
      43 | -    pindex->GetMedianTimePast() <= 1510704000 && // Wed 15 Nov 2017 00:00:00 UTC
  44 | -    !IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus()))
  45 | +int64_t nMedianTimePast = pindex->GetMedianTimePast();
  46 | +if ( (nMedianTimePast >= 1501545600) &&  // Tue 01 Aug 2017 00:00:00 UTC
  47 | +     (nMedianTimePast <= 1510704000) &&  // Wed 15 Nov 2017 00:00:00 UTC
  48 | +     (!IsWitnessLockedIn(pindex->pprev, chainparams.GetConsensus()) &&  // Segwit is not locked in
    shaolinfry commented at 10:58 AM on April 4, 2017:

    You need to define this function since it does not exist in Bitcoin Core.

    da2ce7 commented at 11:23 AM on April 4, 2017:

    Done :)

  13. shaolinfry changes_requested
  14. da2ce7 force-pushed on Apr 4, 2017
  15. da2ce7 force-pushed on Apr 4, 2017
  16. in bip-0148.mediawiki:41 in 76d49fdf01 outdated 
      37 |  
  38 |  === Reference implementation ===
  39 |  
  40 |  <pre>
  41 | +
  42 | +
    shaolinfry commented at 11:53 AM on April 4, 2017:

    two unnecessary lines

  17. in bip-0148.mediawiki:50 in 76d49fdf01 outdated 
      46 | +    LOCK(cs_main);
  47 | +    return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache)
  48 | +        == THRESHOLD_LOCKED_IN);
  49 | +}
  50 | +
  51 | +
    shaolinfry commented at 11:54 AM on April 4, 2017:

    one unnecessary blank line

  18. in bip-0148.mediawiki:47 in 76d49fdf01 outdated 
      43 | +// Check if Segregated Witness is Locked In
  44 | +bool IsWitnessLockedIn(const CBlockIndex* pindexPrev, const Consensus::Params& params)
  45 | +{
  46 | +    LOCK(cs_main);
  47 | +    return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache)
  48 | +        == THRESHOLD_LOCKED_IN);
    shaolinfry commented at 11:54 AM on April 4, 2017:

    dont linebreak here please.

  19. shaolinfry commented at 11:55 AM on April 4, 2017: contributor

    @dooglus comments please?

  20. shaolinfry changes_requested
  21. da2ce7 commented at 2:10 PM on April 4, 2017: contributor

    Sorry, away from computer now. I can fix in a couple of hours.

    Sent from my iPhone

    On 4 Apr 2017, at 13:55, shaolinfry notifications@github.com wrote:

    @shaolinfry requested changes on this pull request.

    In bip-0148.mediawiki:

    While this BIP is active, all blocks must set the nVersion header top 3 bits to 001 together with bit field (1<<1) (according to the existing segwit deployment). Blocks that do not signal as required will be rejected.

    === Reference implementation ===

    <pre> + + two unnecessary lines In bip-0148.mediawiki: > While this BIP is active, all blocks must set the nVersion header top 3 bits to 001 together with bit field (1<<1) (according to the existing segwit deployment). Blocks that do not signal as required will be rejected. === Reference implementation === <pre> + + +// Check if Segregated Witness is Locked In +bool IsWitnessLockedIn(const CBlockIndex* pindexPrev, const Consensus::Params& params) +{ + LOCK(cs_main); + return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache) + == THRESHOLD_LOCKED_IN); +} + + one unnecessary blank line In bip-0148.mediawiki: > While this BIP is active, all blocks must set the nVersion header top 3 bits to 001 together with bit field (1<<1) (according to the existing segwit deployment). Blocks that do not signal as required will be rejected. === Reference implementation === <pre> + + +// Check if Segregated Witness is Locked In +bool IsWitnessLockedIn(const CBlockIndex* pindexPrev, const Consensus::Params& params) +{ + LOCK(cs_main); + return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache) + == THRESHOLD_LOCKED_IN); dont linebreak here please. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

  22. afk11 commented at 2:29 PM on April 4, 2017: contributor

    @da2ce7 If you merge this it'll update this PR with the requested changes: https://github.com/da2ce7/bips/pull/2/files

  23. da2ce7 commented at 2:43 PM on April 4, 2017: contributor

    @afk11 thankyou, I have included your commit.

  24. in bip-0148.mediawiki:54 in 8eb53bb099 outdated 
      56 | +      !IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus())) )   // and is not active.
  57 |  {
  58 | -    // versionbits topbit and segwit flag must be set.
  59 | -    if ((pindex->nVersion & VERSIONBITS_TOP_MASK) != VERSIONBITS_TOP_BITS ||
  60 | -        (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) == 0) {
  61 | +    bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS; // BIP9 bit set
    luke-jr commented at 3:52 PM on April 4, 2017:

    Remove the inaccurate comment.

  25. in bip-0148.mediawiki:33 in 76d49fdf01 outdated 
      29 | @@ -30,21 +30,34 @@ It is hoped that miners will respond to this BIP by activating segwit early, bef
  30 |  
  31 |  All times are specified according to median past time.
  32 |  
  33 | -This BIP will be activate between midnight August 1st 2017 (epoch time 1501545600) and midnight November 15th 2017 (epoch time 1510704000) if the existing segwit deployment is not activated before epoch time 1501545600. This BIP will cease to be active when the existing segwit deployment activates.
  34 | +This BIP will be activate between midnight August 1st 2017 (epoch time 1501545600) and midnight November 15th 2017 (epoch time 1510704000) if the existing segwit deployment is not locked-in or activated before epoch time 1501545600. This BIP will cease to be active when segwit is locked-in.
    mkwia commented at 4:08 PM on April 4, 2017:

    Should be "This BIP will be active between..."

  26. da2ce7 force-pushed on Apr 4, 2017
  27. Stop BIP 148 enforced signalling if SegWit is Locked-In State 1352c367a1
  28. update BIP text for locked-in state 5f34ed741a
  29. da2ce7 force-pushed on Apr 4, 2017
  30. da2ce7 commented at 6:08 PM on April 4, 2017: contributor

    @afk11 I have squashed your commit. @mkwia thank you, I have corrected the phasing. @luke-jr thank you, I have removed the inaccurate comment.

  31. da2ce7 commented at 7:09 PM on April 4, 2017: contributor

    Bitcoin Branch that implements BIP 148 as described by this pull request. https://github.com/da2ce7/bitcoin/tree/bip148

  32. da2ce7 commented at 8:43 AM on April 6, 2017: contributor

    @shaolinfry, can you please re-review this pull request?

  33. in bip-0148.mediawiki:56 in 5f34ed741a 
      58 | -    // versionbits topbit and segwit flag must be set.
  59 | -    if ((pindex->nVersion & VERSIONBITS_TOP_MASK) != VERSIONBITS_TOP_BITS ||
  60 | -        (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) == 0) {
  61 | +    bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;
  62 | +    bool fSegbit = (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) != 0;
  63 | +    if (!(fVersionBits && fSegbit)) {
    shaolinfry commented at 3:13 PM on April 6, 2017:

    I thought I had commented here but seems I didn't press the button :) So while it is bikeshedding, I dislike this formatting, and prefer the previous notation but that is just my personal taste. So long as the logic works :)

    luke-jr commented at 4:17 PM on April 6, 2017:

    IMO this way is cleaner.

  34. shaolinfry commented at 3:18 PM on April 6, 2017: contributor

    I need other's to peer review ACK this first, because when I give mine, it will get merged. I'm reasonably confident this is ok now, but peers please.

  35. da2ce7 commented at 3:39 PM on April 6, 2017: contributor

    @mkwia do you suggest defining a temporary boolean for clearer code?

  36. luke-jr approved
  37. luke-jr commented at 4:18 PM on April 6, 2017: member

    (Code review should really be done on a code project rather than the BIP change itself.)

  38. shaolinfry approved
  39. luke-jr merged this on Apr 6, 2017
  40. luke-jr closed this on Apr 6, 2017
Contributors
da2ce7shaolinfryafk11luke-jrmkwia
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bips. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-01 20:10 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me