Word16 overflow in Jörmungandr binary (SlotId)

[KtorZ]

Release	Operating System	Cause
v2019-11-12	Windows & OSX & Linux)	Code

Context

https://github.com/input-output-hk/cardano-wallet/blob/master/lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs#L196

The actual slot number goes up to 32 bits!

Steps to Reproduce

1. It crashes if it encounters a value that is too big.

Expected behavior

Actual behavior

---

Resolution Plan

• Use a Word32 for SlotNo ==> Fix SlotNo Word16 Overflow #1026
• @jonathanknowles Define a smart-constructor with enforced invariants for EpochNo to make sure that it is never greater than 31-bit ==> TODO

PR

Number	Base
#1026	master

QA

Regression test added -> https://github.com/input-output-hk/cardano-wallet/blob/master/lib/jormungandr/test/unit/Cardano/Wallet/Jormungandr/BinarySpec.hs#L134

[piotr-iohk]

lgtm. Regression test added -> https://github.com/input-output-hk/cardano-wallet/blob/master/lib/jormungandr/test/unit/Cardano/Wallet/Jormungandr/BinarySpec.hs#L134

[Anviking]

#1048 (comment)

Ok, re-opening this then.

There was a remaining silent problem fixed by #1046.

But we should make sure to have regression tests and maybe make it fail harder / faster with an explicit invariant.

Long discussion at https://input-output-rnd.slack.com/archives/GBT05825V/p1574113682014100

[piotr-iohk]

@Anviking OK.
I thought we'll be tracking the above under another issue, but that's also fine. Putting to in progress then.

If I understand correctly we're missing regression test for #1046.

[KtorZ]

And the addition of a proper invariant in the code*

[jonathanknowles]

@KtorZ wrote:

Define a smart-constructor with enforced invariants for EpochNo to make sure that it is never greater than 31-bit ==> TODO

Perhaps one way to enforce this invariant might be to define EpochNo in terms of a Word31:

newtype EpochNo = EpochNo { unEpochNo :: Word31 }   
    deriving stock (Show, Read, Eq, Ord, Generic)   
    deriving newtype (Num, Enum)   

And then at the point where we decode epoch numbers from binary, fail loudly if we decode something that's wider than 31 bits.

I coded up an experimental branch here: https://github.com/input-output-hk/cardano-wallet/tree/jonathanknowles/experimental-word31-epochno

[KtorZ]

Sounds like a good idea. Would you mind making this a PR, I'll pick it up later today 🙏

[jonathanknowles]

@jonathanknowles wrote:

Perhaps one way to enforce this invariant might be to define EpochNo in terms of a Word31

@KtorZ responded:

Sounds like a good idea. Would you mind making this a PR, I'll pick it up later today pray

Sure, no problem.

Question: In terms of "failing loudly" at the point of decoding something that's wider than 31 bits, what would be your preferred mode of failure?

We could use error (along with a suitable message), but that would cause the wallet to crash. Perhaps there is a better way here.

[KtorZ]

Failing loudly is a synonym of "crashing" from my perspective. This is clearly something that should make the application inoperative and there's no way to recover from that.

In practice, it'd only kill the servant request thread and fail with a 500, which is, acceptable.

We don't need to be really fancy here but:

λ> toEnum 875 :: Word8
*** Exception: Enum.toEnum{Word8}: tag (875) is outside of bounds (0,255)

This is already a clear enough message for example.

[piotr-iohk]

ok.