NebulousLabs / Sia

Blockchain-based marketplace for file storage. Project has moved to GitLab: https://gitlab.com/NebulousLabs/Sia
https://sia.tech
MIT License
2.71k stars 440 forks source link

Fix -1 Integer Parsing #3097

Closed eddiewang closed 6 years ago

eddiewang commented 6 years ago

Sia-UI is currently not able to fetch the transactions from siad. It uses the following api call: /wallet/transactions?startheight=0&endheight=-1

When strconv.ParseUint attempts to parse -1, an error is returned to Sia-UI, rather then the intended behaviour, which is to return all wallet transactions from height 0 to max height.

This fix uses the ParseInt method instead. When the -1 is casted to the BlockHeight type, we get an arbitrarily large number 18446744073709551615.

Another way we could fix this bug is to call the current block height from consensus, and set that to the end variable, if the argument sent back is less than 0.

ChrisSchinnerl commented 6 years ago

This is probably the easiest solution and works for any negative number. The question is if that behavior is expected by the user. e.g. when the current blockheight is 100 and I call Transactions with 105, would I expect an error?

DavidVorick commented 6 years ago

This feels like a hack. I would rather have an explicit check for a negative number.

DavidVorick commented 6 years ago

Does the api documentation explicitly state that we can input a negative number to fetch all transactions? I guess we'll have to add that to the documentation if not to preserve compatibility with any other apps that are using this strategy.

lukechampine commented 6 years ago

Agreed, I think -1 is intended as a sentinel rather than implying support for arbitrary negative numbers.

My preference would be for Sia-UI to request some large number like 1000000000 instead of -1. On the other hand, we want to avoid breaking compatibility if possible, so we should probably continue supporting -1, if not all negative numbers. Perhaps we could explicitly convert negative numbers to math.MaxUint64? Technically this changes the behavior for very large negative numbers, but I highly doubt anyone is passing those.

lukechampine commented 6 years ago

I don't think the API docs say anything about negative numbers, so this is a case of Hyrum's law:

With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody.

In my view, Sia-UI was exploiting a behavior of the siad API that wasn't guaranteed to work; it just assumed that -1 would underflow to MaxUint64. So I don't think we have an obligation to preserve that functionality. We could be nice about it and preserve -1 as a sentinel value; I'd be content with that. IIRC bitcoind requires you to use a large number and that's kind of annoying.

DavidVorick commented 6 years ago

Let's explicitly convert negative numbers to MaxUint64 then.

MSevey commented 6 years ago

resolves #3096

Spartan-II-117 commented 6 years ago

I don't know i this is before or after this change code wise, but when i run siac wallet transactions, I get the following: Could not fetch transaction history: parsing integer value for parameter endheight failed: strconv.ParseInt: parsing "18446744073709551615": value out of range s siac version: Sia Client Version 1.3.3 Git Revision 94ea84e Build Time Tue Jun 12 13:54:39 EDT 2018 Sia Daemon Version 1.3.3 Git Revision 94ea84e Build Time Tue Jun 12 13:54:39 EDT 2018

eddiewang commented 6 years ago

@Spartan-II-117 bug fixed in PR #3102