util: Adjust Fraction class addition overload overflow tests #2748
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@Tahvok, the arch maintainer, caught a subtle error in the extreme overflow tests for the newly augmented Fraction class in the 5.4.6.0 release. Arch, unlike other distributions, uses the C++ compiler flag -D_GLIBCXX_ASSERTIONS, and this caught the error.
Essentially the problem is in the fraction addition overflow tests at the extreme negative case. When the fraction addition overload operator goes to add fractions of the same denominator, where both addends are std::numeric_limits<int64_t>::min() / 2, the result on the numerator will be std::numeric_limits<int64_t>::min(). When simplification is called, it will call std::gcd, which calls std::abs. The std::abs of std::numeric_limits<int64_t>::min() is an overflow, because it would be std::numeric_limits<int64_t>::max() + 1, but this is handled as an assert in glibc. So to prevent this, we tightened the negative side overflow constraint in overflow_add by 1 to ensure the numerator will never be less than std::numeric_limits<int64_t>::min() +1, so that the std::abs of that will succeed (i.e. will be std::numeric_limits<int64_t>::max()).
This is really esoteric and will not be reached by any actual wallet operation, but it is being treated as a hotfix to get Arch compiles to work.
Thanks to @div72 for helping me troubleshoot this.