-
Notifications
You must be signed in to change notification settings - Fork 215
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
feat: proof of work audit part 2 #5495
feat: proof of work audit part 2 #5495
Conversation
Test Results (Integration tests) 2 files + 2 11 suites +11 14m 7s ⏱️ + 14m 7s For more details on these failures, see this check. Results for commit b95112d. ± Comparison against base commit 4a9f73c. ♻️ This comment has been updated with latest results. |
aeb68b4
to
ac91604
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, I have one question on a test change
|
||
let achieved_target = | ||
AchievedTargetDifficulty::try_construct(PowAlgorithm::Sha3, achieved - Difficulty::from(1), achieved) | ||
AchievedTargetDifficulty::try_construct(PowAlgorithm::Sha3, achieved, achieved.checked_add(1).unwrap()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2 lines are not equal:
We had (sha3, achieved -1, achieved)
Now we have: (sha3, achieved, achieved + 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original line in the test helper now panicked because it resulted in a difficulty of zero as these tests use localnet
with min and max difficulties set to 1.
The single test that still failed due to this change, base_layer/core/src/chain_storage/blockchain_database.rs/async fn it_correctly_handles_duplicate_blocks()
, because it bargained on a runtime difficulty of zero, was changed to honour minimum difficulty.
ac91604
to
b95112d
Compare
Description --- - Removed `max_block_time` for `struct LinearWeightedMovingAverage`: - Removed manual assignment of `max_block_time` for `struct LinearWeightedMovingAverage` as it is always a fixed multiple of `target_time` according to the **LWMA-1** algorithm specification. - Previously, `max_block_time` was asserted when compiling tests and debug mode with function `fn assert_hybrid_pow_constants(`. - This change resulted in the corresponding removal of `max_target_time` from `struct PowAlgorithmConstants` (and consensus constants) as it is now being calculated by `LinearWeightedMovingAverage`. - Added documentation to public methods. Dependent on #5495 Motivation and Context --- Preparation for code audit How Has This Been Tested? --- All unit tests pass What process can a PR reviewer use to test or verify this change? --- Code walkthrough <!-- Checklist --> <!-- 1. Is the title of your PR in the form that would make nice release notes? The title, excluding the conventional commit tag, will be included exactly as is in the CHANGELOG, so please think about it carefully. --> Breaking Changes --- - [x] None - [ ] Requires data directory on base node to be deleted - [ ] Requires hard fork - [ ] Other - Please specify <!-- Does this include a breaking change? If so, include this line as a footer --> <!-- BREAKING CHANGE: Description what the user should do, e.g. delete a database, resync the chain -->
/// Helper function to provide the difficulty of the hash assuming the hash is big_endian | ||
pub fn big_endian_difficulty(hash: &[u8]) -> Result<Difficulty, DifficultyError> { | ||
let scalar = U256::from_big_endian(hash); // Big endian so the hash has leading zeroes | ||
Difficulty::u256_scalar_to_difficulty(scalar) | ||
} | ||
|
||
/// Helper function to provide the difficulty of the hash assuming the hash is little_endian | ||
pub fn little_endian_difficulty(hash: &[u8]) -> Result<Difficulty, DifficultyError> { | ||
let scalar = U256::from_little_endian(hash); // Little endian so the hash has trailing zeroes | ||
Difficulty::u256_scalar_to_difficulty(scalar) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These will panic if the provided slice length does not match the expected U256
size. This can't happen with the current callers, which are either test functions or use the output from 256-bit SHA3
hashing; however, it may be prudent to document this behavior, or to check the input slice and return an error if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I will add some checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Since the functions already return a Result
, it seems straightforward to add a check, and presumably this would have no performance hit in practice.
Description
Difficulty
indifficulty.rs
.difficulty.rs
.Motivation and Context
Preparation for code audit
How Has This Been Tested?
Added unit tests
All unit tests pass
What process can a PR reviewer use to test or verify this change?
Code walkthrough
Review the new unit test and related code
Breaking Changes