IRC: "listunspent 0 9999999999 [stuff] fails because 9999999999 is too big."
Both json_spirit and univalue call .get_int() from listunspent(), and .get_int() method is defined to return 'int' in both cases.
It appears the out-of-range behavior is different for univalue, which is noticeable via RPC user interface.
How does json_spirit handle out of range integeres? I'd say there is only one sane way to handle it: reject the input?
9999999999 = 0x2 540b e3ff
Yeah, that screams "too big for 32 bit signed integer" to me.
static_cast will just truncate the number to 32 bit, NACK on that
We should however document the new behavior.
Agree with @laanwj. What about if UniValue would throw an exception in case of a detected out-of-range int?
@jonasschnelli That's what it does now, and is tested behavior. And the attached patch disables it.
@laanwj The patch precisely mimics json_spirit behavior, thus restoring the old RPC interface behavior.
The user (@gmaxwell) complained about the exception being thrown, as that causes a trickle down impact of making a previously-working RPC call now not work.
If we are to keep the new behavior, which is cleaner and more type specific, every .get_int() must be audited.
In theory the current get_int() call inside the RPC function is wrong, and an audit will fix that and any others.
The old situation was clearly wrong to me - large numbers should not be silently ignored, let alone silently ANDed with 0xffffffff.
This was a much larger risk than just rejecting the value. @gmaxwell was lucky that his arbitrary big value mapped to another large number in 32 bit integers. It could just as well have mapped to 1, or even -1 instead. The result would be much more "WTF" than a 'number too large' error.
The error signals a clear wrong input. Yes, this may mean people have to change cases where this happens in their interface, but this may help them isolate pesky hidden bugs.
The new behavior is more strict but not wrong in any way. Working around it by restoring the previous broken behavior is not acceptable to me. If this means auditing uses of get_int, that is what should be done (although as said, any 'failure scenario' here is preferable to silently chopping off numbers).
This issue is not resolved. Do not close until it is solved / release noted / audited.
I don't regard it as an issue.
You're providing a large number where a 32 bit integer is parsed. The only sane action is to reject the input.
Truncating was extremely ill-advised. This is an improvement not a reversion!
(pasting from IRC for the record) The issue is
I agree it is an improvement but we cannot simply assume it is a bug-free improvement.
Folks, this is an issue not a PR. There is nothing to NAK :)
This issue is open until the user visible breakage is fully analyzed and documented ,which I've assigned to myself. All .get_int() calls should be examined, not just this one.
The new behavior throws an exception, versus prior behavior of returning a range bound number and not throw an exception. This new behavior is more correct, but does produce user visible interface changes.
We have APIs where there is no way to provide an 'infinite' but where one is really expected. Examples are the depths that listunspent and list transaction go back. The positional arguments require you to provide something in these places, even when you really just want the default.
As far as I can tell, what just about everyone does is mashes the button to get a really big good-as-infinite value. So far so good. They bake these values into their command-lines and scripts etc. And the new behavior now fails with these values that worked before-- just because they hit 10 nines instead of 9.
For most of these things, I think saturating is appropriate. Yes-- it's not the most "type safe" behavior (though a saturating integer is a type that these could be defined as!), but JSON is not type safe. This was also the previously effective behavior.
Consequence of not doing something here is that the interfaces becomes harder to use-- because now you have the mental load of the largest permitted value. Consequence of saturating is that users who've become confused and sent in some totally crazy wrong field and up getting the "effectively infinite" default behavior instead of what they really wanted.
I think saturation would be good in this specific case, as it concerns a limit (and max is more-or-less infinite). It should definitely not be the default behavior of get_int. But adding a get_int_saturate (@gmaxwell's idea) and using it in listunspent would be a valid option.
To be clear, the previous behavior was not saturation:
10000000 → 10000000
100000000 → 100000000
1000000000 → 1000000000
10000000000 → 1410065408
100000000000 → 1215752192
1000000000000 → -727379968
10000000000000 → 1316134912
100000000000000 → 276447232
1000000000000000 → -1530494976
10000000000000000 → 1874919424
100000000000000000 → 1569325056
1000000000000000000 → -1486618624
10000000000000000000 → -1981284352
100000000000000000000 → 1661992960
The saturating parser would have to actually detect an overflow (or underflow) and return INT_MIN or INT_MAX respectively. I remember implementing some of this in ParseInt32 but not sure what's done with it... think it just returns a generic error
Sufficiently old and undesired - closing