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

NodeId should not be part of the configuration file. #314

Closed
karknu opened this issue Nov 20, 2019 · 4 comments
Closed

NodeId should not be part of the configuration file. #314

karknu opened this issue Nov 20, 2019 · 4 comments
Labels
priority medium issues/PRs that SHOULD be addressed. This should be done for the release, but acceptable if it doesn
Milestone
S6 2020-02-13

Comments

@karknu
Copy link
Contributor

karknu commented Nov 20, 2019

The NodeId should be a command line argument like port number.
With the current scheme one is forced to have one node configuration file per node in a topology file.
@deepfire
Copy link
Contributor

@karknu, I think the node Id should be inferred from the PBFT leader credentials, not specified at all.

@edsko
Copy link
Contributor

edsko commented Nov 20, 2019

PBFT indeed doesn't need it anymore (IntersectMBO/ouroboros-network#1192).

@dcoutts
Copy link
Contributor

dcoutts commented Nov 20, 2019

If we need anything like this at all, it's only to allow sharing a network configuration description between multiple nodes. It has no connection whatsoever to any notion of node id from consensus (since there is no such notion).

@deepfire deepfire self-assigned this Nov 28, 2019
@deepfire deepfire added this to the Sprint 1.1.0 milestone Nov 28, 2019
@deepfire
Copy link
Contributor

The topology part of de-NodeId-ification is being handled in #318

@deepfire deepfire removed their assignment Nov 28, 2019
@Jimbo4350 Jimbo4350 mentioned this issue Dec 27, 2019
Merged
@vhulchenko-iohk vhulchenko-iohk modified the milestones: S3 2020-01-02, S4 2020-01-16 Jan 6, 2020
@Jimbo4350 Jimbo4350 added the priority medium issues/PRs that SHOULD be addressed. This should be done for the release, but acceptable if it doesn label Jan 14, 2020
@Jimbo4350 Jimbo4350 modified the milestones: S4 2020-01-16, S6 2020-02-13, S5 2020-01-30 Jan 16, 2020
@Jimbo4350 Jimbo4350 mentioned this issue Jan 27, 2020
Merged
iohk-bors bot added a commit to input-output-hk/iohk-nix that referenced this issue Feb 3, 2020
@iohk-bors @Jimbo4350
Merge #293
40fc58f 
293: Remove `NodeId` from topology r=Jimbo4350 a=Jimbo4350

Proposed topology file change: IntersectMBO/cardano-node#314

From:
```
[
  {
     "nodeId":0,
     "nodeAddress":{
        "addr":"127.0.0.1",
        "port":7776
     },
     "producers":[
        {
           "addr":"3.125.75.199",
           "port":3001,
           "valency":1
        },
        {
           "addr":"18.177.103.105",
           "port":3001,
           "valency":1
        },
        {
           "addr":"18.141.0.112",
           "port":3001,
           "valency":1
        },
        {
           "addr":"52.14.58.121",
           "port":3001,
           "valency":1
        }
     ]
  }
]
```
To:
```
{
   "Producers": [
     {
       "addr": "3.125.75.199",
       "port": 3001,
       "valency": 1
     },
     {
       "addr": "18.177.103.105",
       "port": 3001,
       "valency": 1
     },
     {
       "addr": "18.141.0.112",
       "port": 3001,
       "valency": 1
     },
     {
       "addr": "52.14.58.121",
       "port": 3001,
       "valency": 1
     }
   ]
 }
```



Co-authored-by: Jordan Millar <[email protected]>
iohk-bors bot added a commit that referenced this issue Feb 3, 2020
@iohk-bors @Jimbo4350
Merge #335
448369f 
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
priority medium issues/PRs that SHOULD be addressed. This should be done for the release, but acceptable if it doesn
Projects
None yet
Milestone
S6 2020-02-13
Development

No branches or pull requests
6 participants
@deepfire @edsko @dcoutts @karknu @Jimbo4350 @vhulchenko-iohk