In ConsensusService, ExtendTimerByFactor method calculates and extends the delay. Basically the problem is that the operation is performed between integers, when TimeSpan.FromMilliseconds expects a double as an argument.
There may be a small issue that could result in a loss of accuracy, when casting after the division operation instead of casting before.
“When division is performed on ints, the result will always be an int. You can assign that result to a double, float or decimal with automatic type conversion, but having started as an int, the result will likely not be what you expect” - https://wiki.sei.cmu.edu/confluence/display/java/NUM50-J.+Convert+integers+to+floating+point+for+floating-point+operations
The affected code is
|
TimeSpan nextDelay = expected_delay - (TimeProvider.Current.UtcNow - clock_started) + TimeSpan.FromMilliseconds(maxDelayInBlockTimes * Blockchain.MillisecondsPerBlock / context.M); |
Please see the following proof of concept, where the loss can be appreciated; https://dotnetfiddle.net/iDKRGC
Cast after operation:
4285
8571
Cast before operation:
4285.71428571429
8571.42857142857
We believe that a possible solution would be to cast M before the operation:
TimeSpan.FromMilliseconds(maxDelayInBlockTimes * Blockchain.MillisecondsPerBlock / (double)context.M);
Good news!, with the current number of Validators (M5, F2) this problem does not exist and it only appears from 9 or more consensus nodes (9,10,12,13 ...)
@igormcoelho @vncoelho, what do you think? Is it something we should contemplate?
In
ConsensusService,ExtendTimerByFactormethod calculates and extends the delay. Basically the problem is that the operation is performed between integers, whenTimeSpan.FromMillisecondsexpects a double as an argument.There may be a small issue that could result in a loss of accuracy, when casting after the division operation instead of casting before.
The affected code is
neo/src/neo/Consensus/ConsensusService.cs
Line 250 in ff850c5
Please see the following proof of concept, where the loss can be appreciated; https://dotnetfiddle.net/iDKRGC
We believe that a possible solution would be to cast M before the operation:
TimeSpan.FromMilliseconds(maxDelayInBlockTimes * Blockchain.MillisecondsPerBlock / (double)context.M);Good news!, with the current number of Validators (M5, F2) this problem does not exist and it only appears from 9 or more consensus nodes (9,10,12,13 ...)
@igormcoelho @vncoelho, what do you think? Is it something we should contemplate?