CodeShark commented at 5:36 am on October 2, 2015: contributor

SoftForks Unit

This PR gathers the various soft fork mechanisms into one place making it more orderly. As a positive side effect, it will also make the VersionBits implementation simpler.

This PR depends on #6774

dcousens commented at 5:44 am on October 2, 2015: contributor

concept ACK, once over utACK

luke-jr commented at 6:00 am on October 2, 2015: member

Can you put softforks.{cpp,h} under the consensus/ directory?

CodeShark commented at 6:05 am on October 2, 2015: contributor

@luke-jr yes.

CodeShark force-pushed on Oct 2, 2015

jonasschnelli commented at 6:44 am on October 2, 2015: contributor

Nice! utACK.

btcdrak commented at 7:40 am on October 2, 2015: contributor

Concept ACK

TheBlueMatt commented at 9:33 am on October 2, 2015: member

Long-term isnt the goal to just switch enforcement of most softforks to blockheight/timestamp and then use versionbits globally? (I believe most things were rolled out via a IsSuperMajority-like system, and then the code was just simplified later by replacing the checks with what-the-switchover-date-was checks).

CodeShark commented at 9:35 am on October 2, 2015: contributor

@TheBlueMatt indeed - that’s exactly what will end up happening once things are buried deeply enough.

TheBlueMatt commented at 9:38 am on October 2, 2015: member

Mmm, ok, well I hadnt really read the code to begin with and I figured you were jumping the gun, but the code does simplify things on its own, so concept ack that.

CodeShark force-pushed on Oct 2, 2015

btcdrak commented at 7:11 am on October 4, 2015: contributor

I checked consensus is not affected by spinning up a new node and syncing from scratch. The node synced to the correct tip @ block 377385, 0000000000000000056d10c5f1cc9961ce2476da818a2fb4dd54ca3bcfb99a8d

ACK

CodeShark force-pushed on Oct 5, 2015

laanwj added the label Consensus on Oct 5, 2015

CodeShark force-pushed on Oct 7, 2015

CodeShark commented at 2:30 pm on October 7, 2015: contributor

Rebased atop #6774 to make it easier to review code movements.

CodeShark force-pushed on Oct 7, 2015

CodeShark force-pushed on Oct 9, 2015

jtimon commented at 9:41 am on October 11, 2015: contributor

Apart from my nits on #6774, utACK

in src/main.cpp: in a77e1a9b25 outdated

1719-    // two in the chain that violate it. This prevents exploiting the issue against nodes during their
1720-    // initial block download.
1721-    bool fEnforceBIP30 = (!pindex->phashBlock) || // Enforce on CreateNewBlock invocations which don't have a hash.
1722-                          !((pindex->nHeight==91842 && pindex->GetBlockHash() == uint256S("0x00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec")) ||
1723-                           (pindex->nHeight==91880 && pindex->GetBlockHash() == uint256S("0x00000000000743f190a18c5577a3c2d2a1f610ae9601ac046a38084ccb7cd721")));
1724+    // For soft fork details, look in consensus/softforks.cpp

jtimon commented at 9:44 am on October 11, 2015:

Are these “look in consensus/softfork.cpp” comments really necessary or useful?

jtimon commented at 9:48 am on October 11, 2015: contributor

I liked ‘‘‘Consensus::GetFlags’’’ see https://github.com/jtimon/bitcoin/commit/9ba7d0641019d7f6c08f7b15ce3a2bd2e5e8a4ac even more, but this is definitely a step in the right direction…

dcousens commented at 11:30 pm on October 13, 2015: contributor

re-ACK

jtimon commented at 11:38 am on October 15, 2015: contributor

Wouldn’t it make sense to rebase this on top of #6816 ?

in src/consensus/softforks.h: in a77e1a9b25 outdated

18+    BIP34,
19+    BIP42,
20+    BIP62,
21+    BIP65,
22+    BIP66,
23+    BIP68,

jtimon commented at 11:40 am on October 15, 2015:

In my optinion there’s no need to include future softforks/hardforks here (perhaps with the exception of BIP65 which is already merged as a policy rule), they can add themselves in their own PRs if they’re merged after this.

dcousens commented at 11:42 am on October 15, 2015:

Agreed

jtimon commented at 11:03 pm on January 3, 2016: contributor

Sorry, I meant to say the following here: #6774 (comment)

Created a SoftForks unit containing only IsSuperMajority() function. f82a7831a4

Calling IsSuperMajority() inside SoftForks unit in main.cpp cad891bca4

Removed IsSuperMajority() from main.cpp 2347ba52a8

CodeShark force-pushed on Jan 4, 2016

Added soft fork rules for existing BIPs and SoftForks::CheckBlockVersion() and SoftForks::IsActive() functions to the softforks unit. b9aa4f7bb5

Calling SoftForks::IsActive() in main.cpp instead of calling SoftForks::IsSuperMajority directly. 0b2a516fce

Call SoftForks::CheckBlockVersion() instead of calling SoftForks::IsSuperMajority() directly in main.cpp 08591fe8c8

Made IsSuperMajority() a static function of softforks.cpp and removed it from the unit interface. 2b8466d201

CodeShark force-pushed on Jan 4, 2016

jtimon commented at 6:10 pm on March 16, 2016: contributor

rebase or close for now?

btcdrak commented at 6:13 pm on March 16, 2016: contributor

I think 7575 supersedes this.

jtimon commented at 7:03 pm on March 16, 2016: contributor

@btcdrak well, it would need to be greatly rewritten after #7575, but what I think is the general idea it’s still possible, see https://github.com/jtimon/bitcoin/commit/dd446faf641350fb469865fc3586d1e088bdfb00 and https://github.com/jtimon/bitcoin/commit/032182336d84a420f33be3ab7d204c9b5f8ae8cf But yeah, probably a new PR would be a better idea if it’s going to be rewritten.

MarcoFalke commented at 5:39 pm on March 25, 2016: member

Needs rebase/new pull if still relevant

laanwj commented at 8:09 am on March 28, 2016: member

Closing for now

laanwj closed this on Mar 28, 2016

MarcoFalke locked this on Sep 8, 2021

Softforks unit #6747

SoftForks Unit