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

jtimon · 2015-04-03T23:20:37Z

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.

laanwj · 2015-04-08T08:08:58Z

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.

jtimon · 2015-04-08T12:16:50Z

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 · 2015-04-08T15:28:35Z

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

jtimon · 2015-04-10T09:09:05Z

Rebased

jtimon · 2015-04-10T16:59:41Z

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

theuni · 2015-04-10T19:11:33Z

src/main.cpp

 */
-static bool IsSuperMajority(int minVersion, const CBlockIndex* pstart, unsigned int nRequired);
+static bool IsSuperMajority(int minVersion, const CBlockIndex* pstart, unsigned nRequired, unsigned nToCheck);


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

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 · 2015-04-15T17:40:49Z

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 · 2015-04-16T17:30:23Z

utACK

jtimon · 2015-04-16T18:14:28Z

Squashed the last 2 commits.

theuni · 2015-04-23T21:00:10Z

src/main.cpp

@@ -2571,19 +2570,11 @@ bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& sta
    if (pcheckpoint && nHeight < pcheckpoint->nHeight)
        return state.DoS(100, error("%s: forked chain older than last checkpoint (height %d)", __func__, nHeight));

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


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.

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

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

is less clear than

// Reject block.nVersion=1 blocks when 95% (75% on testnet) of the network has upgraded: // 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?

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

Mhmm, I think putting those in ContextualCheckBlock and Consensus::GetFlags (not existing yet, see jtimon@9ba7d06) 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 · 2015-04-23T21:05:18Z

ut ACK other than the nit above

jtimon · 2015-04-29T11:26:06Z

Updated without introducing the loop

sipa · 2015-04-30T07:05:17Z

src/main.cpp

@@ -2701,11 +2697,10 @@ bool AcceptBlock(CBlock& block, CValidationState& state, CBlockIndex** ppindex,
    return true;
 }

-static bool IsSuperMajority(int minVersion, const CBlockIndex* pstart, unsigned int nRequired)
+static bool IsSuperMajority(int minVersion, const CBlockIndex* pstart, unsigned nRequired, const Consensus::Params& consensusParams)


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

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.

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.

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:

bool IsSuperMajorityUpgrade(int minVersion, const CBlockIndex* pstart, const Consensus::Params& consensusParams); bool 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 · 2015-05-06T10:56:58Z

Needed rebase

laanwj · 2015-05-13T16:07:26Z

src/main.cpp

    unsigned int nFound = 0;
-    for (unsigned int i = 0; i < nToCheck && nFound < nRequired && pstart != NULL; i++)
+    for (int i = 0; i < consensusParams.nMajorityWindow && nFound < nRequired && pstart != NULL; i++)


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

Yes, to avoid introducing a new warning.

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

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

51aa249 Chainparams: Refactor: Decouple IsSuperMajority from Params() (Jorge Timón)

jtimon changed the title ~~Refactor: Cleanup: remove Consensus::Params getters from CChainParams~~ Chainparams: Refactor: Cleanup: remove Consensus::Params getters from CChainParams Apr 4, 2015

jtimon changed the title ~~Chainparams: Refactor: Cleanup: remove Consensus::Params getters from CChainParams~~ Chainparams: Refactor: Cleanup: remove Consensus::Params getters in CChainParams Apr 4, 2015

This was referenced Apr 4, 2015

Trivial: Chainparams: Decouple CAlert from CChainParams #5969

Merged

DEPENDENT: Globals: Avoid calling Params() #5970

Closed

laanwj added the Refactoring label Apr 8, 2015

jtimon force-pushed the params_consensus branch from e998b43 to 9db7e8a Compare April 8, 2015 12:14

jtimon mentioned this pull request Apr 8, 2015

Consensus: Decouple ContextualCheckBlockHeader from checkpoints #5975

Merged

jtimon force-pushed the params_consensus branch from 9db7e8a to 9b0a089 Compare April 8, 2015 14:36

jtimon changed the title ~~Chainparams: Refactor: Cleanup: remove Consensus::Params getters in CChainParams~~ Preparations to remove Consensus::Params getters in CChainParams Apr 8, 2015

jtimon force-pushed the params_consensus branch 3 times, most recently from ac59d37 to 6d04c74 Compare April 10, 2015 09:08

jtimon force-pushed the params_consensus branch from 6d04c74 to cb96bb9 Compare April 10, 2015 10:04

jtimon mentioned this pull request Apr 10, 2015

DEPENDENT: API: Expose bitcoinconsensus_verify_header() in libconsensus #5995

Closed

jtimon force-pushed the params_consensus branch from cb96bb9 to d64931f Compare April 10, 2015 16:57

jtimon changed the title ~~Preparations to remove Consensus::Params getters in CChainParams~~ Chainparams: Refactor: Decouple IsSuperMajority from Params() Apr 10, 2015

theuni reviewed Apr 10, 2015
View reviewed changes

jtimon force-pushed the params_consensus branch 3 times, most recently from 40bc452 to 98fee57 Compare April 15, 2015 17:36

jtimon force-pushed the params_consensus branch from 98fee57 to edba47f Compare April 16, 2015 18:14

jtimon mentioned this pull request Apr 20, 2015

Remove BIP34 switchover logic #5966

Closed

theuni reviewed Apr 23, 2015
View reviewed changes

jtimon mentioned this pull request Apr 24, 2015

Decouple checkpoints from Params() #6055

Merged

jtimon force-pushed the params_consensus branch 2 times, most recently from bd7d718 to 6ef2e51 Compare April 29, 2015 11:24

jtimon force-pushed the params_consensus branch 2 times, most recently from c7ed619 to 55d732f Compare April 29, 2015 13:38

sipa reviewed Apr 30, 2015
View reviewed changes

jtimon force-pushed the params_consensus branch from 55d732f to fd6a1ec Compare May 6, 2015 10:56

Chainparams: Refactor: Decouple IsSuperMajority from Params()

51aa249

jtimon force-pushed the params_consensus branch from fd6a1ec to 51aa249 Compare May 6, 2015 14:39

laanwj reviewed May 13, 2015
View reviewed changes

laanwj merged commit 51aa249 into bitcoin:master May 15, 2015

laanwj added a commit that referenced this pull request May 15, 2015

Merge pull request #5968

f0043c2

51aa249 Chainparams: Refactor: Decouple IsSuperMajority from Params() (Jorge Timón)

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

jtimon commented Apr 3, 2015

Uh oh!

laanwj commented Apr 8, 2015

Uh oh!

jtimon commented Apr 8, 2015

Uh oh!

jtimon commented Apr 8, 2015

Uh oh!

jtimon commented Apr 10, 2015

Uh oh!

jtimon commented Apr 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Apr 15, 2015

Uh oh!

laanwj commented Apr 16, 2015

Uh oh!

jtimon commented Apr 16, 2015

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!

Choose a reason for hiding this comment

Uh oh!

theuni commented Apr 23, 2015

Uh oh!

jtimon commented Apr 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Apr 30, 2015 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented May 6, 2015

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!