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

Max election age precision #4549

Merged
merged 4 commits into from
Apr 11, 2024
Merged

Conversation

RickiNano
Copy link
Contributor

max_election_age is currently being casted as std::chrono::seconds which causes the value to be rounded to nearest second. This pr changes it to use double and returns the value with 2 decimals.
It's unfortunate that I did not notice this issue in the original PR.
The unit test is also updated to test that both election age values are greater than zero. On my machine they are 0.09 seconds during unit test.
I also implemented the syntax Colin suggested for oldest_election_start to avoid the if statement

@dsiganos
Copy link
Contributor

dsiganos commented Apr 10, 2024

The unit test is a little messy and more complicated than it needs to be.
I will tidy it up so that you can see what are the unnecessary elements.

@dsiganos
Copy link
Contributor

There is no need to disable_request_loop because there are no voters on the network and the election will not be voted on and it will sit and wait.

dsiganos
dsiganos previously approved these changes Apr 10, 2024
clemahieu
clemahieu previously approved these changes Apr 10, 2024
@simpago
Copy link
Contributor

simpago commented Apr 10, 2024

Wouldn't it be simpler to return integer milliseconds instead of floating point seconds?

@RickiNano
Copy link
Contributor Author

Good point @simpago
What do you think @dsiganos and @clemahieu ?

@dsiganos
Copy link
Contributor

In general, I always favour integers and fixed point to floating point. Floating point comes with too many traps. I agree that returning milliseconds is preferable.

@RickiNano RickiNano dismissed stale reviews from clemahieu and dsiganos via 291151e April 10, 2024 12:22
@RickiNano
Copy link
Contributor Author

I have updated it to use milliseconds

@dsiganos
Copy link
Contributor

But now the other floating point sticks out like a sore thumb...
We can avoid the other floating point by diving it into smaller units too.

@RickiNano
Copy link
Contributor Author

I don't see anything wrong with having the percentage value as a floating point value. Without any decimals it lacks the required precision.

@dsiganos
Copy link
Contributor

Floats are inaccurate and cause problems and it is good to avoid them. But in RPC code, I don't mind having floats, I would be worried if the floats were in the protocol code.

@dsiganos dsiganos merged commit 52af411 into nanocurrency:develop Apr 11, 2024
24 of 26 checks passed
@qwahzi qwahzi added this to the V27 milestone Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged / V27.0
Development

Successfully merging this pull request may close these issues.

5 participants