Chainparams: Refactor: Decouple IsSuperMajority from Params()

jtimon commented at 11:20 pm on April 3, 2015: contributor

After the struct Consensus::Params was created in #5812, there are some redundant getters in CChainParams. This is not the minimal way to remove it, but the fastest way would involve putting Params().GetConsensus() instead of params or params.GetConsensus(). With a few more extra changes we’re going forward functions depending explicitly on CChainParams or Consensus::Params whenever it’s possible. @sipa suggested removing the redundancies in #5812, this is what I meant by doing it later, better. @laanwj celebrated passing Consensus::Params as a parameter in that same PR, so I hope he likes this one too. @theuni will probably be interested in this one as well since he is working on similar things.

jtimon renamed this:
~~Refactor: Cleanup: remove Consensus::Params getters from CChainParams~~
Chainparams: Refactor: Cleanup: remove Consensus::Params getters from CChainParams
on Apr 4, 2015

jtimon renamed this:
~~Chainparams: Refactor: Cleanup: remove Consensus::Params getters from CChainParams~~
Chainparams: Refactor: Cleanup: remove Consensus::Params getters in CChainParams
on Apr 4, 2015

laanwj commented at 8:08 am on April 8, 2015: member

I like the concept, although this has a lot of ‘collateral’ code impact. Of course this is a necessary step in the process toward explicitly passing in a Consensus or ChainParams structure.

The part that generates most of the diff here is that which removes the getters and replaces them with GetConsensus().x. For the sake of reviewability, let’s do this in a separate pull, with a large diff but apart from that ‘almost-trivial’?

And then do e.g. the GetBlockValue/GetBlockSubsidy change and other more significant changes on top of that, so that it’s easier to see what happens.

laanwj added the label Improvement on Apr 8, 2015

jtimon force-pushed on Apr 8, 2015

jtimon commented at 12:16 pm on April 8, 2015: contributor

I left the last 3 commits for #5970, so the redundant getters aren’t removed yet and the GetBlockValue/GetBlockSubsidy and IsSupermajority changes are also delayed. Is this what you had in mind?

jtimon force-pushed on Apr 8, 2015

jtimon commented at 3:28 pm on April 8, 2015: contributor

Last commit updated to avoid conflicts with #5975 after its latest chainparams fix.

jtimon renamed this:
~~Chainparams: Refactor: Cleanup: remove Consensus::Params getters in CChainParams~~
Preparations to remove Consensus::Params getters in CChainParams
on Apr 8, 2015

jtimon force-pushed on Apr 9, 2015

jtimon force-pushed on Apr 10, 2015

jtimon commented at 9:09 am on April 10, 2015: contributor

Rebased

jtimon force-pushed on Apr 10, 2015

jtimon renamed this:
~~Preparations to remove Consensus::Params getters in CChainParams~~
Chainparams: Refactor: Decouple IsSuperMajority from Params()
on Apr 10, 2015

jtimon commented at 4:59 pm on April 10, 2015: contributor

Updated only with the changes to IsSuperMajority. Other changes are left for other PRs like #5996

in src/main.cpp: in d64931f884 outdated

70@@ -71,10 +71,9 @@ void EraseOrphansFor(NodeId peer);
71 
72 /**
73  * Returns true if there are nRequired or more blocks of minVersion or above
74- * in the last Params().ToCheckBlockUpgradeMajority() blocks, starting at pstart 
75- * and going backwards.
76+ * in the last nToCheck blocks, starting at pstart and going backwards.
77  */
78-static bool IsSuperMajority(int minVersion, const CBlockIndex* pstart, unsigned int nRequired);
79+static bool IsSuperMajority(int minVersion, const CBlockIndex* pstart, unsigned nRequired, unsigned nToCheck);

theuni commented at 7:11 pm on April 10, 2015:

Just pass Consensus::Params and use the parts here?

jtimon commented at 6:32 am on April 11, 2015:

It would only serve for nToCheck. nRequired is sometimes enforceUpgrade and some times rejectOoutdated. I thought that instead of passing the params to take the majority window, I could just pass a number directly.

jtimon force-pushed on Apr 10, 2015

jtimon force-pushed on Apr 15, 2015

jtimon commented at 5:40 pm on April 15, 2015: contributor

Added a squashme commit with @theuni ’s suggestion of passing Consensus::Params instead of nToCheck. At the very least it produces shorter lines. I’m happy to squash it. I also added another squashme commit that makes the PR a little bit harder to read, but since I plan to do that later anyway and I’m now touching those same lines… I would prefer to squash it as well but I’m also happy leaving that for later if it’s going to be a blocker or people aren’t happy with it.

laanwj commented at 5:30 pm on April 16, 2015: member

utACK

jtimon force-pushed on Apr 16, 2015

jtimon commented at 6:14 pm on April 16, 2015: contributor

Squashed the last 2 commits.

in src/main.cpp: in edba47fae4 outdated

2569@@ -2571,19 +2570,11 @@ bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& sta
2570     if (pcheckpoint && nHeight < pcheckpoint->nHeight)
2571         return state.DoS(100, error("%s: forked chain older than last checkpoint (height %d)", __func__, nHeight));
2572 
2573-    // Reject block.nVersion=1 blocks when 95% (75% on testnet) of the network has upgraded:

theuni commented at 9:00 pm on April 23, 2015:

