Introduce flag to enable sentry nodes to participate in grandpa gossip #3018

mxinden · 2019-07-04T13:33:00Z

Given the following situation: A validator 'A' is not supposed to be
connected to the public internet to protect it from e.g. a DoS attack.
Instead it connects to a sentry node 'sentry-A' which connects to the
public internet. Validator 'B' can reach validator 'A' via sentry node
'sentry-A' and vice versa.

A sentry node needs to participate in the grandpa gossip without
becoming a validator itself in order to forward these message to its
validator. This commit adds a new command line flag (--grandpa-voter)
forcing a node to participate in the grandpa voting process even though
no --key was specified. Due to the fact that it does not have a key,
it does not become a validator in the network.

In order to simulate the above situation this commit also adds a Docker
Compose file (scripts/sentry-node/docker-compose.yml) with further
documentation.

Parent Github issue: #2999

parity-cla-bot · 2019-07-04T13:33:03Z

It looks like @mxinden hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

parity-cla-bot · 2019-07-04T13:33:03Z

It looks like @mxinden hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

mxinden · 2019-07-04T13:43:24Z

[clabot:check]

parity-cla-bot · 2019-07-04T13:43:27Z

It looks like @mxinden signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

parity-cla-bot · 2019-07-04T13:43:27Z

It looks like @mxinden signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Given the following situation: A validator 'A' is not supposed to be connected to the public internet to protect it from e.g. a DoS attack. Instead it connects to a sentry node 'sentry-A' which connects to the public internet. Validator 'B' can reach validator 'A' via sentry node 'sentry-A' and vice versa. A sentry node needs to participate in the grandpa gossip without becoming a validator itself in order to forward these message to its validator. This commit adds a new command line flag (`--grandpa-voter`) forcing a node to participate in the grandpa voting process even though no `--key` was specified. Due to the fact that it does not have a key, it does not become a validator in the network. In order to simulate the above situation this commit also adds a Docker Compose file (`scripts/sentry-node/docker-compose.yml`) with further documentation.

andresilva

Maybe we should instead add a --sentry flag and make sure local_key is None even if the user provides a --key argument. In the future we might have some more changes for sentry mode so that's why I'm suggesting adding a sentry flag now.

mxinden · 2019-07-05T14:36:59Z

Maybe we should instead add a --sentry flag

@rphmeier was voting against calling the flag --sentry as users might want to run a grandpa voter outside of the context of a sentry node. While maybe impacting sentry user experience it does make the binary more versatile.

@andresilva What do you think?

rphmeier · 2019-07-05T15:36:58Z

@mxinden we can always add a --sentry flag which implies a number of other options later. But it's nice to have the flag for running a keyless GRANDPA voter.

mxinden · 2019-07-05T16:07:49Z

@mxinden we can always add a --sentry flag which implies a number of other options later. But it's nice to have the flag for running a keyless GRANDPA voter.

Sounds good to me @rphmeier.

@andresilva and @rphmeier any further comments other than the flag renaming?

rphmeier · 2019-07-08T14:51:21Z

@mxinden is it still in-progress or ready for review?

mxinden · 2019-07-08T15:30:41Z

@rphmeier ready for review.

rphmeier · 2019-07-08T16:10:13Z

(then you should put the pleasereview label on, otherwise people will skip it :) )

andresilva

lgtm, just a minor grumble in the docker compose file (I'm not sure how useful it is to be committed but won't grumble on that).

andresilva · 2019-07-08T16:33:22Z

scripts/sentry-node/docker-compose.yml

+      # - "--validator"
+      - "--name"
+      - "CharliesNode"
+      - "--bootnodes"


Maybe use --reserved-nodes here instead? Same for the other nodes configs.

Would you mind adding some more details @andresilva?

If I understand correctly, validator addresses can change at runtime. Thus if the address of validator-b changes, sentry-a would need to be restarted. I would suggest only interconnecting validator-a and sentry-a as reserved nodes and sentry-a and validator-b as bootstrap nodes, given that a and b are controlled by different entities.

What do you think?

On a side note, I am currently looking into how a user can tell a validator node to keep x amount of connections open to other validator nodes. As the validator address set can change at runtime, these long-lived connections would also be adjusted along the way.

In this config A connects to B (through bootnodes). The sentry connects to A and B, and B also tries to connect with A.

