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.

Sign up for GitHub

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

Jump to bottom

* Update to the latest version of ouroboros-network #78

Merged
merged 1 commit into from
Apr 24, 2020
Merged

* Update to the latest version of ouroboros-network #78

merged 1 commit into from
Apr 24, 2020

Conversation

MarcFontaine
Copy link
Contributor

  • Update to the latest version of ouroboros-network
  • Use ready-made codecs

erikd
erikd requested changes Apr 22, 2020
View reviewed changes
Copy link
Contributor

@erikd erikd left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cabal.project file and the stack.yaml file must be in sync (and that's why CI failed).

The easiest way to do this is using a recent version of cardano-repo-tool.

@erikd erikd force-pushed the mafo/update-ouroboros-network branch from a391713 to 66c10ac Compare April 22, 2020 21:31
@erikd
Copy link
Contributor

erikd commented Apr 22, 2020

I ran cardano-repo-tool update-stack-yaml which updated the stack.yaml and then force pushed.

@erikd
Copy link
Contributor

erikd commented Apr 22, 2020

I will still want to build and run this to make sure it works as expected.

@erikd erikd force-pushed the mafo/update-ouroboros-network branch 2 times, most recently from 8935782 to a391713 Compare April 22, 2020 22:58
MarcFontaine
MarcFontaine commented Apr 23, 2020
View reviewed changes
cardano-db-sync/src/Cardano/DbSync.hs Outdated
(NodeToClientProtocols {
localChainSyncProtocol = localChainSyncProtocol
,localTxSubmissionProtocol = localTxSubmissionProtocol
,localStateQueryProtocol = Prelude.error "localTxSubmissionProtocol" -- todo!
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: use the ready-made null protocol

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a null peer now: https://github.com/input-output-hk/ouroboros-network/blob/master/ouroboros-network/src/Ouroboros/Network/NodeToClient.hs#L730

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is used now

@MarcFontaine
Copy link
Contributor Author

OK stack.yaml is updated.
There is still one TODO, then this PR is ready for a review

@MarcFontaine MarcFontaine marked this pull request as ready for review April 23, 2020 15:14
@MarcFontaine MarcFontaine requested review from coot and erikd April 23, 2020 15:15
@erikd
Copy link
Contributor

erikd commented Apr 23, 2020 •
edited

This looks fine. Let me build and run it and make sure it all works!

erikd
erikd requested changes Apr 24, 2020
View reviewed changes
Copy link
Contributor

@erikd erikd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just built and ran this and got:

[db-sync-node.Handshake:Info:36] [2020-04-24 00:12:09.66 UTC]
    {"event":"Send MsgProposeVersions (fromList [(NodeToClientV_1,TInt 1097911063)])"
    ,"kind":"LocalHandshakeTrace"
    ,"bearer":"ConnectionId {localAddress = LocalAddress {getFilePath = \"\"}
    , remoteAddress = LocalAddress {getFilePath = \"state-node-testnet/node.socket\"}}"
    }

I really did expect NodeToClientV_2 there?

@coot
Copy link
Contributor

coot commented Apr 24, 2020 •
edited

@erikd just to double check, the node is after #809 was merged, right? (i.e. after this commit IntersectMBO/cardano-node@0172a96)

coot
coot requested changes Apr 24, 2020
View reviewed changes
cardano-db-sync/src/Cardano/DbSync.hs Outdated
@@ -206,7 +204,7 @@ runDbSyncNodeNodeClient iomgr trce plugin nodeConfig (SocketPath socketPath) = d
logInfo trce $ "localInitiatorNetworkApplication: connecting to node via " <> textShow socketPath
networkState <- newNetworkMutableState
txv <- newEmptyTMVarM @_ @(GenTx blk)
ncSubscriptionWorker_V1
ncSubscriptionWorker_V2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarcFontaine could we use this pattern: https://github.com/input-output-hk/cardano-node/blob/master/cardano-cli/src/Cardano/CLI/Ops.hs#L436 and nsSubscriptionWorker; I'd like to deprecate ncSubscriptionWorker_V1 and ncSubscriptionWorker_V2 and remove them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know when you fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had looked it at when you suggested it on Slack, but had not been sure
if it is just a syntactic changes or if it breaks anything.
But I changed it now.

Copy link
Contributor

@coot coot Apr 24, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not breaking anything, and it's good to use the same pattern across whole ecosystem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

foldMapVersions is like foldMap but for Semigroup rather than for Monoid (and specialised for Versions rather than for any Semigroup, just to make the api clearer). It folds the results with (<>) (one possible implementation is foldMapVersions = \f -> foldr1 (<>) . fmap f. ncSubscriptionWorker_V2 combines two versions explicitly with (<>) - so there's no difference.

@erikd
Copy link
Contributor

erikd commented Apr 24, 2020 •
edited

@coot I am building/running db-sync from the mafo/update-ouroboros-network branch at commit e2cf2ec which is why I assume this should be running V2.

@coot
Copy link
Contributor

coot commented Apr 24, 2020

You're right, it is not even proposing NodeToClientV_2, which I strange; maybe the change I suggested in review will fix it.

@erikd
Copy link
Contributor

erikd commented Apr 24, 2020 •
edited

Nevermind, I just updated my node to current HEAD (commit 376562b0cb6d) and now db-sync says:

[db-sync-node.Handshake:Info:38] [2020-04-24 05:16:42.57 UTC]
  [String "Send MsgProposeVersions (fromList [(NodeToClientV_1,TInt 764824073
  (NodeToClientV_2,TInt 764824073)])",String "LocalHandshakeTrace"
  ,String "ConnectionId {localAddress = LocalAddress {getFilePath = \"\"}
   , remoteAddress = LocalAddress {getFilePath = \"state-node-mainnet/node.socket\"}}"]

so it supports both versions and then

[db-sync-node.Handshake:Info:38] [2020-04-24 05:16:42.57 UTC]
  [String "Recv MsgAcceptVersion NodeToClientV_2 (TInt 764824073)"
  ,String "LocalHandshakeTrace"
  ,String "ConnectionId {localAddress = LocalAddress {getFilePath = \"\"}
  , remoteAddress = LocalAddress {getFilePath = \"state-node-mainnet/node.socket\"}}"
  ]

suggesting it is running V2.

@erikd
Copy link
Contributor

erikd commented Apr 24, 2020

🤞

erikd
erikd approved these changes Apr 24, 2020
View reviewed changes
Copy link
Contributor

@erikd erikd left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

coot
coot requested changes Apr 24, 2020
View reviewed changes
cardano-db-sync/src/Cardano/DbSync.hs Show resolved Hide resolved
cardano-db-sync/src/Cardano/DbSync.hs Outdated Show resolved Hide resolved
coot
coot reviewed Apr 24, 2020
View reviewed changes
cardano-db-sync/src/Cardano/DbSync.hs
(NodeToClientVersionData { networkMagic = nodeNetworkMagic (Proxy @blk) topLevelConfig })
(dbSyncProtocols v trce plugin topLevelConfig txv)
)
(supportedNodeToClientVersions (Proxy @blk))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erikd is cardano-db-sync using the same config file as cardano-node?
If it is, once IntersectMBO/cardano-node#830 is merged we can use the specified versions rather than supportedNodeToClientVersions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, the configs are different.

coot
coot approved these changes Apr 24, 2020
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks good now, thanks @MarcFontaine

@MarcFontaine
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Apr 24, 2020
@iohk-bors @MarcFontaine
Merge #78
94b2df2 
78: * Update to the latest version of ouroboros-network r=MarcFontaine a=MarcFontaine

* Update to the latest version of ouroboros-network
* Use ready-made codecs

Co-authored-by: MarcFontaine <MarcFontaine@users.noreply.github.com>
@MarcFontaine MarcFontaine force-pushed the mafo/update-ouroboros-network branch 3 times, most recently from 9b7f9c2 to 71397d4 Compare April 24, 2020 09:30
@MarcFontaine
* Update cabal.project and stack.yaml to the latest version od ourobo…
f37fbea 
…ros-network

* Use ready-made codecs
* Replace ncSubsciptionWorker_V1 with foldMapVersions pattern
* Replace hard-coded protocol version with correct parameter
@MarcFontaine
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 24, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit bd9a0a9 into master Apr 24, 2020
@iohk-bors iohk-bors bot deleted the mafo/update-ouroboros-network branch April 24, 2020 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikd erikd erikd approved these changes

@coot coot coot approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@MarcFontaine @erikd @coot