-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
coot/cardano ping #3204
coot/cardano ping #3204
Conversation
coot
commented
Jun 10, 2021
- Add support for V7 keepalive
- cardano-ping: adhare to keep alive protocol
network-mux/demo/cardano-ping.hs
Outdated
keepAliveDone :: ByteString | ||
keepAliveDone = | ||
CBOR.toLazyByteString $ | ||
CBOR.encodeListLen 1 | ||
<> CBOR.encodeWord 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should cover the pre v7 version of the protocol too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the shortcoming of the approach of integration of version and version data: we need to pattern match on the version number: when we will add v8
for node-to-node
the cardano-ping
will break if one forgets to update the encoder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed one more commit to fix this.
* send terminating message (`MsgDone`), * add 5 sec protocol idle timeout, this allows for a clean connection termination.
Instead of pattern matching on a version use the `Ord` instance, though it is a bit artificial to use `NodeToNodeVersionV7 minBound minBound`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
bors merge |
Build succeeded: |