Consensus: Decouple pow.o from util.o

jtimon commented at 3:40 pm on February 2, 2016: contributor

If we want to build pow.o as part of libconsensus, it cannot depend on util.h, which contains globals among other undesired dependencies.

As an alternative, to maintain the error messages in CheckProofOfWork() we could pass it a CValidationState, but that would be more disruptive, as it would accept every current caller of CheckProofOfWork().

sipa commented at 3:47 pm on February 2, 2016: member

utACK; I don’t think those error() calls should be there in the first place.

paveljanik commented at 4:26 pm on February 2, 2016: contributor

utACK

Minor nit: can we move the retarget debug output somewhere else instead of deleting it?

jtimon commented at 5:07 pm on February 2, 2016: contributor

@paveljanik I guess they could be maintained in a less efficient way while moving them out of the consensus code with a non trivial amount of work and disruption. Are those debug strings really that useful? I’ll think more about it, maybe printing some things to the log on connectBlock when it’s needed by duplicating some of the code in https://github.com/bitcoin/bitcoin/pull/7459/files#diff-ba91592f703a9d0badf94e67144bc0aaR58 (to check the condition) doesn’t look so bad. But I’m sorry, at this point it doesn’t look like a minor nit to me, but as something that could potentially make the PR very different.

In any case, if you have some specific new place to put the messages in mind (outside of pow, of https://github.com/jtimon/bitcoin/blob/consensus-moveonly-0.13.99/src/consensus/consensus.cpp and https://github.com/bitcoin/bitcoin/pull/7091/files#diff-480477e89f9b6ddafb30c4383dcdd705R251), please, propose it.

paveljanik commented at 5:15 pm on February 2, 2016: contributor

Do not get me wrong, the nit is really minor. The info can be useful sometimes though.

sipa commented at 5:21 pm on February 2, 2016: member

They’re already reported through the normal means of block validation checks failing; main.cpp’s CheckBlockHeader will put the reason for failure in the CValidationState.

paveljanik commented at 5:27 pm on February 2, 2016: contributor

@sipa We re talking about this in CalculateNextWorkRequired:

0    /// debug print
1    LogPrintf("GetNextWorkRequired RETARGET\n");
2    LogPrintf("params.nPowTargetTimespan = %d    nActualTimespan = %d\n", params.nPowTargetTimespan, nActualTimespan);
3    LogPrintf("Before: %08x  %s\n", pindexLast->nBits, bnOld.ToString());
4    LogPrintf("After:  %08x  %s\n", bnNew.GetCompact(), bnNew.ToString());

jtimon commented at 5:44 pm on February 2, 2016: contributor

@paveljanik and it’s not that I don’t want to solve the nit, is that I don’t see a trivial solution. Which times can the information be useful? Maybe that would help us determine where these messages should be printed and what parts of the information are more important (say, only the new difficulty, but the old is not so interesting).

paveljanik commented at 5:51 pm on February 2, 2016: contributor

Yes, when we print the new difficulty, we can grep for the previous one…

jtimon commented at 6:16 pm on February 2, 2016: contributor

Ok, that should simplify things.

CalculateNextWorkRequired() is only called in tests (I assume you don’t care about this) and GetNextWorkRequired() which itself is called only from miner.cpp (I assume you don’t care about this) and ContextualCheckBlockHeader(), which is only called from TestBlockValidity() and AcceptBlockHeader()…would you miss the logs in both functions or just one? TestBlockValidity() is only called from miner and rpc/mining, so I guess you would miss it in AcceptBlockHeader(), which is called from ProcessMessage() if (strCommand == NetMsgType::HEADERS ... and AcceptBlock(), called from ProcessNewBlock(), which is also called from mining code, in addition to LoadExternalBlockFile() and ProcessMessage() if (strCommand == NetMsgType::BLOCK .... LoadExternalBlockFile() is called only from init.cpp.

So my conclusion is that where you really want those logs are in ProcessMessage(), in the NetMsgType::HEADERS and NetMsgType::BLOCK cases (or maybe just one?). Does this make sense?

paveljanik commented at 6:20 pm on February 2, 2016: contributor

Thanks for your investigation. I think it is only needed after receiving new block, that should be enough.

jtimon commented at 8:43 pm on February 2, 2016: contributor

Thinking more about it…it would be much simpler in code to just log the current difficulty with every block received than only when a block is received and the difficulty has changed. Would that be a reasonable solution?

sipa commented at 8:49 pm on February 2, 2016: member

I like that idea. Just print the difficulty on the same line that also logs to total chainwork and timestamp etc.

dcousens commented at 9:32 pm on February 2, 2016: contributor

concept ACK, waiting on changes for final ACK, utACK bf462dd

laanwj commented at 8:57 am on February 3, 2016: member

Concept ACK

it would be much simpler in code to just log the current difficulty with every block received than only when a block is received and the difficulty has changed. Would that be a reasonable solution?

Yes

jtimon commented at 1:06 pm on February 3, 2016: contributor

Added two commits to potentially squash (the first or both depending on feedback).

laanwj added the label Refactoring on Feb 3, 2016

paveljanik commented at 8:16 am on February 4, 2016: contributor

I like the simplified version.

jtimon commented at 1:10 pm on February 4, 2016: contributor

Great, I will squash soon unless anyone likes the other version more.

laanwj commented at 2:19 pm on February 4, 2016: member

utACK new version after squash

sipa commented at 4:39 pm on February 4, 2016: member

utACK

Consensus: Decouple pow.cpp from util.h f3757a0391

jtimon force-pushed on Feb 4, 2016

jtimon commented at 6:24 pm on February 4, 2016: contributor

Squashed.

in src/main.cpp: in f3757a0391

2290@@ -2291,8 +2291,9 @@ void static UpdateTip(CBlockIndex *pindexNew) {
2291     nTimeBestReceived = GetTime();
2292     mempool.AddTransactionsUpdated(1);
2293 
2294-    LogPrintf("%s: new best=%s  height=%d  log2_work=%.8g  tx=%lu  date=%s progress=%f  cache=%.1fMiB(%utx)\n", __func__,
2295-      chainActive.Tip()->GetBlockHash().ToString(), chainActive.Height(), log(chainActive.Tip()->nChainWork.getdouble())/log(2.0), (unsigned long)chainActive.Tip()->nChainTx,
2296+    LogPrintf("%s: new best=%s  height=%d bits=%d log2_work=%.8g  tx=%lu  date=%s progress=%f  cache=%.1fMiB(%utx)\n", __func__,

paveljanik commented at 10:08 pm on February 4, 2016:

The rest is using two spaces here. Is this intentional?

jtimon commented at 0:57 am on February 5, 2016:

I don’t know, I just didn’t notice. I can use two spaces as well, but doesn’t seem important.

paveljanik commented at 6:29 am on February 5, 2016:

Two spaces look strange, yes. But its already there… Can you please add them?

laanwj commented at 10:03 am on February 5, 2016:

It looks like the spacing now creates a kind of hierarchical grouping, two spaces between a group one between related infos (height/bits/log2_work and date/progress). Fine with me really.

rebroad commented at 4:54 pm on March 8, 2016:

This line is getting crazily long now….

paveljanik commented at 10:07 am on February 5, 2016: contributor

@laanwj I also do not care that much. ACK It is now even better than before refactoring. Thanks @jtimon

dcousens commented at 10:17 am on February 5, 2016: contributor

re-ACK f3757a0

laanwj merged this on Feb 5, 2016

laanwj closed this on Feb 5, 2016

laanwj referenced this in commit e7ea5db0c1 on Feb 5, 2016

GamerSg referenced this in commit cd1c478fd0 on Feb 27, 2016

laanwj referenced this in commit 20f9ecd343 on Apr 28, 2016

dexX7 referenced this in commit a400d26561 on Jun 8, 2017

dexX7 referenced this in commit 2476c64723 on Jun 8, 2017

dexX7 referenced this in commit 3edbe7cc79 on Jun 8, 2017

codablock referenced this in commit 9853389588 on Sep 16, 2017

codablock referenced this in commit f5675c746e on Sep 16, 2017

codablock referenced this in commit 094c0352e7 on Sep 19, 2017

codablock referenced this in commit 48b5382208 on Sep 19, 2017

codablock referenced this in commit bcd5ab7155 on Dec 9, 2017

codablock referenced this in commit 7932d725f9 on Dec 9, 2017

codablock referenced this in commit 7ec1e18874 on Dec 11, 2017

codablock referenced this in commit 61227bb21c on Dec 20, 2017

CryptoCentric referenced this in commit 4c16c8d9e2 on Feb 15, 2019

MarkLTZ referenced this in commit 3977804911 on Apr 27, 2019

zkbot referenced this in commit 173d6a7c18 on Dec 3, 2019

MarcoFalke locked this on Sep 8, 2021

Consensus: Decouple pow.o from util.o #7459