ethresearch / sharding-p2p-poc

Proof of Concept of Ethereum Serenity Peer-to-Peer Layer on libp2p PubSub System
40 stars 19 forks source link

New RPCs: `bootstrap`, `connectshardpeer`, `identify` #108

Closed mhchia closed 5 years ago

mhchia commented 5 years ago

What is wrong?

mhchia commented 5 years ago

identify.peerID is included in https://github.com/ethresearch/sharding-p2p-poc/issues/108

NIC619 commented 5 years ago

Regarding connectshardpeer, do we need a full control over how topology is constructed? Perhaps to test some edge cases? cc @mhchia @zscole If not, what about instead connecting to randomly selected k shard peers when we subscribe to a new shard? Or randomly selected k shard peers by default while adding a -dontAddPeer flag to instruct the node to not add peers when subscribing to a new shard?

NIC619 commented 5 years ago

@mhchia @araskachoi Requesting input on what should a node do when it subscribes to a new shard.

Before #101 , when the node subscribe to a new shard it will connect to all the peers from that shard, see: https://github.com/ethresearch/sharding-p2p-poc/blob/c1aff06a207636b7af6159858a50f464672f1de7/shardmanager.go#L151

After #101 , connectShardNodes was removed which means when the node subscribe to a new shard it does not automatically connect to any peers from that shard. This has the advantage that we can construct the topology of the shard network at will. I suppose this helps testing as we can simulate the topology of some edge cases.

Though I was also thinking about adding connectShardNodes back but instead of connecting to all the peers from that shard it connects to randomly chosen k peers(value of k is left for further discussion) from that shard. But I understand that this can be easily simulated in test script so I don't hold a strong opinion about this. Just want to get some feedback.

NIC619 commented 5 years ago

@mhchia @araskachoi Just letting you know that I'm going with:

  1. adding connectShardNodes back but instead of connecting to all the peers from that shard it connects to randomly chosen k peers
  2. add a -nodiscover option to disable discovery and use manual peer addition
  3. add a discover shard_id RPC call which returns peers discovered from shard_id shard
araskachoi commented 5 years ago
  1. need to be wary of how randomization is implemented here.
  2. that seems useful but might also introduce a vulnerability as bad actors can collude and start their own shard and broadcast bad information
  3. no plans on peering with those peers discovered from that shard_id?
NIC619 commented 5 years ago

that seems useful but might also introduce a vulnerability as bad actors can collude and start their own shard and broadcast bad information no plans on peering with those peers discovered from that shard_id?

So -nodiscover and discover shard_id are only for testing purpose(in case we want to construct the topology in our favor by manually adding peers from the shard). Normal user would just subscribe to a new shard and randomly connect to k peers.

Regarding randomization, it will not be like how we generate our node's private key. We need determinism in generating our node's private key because it makes testing easier(i.e., we can be sure that feeding with seed 1 would generate the key pair that's encoded into PID XtW5fX for example.)

For the randomization used in connectShardNodes, the entropy is probably fetched from the machine's local environment etc. instead of being fed by us.

mhchia commented 5 years ago

Fixed by https://github.com/ethresearch/sharding-p2p-poc/pull/131