Optimize construct_bm_job#1321
Conversation
This PR eliminates several inefficient codepaths * Limit back and forth reversing of byte or word orders; * Remove merkle_root_be and prev_block_hash_be from bm_job; * All parameters are converted to binary once; * Stack allocation instead of malloc; * Remove unused functions; * Unroll byte and word reversal functions.
Georges760
left a comment
Georges760
left a comment
There was a problem hiding this comment.
A very GOOD PR, GG Mutatrum!
Didn't test it on HW but since you did it on both 97 and 70, should be fine, this kind of change easily brake mining, so as long as it mine well, it is fine.
Thanks, it messes with your sanity this stuff.
Indeed, if something is wrong, there's three options:
|
|
|
bold PR; thanks for risking your sanity to improve the mining job construction. I'll give this a try on my BM1368 Supra today. @johnny9 you might be interested in seeing this. |
| TEST_ASSERT_EQUAL_UINT8_ARRAY(expected_midstate_bin, job.midstate, 32); | ||
| uint8_t expected_midstate_bin_reversed[32]; | ||
| reverse_32bit_words(expected_midstate_bin, expected_midstate_bin_reversed); | ||
| reverse_endianness_per_word(expected_midstate_bin_reversed); |
There was a problem hiding this comment.
I don't like this, my suspicion is that the hex string above is not in the same order as it would actually come out of the midstate. In code, the endianness isn't changed, only word order.
Paving the way for #420 and dropping the job queues, slowly getting there. |
|
Looks like good cleanup at first glance. I need to take a closer look at the test case changes to be sure it's functionally working well. |
Only the setup of test cases has changed, or expected values are converted to binary, all the assertions in the test cases are the same. The only one I'm not happy with is this one: #1321 (comment) One other thing I'm considering is to move out all the The only manipulation happening then is the word endianness swap of prev_block_hash, but that is an actual bitcoin vs stratum difference. |
* Optimize construct_bm_job This PR eliminates several inefficient codepaths * Limit back and forth reversing of byte or word orders; * Remove merkle_root_be and prev_block_hash_be from bm_job; * All parameters are converted to binary once; * Stack allocation instead of malloc; * Remove unused functions; * Unroll byte and word reversal functions. * Restore comment * Fix test * More test fix * Fix some test_mining tests * Slowly fixing more tests * One more time * Maybe the last one? * So close * Revert coinbase tx construction setup * Missing include * Fix * Add tests for utils * Don'y copy new bm_job * Revert variable rename * Swawp endianness instead of full 32 byte order * Fix compile error in test_mining.c * Fix test_utils * Fix mining test * Fix
* Optimize construct_bm_job This PR eliminates several inefficient codepaths * Limit back and forth reversing of byte or word orders; * Remove merkle_root_be and prev_block_hash_be from bm_job; * All parameters are converted to binary once; * Stack allocation instead of malloc; * Remove unused functions; * Unroll byte and word reversal functions. * Restore comment * Fix test * More test fix * Fix some test_mining tests * Slowly fixing more tests * One more time * Maybe the last one? * So close * Revert coinbase tx construction setup * Missing include * Fix * Add tests for utils * Don'y copy new bm_job * Revert variable rename * Swawp endianness instead of full 32 byte order * Fix compile error in test_mining.c * Fix test_utils * Fix mining test * Fix
5 participants
This PR eliminates several inefficient codepaths in constructing new jobs:
merkle_root_beandprev_block_hash_befrombm_job;calculate_coinbase_tx_hashandcalculate_merkle_root_hashreturn hashes instead of hex strings;Verified on BM1397 and BM1370.