Add labels to binary decoders for improved debugging

[Anviking]

Overview

• I have added a few labels

Comments

Previously:

Right (Just *** Exception: Data.Binary.Get.runGet at position 84: isolate: the decoder consumed 82 bytes which is less than the expected 178 bytes
CallStack (from HasCallStack):
  error, called at libraries/binary/src/Data/Binary/Get.hs:351:5 in binary-0.8.6.0:Data.Binary.Get
λ> runExceptT $ getBlock j t

Now:

Right *** Exception: Data.Binary.Get.runGet at position 84: isolate: the decoder consumed 82 bytes which is less than the expected 178 bytes
getBlockHeader
getBlock
CallStack (from HasCallStack):
  error, called at libraries/binary/src/Data/Binary/Get.hs:351:5 in binary-0.8.6.0:Data.Binary.Get

[jonathanknowles]

The individual code changes look fine to me!

However, I can't help wondering if there isn't an approach we could use that doesn't involve repeating the name of each function (which is a kind of boilerplate).

For example, would it be enough to add HasCallStack constraints to each of the functions we care about, instead of adding labels to them?

[jonathanknowles]

Just nitpicking, but shouldn't this line (and the lines following it) be further indented by one block? Like this:

getBlockHeader = label "getBlockHeader" $
    (fromIntegral <$> getWord16be) >>= \s -> isolate s $ do
        version <- getWord16be
        contentSize <- getWord32be
        slotEpoch <- fromIntegral <$> getWord32be
        slotId <- fromIntegral <$> getWord32be
        ...

[Anviking]

Should it?

I view this as

function = auxiliarySetup $ do
   important
   things

[jonathanknowles]

I would argue that this function is slightly different from the others though, as the first line is wrapped, which introduces an extra level of indentation.

Clearly, both of the variants below are valid Haskell syntax:

Variant 1:

getBlockHeader = label "getBlockHeader" $
    (fromIntegral <$> getWord16be) >>= \s -> isolate s $ do
    version <- getWord16be
    contentSize <- getWord32be
    slotEpoch <- fromIntegral <$> getWord32be
    slotId <- fromIntegral <$> getWord32be

Variant 2:

getBlockHeader = label "getBlockHeader" $
    (fromIntegral <$> getWord16be) >>= \s -> isolate s $ do
        version <- getWord16be
        contentSize <- getWord32be
        slotEpoch <- fromIntegral <$> getWord32be
        slotId <- fromIntegral <$> getWord32be

In my opinion, the extra indentation of variant 2 makes the code more readable, as it gives a visual indication of exactly where the do block starts, i.e., that it is separate from the line starting (fromIntegral <$> getWord16be) >>= \s -> isolate s.

What do you think?

[Anviking]

Yes — I suppose 😄

I guess the extra indentation shattered my ability to ignore the (fromIntegral [...]) line, but that might have been 1) just because I was used to it, and 2) bad because it's obfuscating.

So I think this makes perfect sense. Thanks! 👍

[Anviking]

HasCallStack

Thanks for the suggestion, @jonathanknowles!

SrcLoc for free seems neat. I wonder if won't require some trickery though, and I wonder if it's worth looking into now.

I guessed we would have

label' :: HasCallStack => Get a -> Get a
label' = Data.Binary.Get.label (someVariantOf prettyCallStack callStack)

getFoo :: HasCallStack => Get Foo
getFoo = label' $ do
    ...

or am I missing some obvious use of it?

And I kind of like the simplicity of label "getFoo" and that it signals clearly what it does.

Of course, it's possible to provide a misleading label…

[Anviking]

Reverted to do-notation for consistency with the rest of the module, to show variable names, and separate order of the binary format with the order of the LinearFee arguments.

[jonathanknowles]

separate order of the binary format with the order of the LinearFee arguments.

Fair enough!

[akegalj]

This looks good to me.

As of discussion about making this more automatic with HasCallStack and similar I would propose to add it to wiki Blackboard.

This already improves debugging information and we can make it more elegant if needed later