fix: apply Math.Round to FeeRate to mitigate rounding issues

[lukasdll]

Problem: The FeeRate constructor FeeRate(Money feePaid, int size) might lead to minor rounding issues due to direct casting to long, especially when feePaid.Satoshi/size has a fractional part >= 0.5.

This issue manifest in other projects using NBitcoin, in particular: https://github.com/zkSNACKs/WalletWasabi/issues/11215

E.g.: The fee rate per byte would be: 101/70 = 1.442857... The fee rate per kilobyte would be: 1.442857 * 1000 = 1442.857... But, when converted to long, the value becomes 1442, effectively rounding down and losing the 0.857 fractional value.

Solution: Implemented Math.Round to handle rounding more predictably and prevent potential issues with transaction fee calculation precision.

By using Math.Round with MidpointRounding.AwayFromZero, any decimal value from .5 to .999' is always rounded up to the next whole number. Without specifying MidpointRounding.AwayFromZero, and thus using the default MidpointRounding.ToEven (Banker’s Rounding), values ending in .5 are rounded to the nearest even whole number, which may produce discrepancies or undesired results in certain circumstances.

This fix ensures that fee rate calculations are more accurate and prevents underpaying or overpaying fees due to rounding discrepancies.

[lontivero]

I think the real problem is that the fee rate is represented internally as a long. A rate is a fractional number and there is no good way to represent it as an integer without losing precision. I mean, the trick here has been to multiply it by 1000 so it can be expressed as fee rate per kilo but that's not enough.

[knocte]

I think the real problem is that the fee rate is represented internally as a long.

So why not fix this root cause instead of improving the rounding?

[lukasdll]

Thanks for the input! 👍 If I understood correctly, there are two main options.

Option 1: Changing the _FeePerK type from Money to decimal. This would bring more precision but requires extensive code review and testing to ensure backward compatibility. It might also come at a cost as it increases resource usage due to decimal operations being more costly than long operations.

Option 2: Adopting a larger multiplier, say per MB instead of KB (i.e., using 1,000,000) - this would increase the precision to a more than sufficient level without a major refactor.

Keen to hear more thoughts!

[lontivero]

Those were not the two options. The fee rate is a rate, a fractional number. No trick or data type (other than a Fractional data type, which is not built in the BCL) can represent a rate without losing precision. The FeeRate should be represented internally as a fraction and only lose precision in the conversion to other types as long or Money.

[lontivero]

I was thinking a bit more about this issue and given we never operate on fee rates then it makes little sense how we represent it internally. I mean, this class is not designed to be used to do math with fee rates, it was designed to simply express a datum and that's it so, using fractions would be overkilling. Using a decimal should be enough.

[lukasdll]

Indeed, the second option I pictured above would be an improvement of a fix, from the fee rate/kilo to fee rate/mega to earn in precision. While it might work and be simpler to implement, using the decimal type internally would be more elegant, and I agree is a more pertinent choice.

Strictly speaking, from a rounding perspective, the difference narrows down to 6 digits in the case of the fee rate / mega, compared to the 29 digits from the decimal type.

I have the intuition that 6 digits would be sufficient, because I don't think that anyone realistically need to express a fee rate with more than 6 decimals, however, the code would be easier to understand using decimals than the fee rate/kilo, or fee rate/mega, therefore, if there are no drawbacks from the additional computational cost handling 29 decimals implies, I convey (also from a practical point of view) that using decimals is the way to go.

Using decimals could also eliminate a few math calcs, improving the result from a design perspective. If it's not possible to completely eliminate all mathematical operations from the FeeRate class, we could deprecate the related method(s) and move the calculations to a helper class to keep the FeeRate class strictly for data representation. I would have to dig deeper into NBitcoin to determine whether this aligns with the rest of the code or if there is already some sort of helper class that could be utilized for that purpose

An additional minor detail. The null checks could also be factored into a helper method (it is repeated 6 times in that class):

if (left is null)
    throw new ArgumentNullException(nameof(left));
if (right is null)
    throw new ArgumentNullException(nameof(right));

I'm going to do some changes & tests to cover the above. I will try the decimal type approach, and try to remove math calcs as much as possible, or suggest changes as appropriate.

[NicolasDorier]

@lukasdll can you add a small unit test?