Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[DO NOT MERGE] Command peer connections from runtime #6863

Closed
wants to merge 53 commits into from
Closed

[DO NOT MERGE] Command peer connections from runtime #6863

wants to merge 53 commits into from

Conversation

kaichaosun
Copy link
Contributor

@kaichaosun kaichaosun commented Aug 10, 2020

Related paritytech/polkadot-sdk#358.

This PR is in early stage and unclean, I'd like take some feedback before move too far in the wrong direction.

The intention is: a chain can

  • include the node-permission pallet,
  • configure genesis permissioned nodes with chain_spec,
  • then start a permissioned node with cli param permissioned-network.

TODOs:

  • more dispatchable calls for remove & set nodes
  • Configurable node identifier in runtime and decodable in network layer.
  • struct Peer in client/network/test/src/lib.rs need Backend trait bound for NetworkWorker, LightBackend and Backend both used and stuck at abstraction. (help needed)
  • runtime module tests
  • remove useless logs
  • better error
  • decide if node wants to contain this module or not since there is another cli flagpermissioned_network

Thanks for the mentorship from @tomaka.


Edited:
The benchmark weight will be put in another PR if make sense.

@kaichaosun kaichaosun added the A3-in_progress Pull request is in progress. No review needed at this stage. label Aug 10, 2020
@kaichaosun kaichaosun requested a review from tomaka August 10, 2020 03:18
client/service/src/lib.rs Outdated Show resolved Hide resolved
client/network/src/config.rs Outdated Show resolved Hide resolved
client/network/src/error.rs Outdated Show resolved Hide resolved
client/network/src/service.rs Outdated Show resolved Hide resolved
client/network/src/chain.rs Outdated Show resolved Hide resolved
client/peerset/src/lib.rs Outdated Show resolved Hide resolved
client/service/src/config.rs Outdated Show resolved Hide resolved
@kaichaosun kaichaosun marked this pull request as draft August 13, 2020 00:59
@kaichaosun
Copy link
Contributor Author

Hi, @bkchr @shawntabrizi do you guys mind giving another review? I'd like take the feedback early and hopefully it can be merged soon and catch the release train. Also Ping @gavofyork here in case you are interested and missed the request for review.

Copy link
Contributor

@riusricardo riusricardo left a comment

Choose a reason for hiding this comment

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

Would be good to add this pallet to the node client.

frame/node-authorization/src/lib.rs Outdated Show resolved Hide resolved
frame/node-authorization/src/lib.rs Outdated Show resolved Hide resolved
@kaichaosun
Copy link
Contributor Author

Would be good to add this pallet to the node client.

@riusricardo From my understanding, node client also serves for a public testnet, node-authorization pallet is not meant for the public network, a tutorial to walk through the steps to add such functionality makes more sense IMO.

@mattdean-digicatapult
Copy link

This looks really cool! I'd just started looking at doing something similar myself when @gautamdhameja pointed me to this.

I'd be really interested in hearing why the approach taken was to use a well-known storage item? When I first saw paritytech/polkadot-sdk#358 I felt that option was really restrictive in terms of network topologies that can be created. It would be really nice if this was abstracted out such that the runtime could decide how the peer list should be determined.

I guess I'm kind of concerned, especially given where the changes are needed, that this will make it difficult to expand upon this functionality in future.

@kaichaosun
Copy link
Contributor Author

why the approach taken was to use a well-known storage item

@mattdean-digicatapult From my understanding to the issue and requirements of a permissioned network, the list of peers should reach consensus within participants, which means the list should be stored on-chain. I found two ways for this purpose:

  • runtime api to get a regular storage
  • well known storage, it's also mentioned in the issue itself, the API is pretty clean and straightforward comparing with runtime api.

It would be really nice if this was abstracted out such that the runtime could decide how the peer list should be determined.

A runtime can always has its own logic for this well known storage. It's now assuming to use governance system to determine the peer list.
I see there is possibility that maintain a more complex network with multiple groups / channels existed on the same underlying chain, but that's probably much more changes and out of my current knowledge. I'm also interested in your scenarios of the peer list determination.

@mattdean-digicatapult
Copy link

From my understanding to the issue and requirements of a permissioned network, the list of peers should reach consensus within participants, which means the list should be stored on-chain. I found two ways for this purpose:

  • runtime api to get a regular storage
  • well known storage, it's also mentioned in the issue itself, the API is pretty clean and straightforward comparing with runtime api.

