Skip to content

Conversation

@jtimon
Copy link
Contributor

@jtimon jtimon commented Jun 26, 2014

Get rid of HexBits.

src/miner.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jtimon
Copy link
Contributor Author

jtimon commented Jun 27, 2014

@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
Copy link
Member

laanwj commented Jun 27, 2014

@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
Copy link
Contributor Author

jtimon commented Jun 27, 2014

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 jtimon changed the title Replace HexBits with HexStr template specialization Replace HexBits with strprintf Jun 27, 2014
@laanwj
Copy link
Member

laanwj commented Jun 27, 2014

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
Copy link
Member

sipa commented Jun 27, 2014

Untested ACK.

@jtimon
Copy link
Contributor Author

jtimon commented Jun 27, 2014

Yep, this won't show up on the automatic tests so it will need to be tested manually.

@jgarzik
Copy link
Contributor

jgarzik commented Jun 27, 2014

u-t ACK

@jtimon
Copy link
Contributor Author

jtimon commented Jun 27, 2014

Tested both functions in an independent program and they produce the same results.

@ghost
Copy link

ghost commented Jun 27, 2014

ACK

@BitcoinPullTester
Copy link

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 laanwj merged commit 645d497 into bitcoin:master Jun 28, 2014
laanwj added a commit that referenced this pull request Jun 28, 2014
645d497 Replace HexBits with strprintf (jtimon)
@jtimon jtimon deleted the hexstr branch June 28, 2014 16:23
@jtimon jtimon mentioned this pull request Aug 30, 2014
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants