Unsafe Integers about sia HOT 11 CLOSED

Since we're using unsigned ints everywhere, overflow check should be as simple as:

func (c Currency) Add(d Currency) (sum Currency, err error) {
    sum := c + d
    if sum < c || sum < d {
        err = errors.New("integer overflow")
    }
    return
}

Makes arithmetic a bit more verbose though. And this approach won't work for multiplication.

from sia.

I don't think that we use multiplication anywhere. Being more verbose might be worthwhile.

In fact..., we might want to switch to

type Currency struct {
    amount uint64
}

so that the compiler can't understand what currencyA + currencyB means. All it takes is for someone to use a + b instead of a.Add(b) and we're back to unsafe integers.

I know a lot of the typing is a PITA but an overflow error is very significant. So are other typing mistakes.

from sia.

agreed. It wouldn't even hurt the literals much, you'd just be writing Currency{10000} instead of Currency(10000).

from sia.

lol Bitcoin got burned by this one, to the tune of 18.4 quintillion bitcoins: https://bitcointalk.org/index.php?topic=823.0
looks like they fixed it with a manual overflow check. It'd be pretty easy for us to do the same, but it's probably for the best that overflow checks are implemented everywhere.

On that note, is it necessary to check for overflows on types other than Currency? Is there, e.g., a similar exploit for timestamps?

from sia.

I don't think so. It's only a problem if you are doing math on the type.

Oh wait, we do math on the timestamps when calculating the difficulty, but we only calculate the difficulty if the timestamp is valid, which means that it's less than 2 hours into the future according to network time (btw, for us during beta, network time will be equal to local time (IE whatever your hw clock says)). So we're fine here, but maybe we should restrict that type too...

😣

At some point we should scroll through everything and see how much of it can be simplified. It's a lot. For example, I very commonly use s.BlockMap[s.ConsensusState.CurrentBlock].Height, which could be reduced to s.ConsensusState.height().

Actually, a good time to do this is when we write the specification.

from sia.

While we're at it, we should make the currency type 128 bits. That level of resolution is necessary when you can pay for things by-the-byte

from sia.

This is the next thing that needs to happen. When we're done with our current PR's we should attack this.

from sia.

I'm starting to implement 128 bit integers. Currently they look like this:

// An int128 is a 128-bit signed integer. Operations on int128s are performed
// via math/big.
//
// The int128 also keeps track of whether an overflow has occurred during
// arithmetic operations. Once the 'overflow' flag has been set to true, it
// can never be reset; a new int128 must be created. Callers can check for
// overflow using the Overflow method. This allows arithmetic operations to be
// chained together without needing to check an error value after each
// operation.
type int128 struct {
    b  [16]byte
    of bool // has an overflow ever occurred?
}

I like the use of an overflow flag instead, since you only have to check for overflow once. As an optimization, we could even make Add, Sub, etc. no-ops if an overflow has previously occurred. There is one potential downside, which is that you have to explicitly check for an overflow rather than having an err shoved in your face.

Another problem is with the type system. Since Currency is now defined as a int128, you don't get the Add, Sub, etc. methods for free. You'd have to define Currency as a struct with an embedded int128. This goes for any other types that will be using int128 as well. This would be a non-issue if Currency is the only 128-bit type, though.

Finally, there's the issue of pointers. The math/big library works almost exclusively with pointers. We could do the same, but it complicates things somewhat; do we define Currency as a *int128, or do we just use *Currency?

It would also be nice to know exactly where we should be checking for overflows. We check when verifying transactions, but what about when we're creating a block? Can we put it off until we call AcceptBlock? The overflow flag will be preserved up until the block gets encoded, so we can put it off for a while.

from sia.

Currency is the only 128bit integer that we use, so you can just call it a Currency.

I don't like using an overflow bool. Because that means you have to explicitly check, which means you might forget to check. And might forget is an unfortunate circumstance.

I know it means a lot more error checking lines, but I think it's the safer and smarter option.

from sia.

Another approach would be to just use big.Int everywhere to do math, and then have a BigToCurrency function that converts to the Currency type and returns an error on overflow. That might make more sense than writing a bunch of pass-through functions anyway. It also simplifies a lot of the pointer stuff.

No way am I making functions like Add return an error though. I absolutely believe it's avoidable.

from sia.

fixed by #314

from sia.