-
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
Version codec #1981
Merged
Merged
Version codec #1981
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
All function take now handshake codec as an argument, this is a preparation for further changes.
coot
added
networking
node-to-client
Issues & PRs related to node-to-client protocols
node-to-node
Issues and PRs relate to node-to-node protocols
labels
Apr 21, 2020
coot
force-pushed
the
coot/protocol-versions
branch
from
April 21, 2020 13:11
80dd240
to
f13157d
Compare
This was referenced Apr 21, 2020
karknu
reviewed
Apr 22, 2020
karknu
approved these changes
Apr 22, 2020
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.
ouroboros-network-framework/src/Ouroboros/Network/Protocol/Handshake/Codec.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/protocol-tests/Ouroboros/Network/Protocol/Handshake/Test.hs
Outdated
Show resolved
Hide resolved
Decoding version numbers must be done in two steps to avoid failing if we receive a verions number that we don't know how to decode. For that reason we use 'CodecCBORTerm' record which encodes / decodes 'Codec.CBOR.Term.Term's. The requirement of 'Serialised' instance on version number is not yet removed, this will be done in next patch.
There are two changes in this patch: - Added argument to 'VersionMismatch' which includes tags of versions that we were unable to decode. Knowning only about versions we know is not that helpful. - Reimplement its codec using 'CodecCBORTerm' for encoding / decoding version numbers.
This is 'Hanshake' codec which which is using 'Unversioned' version numbers - used in tests.
test-cddl requires path to the `message.cddl` file, which is different for `nix` and `cabal`.
This allows to configure node to use a particular set of protocols, instead of just the compiled it ones.
This only seems unrelated to this patch set, I will use this test in the following patch.
Asymmetric in the sense that the client and server codecs are different. The server understands only a subset of versions that the client can propose.
coot
force-pushed
the
coot/protocol-versions
branch
from
April 22, 2020 10:51
f13157d
to
6401867
Compare
bors merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
node-to-client
Issues & PRs related to node-to-client protocols
node-to-node
Issues and PRs relate to node-to-node protocols
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes how Handshake codec works. Previously it would fail if
an unknown version number was submitted, now it will not.
This change should not change wire format of the handshake messages.
In addition it allows to confiugure which protocols versions are used by
consensus, there will be an accompanying PR in
cardano-node
repository whichadds an option to the confituration file.
I hope this fixes #1971, though I havent tested it (for that we need a new protocol
version).