Replace HexBits with strprintf #4422

pull jtimon wants to merge 1 commits into bitcoin:master from jtimon:hexstr changing 4 files +2 −13

jtimon commented at 10:36 PM on June 26, 2014: contributor

Get rid of HexBits.

in src/miner.cpp:None in 2f6718f5bd outdated

 412 | @@ -412,7 +413,7 @@ bool CheckWork(CBlock* pblock, CWallet& wallet, CReserveKey& reservekey)
 413 |  
 414 |      //// debug print
 415 |      LogPrintf("BitcoinMiner:\n");
 416 | -    LogPrintf("proof-of-work found  \n  hash: %s  \ntarget: %s\n", hash.GetHex(), hashTarget.GetHex());
 417 | +    LogPrintf("proof-of-work found  \n  hash: %s  \ntarget: %s\n", pblock->GetHash().GetHex(), HexStr(pblock->nBits));

sipa commented at 10:48 PM on June 26, 2014:

Not that I care much about this debug output line but you're aware pblock->nBits is not the same as hashTarget?

in src/util.h:None in 2f6718f5bd outdated

 281 | @@ -282,6 +282,17 @@ inline std::string HexStr(const T& vch, bool fSpaces=false)
 282 |      return HexStr(vch.begin(), vch.end(), fSpaces);
 283 |  }
 284 |  
 285 | +template<>
 286 | +inline std::string HexStr<unsigned int>(const unsigned int& nBits, bool fSpaces)

laanwj commented at 8:27 AM on June 27, 2014:

What about just strprintf("%08x", nBits)?

jtimon commented at 10:56 AM on June 27, 2014: contributor

@sipa I assumed HexStr(pblock->nBits) was going to be equivalent to uint256().SetCompact(pblock->nBits).GetHex() but I didn't tested the output. I can leave as it was. @laanwj using strprintf("%08x", nBits) to replace HexBits instead of defining the new specialized HexStr seems better (less additions, the same deletions) if the outputs are equivalent. Would this serve for the case sipa is discussing too? Either way seems better to remove HexBits, which is just too specific.
laanwj commented at 11:00 AM on June 27, 2014: member

@jtimon No, it wouldn't address sipa's problem. Let's leave the line in CheckWork as it is, as I see it it doesn't even use hexbits?
jtimon commented at 11:22 AM on June 27, 2014: contributor

No, it doesn't, I thought that could be a use outside of the rpc stuff, but yeah, I'll just leave it as it was. Replace HexBits( with strprintf("%08x", it is.
jtimon renamed this:
~~Replace HexBits with HexStr template specialization~~
Replace HexBits with strprintf
on Jun 27, 2014
laanwj commented at 11:40 AM on June 27, 2014: member

OK! We indeed still have to check that the output is the same, but looking at the source code the intention is the same. Ie, write the bytes of 0xAABBCCDD as AA BB CC DD in memory using htonl then print them as hex bytes, resulting in AABBCCDD.
sipa commented at 11:41 AM on June 27, 2014: member

Untested ACK.
jtimon commented at 11:43 AM on June 27, 2014: contributor

Yep, this won't show up on the automatic tests so it will need to be tested manually.
jgarzik commented at 1:35 PM on June 27, 2014: contributor

u-t ACK
jtimon commented at 5:53 PM on June 27, 2014: contributor

Tested both functions in an independent program and they produce the same results.
ghost commented at 6:58 PM on June 27, 2014: none

ACK
Replace HexBits with strprintf 645d497aa0
BitcoinPullTester commented at 12:28 PM on June 28, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4422_645d497aa0010525441ce409e8e6d327a157ab39/ 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 merged this on Jun 28, 2014
laanwj closed this on Jun 28, 2014
laanwj referenced this in commit f62784f961 on Jun 28, 2014
jtimon deleted the branch on Jun 28, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4794 Finish Proof class