Perhaps these should be moved to a chain params class?

Right, a function on CChainParams

Yes, @luke-jr if you mean those 3 constants like @Drak says, that makes sense to me.

Do you mean move also the functions?

Yeah, I mean move the entire functions.

@luke-jr my understanding (based on what @laanwj and @sipa have told me) is that CChainParams should only contain constants, so I think only the functions should remain out of chainparams like @Drak suggested.
I could add those changes on this pull request or make them later (since they would make this simple pull request less likely to be accepted). I'll wait to have more feedback before adding anything.

Right, CChainParams should only contain constants. And we don't intend to support multiple POWs in bitcoin.

I'm fine with moving these to a seperate file, main is much too large and this is a clearly delimited separable piece of functionality.

Don't move ScanHash_CryptoPP from miner.cpp here: it's a static function and keeping the internal miner functions together makes sense.

Ok, I'll just leave it as it is and move nTargetTimespan, nTargetSpacing and nInterval to CChainParams in another pull request after/if this one gets merged. More concretely, I want to avoid merge conflicts with #3824 and this is not a high priority change anyway, just refactoring to improve readability and move towards a more modular codebase.

@jtimon I think you should just go ahead an commit the change to move nTargetTimeSpan, nTargetSpacing and nInterval. It's just one small commit and I don't see how moving the params will create a merge conflict with #3824. There is nothing worse than splitting up PRs unnecessarily. More chance of things getting forgotten.

It may create a merge conflict with the other PR because it edits the same interface (abstract class) CChainParams, but they should be easily solvable so I moved the constants as suggested.

Nice. This PR looks good to me (untested but the diff seems pretty straightforward).

Rebase please?

Rebased

Untested ACK... though I did a line-by-line scan to look for aberrant changes mixed into the code movement.

While rebasing it came to mind that maybe these pow-specific constants don't really belong to chainparams. There could be a pow class with them as arguments and having these functions as methods.
This would make it easier for altchains to maintain: just be extending this class and overwriting the pow functions many altcoins would become trivial to maintain on top of bitcoin (less excuses to compete for development resources).

Just to be clear, Freicoin uses the same SHA256 pow as bitcoin, and although I got to this pool request by looking for the places I would need to touch for implementing private chains (also non-bitcoinish), the main objective of the PR is nothing but getting things that belong together out of main.
Well, now that I think about it we have a different difficulty filter so a pow class should help with our maintainability, although I don't think it will be a great difference with this functions already separated.
The question remains, in this case that it's clear it has no benefit at all for bitcoin, is it desirable to facilitate things for projects based on bitcoin's codebase or not?
The ideal solution for altchains would be to directly include the functions on chainparams, similar to to what @luke-jr previously proposed and what it's done in libcoin to make the library "chain agnostic" : https://github.com/libcoin/libcoin/blob/master/src/coinChain/Chain.cpp
but that seems incompatible with the objective of maintaining only constant values on chainparams. Also, it could negatively affect bitcoin's modularity if you start to encapsulates many differences from different places other than pow in there, chainparams. It could easily morph into a hard to maintain beast if you start to try to be "agnostic" in relation to, say, a chain with explicit support for asset issuance.
So it seems a Pow class would be more rational for bitcoin even if it requires a little more work from the altchain developers than the chain-agnostic approach.
But still, a class instead of the independent functions and the constants in chainparams (this PR as it currently stands) doesn't offer anything interesting to bitcoind's code itself.
That change wouldn't imply any risks because it would be a trivial refactoring like moving the constants to chainparams.
Sorry for getting so long and reiterative, maybe this belongs to the mailing list, but it's just seems a simple and interesting example to debate a general policy for "non-bitcoinish" contribution. To define more clearly what kind of things can be accepted and which would represent unnecessary weight for bitcoin.
I'm particularly interested in @gavinandresen @mikehearn and @Drak 's options, since they have expressed some concerns about making it easier for other projects to just rebase on top of bitcoin's latest version and try to keep the differences minimal and the general improvements in bitcoin.
I can also move this discussion to another PR with some actual code that shows the little differences more clearly instead of the mailing list, and leave this one as the simple "move things out of main one functional thing at a time" it is.
Whatever you think it's best.

I don't think usefulness for derived codebases is a priority here. If code quality improvements benefit them, so much the better, but I don't think we should make our code more complex to accommodate potential forks...

I like this because it moves responsibilities away from main, and the code looks sane to me (though I'd like to go over it line by line).

Thank you for the feedback, makes sense.
It was already rebased just before my previous comment, it contains the merge of the PR 4300

@sipa Agree it is not a primary priority. If we can refactor, make our code more modular, and make life less difficult for clones/forks, that is good.

Just for the record, I have been running two nodes with this patch since Jun 1 with no problems or funny stuff in the logs.

@Drak thanks for the testing.
Rebased.
The additional refactorings I was talking about seem to be more complex than I initially thought, so I will just leave this PR as it is and maybe propose further changes in another PR later.
So, yes, better do it through incremental refactoring.

I have also moved UpdateTime.

I'm working on more changes so I will close this for now. I will reopen it soon.

@jtimon The PR looks good, but generally a PR like this should just be shuffing code. Makes it easier to read the diff and know what is going on. referring specifically to 3931601 which should be the subject of a separate PR.

No problem, I will separate the commits in 2 different PR.

Ok, looks good to me.

ACK.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p3839_fd704c7b2c5ab8b24b1829f000b829d7156b8b3c/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@laanwj is there anything holding up this being merged?

@Drak No, looks good to me now. I'll check the code moves and try to merge this today.