Refactor proof of work related functions out of main

jtimon commented at 7:11 pm on March 10, 2014: contributor

I’m not sure if ScanHash_CryptoPP on miner.cpp would fit here too.

luke-jr commented at 7:52 pm on March 10, 2014: member

Perhaps these should be moved to a chain params class?

in src/pow.cpp: in a6565c167f outdated

10+#include "main.h"
11+#include "uint256.h"
12+
13+static const int64_t nTargetTimespan = 14 * 24 * 60 * 60; // two weeks
14+static const int64_t nTargetSpacing = 10 * 60;
15+static const int64_t nInterval = nTargetTimespan / nTargetSpacing;

unknown commented at 8:04 pm on March 10, 2014:

@luke-jr do you mean move these to chainparams?

luke-jr commented at 10:35 pm on March 10, 2014: member

Right, a function on CChainParams

jtimon commented at 10:36 pm on March 10, 2014: contributor

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

Do you mean move also the functions?

luke-jr commented at 10:52 pm on March 10, 2014: member

Yeah, I mean move the entire functions.

jtimon commented at 10:43 pm on March 14, 2014: contributor

@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.

laanwj commented at 9:24 am on March 17, 2014: member

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.

jtimon commented at 4:54 pm on March 17, 2014: contributor

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.

ghost commented at 3:42 pm on March 19, 2014: none

@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.

jtimon commented at 5:34 pm on March 22, 2014: contributor

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.

ghost commented at 8:15 pm on March 22, 2014: none

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

sipa commented at 11:27 am on June 1, 2014: member

Rebase please?

jtimon commented at 3:25 pm on June 1, 2014: contributor

Rebased

jgarzik commented at 1:42 am on June 7, 2014: contributor

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

jtimon commented at 7:36 pm on June 9, 2014: contributor

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.

sipa commented at 8:26 pm on June 9, 2014: member

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).

jtimon commented at 10:19 pm on June 9, 2014: contributor

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

jgarzik commented at 10:59 pm on June 9, 2014: contributor

@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.

ghost commented at 9:12 am on June 10, 2014: none

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.

jtimon commented at 11:26 am on June 17, 2014: contributor

@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.

jtimon commented at 3:23 pm on June 19, 2014: contributor

I have also moved UpdateTime.

jtimon commented at 7:37 pm on June 20, 2014: contributor

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

jtimon closed this on Jun 20, 2014

jtimon reopened this on Jun 21, 2014

ghost commented at 9:42 am on June 21, 2014: none

@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.

jtimon commented at 10:35 am on June 21, 2014: contributor

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

ghost commented at 12:26 pm on June 21, 2014: none

Ok, looks good to me.

ACK.

Refactor proof of work related functions out of main df852d2bcc

move pow constants to chainparams fd704c7b2c

BitcoinPullTester commented at 10:44 pm on June 23, 2014: none

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.

ghost commented at 5:29 am on June 25, 2014: none

@laanwj is there anything holding up this being merged?

laanwj commented at 5:49 am on June 25, 2014: member

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

laanwj merged this on Jun 25, 2014

laanwj closed this on Jun 25, 2014

laanwj referenced this in commit 208bf5b9e0 on Jun 25, 2014

jtimon deleted the branch on Jun 25, 2014

DrahtBot locked this on Sep 8, 2021

Refactor proof of work related functions out of main #3839