Skip to content
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

Refactor network/consensus versioning #2358

Merged
merged 1 commit into from
Jul 3, 2020
Merged

Conversation

mrBliss
Copy link
Contributor

@mrBliss mrBliss commented Jul 3, 2020

Fixes #2309.

Previously, we had a list of supported BlockNodeToNodeVersions and a method
to translate them to a NodeToNodeVersion. This meant that there were two
layers of "support": (1) include the version in BlockNodeToNodeVersion
and (2) return a non-error for it in the translation function.

Replace these methods by a map from NodeToNodeVersion to
BlockNodeToNodeVersion. Similarly for NodeToClient. An important insight
is that not each network version requires a consensus-side block version. For
example, enabling the LocalStateQuery NodeToClient protocol requires a new
network version, but no block version, as the serialisation format doesn't
change. A new block version is only needed when the serialisation changes, so
not when protocols get added/removed or the protocol messages are
added/removed.

This means we don't need separate Byron (nor Cardano) versions for
NodeToClientV_2, as the serialisation format didn't change, so remove these
redundant versions and map NodeToClientV_2 to ByronNodeToClientVersion1.

The mostRecentSupportedNodeToNode method was only used for the ThreadNet
tests to make sure we were testing the most recent version. Remove this method
in favour of randomly picking a supported version in the tests. The
Test.ThreadNet.Util.NodeToNodeVersion was added to aid in picking a version.

  • Rewrite foldMapVersions and combineVersions to work for any Foldable
    instead of a NonEmpty, as we now use a Map instead of a NonEmpty for
    the supported versions.

  • Rewrite Cardano.Client.Subscription to take a CodecConfig +
    NetworkMagic instead of an entire TopLevelConfig. Producing a
    TopLevelConfig is a lot of work for clients. The whole point of
    introducing the CodecConfig was to make it easier for clients.

@mrBliss mrBliss added the consensus issues related to ouroboros-consensus label Jul 3, 2020
@mrBliss mrBliss requested review from coot and edsko July 3, 2020 12:37
@edsko
Copy link
Contributor

edsko commented Jul 3, 2020

Good to clarify this logic, much cleaner.

Fixes #2309.

Previously, we had a list of supported `BlockNodeToNodeVersion`s and a method
to translate them to a `NodeToNodeVersion`. This meant that there were two
layers of "support": (1) include the version in `BlockNodeToNodeVersion`
and (2) return a non-`error` for it in the translation function.

Replace these methods by a map from `NodeToNodeVersion` to
`BlockNodeToNodeVersion`. Similarly for `NodeToClient`. An important insight
is that not each network version requires a consensus-side block version. For
example, enabling the `LocalStateQuery` `NodeToClient` protocol requires a new
network version, but no block version, as the serialisation format doesn't
change. A new block version is only needed when the serialisation changes, so
not when protocols get added/removed or the protocol messages are
added/removed.

This means we don't need separate Byron (nor Cardano) versions for
`NodeToClientV_2`, as the serialisation format didn't change, so remove these
redundant versions and map `NodeToClientV_2` to `ByronNodeToClientVersion1`.

The `mostRecentSupportedNodeToNode` method was only used for the ThreadNet
tests to make sure we were testing the most recent version. Remove this method
in favour of randomly picking a supported version in the tests. The
`Test.ThreadNet.Util.NodeToNodeVersion` was added to aid in picking a version.

* Rewrite `foldMapVersions` and `combineVersions` to work for any `Foldable`
  instead of a `NonEmpty`, as we now use a `Map` instead of a `NonEmpty` for
  the supported versions.

* Rewrite `Cardano.Client.Subscription` to take a `CodecConfig` +
  `NetworkMagic` instead of an entire `TopLevelConfig`. Producing a
  `TopLevelConfig` is a lot of work for clients. The whole point of
  introducing the `CodecConfig` was to make it easier for clients.
@mrBliss
Copy link
Contributor Author

mrBliss commented Jul 3, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 3, 2020

@iohk-bors iohk-bors bot merged commit d237a78 into master Jul 3, 2020
@iohk-bors iohk-bors bot deleted the mrBliss/versioning branch July 3, 2020 15:21
@mrBliss mrBliss mentioned this pull request Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove supportedNodeToNodeVersions
2 participants