Why change this? Seems less clear and less conducive to documentation. If anything, I’d prefer to see comments added for when block versions stopped being accepted.

jtimon commented at 9:49 pm on April 23, 2015:

Well, if we have 10 voted softfork at the same time I prefer to have a loop over 10 practically identical unrolled ifs… I would also like to have a “time proof” solution for forks that are enforced by height instead of voting as suggested in #5966 . This is the only place where you can have a generic solution for block versions. Anyway, I don’t think

0// Reject block.nVersion=n blocks when 95% (75% on testnet) of the network has upgraded (last version=3)

is less clear than

0// Reject block.nVersion=1 blocks when 95% (75% on testnet) of the network has upgraded:
1// Reject block.nVersion=2 blocks when 95% (75% on testnet) of the network has upgraded:

just less repetitive. But if you have a better idea I’m happy to change the comments. Or are you directly against the loop?

theuni commented at 9:56 pm on April 23, 2015:

Not completely against, but if we had 10 voted softforks, it’d probably be a good place to have 10 short explanations.

jtimon commented at 8:10 am on April 24, 2015:

Mhmm, I think putting those in ContextualCheckBlock and Consensus::GetFlags (not existing yet, see https://github.com/jtimon/bitcoin/commit/9ba7d0641019d7f6c08f7b15ce3a2bd2e5e8a4ac) would be more informative. But anyway, it seems I squashed the loop change too fast, I can separate that again if it’s going to delay this.

theuni commented at 9:05 pm on April 23, 2015: member

ut ACK other than the nit above

jtimon force-pushed on Apr 29, 2015

jtimon commented at 11:26 am on April 29, 2015: contributor

Updated without introducing the loop

jtimon force-pushed on Apr 29, 2015

in src/main.cpp: in 55d732fb66 outdated

2696@@ -2701,11 +2697,10 @@ bool AcceptBlock(CBlock& block, CValidationState& state, CBlockIndex** ppindex,
2697     return true;
2698 }
2699 
2700-static bool IsSuperMajority(int minVersion, const CBlockIndex* pstart, unsigned int nRequired)
2701+static bool IsSuperMajority(int minVersion, const CBlockIndex* pstart, unsigned nRequired, const Consensus::Params& consensusParams)

sipa commented at 7:05 am on April 30, 2015:

I would just pass both nMajorityWindow and nRequired as argument, and leave this as a utility function.

jtimon commented at 7:43 am on April 30, 2015:

the initial version took both ints instead of taking Consensus::Params as the second. But I changed it solving a nit because this produces shorter lines. If you mean maintaining a utility function still coupled to Params(), no, only consensus functions call this function (well, plus the consensus part of connectBlock) and they need it decoupled.

sipa commented at 9:20 am on April 30, 2015:

This is a very minor nit, feel free to ignore.

I mean not taking Params, but do taking both the number to check and the number required. It’s a bit ugly to pass both params and something extracted from params, imho.

jtimon commented at 10:39 am on April 30, 2015:

As said, at the beginning it was getting nRequired and nToCheck, but @theuni suggested the change. I’m happy to change it back but I don’t have strong feelings either way. You could also have a version with nRequired and nToCheck and two other inlined functions:

0bool IsSuperMajorityUpgrade(int minVersion, const CBlockIndex* pstart, const Consensus::Params& consensusParams);
1bool IsSuperMajorityRejectOld(int minVersion, const CBlockIndex* pstart, const Consensus::Params& consensusParams);

that call the Consensus::Params-independent IsSuperMajority.

That should also make things easier for replacing the softfork feature voting since one would only need to touch inside those methods and Consensus::Params.

I think those are all options but I’m happy just decoupling IsSuperMajority from the Params() factory, so if anyone has stronger opinions between the 3 options I don’t really care which one to use.

jtimon force-pushed on May 6, 2015

jtimon commented at 10:56 am on May 6, 2015: contributor

Needed rebase

Chainparams: Refactor: Decouple IsSuperMajority from Params() 51aa24927e

jtimon force-pushed on May 6, 2015

in src/main.cpp: in 51aa24927e

2779+static bool IsSuperMajority(int minVersion, const CBlockIndex* pstart, unsigned nRequired, const Consensus::Params& consensusParams)
2780 {
2781-    unsigned int nToCheck = Params().ToCheckBlockUpgradeMajority();
2782     unsigned int nFound = 0;
2783-    for (unsigned int i = 0; i < nToCheck && nFound < nRequired && pstart != NULL; i++)
2784+    for (int i = 0; i < consensusParams.nMajorityWindow && nFound < nRequired && pstart != NULL; i++)

laanwj commented at 4:07 pm on May 13, 2015:

You’re changing the counter data type here, is that intentional?

jtimon commented at 6:39 pm on May 13, 2015:

Yes, to avoid introducing a new warning.

jtimon commented at 6:40 pm on May 13, 2015:

Another option would be to change consensusParams.nMajorityWindow to unsigned.

laanwj commented at 11:46 am on May 15, 2015:

I’m happy with an int counter, just asked to be sure.

laanwj merged this on May 15, 2015

laanwj closed this on May 15, 2015

laanwj referenced this in commit f0043c2d6d on May 15, 2015

MarcoFalke locked this on Sep 8, 2021

Chainparams: Refactor: Decouple IsSuperMajority from Params() #5968