-
Notifications
You must be signed in to change notification settings - Fork 219
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
fix: estimated hashrate calculation is incorrect #3996
fix: estimated hashrate calculation is incorrect #3996
Conversation
…fix-estimated-hashrate
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.
Nice work. Happy to merge if you keep the protobuf numbers in NetworkDifficultyResponse
uint64 height = 3; | ||
uint64 timestamp = 4; | ||
uint64 pow_algo = 5; | ||
uint64 sha3_estimated_hash_rate = 3; |
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.
Rather add these fields at the end. numbers in protobuf should not change.
uint64 sha3_estimated_hash_rate = 3; | |
uint64 sha3_estimated_hash_rate = 6; | |
uint64 height =3; |
|
||
/// The number of past blocks to be used on moving averages for (smooth) estimated hashrate | ||
/// The hash rate is calculated as the difficulty divided by the target block time | ||
const SHA3_HASH_RATE_MOVING_AVERAGE_WINDOW: usize = 12; |
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.
Need a comment here for why these values are different
/// Adds a new hash rate entry in the moving average and recalculates the average | ||
pub fn add(&mut self, height: u64, difficulty: Difficulty) { | ||
// target block time for the current block is provided by the consensus rules | ||
let target_time = self |
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.
Nice
sum / count | ||
} | ||
|
||
pub fn get_average(&self) -> u64 { |
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.
pub fn get_average(&self) -> u64 { | |
pub fn average(&self) -> u64 { |
…/tari into fix-estimated-hashrate
Implemented all suggested changes |
Description
estimated_hash_rate
field but now it corresponds to the sum of the sha3 and monero hash rates, so existing clients (i.e. web explorer) will not breakMotivation and Context
The current hash rate calculation is incorrect, we want to use a separated moving average for each PoW algo independently
How Has This Been Tested?
Unit testing for the moving average hash rate calculation