Stricter jörmungandr binary encoders regarding integer overflow

[Anviking]

Issue Number

#219

Overview

• I have replaced a few fromIntegral with toEnum where it might overflow.
• I have adapted and slightly expanded the integration tests (separate commit)

Comments

• Not all failures are covered by integration tests. They are all covered by the roundtrips in Jörmungandr binary roundtrips #476 though.
• We could maybe introduce types prohibiting this, but 🤷‍♂ (that might be a rabbit hole and this is a first step in that case)

[KtorZ]

The original idea is good, but its implementation needs revisions:

It will be sufficient and give us the same level of guarantees as this PR to use toEnum instead of fromIntegral for unsafe conversions. Conversions is unsafe when the codomain is smaller than the domain.

[KtorZ]

Instead of magic number, it'd be better to use maxBound @Word8 to show where this 256 comes from.

[Anviking]

Done:

    unless (length inputs <= fromIntegral (maxBound :: Word8)) $
        fail "number of inputs cannot be greater than 255"

• maxBound :: Word8 is 255 so I've used 255] instead of 256) as bound
• Preserved the 255 in the message despite the risk of going out of sync
• Avoided to state the entire allowed range, e.g. number of inputs must be within the range [0,255], since I believe the 0 is a grey-zone.

[KtorZ]

We should either check the length with unless or with overflowSafeCoerce. Doing both is overkill.

[rvl]

Integer type promotions in the deserialisation (Get) shouldn't be too much of a problem. (Unless there are values which cause us to allocate an excessively large amount of memory).

But it's good to add checks in the serialisation (Put) before squeezing values into 8 bits - as you have done.

I am wondering what we should do about the error handling here. Should transactions with >= 255 inputs or outputs be rejected at the API level, or should we wait for serialisation to fail and return that error? I think the latter is OK, as long as it's clear to the user why the error happens.

[KtorZ]

I am wondering what we should do about the error handling here. Should transactions with >= 255 inputs or outputs be rejected at the API level, or should we wait for serialisation to fail and return that error? I think the latter is OK, as long as it's clear to the user why the error happens.

We will most likely never create a transaction with more than 255 inputs, because of the way our CS work. So it's unlikely that such transaction would make it to being serialized. Having said that, I'd be more inclined to catch the erroneous transaction much earlier and avoid creating it in a first place. Failures in the Binary module should really be our last-resort / safety net; being the sign that something went wrong in the upper layers.

[Anviking]

New approach:

• Default to using fromIntegral as before where we deem it safe.
• Only otherwise use toEnum