feat: proof of work audit part 2 #5495
Conversation
ghpbot-tari-project
added
P-acks_required
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. |
hansieodendaal
force-pushed
the
ho_pow_audit_02
branch
3 times, most recently
from
aeb68b4
to
ac91604
Compare
|
|
||
| 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.
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.
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.
hansieodendaal
force-pushed
the
ho_pow_audit_02
branch
from
ac91604
to
b95112d
Compare
ghpbot-tari-project
removed
the
P-reviews_required
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.
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.
Thanks! I will add some checks.
There was a problem hiding this comment.
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
Difficultyindifficulty.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