Yes my first thought would be a runtime api with regular storage. This means a runtime could implement it's own interpretation of how the network should be laid out. My instinct btw was to have an api that took the current node's identity and returned the identities of nodes that it can connect to.

I see there is possibility that maintain a more complex network with multiple groups / channels existed on the same underlying chain, but that's probably much more changes and out of my current knowledge. I'm also interested in your scenarios of the peer list determination.

When I was thinking about this @kaichaosun I was thinking of a single multi-organisation permissioned chain. In a setup like this you need to decide who can add or remove nodes from the peer list, how and what peers should connect to what other peers. That may depend on the role of the node; so for a simple example

  1. Nodes which will be validators all need to be able to talk to one another. New validator nodes would connect accross organisations and therefore consensus across organisations would be required to add new nodes
  2. Nodes that are used as application api endpoints for a single organisation. From a networking security model you might not want your validators to behave like an application api service. Instead an organisation might want to deploy additional nodes to serve this purpose for an application. In this case it makes sense that an organisation can unilaterally add a new node but that it can only connect to other nodes in that organisation (including validators withing that organisation).

You also have more complex examples with sub-organisations etc. I've seen these sorts of models come up in other private chain solutions as well.

@kaichaosun
Copy link
Contributor Author

kaichaosun commented Aug 21, 2020

In this case it makes sense that an organisation can unilaterally add a new node but that it can only connect to other nodes in that organisation (including validators withing that organisation).

It's now quite blurred to me which part should be on-chain and which part off-chain. There is an alternative to use CLI to command a node with flag --reserved-only and --reserved-nodes (it now can't use together with this new pallet though, need to fix it once core devs has a more clear vision on priority groups).

My instinct btw was to have an api that took the current node's identity and returned the identities of nodes that it can connect to.

It makes sense to me, but before implementing this runtime api, I'd like we think about the second option listed in the issue, since it seems aimed for more adaptive scenarios, probably includes yours.

This node-authorization pallet and related functionality is an early effort to explore on how a permissioned network in Substrate should be formed. Hopefully it can evolve with new user cases and visions.

@mattdean-digicatapult
Copy link

It's now quite blurred to me which part should be on-chain and which part off-chain. There is an alternative to use CLI to command a node with flag --reserved-only and --reserved-nodes (it now can't use together with this new pallet though, need to fix it once core devs has a more clear vision on priority groups).

I think to a point having these things on-chain is a convenience thing. You could synchronise this information, either within an organisation or with the wider network, via a side channel and just use the cli args. Fundementally if someone wants to break the rules on sharing data from the private network they can do this unilaterally anyway so there's not a whole lot of protection against bad actors anyway. Using a side channel would be error prone however and given we have a bft data sharing mechanism and the data volumes are v. low it makes sense to me to use it imho.

It makes sense to me, but before implementing this runtime api, I'd like we think about the second option listed in the issue, since it seems aimed for more adaptive scenarios, probably includes yours.

The second option is very interesting and definitely would provide for the sort of scenarios I mentioned. The main benefit I think over the runtime api approach is that it allows the setting of the network conditions to be governed by the runtime with very little involvement from the node. This looks a lot more complex to me though to implement and the node could still relatively easily change the rules for itself by modifying the client.

My concern with this PR however is that any approach beyond this may be excluded meerly by the presense of the NODE_ALLOW_LIST well known key mechanism. The way peersets are managed is already complicated enough without introducing another mechanism (via a runtime api or an OCW mechanism) where the maintainers then have to consider how it interacts with existing mechanisms. I'm also not sure how you would upgrade an existing chain to a different mechanism but maybe that's simpler than I imagine. Hope that makes sense

@kaichaosun
Copy link
Contributor Author

Please do not merge this PR but happy to take more feedback, I am also working on using offchain worker to set reserved nodes.

@kaichaosun kaichaosun changed the title Command peer connections from runtime [DO NOT MERGE] Command peer connections from runtime Aug 26, 2020
@g2udevelopment
Copy link
Contributor

Please do not merge this PR but happy to take more feedback, I am also working on using offchain worker to set reserved nodes.

I think this approach has the most potential. Because you can still decide in a pallet to store the list of nodes and use one of the democracy pallets to manage this list (or using a sudo). Really interested in this PR because it will really pave the way forward for permissioned chains. Let me know if you need some help.

@kaichaosun
Copy link
Contributor Author

Close it since we have an better solution #6996.

@kaichaosun kaichaosun closed this Sep 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants