Conversation
2919ada to
05afd58
Compare
05afd58 to
e9d987e
Compare
Bumps the external ouroboros-network source-repository-package to the updated peras-staging/pr-5202, which incorporates the changes from: IntersectMBO/ouroboros-network#5202 In addition, it tweak call sites of `nodeToNodeProtocols` to match its updated signature, passing down the enabled feature flags.
Bumps the external ouroboros-network source-repository-package to the updated peras-staging/pr-5202, which incorporates the changes from: IntersectMBO/ouroboros-network#5202 In addition, it tweak call sites of `nodeToNodeProtocols` to match its updated signature, passing down the enabled feature flags.
0a82a45 to
8ef5288
Compare
fccf7c8 to
227f9d7
Compare
Bumps the external ouroboros-network source-repository-package to the updated peras-staging/pr-5202, which incorporates the changes from: IntersectMBO/ouroboros-network#5202 In addition, it tweak call sites of `nodeToNodeProtocols` to match its updated signature, passing down the enabled feature flags. Conflicts: cabal.project ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Network/NodeToNode.hs
|
Hi @coot, this PR should be ready, and we would need to depend on these changes for another PR against |
Bumps the external ouroboros-network source-repository-package to the updated peras-staging/pr-5202, which incorporates the changes from: IntersectMBO/ouroboros-network#5202 In addition, it tweak call sites of `nodeToNodeProtocols` to match its updated signature, passing down the enabled feature flags. Conflicts: cabal.project ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Network/NodeToNode.hs
Bumps the external ouroboros-network source-repository-package to the updated peras-staging/pr-5202, which incorporates the changes from: IntersectMBO/ouroboros-network#5202 In addition, it tweak call sites of `nodeToNodeProtocols` to match its updated signature, passing down the enabled feature flags. Conflicts: cabal.project ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Network/NodeToNode.hs
Bumps the external ouroboros-network source-repository-package to the updated peras-staging/pr-5202, which incorporates the changes from: IntersectMBO/ouroboros-network#5202 In addition, it tweak call sites of `nodeToNodeProtocols` to match its updated signature, passing down the enabled feature flags. Conflicts: cabal.project ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Network/NodeToNode.hs
…ObjectDiffusion) (#1679) This pull request introduces support for the ObjectDiffusion mini-protocol, required for Peras (for certificates and votes diffusion). It also plugs the PerasCertDiffusion (instance of ObjectDiffusion) mini-protocol in the networking layer. This PR depends on an updated version of `ouroboros-network`, see IntersectMBO/ouroboros-network#5202 and https://github.com/IntersectMBO/ouroboros-network/tree/peras-staging/pr-5202-v2 --- ### **Peras ObjectDiffusion Mini-protocol:** - Added modules `Ouroboros.Consensus.MiniProtocol.ObjectDiffusion{.Inbound,.Outbound}` with implementations of the ObjectDiffusion protocol (quite similar/inspired from TX-submission, except that client = inbound, server = outbound) - Added module `Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.ObjectPool.API` defining `ObjectPool{Reader,Writer}` interfaces, through which ObjectDiffusion accesses/stores the objects to send/that have been received. - Added modules `Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.PerasCert` and `Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.ObjectPool.PerasCert` containing definitions specific to `PerasCert` diffusion through the ObjectDiffusion mini-protocol - Modifies `Ouroboros.Consensus.Node.Serialisation` to add CBOR serialisation (`SerialiseNodeToNode`) for `Point blk`, `Tip blk`, and `PerasCert blk` - Modifies `Ouroboros.Consensus{.Node,.Node.Tracer,.Network.NodeToNode}` to wire-in PerasCertDiffusion similarly to other mini-protocols (e.g. TX-submission) --- ### **Tests and benchmarks** - Added module `Test.Consensus.MiniProtocol.ObjectDiffusion.Smoke` with smoke test for the general ObjectDiffusion mini-protocol (using mock objects) - Added module `Test.Consensus.MiniProtocol.ObjectDiffusion.PerasCert.Smoke` with smoke test specific to `PerasCert` diffusion through ObjectDiffusion - Updates `Test.ThreadNet.Network` in `unstable-diffusion-testlib` accordingly to the changes made in `Ouroboros.Consensus.Network.NodeToNode` --- **Network Protocol Version Updates:** - Depends on `ouroboros-network` rev https://github.com/IntersectMBO/ouroboros-network/tree/peras-staging/pr-5202-v2 through source-repository-package directive - Added support in `ouroboros-consensus` for `NodeToNodeV_16`
coot
left a comment
There was a problem hiding this comment.
It looks decent, but there are a few issues to be solved - especially around how Pares is negotiated in the handshake.
ouroboros-network/protocols/lib/Ouroboros/Network/Protocol/ObjectDiffusion/Type.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/protocols/lib/Ouroboros/Network/Protocol/ObjectDiffusion/Type.hs
Show resolved
Hide resolved
ouroboros-network/protocols/lib/Ouroboros/Network/Protocol/ObjectDiffusion/Type.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/protocols/lib/Ouroboros/Network/Protocol/ObjectDiffusion/Type.hs
Show resolved
Hide resolved
ouroboros-network/protocols/lib/Ouroboros/Network/Protocol/ObjectDiffusion/Codec.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/protocols/tests-lib/Ouroboros/Network/Protocol/ObjectDiffusion/Test.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/protocols/lib/Ouroboros/Network/Protocol/ObjectDiffusion/Codec.hs
Show resolved
Hide resolved
ouroboros-network/protocols/tests-lib/Ouroboros/Network/Protocol/ObjectDiffusion/Test.hs
Outdated
Show resolved
Hide resolved
cardano-diffusion/protocols/cddl/specs/handshake-node-to-node-v14.cddl
Outdated
Show resolved
Hide resolved
cardano-diffusion/api/lib/Cardano/Network/NodeToNode/Version.hs
Outdated
Show resolved
Hide resolved
…ObjectDiffusion) (#1679) This pull request introduces support for the ObjectDiffusion mini-protocol, required for Peras (for certificates and votes diffusion). It also plugs the PerasCertDiffusion (instance of ObjectDiffusion) mini-protocol in the networking layer. This PR depends on an updated version of `ouroboros-network`, see IntersectMBO/ouroboros-network#5202 and https://github.com/IntersectMBO/ouroboros-network/tree/peras-staging/pr-5202-v2 --- - Added modules `Ouroboros.Consensus.MiniProtocol.ObjectDiffusion{.Inbound,.Outbound}` with implementations of the ObjectDiffusion protocol (quite similar/inspired from TX-submission, except that client = inbound, server = outbound) - Added module `Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.ObjectPool.API` defining `ObjectPool{Reader,Writer}` interfaces, through which ObjectDiffusion accesses/stores the objects to send/that have been received. - Added modules `Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.PerasCert` and `Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.ObjectPool.PerasCert` containing definitions specific to `PerasCert` diffusion through the ObjectDiffusion mini-protocol - Modifies `Ouroboros.Consensus.Node.Serialisation` to add CBOR serialisation (`SerialiseNodeToNode`) for `Point blk`, `Tip blk`, and `PerasCert blk` - Modifies `Ouroboros.Consensus{.Node,.Node.Tracer,.Network.NodeToNode}` to wire-in PerasCertDiffusion similarly to other mini-protocols (e.g. TX-submission) --- - Added module `Test.Consensus.MiniProtocol.ObjectDiffusion.Smoke` with smoke test for the general ObjectDiffusion mini-protocol (using mock objects) - Added module `Test.Consensus.MiniProtocol.ObjectDiffusion.PerasCert.Smoke` with smoke test specific to `PerasCert` diffusion through ObjectDiffusion - Updates `Test.ThreadNet.Network` in `unstable-diffusion-testlib` accordingly to the changes made in `Ouroboros.Consensus.Network.NodeToNode` --- **Network Protocol Version Updates:** - Depends on `ouroboros-network` rev https://github.com/IntersectMBO/ouroboros-network/tree/peras-staging/pr-5202-v2 through source-repository-package directive - Added support in `ouroboros-consensus` for `NodeToNodeV_16`
3b6cecb to
a3b1243
Compare
|
Hey @coot, thanks for your review! I've been trying to modify the code so that nodes negociate the Peras enablement status using However, I'm wondering how to deal with this new extra field in
Thanks in advance for your help! |
|
Both should coexists. We must support multiple versions on mainnet simultanously (we cannot require all SPOs to update node all at the same time). Nodes negotiate the highest protocol version and the There's no need for new data constructor for |
tbagrel1
left a comment
There was a problem hiding this comment.
I've addressed a bunch of comments, I'll do the rest tomorrow
cardano-diffusion/api/lib/Cardano/Network/NodeToNode/Version.hs
Outdated
Show resolved
Hide resolved
cardano-diffusion/api/lib/Cardano/Network/NodeToNode/Version.hs
Outdated
Show resolved
Hide resolved
cardano-diffusion/api/tests/Test/Cardano/Network/NodeToNode/Version.hs
Outdated
Show resolved
Hide resolved
cardano-diffusion/api/lib/Cardano/Network/NodeToNode/Version.hs
Outdated
Show resolved
Hide resolved
|
I think I've addressed all the points you raised @coot :) |
ouroboros-network/protocols/tests-lib/Ouroboros/Network/Protocol/ObjectDiffusion/Direct.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/protocols/tests-lib/Ouroboros/Network/Protocol/ObjectDiffusion/Direct.hs
Outdated
Show resolved
Hide resolved
3595c9b to
ad4cbb8
Compare
|
Hi @coot, With ad4cbb8, I introduced and fixed the I believe this was the last item requested in the latest review round, but please let me know if I missed anything. Regarding comments We currently have a test client/server implementation in We now have Not sure if this fully answers your questions. In any case, we plan to join this week’s network meeting on Wednesday to discuss this live with you (if that works), so we can hopefully reach a mergeable state soon. If you agree, could you please send us an invite (cc @agustinmista @nbacquey )? Thanks for your help and feedback. |
cardano-diffusion/api/tests/Test/Cardano/Network/NodeToNode/Version.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/protocols/lib/Ouroboros/Network/Protocol/ObjectDiffusion/Inbound.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/protocols/lib/Ouroboros/Network/Protocol/ObjectDiffusion/Inbound.hs
Outdated
Show resolved
Hide resolved
|
f44f646 to
92452e1
Compare
Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io> Co-authored-by: Georgy Lukyanov <georgy.lukyanov@iohk.io> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io> Co-authored-by: Nicolas "Niols" Jeannerod <nicolas.jeannerod@moduscreate.com>
…`NodeToNodeVersionData` Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io> Co-authored-by: Georgy Lukyanov <georgy.lukyanov@iohk.io> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io> Co-authored-by: Nicolas "Niols" Jeannerod <nicolas.jeannerod@moduscreate.com>
Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io> Co-authored-by: Georgy Lukyanov <georgy.lukyanov@iohk.io> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io> Co-authored-by: Nicolas "Niols" Jeannerod <nicolas.jeannerod@moduscreate.com>
…bjectDiffusion mini-protocol Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io> Co-authored-by: Georgy Lukyanov <georgy.lukyanov@iohk.io> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io> Co-authored-by: Nicolas "Niols" Jeannerod <nicolas.jeannerod@moduscreate.com>
…sion miniprotocol in `NodeToNode.hs` Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io> Co-authored-by: Georgy Lukyanov <georgy.lukyanov@iohk.io> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io> Co-authored-by: Nicolas "Niols" Jeannerod <nicolas.jeannerod@moduscreate.com>
92452e1 to
bff39e5
Compare
Description
Add
NodeToNodeV_16for Peras, and depend onCardano.Base.FeatureFlagWe add a new (experimental)
NodeToNodeVersionfor Peras:NodeToNodeV_16.Additionally, we add a function
which additionally checks that the
PerasFlagis set, cf IntersectMBO/cardano-base#547. Eventually, once Peras is non-experimental, thePerasFlagwill be removed again.Peras enablement status is negociated through the field
perasSupportinNodeToNodeVersionData. ThedecoderforNodeToNodeVersionDatawhenversion < v16has been updated to setperasSupportto valuePerasUnsupported(since this field is not part of the CBOR representation for earlier versions).Add generic ObjectDiffusion mini-protocol
This protocol is going to be used for Peras vote and certificate diffusion. It is essentially a copy of TxSubmission, except that
MsgInitandMsgDoneare sent by the client/inbound side, as the flow of information is the reverse of that of TxSubmission.We also add codec roundtrip tests, CDDL tests, direct, connect, and channel-based tests.
MsgInit: currently pointless, but will get a payload indicating that we are only interested in objects after some indicator.The representation of object ids currently always is a list of object ids. We are potentially going to make this customizable to optimize for cases where a more succinct representation is possible (such as requesting "ranges" of ids).
We might make some changes to how IDs are requested, such as allowing the server to reply with
MsgAwaitReplywhen we do a blocking request, cf Make idleness explicit in ObjectDiffusion (V2) mini-protocol tweag/cardano-peras#144.Register Peras vote and certificate diffusion mini-protocols
In
Cardano.Network.NodeToNode, make the Network layer aware of Peras certificate and vote diffusion, gated behindperasSupportStatusfrom the negociatedNodeToNodeVersionData.Checklist
Quality
Maintenance
ouroboros-networkproject.