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.

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.

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

utACK

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

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

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 [email protected] 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/bitcoin@1164199

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

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

ut ACK

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.

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

Needed rebase.

ACK.

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

Fixed include duplication reported by @laanwj

utACK