Consensus: Refactor: Decouple pow from chainparams

jtimon commented at 3:03 am on February 21, 2015: contributor

Introduce a Consensus::Params struct with only consensus specific params. This will be useful for more consensus refactors (see https://github.com/jtimon/bitcoin/commits/consensus_tip). Then use it in proof of work related methods (which are part of the consensus).

theuni commented at 3:21 am on February 21, 2015: member

Looks good, but I still don’t see why fSkipProofOfWorkCheck is moved into Consensus::Params.

I’ve been carrying the 2nd patch in one of my refactor trees (without the change to Consensus::Params though, of course). So ACK that part for sure.

jtimon commented at 3:46 am on February 21, 2015: contributor

As discussed on IRC it is used by CUnitTestParams (which switches this param on and off during tests). It could become a parameter of ContextualCheckBlockHeader() and CheckBlockHeader(), and all the other calls to CheckProofOfWork() can check this outside that function. This is cleaner and it’s not clear to me why fSkipProofOfWorkCheck should not be part of it. Maybe @SergioDemianLerner who created CUnitTestParams has a stronger opinion on this.

theuni commented at 8:37 pm on February 22, 2015: member

After an IRC discussion, I drop my objection to fSkipProofOfWorkCheck in Params. It’s odd, but reasonable.

in src/chainparams.h: in 1164199772 outdated

46+    const Consensus::Params& GetConsensus() const { return consensus; }
47+    const uint256& HashGenesisBlock() const { return consensus.hashGenesisBlock; }
48     const MessageStartChars& MessageStart() const { return pchMessageStart; }
49     const std::vector<unsigned char>& AlertKey() const { return vAlertPubKey; }
50     int GetDefaultPort() const { return nDefaultPort; }
51-    const arith_uint256& ProofOfWorkLimit() const { return bnProofOfWorkLimit; }

sipa commented at 11:16 am on March 1, 2015:

I guess we can get rid of these later, by having callers use Params().GetConsensus().GetX(), rather than Params().GetX(). It’s fine for now though, assuming it minimizes diffs.

sipa commented at 11:17 am on March 1, 2015: member

utACK

jtimon commented at 3:54 pm on March 2, 2015: contributor

Well, the plan is actually to have the methods that require consensus params take the struct as a parameter, getting called function(..., Params().GetConsensus()) and then inside access the fields of the struct directly, params.consensusParam. Like in https://github.com/jtimon/bitcoin/commit/11641997721173d342b1a9df1975adf1d97bbf25

After that is done with enough functions, all these methods can be removed from CChainParams, like in https://github.com/jtimon/bitcoin/commit/bfda15f15f5a310c0388ba364aa530038b43a5e7

sipa commented at 12:08 pm on March 3, 2015: member

@jtimon Even better; but let’s do that later.

gavinandresen commented at 12:20 pm on March 3, 2015: contributor

Looks good to me, but should be combined with simple POW unit tests to exercise/demonstrate the code.

On Mar 2, 2015, at 10:54 AM, Jorge Timón notifications@github.com wrote:

Well, the plan is actually to have the methods that require consensus params take the struct as a parameter, getting called function(…, Params().GetConsensus()) and then inside access the fields of the struct directly, params.consensusParam. Like in jtimon@1164199

After that is done with enough functions, all these methods can be removed from CChainParams, like in jtimon@bfda15f

— Reply to this email directly or view it on GitHub.

laanwj added the label Improvement on Mar 9, 2015

jgarzik commented at 2:00 pm on March 11, 2015: contributor

ut ACK

jtimon force-pushed on Mar 11, 2015

jtimon commented at 4:37 pm on March 11, 2015: contributor

Needed rebase. After #5871, no more doubts about fSkipProofOfWork… @gavinandresen yes, it would be nice to have more tests for pow, incidentally I just found out that someone already started that by separating CalculateNextWorkRequired from GetNextWorkRequired and testing it (I would have preferred that GetNextWorkRequired was tested directly but better is better). In any case that seems out of scope for this simple refactor, probably better to do it with something that changes the interface more like #5171 (if I ever reopen that) or to just do it independently.

jtimon force-pushed on Mar 13, 2015

jtimon commented at 2:03 pm on March 13, 2015: contributor

Rebased after #5883 has been merged to make travis happy.

jtimon force-pushed on Mar 25, 2015

jtimon commented at 11:33 am on March 25, 2015: contributor

Needed rebase.

theuni commented at 7:08 pm on March 25, 2015: member

ACK.

Consensus: Refactor: Introduce Consensus::Params class bd006110fb

jtimon force-pushed on Mar 25, 2015

jtimon commented at 7:40 pm on March 25, 2015: contributor

Oops, good call. First commit edited with @gavinandresen ’s suggestion to correct the documentation.

Consensus: Refactor: Decouple pow.o from chainparams.o d698ef690f

in src/main.cpp: in aac187fe0b outdated

14@@ -15,6 +15,7 @@
15 #include "merkleblock.h"
16 #include "net.h"
17 #include "pow.h"
18+#include "pow.h"

laanwj commented at 9:26 pm on March 25, 2015:

There’s a duplicate include

jtimon force-pushed on Mar 25, 2015

jtimon commented at 11:48 pm on March 25, 2015: contributor

Fixed include duplication reported by @laanwj

laanwj commented at 6:38 am on March 26, 2015: member

utACK

in src/pow.cpp: in d698ef690f

11 #include "primitives/block.h"
12 #include "uint256.h"
13 #include "util.h"
14 
15-unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHeader *pblock)
16+unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHeader *pblock, const Consensus::Params& params)

laanwj commented at 6:51 am on March 26, 2015:

Kudos for passing these parameters explicitly instead of referring to global Params(), breaking the dependency on a global parameter setting will make e.g. testing easier.

laanwj merged this on Mar 26, 2015

laanwj closed this on Mar 26, 2015

laanwj referenced this in commit 687f10d9ec on Mar 26, 2015

MarcoFalke locked this on Sep 8, 2021

Consensus: Refactor: Decouple pow from chainparams #5812