Jörmungandr: mkNetworkLayer with networkTip

[Anviking]

Issue Number

#219

Overview

• Implemented mkNetworkLayer with networkTip
• I have no tests

Comments

On ExceptT ErrNetworkUnreachable m (Maybe Block)

I found it particularly nightmarish to chain getTipId and getBlock.

Therefore I made all JormungandrLayer functions return ExceptT ErrNetworkUnreachable, favouring Right $ Nothing to indicate 404. f011447

• Implementing NetworkLayer becomes easier.
• There is no need for descriptive errors in JormungandrLayer. I don't see why we can't handle that in the NetworkLayer instead.

Alternative

• Closest I got was

        t@(BlockId hash) <- lift $ getTipId j
        let r = runExceptT $ getBlock j t
        case _runM r of -- need to deal with the `m` somehow
            Left (ErrGetBlockNetworkUnreachable msg) -> throwE $ ErrNetworkTipNetworkUnreachable msg
            Left e@(ErrGetBlockNotFound _) -> undefined throwM e
            Right b -> return (hash, header b)

Edit: the throwIO made it complicated, but I now realise that it actually was I who introduced the throwIO. The HttpBridge's throwIO (actually throwM) is for a different class of errors.

[Anviking]

nextBlocks would then be like

    , nextBlocks = \_slotId -> do
        let blockId = undefined
        let chunkSize = 1000
        ids <- getDescendantIds j blockId chunkSize >>= \case
            Just a -> return a
            Nothing -> liftIO . throwIO $ ErrGetBlockNotFound blockId
        forM ids getBlock'

    , postTx = undefined
    }
  where
    getBlock' :: BlockId -> ExceptT ErrNetworkUnreachable m Block
    getBlock' t = getBlock j t >>= \case
            Just b -> return b
            Nothing -> liftIO . throwIO $ ErrGetBlockNotFound t

[Anviking]

Could wrap this in some ErrInternalPanicOrSomething-wrapper… or handle the error entirely differently

[rvl]

I think adding another constructor to ErrNetworkTip would be fine.
networkTip is used to find out the tip slot id.

[rvl]

Good, looks like all that's left in this PR is handling Nothing from getBlock. 👍

[rvl]

I think adding another constructor to ErrNetworkTip would be fine.
networkTip is used to find out the tip slot id.

[rvl]

Yep, I think Hash "BlockHeader" is correct, and BlockId probably isn't needed - as either a newtype or type synonym.

[Anviking]

It doubles as an ApiT to avoid orphans so it is needed (in the Api type). I kept it in JormungandrLayer for now.

Also I introduced it because jormungandr/JormungandrLayer often refers to "id" in the method-names.

[rvl]

Nothing denotes that the node is not aware of a block with that hash. Maybe put that in a comment.

[KtorZ]

As discussed in another review, returning Maybe here isn't very ideal since in most (all?) cases, we are actually expecting a block when calling getBlock. Using Maybe, we are loosing information (hence the need for an extra comment). With a clear error, there's no need for a comment or any doubt about what the behavior of that function is.

[KtorZ]

@Anviking

I found it particularly nightmarish to chain getTipId and getBlock.

May I ask what is nightmarish here? In an Except e monad, you can most likely get away with withExceptT to map an error to some common ones and to be honest, I don't see how using Maybes makes it better 🤔 ?

[KtorZ]

@Anviking

I don't see why we can't handle that in the NetworkLayer instead.

That would be a much more logical choice in my opinion. We didn't bother with this in the bridge but, for Jormungandr, being more resilient to and more expressive about faults is a good idea. So instead of making Jormungandr internals less expressive, can't we make the network layer more expressive if that makes it easier?

[Anviking]

Yes, without throwIO we can use withExceptT.

        t@(BlockId hash) <- (getTipId j)
            `mappingError` ErrNetworkTipNetworkUnreachable
        b <- (getBlock j t)
            `mappingError` \case
            ErrGetBlockNotFound (BlockId h) ->
                ErrNetworkTipBlockNotFound h
            ErrGetBlockNetworkUnreachable e ->
                ErrNetworkTipNetworkUnreachable e
        return (hash, header b)
  where
    mappingError = flip withExceptT

I believe Maybe still has two small advantages:

• No need to map ErrGetBlockNetworkUnreachable e -> ErrNetworkTipNetworkUnreachable e
• Less verbose (don't have to pattern-match on ErrGetBlockNotFound, which is irrelevant information given the context)

[Anviking]

• I kept the errors in JormungandrLayer now
• Some small awkwardness caused from extending ErrNetworkTip but I think it should be fine, really

[Anviking]

Semi-awkward