Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Add difficulty to boundary conversion method #191

Merged
merged 2 commits into from
Oct 26, 2021
Merged

Conversation

chfast
Copy link
Owner

@chfast chfast commented Oct 11, 2021

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2021

Codecov Report

Merging #191 (644295f) into master (262adf0) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   98.52%   98.58%   +0.06%     
==========================================
  Files          22       24       +2     
  Lines        1765     1840      +75     
==========================================
+ Hits         1739     1814      +75     
  Misses         26       26              
Flag Coverage Δ
be 53.68% <94.52%> (+1.70%) ⬆️
default 99.77% <100.00%> (+<0.01%) ⬆️
x86_64 89.95% <100.00%> (+0.43%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/ethash/endianness.hpp 100.00% <100.00%> (ø)
test/experimental/difficulty.cpp 100.00% <100.00%> (ø)
test/unittests/test_difficulty.cpp 100.00% <100.00%> (ø)

@chfast chfast force-pushed the difficulty branch 2 times, most recently from cd8d9d2 to bf1d64b Compare October 12, 2021 11:14
@chfast chfast changed the title Difficulty test: Add difficulty to boundary conversion method Oct 12, 2021
@chfast chfast marked this pull request as ready for review October 12, 2021 11:15
@chfast chfast requested a review from gumb0 October 12, 2021 11:16
Comment on lines 31 to 33
uint32_t d[8];
for (int i = 0; i < n; ++i)
d[i] = ethash::be::uint32(difficulty->word32s[8 - 1 - i]);

Choose a reason for hiding this comment

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

This is a bit hard to read: not immediate to understand you're swapping also words positions.

Suggested change
uint32_t d[8];
for (int i = 0; i < n; ++i)
d[i] = ethash::be::uint32(difficulty->word32s[8 - 1 - i]);
int src{7 /*8-1*/};
for (int tgt = 0; tgt < n; ++tgt, --src) {
d[tgt] = ethash::be::uint32(difficulty->word32s[src]);
}

Comment on lines 35 to 36
// For difficulty of 0 (division by 0) or 1 (256-bit overflow) return max boundary value.
if (n == 0 || (n == 1 && d[0] == 1))
return max_boundary;

Choose a reason for hiding this comment

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

Maybe this check can be moved before the swapping loop ?

Suggested change
// For difficulty of 0 (division by 0) or 1 (256-bit overflow) return max boundary value.
if (n == 0 || (n == 1 && d[0] == 1))
return max_boundary;
// For difficulty of 0 (division by 0) or 1 (256-bit overflow) return max boundary value.
static const be_one = ethash::be::(1u);
if (n == 0 || (n == 1 && difficulty->word32s[7] == be_one)
return max_boundary;

Copy link
Owner Author

Choose a reason for hiding this comment

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

I did that at first, but the check is simpler if we have the native order. And where the check is does not matter very much as these cases are only for completeness but do not happen in practice.

Comment on lines 134 to 136
ethash_hash256 boundary{};
for (int i = 0; i < 8; ++i)
boundary.word32s[i] = ethash::be::uint32(q[8 - 1 - i]);

Choose a reason for hiding this comment

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

As a personal preference I'd use the suggestion above for better reading

Copy link
Owner Author

Choose a reason for hiding this comment

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

I moved these to separate function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants