Implement real vs mock protocol selection #335

Jimbo4350 · 2019-11-26T15:26:04Z

Implements the NodeProtocolMode type which differentiates whether the node is running a real protocol vs a mock protocol.
Changes:

When running a real protocol, the topology comes from a JSON file which is decoded to RealNodeTopology which specifies the addresses the node intends to connect to.
socket-dir command line argument is changed to socket-path i.e you must specify socket paths and not dirs anymore.
NodeId only needs to be specified for mock protocols (https://github.com/input-output-hk/cardano-node/blob/d24cefd7a4fb1afbf13e8e7f7971e22704673f98/cardano-config/src/Cardano/Config/Protocol.hs#L129)

The cardano-node client is now as follows (with further help for both run and run-mock via cabal exec cardano-node run/run-mock -- --help):

Usage: cardano-node (run | run-mock) [--help]
  Start node of the Cardano blockchain.

Available options:
  --help                   Show this help text

Execute node with a real protocol.
  run                      Execute node with a real protocol.

Execute node with a mock protocol.
  run-mock                 Execute node with a mock protocol.

Relevant: #297, #318, #314

dcoutts

A few things I don't get yet:

the mock protocol mode seems to be being used in more places than I expected, especially scripts
how is the specific test/mock protocol selected? In the old CLI there was --bft --praos --mock-praos etc.

Do the scripts still work?

dcoutts · 2019-11-27T08:54:12Z

cardano-node/src/Cardano/Common/Parsers.hs

+nodeRealProtocolModeParser = subparser
+                           (  commandGroup "Execute node with a real protocol."
+                           <> metavar "real-protocol"
+                           <> command "real-protocol"


Is that the CLI name, like cardano-cli real-protocol --blah --blah? Using "mock-protocol" is fine for the mock one since it makes it clear it's different, but I don't think symmetry justifies making what is really the default one be a name that'll be surprising to users.

Perhaps we want "run" and "run-mock" or something?

For the node-cli the protocol is defined in the configuration .yaml file. The cardano-cli uses --real-pbft etc to specific which protocol it's using. I can change it to run and run-mock.

dcoutts · 2019-11-27T22:22:13Z

configuration/topology-proxy-follower.json

-        }
-      ]
+{
+  "rProducers": [


"rProducers"?

Meant to be short for real producers. Any naming suggestions?

Changed to "Producers"

dcoutts · 2019-11-27T22:24:42Z

scripts/shelley-testnet.sh

 tmux select-pane -t 3
-tmux send-keys "cd '${PWD}'; ${CMD} exe:cardano-node $(nodeargs 2 "${ALGO} $(echo -n ${EXTRA})") " C-m
+tmux send-keys "cd '${PWD}'; ${CMD} exe:cardano-node mock-protocol $(nodeargs 2 "${ALGO} $(echo -n ${EXTRA})") " C-m


Was it using one of the test protocols before? Which one? How was that specified before?

I made an error here, these are using log-config-*.yaml files which use RealPBFT. I associated mock with test, I'll fix it.

dcoutts · 2019-11-27T22:25:06Z

scripts/shelley-testnet-dns.sh

 #CMD="stack exec --nix cardano-node --"
-CMD="cabal new-run exe:cardano-node --"
+CMD="cabal new-run exe:cardano-node -- mock-protocol"


Is this for the testnet? Why is this a mock protocol?

Same as: #335 (comment)

dcoutts · 2019-11-27T22:25:38Z

benchmarking/chain-sync/benchmark-chain-sync-mainnet.sh

@@ -20,7 +20,7 @@ ulimit -t $CPU_TIME_LIMIT

 date --iso-8601=seconds > STARTTIME

-NODE="cabal new-run exe:cardano-node -- "
+NODE="cabal new-run exe:cardano-node -- mock-protocol "


I don't understand. This is for the mainnet.

Same as: #335 (comment)

Jimbo4350 · 2019-11-29T13:36:28Z

A few things I don't get yet:

the mock protocol mode seems to be being used in more places than I expected, especially scripts

how is the specific test/mock protocol selected? In the old CLI there was --bft --praos --mock-praos etc.

Do the scripts still work?

The protocols are currently specified in the config .yaml files. I've also updated the scripts to reflect that they are running real protocols 🙂.

dcoutts

So far looks fine.

I realise we don't want to load too much into each PR. But my main question is below, about when we plan to differentiate between the config content of the NodeMockCLI vs the NodeRealCLI to reflect what is actually used for each one.

dcoutts · 2019-12-02T16:20:07Z

cardano-config/src/Cardano/Config/Logging.hs

+    configYamlFp <- case nodeProtocolMode of
+                      RealProtocolMode (NodeRealCLI _ _ rConfigFp _) -> pure rConfigFp
+                      MockProtocolMode (NodeMockCLI _ _ mConfigFp _) -> pure mConfigFp


This could probably be a separate projection function, e.g. defined along with NodeProtocolMode.

I'll make the change.

dcoutts · 2019-12-02T16:22:30Z

cardano-config/src/Cardano/Config/Types.hs

+    , realTraceOpts :: !TraceOptions
+    }
+
+data NodeMockCLI = NodeMockCLI


Naming suggestion: perhaps the real ones should get the short names, and the mock ones a "mock" name prefix.

dcoutts · 2019-12-02T18:33:08Z

cardano-config/src/Cardano/Config/Types.hs

-  { unSocket :: FilePath }
-  deriving Show
+data SocketPath = SocketFile FilePath
+                | SocketDir FilePath


Do we have a TODO or ticket to remove the dir case again once we finish eliminating the node ids?

@dcoutts I was under the impression we still needed the NodeIds for the mock protocols and I believe DevOps finds it easier to work with socket dirs. @deepfire can you clarify re: socket dirs?

The mock/test protocols need the node ids for the consensus algorithms. I'm pretty sure devops have said they want simple "do exactly what I tell you" style with files, and not impose any naming convention.

I think @deepfire wanted the ability to specify a socket dir for scripts. If not I can remove it.

It can be removed -- but then all the users will have to be updated.

dcoutts · 2019-12-02T18:35:02Z

cardano-node/src/Cardano/Common/Parsers.hs

+              (GenesisFile genFp)
+              (DelegationCertFile <$> delCertFp)
+              (SigningKeyFile <$> sKeyFp)


Here's an example. I think none of the mock protocols use these 3 options.

Removed these three from the parser.

dcoutts · 2019-12-02T18:36:23Z

cardano-config/src/Cardano/Config/Types.hs

+data NodeRealCLI = NodeRealCLI
+    { realMscFp :: !MiscellaneousFilepaths
+    , realNodeAddr :: !NodeAddress
+    , realConfigFp :: !ConfigYamlFilePath
+    , realTraceOpts :: !TraceOptions
+    , validateDB :: !Bool
+    }


When are we planning to introduce differences between the options for the real and mock modes? Many of the options are specific to either real or mock.

For example the (ncProtocol nc) when we run the node is an example where for the real mode the choice is pre-determined, but for the mock more there is a choice of mock/test protocols. So the mock side would have a field for protocol choice that the real side does not have, since there is only one choice for the real side.

So the first differentiation would be removing NodeId from the configuration.yaml file and moving that into a command line argument for mock protocols.

Re: (ncProtocol nc): If I understand what you are saying, (ncProtocol nc) can give any of the protocols: https://github.com/input-output-hk/cardano-node/blob/52e6fb4befd5c89e7a3923166358f0f5a829c573/cardano-config/src/Cardano/Config/Types.hs#L407

Some need to be re-named however to reflect that they are actually mock protocols e.g BFT: https://github.com/input-output-hk/cardano-node/blob/04894d0082921ced7f478631add4191af3374af5/cardano-config/src/Cardano/Config/Protocol.hs#L120

I'm not sure if I've answered your concern.

Jimbo4350 · 2020-01-27T14:49:22Z

If approved (again) do not merge as I have to merge https://github.com/input-output-hk/iohk-nix/tree/jordan/update-mkEdgeTopology and then re-point sources.json to iohk-nix.

dcoutts

LGTM.

While running a real protocol, the node determines the peers it will connect to from a JSON configuration file.

…ocol

Fix `localSocketAddrInfo`

Update `nix/cardano-node-service.nix` Update `nix/scripts.nix` Update `nix/svclib.nix`

Jimbo4350 · 2020-02-03T17:34:39Z

bors r+

335: Implement real vs mock protocol selection r=Jimbo4350 a=Jimbo4350 Implements the `NodeProtocolMode` type which differentiates whether the node is running a real protocol vs a mock protocol. Changes: - When running a real protocol, the topology comes from a `JSON` file which is decoded to `RealNodeTopology` which specifies the addresses the node intends to connect to. - `socket-dir` command line argument is changed to `socket-path` i.e you must specify socket paths and not dirs anymore. - `NodeId` only needs to be specified for mock protocols (https://github.com/input-output-hk/cardano-node/blob/d24cefd7a4fb1afbf13e8e7f7971e22704673f98/cardano-config/src/Cardano/Config/Protocol.hs#L129) The `cardano-node` client is now as follows (with further help for both `run` and `run-mock` via `cabal exec cardano-node run/run-mock -- --help`): ``` Usage: cardano-node (run | run-mock) [--help] Start node of the Cardano blockchain. Available options: --help Show this help text Execute node with a real protocol. run Execute node with a real protocol. Execute node with a mock protocol. run-mock Execute node with a mock protocol. ``` Relevant: #297, #318, #314 Co-authored-by: Jordan Millar <jordan.millar@iohk.io>

iohk-bors · 2020-02-03T17:50:40Z

Build succeeded

mrBliss · 2020-02-11T17:13:28Z

I just noticed the changes to Cardano.Node.Run.handleSimpleNode in 59f66cc#diff-6f98f460a187cbfae2f479b840dc7ae4: please can we not copy-paste that much code? So much duplication now! I don't want to have to update everything twice. I see the code already diverging in master (only formatting though).

Jimbo4350 changed the title ~~Implement real vs mock protocol selection-~~ Implement real vs mock protocol selection Nov 26, 2019

Jimbo4350 marked this pull request as ready for review November 26, 2019 17:59

Jimbo4350 assigned deepfire Nov 26, 2019

Jimbo4350 force-pushed the jordan/mock-vs-real branch from 09fa19d to b1b2ea8 Compare November 26, 2019 18:05

Jimbo4350 unassigned deepfire Nov 26, 2019

Jimbo4350 requested review from deepfire and karknu November 26, 2019 18:05

Jimbo4350 force-pushed the jordan/mock-vs-real branch from b1b2ea8 to 3c883d7 Compare November 26, 2019 18:09

Jimbo4350 requested a review from CodiePP November 26, 2019 18:12

Jimbo4350 force-pushed the jordan/mock-vs-real branch 3 times, most recently from deabf69 to 91d099a Compare November 27, 2019 16:03

dcoutts reviewed Nov 27, 2019

View reviewed changes

Jimbo4350 force-pushed the jordan/mock-vs-real branch 2 times, most recently from 61bf2d7 to f02c173 Compare November 29, 2019 13:33

Jimbo4350 force-pushed the jordan/mock-vs-real branch 4 times, most recently from 8c7375b to b5bf8dd Compare December 2, 2019 16:25

dcoutts approved these changes Dec 2, 2019

View reviewed changes

Jimbo4350 force-pushed the jordan/mock-vs-real branch 9 times, most recently from ccb0657 to 8b0525d Compare December 6, 2019 18:07

Jimbo4350 force-pushed the jordan/mock-vs-real branch 5 times, most recently from 41baaf3 to bcecc7f Compare January 24, 2020 18:58

Jimbo4350 mentioned this pull request Jan 27, 2020

Simpler topology #318

Closed

1 task

Jimbo4350 force-pushed the jordan/mock-vs-real branch 2 times, most recently from bcf6ab8 to 8229d16 Compare January 27, 2020 14:45

dcoutts approved these changes Jan 29, 2020

View reviewed changes

Jimbo4350 added 8 commits February 3, 2020 09:06

Implement mock vs real protocol selection in cardano-node.hs

5794570

Propagate NodeProtocolMode

59f66cc

Implement SocketPath

b4bdd7f

Implement RealNodeTopology

e888b57

While running a real protocol, the node determines the peers it will connect to from a JSON configuration file.

Remove NodeId value from all config files using the RealPBFT prot…

d47c90f

…ocol

Update Scripts

f4cc305

Fix `localSocketAddrInfo`

Update SubmitTx to only require a socket path and tx file

98cbd46

Update setTopology

4f2a986

Jimbo4350 force-pushed the jordan/mock-vs-real branch 3 times, most recently from d2bb9f0 to 490d9ec Compare February 3, 2020 16:09

Update iohk-nix

caf342f

Update `nix/cardano-node-service.nix` Update `nix/scripts.nix` Update `nix/svclib.nix`

Jimbo4350 force-pushed the jordan/mock-vs-real branch from 490d9ec to caf342f Compare February 3, 2020 17:10

iohk-bors bot merged commit caf342f into master Feb 3, 2020

iohk-bors bot deleted the jordan/mock-vs-real branch February 3, 2020 17:50

Jimbo4350 mentioned this pull request Mar 4, 2020

Fix shelley-testnet.sh to put all state in a single directory #393

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement real vs mock protocol selection #335

Implement real vs mock protocol selection #335

Jimbo4350 commented Nov 26, 2019 •

edited

dcoutts left a comment

dcoutts Nov 27, 2019

Jimbo4350 Nov 28, 2019

dcoutts Nov 27, 2019

Jimbo4350 Nov 28, 2019

Jimbo4350 Nov 29, 2019

dcoutts Nov 27, 2019

Jimbo4350 Nov 28, 2019

dcoutts Nov 27, 2019

Jimbo4350 Nov 28, 2019

dcoutts Nov 27, 2019

Jimbo4350 Nov 28, 2019

Jimbo4350 commented Nov 29, 2019

dcoutts left a comment

dcoutts Dec 2, 2019

Jimbo4350 Dec 2, 2019

dcoutts Dec 2, 2019

dcoutts Dec 2, 2019

Jimbo4350 Dec 2, 2019

dcoutts Dec 2, 2019

Jimbo4350 Dec 2, 2019

deepfire Dec 3, 2019

dcoutts Dec 2, 2019

Jimbo4350 Dec 2, 2019

dcoutts Dec 2, 2019

Jimbo4350 Dec 2, 2019 •

edited

Jimbo4350 commented Jan 27, 2020

dcoutts left a comment

Jimbo4350 commented Feb 3, 2020

iohk-bors bot commented Feb 3, 2020

mrBliss commented Feb 11, 2020

Implement real vs mock protocol selection #335

Implement real vs mock protocol selection #335

Conversation

Jimbo4350 commented Nov 26, 2019 • edited

dcoutts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 commented Nov 29, 2019

dcoutts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 Dec 2, 2019 • edited

Choose a reason for hiding this comment

Jimbo4350 commented Jan 27, 2020

dcoutts left a comment

Choose a reason for hiding this comment

Jimbo4350 commented Feb 3, 2020

iohk-bors bot commented Feb 3, 2020

Build succeeded

mrBliss commented Feb 11, 2020

Jimbo4350 commented Nov 26, 2019 •

edited

Jimbo4350 Dec 2, 2019 •

edited