My understanding is that in a real scenario the validator nodes that are using a sentry will only be connected to the sentry (or multiple sentries). So I think in this config Validator-A should set Sentry-A as a reserved node (so that it only connects to this node), and the rest of the nodes (Sentry-A and Validator-B) can add everyone as bootnode (even though Validator-B won't be able to connect to Validator-A).

mxinden · 2019-07-09T08:17:38Z

Guessing from the contributing guidelines I am the one squashing and merging this pull request.

Generally PRs should sit with this label for 48 hours in order to garner feedback.

I will wait for a little longer and then merge if their are no objections.

bkchr · 2019-07-09T08:20:21Z

Just merge it :)

mxinden · 2019-07-09T08:45:19Z

(I'm not sure how useful it is to be committed but won't grumble on that)

@andresilva I am also fine with removing it. No strong opinion. Just thought it might be helpful documenting the use case as code.

mxinden added A3-in_progress Pull request is in progress. No review needed at this stage. J0-enhancement An additional feature request. labels Jul 4, 2019

mxinden requested a review from rphmeier July 4, 2019 13:33

mxinden force-pushed the sentry branch from 531f659 to 850455f Compare July 4, 2019 13:58

andresilva reviewed Jul 5, 2019

View reviewed changes

mxinden marked this pull request as ready for review July 8, 2019 08:31

mxinden changed the title ~~[WIP] Introduce flag to enable sentry nodes to participate in grandpa gossip~~ Introduce flag to enable sentry nodes to participate in grandpa gossip Jul 8, 2019

rphmeier added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 8, 2019

rphmeier approved these changes Jul 8, 2019

View reviewed changes

andresilva reviewed Jul 8, 2019

View reviewed changes

Merge branch 'master' into sentry

d1e4706

gavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. J0-enhancement An additional feature request. labels Jul 9, 2019

gavofyork merged commit cf5b132 into paritytech:master Jul 9, 2019

mxinden mentioned this pull request Jul 10, 2019

scripts/sentry-node: Ensure release mode and reserved connection for A #3077

Merged

ansonla3 mentioned this pull request Aug 28, 2019

Secure validator setup tutorial w3f/polkadot-wiki-old#254

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce flag to enable sentry nodes to participate in grandpa gossip #3018

Introduce flag to enable sentry nodes to participate in grandpa gossip #3018

mxinden commented Jul 4, 2019 •

edited

Loading

parity-cla-bot commented Jul 4, 2019

parity-cla-bot commented Jul 4, 2019

mxinden commented Jul 4, 2019

parity-cla-bot commented Jul 4, 2019

parity-cla-bot commented Jul 4, 2019

andresilva left a comment

mxinden commented Jul 5, 2019

rphmeier commented Jul 5, 2019

mxinden commented Jul 5, 2019

rphmeier commented Jul 8, 2019

mxinden commented Jul 8, 2019

rphmeier commented Jul 8, 2019

andresilva left a comment

andresilva Jul 8, 2019

mxinden Jul 9, 2019

andresilva Jul 9, 2019

mxinden commented Jul 9, 2019

bkchr commented Jul 9, 2019

mxinden commented Jul 9, 2019

Introduce flag to enable sentry nodes to participate in grandpa gossip #3018

Introduce flag to enable sentry nodes to participate in grandpa gossip #3018

Conversation

mxinden commented Jul 4, 2019 • edited Loading

parity-cla-bot commented Jul 4, 2019

parity-cla-bot commented Jul 4, 2019

mxinden commented Jul 4, 2019

parity-cla-bot commented Jul 4, 2019

parity-cla-bot commented Jul 4, 2019

andresilva left a comment

Choose a reason for hiding this comment

mxinden commented Jul 5, 2019

rphmeier commented Jul 5, 2019

mxinden commented Jul 5, 2019

rphmeier commented Jul 8, 2019

mxinden commented Jul 8, 2019

rphmeier commented Jul 8, 2019

andresilva left a comment

Choose a reason for hiding this comment

andresilva Jul 8, 2019

Choose a reason for hiding this comment

mxinden Jul 9, 2019

Choose a reason for hiding this comment

andresilva Jul 9, 2019

Choose a reason for hiding this comment

mxinden commented Jul 9, 2019

bkchr commented Jul 9, 2019

mxinden commented Jul 9, 2019

mxinden commented Jul 4, 2019 •

edited

Loading