Stop UASF enforcement if SegWit has IsLockedIn state. by da2ce7 · Pull Request #512 · bitcoin/bips

da2ce7 · 2017-04-04T09:41:34Z

Includes basic refactor.

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

shaolinfry · 2017-04-04T09:56:42Z

More importantly, there is this question to be answered 60d4b51#commitcomment-21581456

shaolinfry · 2017-04-04T10:35:32Z

+int64_t nMedianTimePast = pindex->GetMedianTimePast();
+if ( (nMedianTimePast >= 1506816000) &&   // Sun Oct 1 00:00:00 UTC 2017
+     (nMedianTimePast <= 1510704000) &&   // Sun Nov 15 00:00:00 UTC 2017
+     (!IsWitnessLockedIn(pindex->pprev, chainparams.GetConsensus()) &&  // Segwit is not locked in


This must be defined in the code example.

shaolinfry · 2017-04-04T10:35:58Z

-    pindex->GetMedianTimePast() <= 1510704000 && // Wed 15 Nov 2017 00:00:00 UTC
-    !IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus()))
+int64_t nMedianTimePast = pindex->GetMedianTimePast();
+if ( (nMedianTimePast >= 1506816000) &&   // Sun Oct 1 00:00:00 UTC 2017


Both timestamp and date are wrong;

shaolinfry · 2017-04-04T10:36:18Z

-    !IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus()))
+int64_t nMedianTimePast = pindex->GetMedianTimePast();
+if ( (nMedianTimePast >= 1506816000) &&   // Sun Oct 1 00:00:00 UTC 2017
+     (nMedianTimePast <= 1510704000) &&   // Sun Nov 15 00:00:00 UTC 2017


Day is wrong, it's Weds not Sun

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

da2ce7 · 2017-04-04T10:49:58Z

@shaolinfry sorry about that: that was embarrassing. 😳

Adjust start time to Aug 1st 2017 Fix code sample logic.

shaolinfry · 2017-04-04T10:58:08Z

+int64_t nMedianTimePast = pindex->GetMedianTimePast();
+if ( (nMedianTimePast >= 1501545600) &&  // Tue 01 Aug 2017 00:00:00 UTC
+     (nMedianTimePast <= 1510704000) &&  // Wed 15 Nov 2017 00:00:00 UTC
+     (!IsWitnessLockedIn(pindex->pprev, chainparams.GetConsensus()) &&  // Segwit is not locked in


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

shaolinfry · 2017-04-04T11:55:28Z

@dooglus comments please?

shaolinfry · 2017-04-04T11:53:50Z


 <pre>
+
+


two unnecessary lines

shaolinfry · 2017-04-04T11:54:00Z

+        == THRESHOLD_LOCKED_IN);
+}
+
+


one unnecessary blank line

shaolinfry · 2017-04-04T11:54:30Z

+{
+    LOCK(cs_main);
+    return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache)
+        == THRESHOLD_LOCKED_IN);


dont linebreak here please.

da2ce7 · 2017-04-04T14:10:14Z

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 ***@***.***> 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.

afk11 · 2017-04-04T14:29:21Z

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

da2ce7 · 2017-04-04T14:43:17Z

@afk11 thankyou, I have included your commit.

luke-jr · 2017-04-04T15:52:09Z

-    // versionbits topbit and segwit flag must be set.
-    if ((pindex->nVersion & VERSIONBITS_TOP_MASK) != VERSIONBITS_TOP_BITS ||
-        (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) == 0) {
+    bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS; // BIP9 bit set


Remove the inaccurate comment.

mkwia · 2017-04-04T16:08:13Z

 All times are specified according to median past time.

-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.
+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.


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

da2ce7 · 2017-04-04T18:08:07Z

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

da2ce7 · 2017-04-04T19:09:03Z

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

da2ce7 · 2017-04-06T08:43:42Z

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

shaolinfry · 2017-04-06T15:13:54Z

-        (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) == 0) {
+    bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;
+    bool fSegbit = (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) != 0;
+    if (!(fVersionBits && fSegbit)) {


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

IMO this way is cleaner.

shaolinfry · 2017-04-06T15:18:36Z

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.

da2ce7 · 2017-04-06T15:39:45Z

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

luke-jr

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

luke-jr · 2017-04-06T16:17:53Z

-        (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) == 0) {
+    bool fVersionBits = (pindex->nVersion & VERSIONBITS_TOP_MASK) == VERSIONBITS_TOP_BITS;
+    bool fSegbit = (pindex->nVersion & VersionBitsMask(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) != 0;
+    if (!(fVersionBits && fSegbit)) {


IMO this way is cleaner.

da2ce7 changed the title ~~Refactor Code-Example with Better Formatting~~ Stop UASF enforcement if SegWit has IsLockedIn state. Apr 4, 2017

shaolinfry suggested changes Apr 4, 2017

View reviewed changes

Refactor Code-Example with Better Formatting

2a97af8

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

da2ce7 force-pushed the bip148 branch from 1b9ecb1 to b69620a Compare April 4, 2017 10:47

shaolinfry referenced this pull request Apr 4, 2017

Update BIP148

60d4b51

Adjust start time to Aug 1st 2017 Fix code sample logic.

shaolinfry suggested changes Apr 4, 2017

View reviewed changes

da2ce7 force-pushed the bip148 branch 2 times, most recently from ae3e33a to 76d49fd Compare April 4, 2017 11:20

shaolinfry suggested changes Apr 4, 2017

View reviewed changes

luke-jr reviewed Apr 4, 2017

View reviewed changes

mkwia reviewed Apr 4, 2017

View reviewed changes

da2ce7 force-pushed the bip148 branch from 8eb53bb to c3fdc0e Compare April 4, 2017 18:04

da2ce7 added 2 commits April 4, 2017 20:05

Stop BIP 148 enforced signalling if SegWit is Locked-In State

1352c36

update BIP text for locked-in state

5f34ed7

da2ce7 force-pushed the bip148 branch from c3fdc0e to 5f34ed7 Compare April 4, 2017 18:05

shaolinfry reviewed Apr 6, 2017

View reviewed changes

luke-jr approved these changes Apr 6, 2017

View reviewed changes

shaolinfry approved these changes Apr 6, 2017

View reviewed changes

luke-jr merged commit 1f6888e into bitcoin:master Apr 6, 2017

Conversation

da2ce7 commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaolinfry commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

da2ce7 commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaolinfry commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

da2ce7 commented Apr 4, 2017 via email

Uh oh!

afk11 commented Apr 4, 2017

Uh oh!

da2ce7 commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

da2ce7 commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

da2ce7 commented Apr 4, 2017

Uh oh!

da2ce7 commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaolinfry commented Apr 6, 2017

Uh oh!

da2ce7 commented Apr 6, 2017

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

da2ce7 commented Apr 4, 2017 •

edited

Loading

da2ce7 commented Apr 4, 2017 •

edited

Loading