Export static codecs #1894

MarcFontaine · 2020-04-02T15:39:54Z

Export a record with all NodeToClient codecs,
These codecs (currently) do not depend on ToplevelConfig.
This PR makes these codecs usable in the wallet.

mrBliss

I don't think this is the right approach, see my inline comments.

ouroboros-consensus/src/Ouroboros/Consensus/NodeNetwork.hs

mrBliss · 2020-04-03T06:55:43Z

ouroboros-consensus/src/Ouroboros/Consensus/NodeNetwork.hs

-          nodeDecodeQuery
-          nodeEncodeResult
-          nodeDecodeResult
+    , pcLocalStateQueryCodec      = staticLocalStateQueryCodec
    }



staticBlockFetchCodec is missing. Ah, I see that's because it requires the TopLevelConfig. This makes me think this is not such a good idea after all. Now you're just lucky that the codecs you need don't depend on the TopLevelConfig and aren't versioned. What if they become versioned in the future? Or what if any of the node(Encode|Decode)X methods suddenly start requiring the TopLevelConfig? Then they can no longer remain "static".

Yes, the idea is to replace the locally defined codecs in the Wallet
and cardano-rest and other places with these static-Versions of the codecs.
If in the future one of codecs becomes configurable,
the codec has to be removed from the exports.
This will break the libraries that uses this codec and the problem will be noticed.

@mrBliss let's keep this part as simple as possible. The requirement here is to make it as easy as possible to run and maintain client applications (good example is cardano-cli get-tip, it just needs to know the block type). Since node-to-client protocols don't depend on TopLevelConfig let's not include it. Do you see some dependency in forcible future? Will the LocalStateQuery or LocalTxMonitor protocols depend on parts of TopLevelConfig? If so I think it's good to start with no dependency, and then add only the necessary dependencies. This will make us think how to properly do it when we will need it.

@MarcFontaine these codes should be versioned by NodeToClientVersion. There's only one version now, but still it's better to put that already in place. I guess, they no longer become static.

I guess it's fine for now. But as soon as they do start depending on the TopLevelConfig, I don't want to be blocked by these static* definitions.

I actually want us to be blocked by them - this will give us moment to think how to make it as simple as possible for the clients as well.

mrBliss · 2020-04-06T11:04:59Z

ouroboros-consensus/src/Ouroboros/Consensus/NodeNetwork.hs

+staticBlockFetchCodecSerialised
+    = codecBlockFetchSerialised
+        (nodeEncodeHeaderHash (Proxy @blk))
+        (nodeDecodeHeaderHash (Proxy @blk))


BTW, the style is:

Suggested change

staticBlockFetchCodecSerialised

= codecBlockFetchSerialised

(nodeEncodeHeaderHash (Proxy @blk))

(nodeDecodeHeaderHash (Proxy @blk))

staticBlockFetchCodecSerialised =

codecBlockFetchSerialised

(nodeEncodeHeaderHash (Proxy @blk))

(nodeDecodeHeaderHash (Proxy @blk))

mrBliss · 2020-04-06T11:06:57Z

ouroboros-consensus/src/Ouroboros/Consensus/NodeNetwork.hs

-          nodeDecodeQuery
-          nodeEncodeResult
-          nodeDecodeResult
+    , pcLocalStateQueryCodec      = staticLocalStateQueryCodec
    }



I guess it's fine for now. But as soon as they do start depending on the TopLevelConfig, I don't want to be blocked by these static* definitions.

coot · 2020-04-06T11:36:59Z

@MarcFontaine I think it's worth putting all the client codecs in a recrod. Check this out how a client is instantiated: https://github.com/input-output-hk/cardano-node/blob/master/cardano-node/src/Cardano/CLI/Ops.hs#L406 it needs all the codecs, even if one of the mini-protocols is not sending anything.

Could you go around various clients and check that we are right that there are clients that do not need TopLevelConfig?

cardano-wallet
cardano-rest
clients in cardano-node

MarcFontaine · 2020-04-06T12:47:45Z

The wallet uses:

codec :: Codec protocol DeserialiseFailure m ByteString
codec = codecChainSync
        encodeByronBlock
        (unwrapCBORinCBOR $ decodeByronBlock (toEpochSlots getEpochLength))
        (encodePoint encodeByronHeaderHash)
        (decodePoint decodeByronHeaderHash)
        (encodeTip encodeByronHeaderHash)
        (decodeTip decodeByronHeaderHash)

TopLevelConfig is not available in the wallet

MarcFontaine · 2020-04-06T12:51:16Z

cardano-rest uses:

localTxSubmissionCodec
  :: forall m blk . (RunNode blk, MonadST m)
  => Codec (LocalTxSubmission (GenTx blk) (ApplyTxErr blk)) DeserialiseFailure m LBS.ByteString
localTxSubmissionCodec =
  codecLocalTxSubmission
    nodeEncodeGenTx
    nodeDecodeGenTx
    (nodeEncodeApplyTxError (Proxy @blk))
    (nodeDecodeApplyTxError (Proxy @blk))
```
But `TopLevelConfig` is available.

coot · 2020-04-06T12:53:17Z

ok, good enough evindence for me we should avoid TopLevelConfig

mrBliss · 2020-04-06T13:36:55Z

ouroboros-consensus/src/Ouroboros/Consensus/NodeNetwork.hs

+    nodeToClientChainSyncCodec       :: Codec (ChainSync (Serialised blk) (Tip blk))
+                                         failure m bytesLCS
+  , nodeToClientTxSubmissionCodec    :: Codec (LocalTxSubmission (GenTx blk) (ApplyTxErr blk))
+                                         failure m bytesLTX
+  , nodeToClientStateQueryCodec      :: Codec (LocalStateQuery blk (Query blk))
+                                         failure m bytesLSQ


Are these :: aligned with those in ProtocolCodecs? That's not necessary, just align these three with each other, like stylish-haskell does.

coot · 2020-04-06T13:37:01Z

ouroboros-consensus/src/Ouroboros/Consensus/NodeNetwork.hs

+  , nodeToClientTxSubmissionCodec = staticLocalTxSubmissionCodec
+  , nodeToClientStateQueryCodec   = staticLocalStateQueryCodec
+  }
+
 staticBlockFetchCodecSerialised


Please hide them in a where clause, then you can rename them as nodeToClientCHainSyncCodec, ... and take advantage of NamedFieldPuns extension:

nodeToClientCodecs NodeToClientV_1 = NodeToClientCodecs { nodeToClientChainSyncCodec , nodeToClientTxSubmissionCodec , nodeToClientStateQueryCodec } where nodeToClientChainSyncCodec = ... nodeToClientTxSubmissionCodec = ... nodeToClientStateQueryCodec = ...

At the moment the same codecs are exported via two different ways via ProtocolCodecs and via NodeToClientCodecs.
Maybe the nodeToClient codecs should be removed from ProtocolCodecs altogther ?
staticLocalChainSyncCodec is the single definition that is uses for both exports.

I see; you should have access to NodeToClientV_1 there too, no? Then you can unpack the record and add the codes. This way in the future one will only need to modify nodeToClientCodecs to update codecs in consensus.

These codecs (currently) do not depend on ToplevelConfig.

MarcFontaine · 2020-04-07T11:56:40Z

bors r+

iohk-bors · 2020-04-07T12:14:43Z

Build succeeded

mrBliss suggested changes Apr 3, 2020

View reviewed changes

mrBliss reviewed Apr 6, 2020

View reviewed changes

MarcFontaine force-pushed the mafo/static-codecs branch from b343d99 to 9653e01 Compare April 6, 2020 13:32

mrBliss reviewed Apr 6, 2020

View reviewed changes

coot reviewed Apr 6, 2020

View reviewed changes

MarcFontaine force-pushed the mafo/static-codecs branch 2 times, most recently from 6c64362 to 92fd70f Compare April 7, 2020 10:40

mrBliss approved these changes Apr 7, 2020

View reviewed changes

MarcFontaine force-pushed the mafo/static-codecs branch from 92fd70f to a9290cc Compare April 7, 2020 11:28

Export a recorde with all node-to-client codecs.

9dcad93

These codecs (currently) do not depend on ToplevelConfig.

MarcFontaine force-pushed the mafo/static-codecs branch from a9290cc to 9dcad93 Compare April 7, 2020 11:44

iohk-bors bot merged commit d8cf08c into master Apr 7, 2020

iohk-bors bot deleted the mafo/static-codecs branch April 7, 2020 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export static codecs #1894

Export static codecs #1894

MarcFontaine commented Apr 2, 2020 •

edited

Loading

mrBliss left a comment

mrBliss Apr 3, 2020

MarcFontaine Apr 3, 2020

coot Apr 6, 2020

mrBliss Apr 6, 2020

coot Apr 6, 2020

mrBliss Apr 6, 2020

mrBliss Apr 6, 2020

coot commented Apr 6, 2020

MarcFontaine commented Apr 6, 2020

MarcFontaine commented Apr 6, 2020

coot commented Apr 6, 2020

mrBliss Apr 6, 2020

coot Apr 6, 2020 •

edited

Loading

MarcFontaine Apr 6, 2020

coot Apr 6, 2020

MarcFontaine Apr 7, 2020

MarcFontaine commented Apr 7, 2020

iohk-bors bot commented Apr 7, 2020

Export static codecs #1894

Export static codecs #1894

Conversation

MarcFontaine commented Apr 2, 2020 • edited Loading

mrBliss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coot commented Apr 6, 2020

MarcFontaine commented Apr 6, 2020

MarcFontaine commented Apr 6, 2020

coot commented Apr 6, 2020

Choose a reason for hiding this comment

coot Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcFontaine commented Apr 7, 2020

iohk-bors bot commented Apr 7, 2020

Build succeeded

MarcFontaine commented Apr 2, 2020 •

edited

Loading

coot Apr 6, 2020 •

edited

Loading