chore: LongRange Does Not Support Empty Range

[ata-nas]

• Removed the special case for -1 -> -1 LongRange
  • We used to have this to denote "empty" range
  • We've since moved on from this, because this is extra complexity
    with no added benefit
• Fixed unit tests that asserted the possibility of -1 -> -1 LongRange

NOTE: This change also comes with the need to validate that we do not
disrupt or break current logic, which could be using the empty range
notion! LongRange has almost 400 usages at the time of this commit.
Thorough walkthrough is needed to validate that nothing is broken.

Reviewer Notes

Please throughly review all usages to find potential breaking points, where logic could be utilizing empty ranges.

Related Issue(s)

Resolves #1072

[ata-nas]

Please note: this change should be very thoroughly tested that the logic we have (in the almost 400 usages of LongRange) does not allow the creation of an empty range (i.e. -1 -> -1) as this now throws exception! As far as I tested, we should not have such places, CI is also passing, but all reviewers must take heed.

[jsync-swirlds]

Why the -1 here? Shouldn't we support a Long range that ends at Long.MAX_VALUE?
That would also eliminate the need to check here, as long cannot be greater than max vlaue.

[ata-nas]

I believe this is because java uses signed longs, but we use unsigned longs in the proto. It was firstly developed like so. In order to not overflow on the edge case, or because of the size() method, as explained there.

[jsync-swirlds]

I suspect that it's safe to use the actual max value now, it was probably always safe, because the unsigned usage in the proto just means we get negative values (in java) for proto values greater than Long.MAX_VALUE.

Overflow throws an exception in most cases; and even if it doesn't (e.g. someone uses bit manipulation instead of arithmetic functions) we just end up with a negative number (which will be rejected).

[ata-nas]

So, this is all about the correctness of the size method then. As in order for it to be correct, it has to add a +1 on the difference between first and last. If range is 0 -> max long, then we will add a +1 to the max long, which will be problematic.

[jsync-swirlds]

Adding +1 to max long results in Long.MIN_VALUE, which should be easy to handle if necessary.
The net effect of this should still be correct.

Also, why is size adding +1? That seems like it's inviting an error sooner or later.

[jsync-swirlds]

We don't really plan to support unsigned long in Java; it's something we'll need to consider someday (but again, we shouldn't reach those block numbers for a very long time).
I'm just noting that overflow is easy to detect, and perhaps size should do that rather than adding a confusing edge here that is very easy to be incorrectly modified by our future selves.

[ata-nas]

The issue is that a range 0 -> long max (which is valid) indeed has a size of long max + 1 which will overflow in java. So there is really nothing we can do, we cannot return long max + 1.

P.S. based on your message, I thought we will have to support the unsigned longs (soon), nevertheless that we will not reach such values for a very long time, that is why I thought we are taking some action. But in that case, I guess it is ok to leave the current implementation as it will not have issues with the size method.

[jsync-swirlds]

I would have size() detect the overflow and return the negative (with appropriate documentation that this is an overflow) rather than having this check here that is more likely to become an error eventually.

It's not a critical issue, more a recommendation to put the edge case handling in the place that needs them rather than pushing them out to another element.

[ata-nas]

We could do that. Documenting that size could return Long.MIN_VALUE in the case where we have the range 0 -> Long.MAX_VALUE. Still torn which is the better option. We then rely on the caller to actually be aware of this.

[jsync-swirlds]

Note, size could return any negative value, we're basically acknowledging (in javadoc) that size is returning a long that should be treated as unsigned if necessary, rather than setting an early limit on the permitted ranges.

Either is acceptable; feel free to resolve this comment when you've decided which approach to proceed with.

[AlfredoG87]

Looks good to me, however @jsync-swirlds brings up some interesting arguments.

[codecov]

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ero/block/node/spi/historicalblocks/LongRange.java	84.61%	1 Missing and 1 partial ⚠️

@@             Coverage Diff              @@
##               main    #1982      +/-   ##
============================================
+ Coverage     78.99%   79.16%   +0.16%     
- Complexity     1241     1242       +1     
============================================
  Files           130      130              
  Lines          5952     5955       +3     
  Branches        646      646              
============================================
+ Hits           4702     4714      +12     
+ Misses          955      946       -9     
  Partials        295      295              

Files with missing lines	Coverage Δ	Complexity Δ
...ero/block/node/spi/historicalblocks/LongRange.java	92.00% <84.61%> (-8.00%)	23.00 <3.00> (-2.00)

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.