Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
57febc4
add offchain worker api to set reserved nodes.
kaichaosun Aug 26, 2020
3dc7a45
new offchain api to get node public key.
kaichaosun Aug 26, 2020
3664999
node public key from converter
kaichaosun Aug 26, 2020
6b43f9e
refactor set reserved nodes ocw api.
kaichaosun Aug 26, 2020
4d17e4c
new ndoe authorization pallet
kaichaosun Aug 26, 2020
46b4078
remove unnecessary clone and more.
kaichaosun Aug 28, 2020
70e616e
more
kaichaosun Aug 30, 2020
613a84d
tests for node authorization pallet
kaichaosun Aug 31, 2020
27152b9
remove dependency
kaichaosun Aug 31, 2020
acbc8d6
fix build
kaichaosun Aug 31, 2020
d4e5ae8
more tests.
kaichaosun Sep 1, 2020
b62beea
Merge branch 'master' of https://github.com/paritytech/substrate into…
kaichaosun Sep 1, 2020
7e2a026
refactor
kaichaosun Sep 1, 2020
03ab075
Update primitives/core/src/offchain/testing.rs
kaichaosun Sep 1, 2020
6d659f5
Update frame/node-authorization/src/lib.rs
kaichaosun Sep 1, 2020
0c325fe
Update frame/node-authorization/src/lib.rs
kaichaosun Sep 1, 2020
5cc6ae1
Update frame/node-authorization/src/lib.rs
kaichaosun Sep 1, 2020
4c269a5
format code
kaichaosun Sep 1, 2020
4907592
expose NetworkService
kaichaosun Sep 2, 2020
f112fa7
remove NetworkStateInfo in offchain
kaichaosun Sep 2, 2020
420072f
replace NodePublicKey with PeerId.
kaichaosun Sep 2, 2020
e0dce84
set max length of peer id.
kaichaosun Sep 2, 2020
65cd8ed
clear more
kaichaosun Sep 2, 2020
d429363
use BTreeSet for set of peers.
kaichaosun Sep 2, 2020
12f0386
decode opaque peer id.
kaichaosun Sep 2, 2020
82359b8
Merge branch 'master' of https://github.com/paritytech/substrate into…
kaichaosun Sep 8, 2020
a18b248
extract NetworkProvider for client offchain.
kaichaosun Sep 8, 2020
76c7535
use OpaquePeerId in node authorization pallet.
kaichaosun Sep 8, 2020
2430b16
fix test
kaichaosun Sep 8, 2020
a6929c1
better documentation
kaichaosun Sep 8, 2020
36f3829
fix test
kaichaosun Sep 8, 2020
889dfe6
doc
kaichaosun Sep 8, 2020
982bcaf
more fix
kaichaosun Sep 8, 2020
2c893ac
Update primitives/core/src/offchain/mod.rs
kaichaosun Sep 9, 2020
e3882be
Update client/offchain/src/api.rs
kaichaosun Sep 9, 2020
517f97a
derive serialize and deserialize
kaichaosun Sep 10, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ members = [
"frame/metadata",
"frame/multisig",
"frame/nicks",
"frame/node-authorization",
"frame/offences",
"frame/proxy",
"frame/randomness-collective-flip",
Expand Down
16 changes: 16 additions & 0 deletions client/authority-discovery/src/worker/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ use futures::task::LocalSpawn;
use futures::poll;
use libp2p::{kad, core::multiaddr, PeerId};

use sc_network::config::identity;
use sc_peerset::{Peerset, PeersetConfig, PeersetHandle};
use sp_api::{ProvideRuntimeApi, ApiRef};
use sp_core::{crypto::Public, testing::KeyStore};
use sp_runtime::traits::{Zero, Block as BlockT, NumberFor};
Expand Down Expand Up @@ -219,6 +221,20 @@ impl NetworkStateInfo for TestNetwork {
fn external_addresses(&self) -> Vec<Multiaddr> {
self.external_addresses.clone()
}

fn local_public_key(&self) -> identity::PublicKey {
identity::Keypair::generate_ed25519().public()
}

fn peerset(&self) -> PeersetHandle {
Peerset::from_config(PeersetConfig {
in_peers: 25,
out_peers: 25,
bootnodes: vec![],
reserved_only: false,
priority_groups: vec![],
}).1
}
}

#[test]
Expand Down
8 changes: 8 additions & 0 deletions client/network/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,8 @@ pub use protocol::{event::{DhtEvent, Event, ObservedRole}, sync::SyncState, Peer
pub use service::{NetworkService, NetworkWorker, RequestFailure, OutboundFailure};

pub use sc_peerset::ReputationChange;
use config::identity::PublicKey;
use sc_peerset::PeersetHandle;
use sp_runtime::traits::{Block as BlockT, NumberFor};

/// The maximum allowed number of established connections per peer.
Expand All @@ -294,6 +296,12 @@ pub trait NetworkStateInfo {

/// Returns the local Peer ID.
fn local_peer_id(&self) -> PeerId;

/// Returns the local Peer PublicKey.
fn local_public_key(&self) -> PublicKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that this pull request treats a PublicKey as an opaque data set? In other words, does this pull request make sure of PublicKey verification? If not, why not use the PeerId abstraction as the base network identity?

Copy link
Contributor Author

@kaichaosun kaichaosun Sep 1, 2020

Choose a reason for hiding this comment

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

There is no extra verification (except the length of the bytes) for PublicKey in this PR, the reasons I didn't use PeerId are,

  • PeerId is from libp2p, the encode/decode for it is pretty a challenge to me. I guess you mean using an opaque data like Vec<u8> to represent the PeerId when storing and convert it back when using it, that's probably also work in my scenario, but sounds a bit wild.
  • It also needs to pass in and read by a user, I'm not sure how convenient to use it.
  • trying to avoid the runtime user to care about the network layer, and PeerId is something fall in this.

Copy link
Contributor

Choose a reason for hiding this comment

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

but sounds a bit wild

That's actually the preferred approach IMO.
The conversion from PeerId to Vec<u8> and vice versa is precisely spec'ced by libp2p and not something we have invented.

It also needs to pass in and read by a user, I'm not sure how convenient to use it.
trying to avoid the runtime user to care about the network layer, and PeerId is something fall in this.

In terms of difficulty don't really see the difference between letting users manipulate a PublicKey and letting users manipulate a PeerId.
It is true that if you actually want to analyse the content of the PublicKey or PeerId, then it's more complicated if you have a PeerId. However, I don't think users actually want to do that.
Furthermore, libp2p public keys are never exposed anywhere in the node. Users can easily see which PeerId they have, but not which public key they have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was imagining a scenario that user generate node key so that it can be passed to the CLI when starting the node, at this time the public key is available and recorded by the user. Then be used in this node-authorization pallet.

I'm not in favor of putting PeerId to the user side. Even it's a mandatory concept in libp2p, but not so in Substrate runtime development AFAIK. Most of the time we are assuming the user don't need much knowledge such as PeerId about libp2p, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just find this P2P Networking in polkadot wiki, which eliminate a few concerns from me to use PeerId in runtime.
Still interested in others preference between PublicKey and PeerId.


/// Returns the peerset
fn peerset(&self) -> PeersetHandle;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think exposing the peerset in a trait "for providing information about the local network state" (see trait doc comment) is breaking the abstraction. Why not go through the NetworkService directly?

In addition, instead of exposing the entire peerset to the outside world, I would suggest keeping it an implementation detail of client/network, and only exposing methods on NetworkService like NetworkService::set_priority_group.

@tomaka probably has more input in this regard.

Copy link
Contributor

@tomaka tomaka Sep 1, 2020

Choose a reason for hiding this comment

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

Yes, the peerset is purely an implementation detail. Please don't expose it in the API.
I also prefer the approach of just using an Arc<NetworkService> everywhere. I really don't understand the purpose of the NetworkStateInfo trait in general.

Copy link
Contributor Author

@kaichaosun kaichaosun Sep 2, 2020

Choose a reason for hiding this comment

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

Refactored to use Arc<NetworkService>, but leave the NetworkStateInfo unchanged since it involves traits used in authority-discovery and I'm not sure the risk to change it. Looks a bit redundant by keeping both in offchain worker though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleared the NetworkStateInfo use in offchain worker.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks a bit redundant by keeping both in offchain worker though.

The idea is to build small coherent abstractions, allowing small components to only depend on subsets of a larger interface.

}

/// Overview status of the network.
Expand Down
22 changes: 19 additions & 3 deletions client/network/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
//!
//! There are two main structs in this module: [`NetworkWorker`] and [`NetworkService`].
//! The [`NetworkWorker`] *is* the network and implements the `Future` trait. It must be polled in
//! order fo the network to advance.
//! order for the network to advance.
//! The [`NetworkService`] is merely a shared version of the [`NetworkWorker`]. You can obtain an
//! `Arc<NetworkService>` by calling [`NetworkWorker::service`].
//!
Expand All @@ -30,7 +30,10 @@
use crate::{
ExHashT, NetworkStateInfo,
behaviour::{self, Behaviour, BehaviourOut},
config::{parse_str_addr, NonReservedPeerMode, Params, Role, TransportConfig},
config::{
parse_str_addr, NonReservedPeerMode, Params, Role, TransportConfig,
identity::PublicKey,
},
DhtEvent,
discovery::DiscoveryConfig,
error::Error,
Expand Down Expand Up @@ -94,6 +97,8 @@ pub struct NetworkService<B: BlockT + 'static, H: ExHashT> {
is_major_syncing: Arc<AtomicBool>,
/// Local copy of the `PeerId` of the local node.
local_peer_id: PeerId,
/// Local copy of the `PublicKey` of the local node.
local_public_key: PublicKey,
/// Bandwidth logging system. Can be queried to know the average bandwidth consumed.
bandwidth: Arc<transport::BandwidthSinks>,
/// Peerset manager (PSM); manages the reputation of nodes and indicates the network which
Expand Down Expand Up @@ -317,7 +322,7 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkWorker<B, H> {
protocol,
params.role,
user_agent,
local_public,
local_public.clone(),
block_requests,
finality_proof_requests,
light_client_handler,
Expand Down Expand Up @@ -398,6 +403,7 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkWorker<B, H> {
is_major_syncing: is_major_syncing.clone(),
peerset: peerset_handle,
local_peer_id,
local_public_key: local_public,
to_worker,
peers_notifications_sinks: peers_notifications_sinks.clone(),
protocol_name_by_engine,
Expand Down Expand Up @@ -1054,6 +1060,16 @@ impl<B, H> NetworkStateInfo for NetworkService<B, H>
fn local_peer_id(&self) -> PeerId {
self.local_peer_id.clone()
}

/// Returns the PublicKey of local peer.
fn local_public_key(&self) -> PublicKey {
self.local_public_key.clone()
}

/// Returns the peerset.
fn peerset(&self) -> PeersetHandle {
self.peerset.clone()
}
}

/// A `NotificationSender` allows for sending notifications to a peer with a chosen protocol.
Expand Down
1 change: 1 addition & 0 deletions client/offchain/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ rand = "0.7.2"
sp-runtime = { version = "2.0.0-rc6", path = "../../primitives/runtime" }
sp-utils = { version = "2.0.0-rc6", path = "../../primitives/utils" }
sc-network = { version = "0.8.0-rc6", path = "../network" }
sc-peerset = { version = "2.0.0-rc6", path = "../peerset" }
sc-keystore = { version = "2.0.0-rc6", path = "../keystore" }

[target.'cfg(not(target_os = "unknown"))'.dependencies]
Expand Down
57 changes: 55 additions & 2 deletions client/offchain/src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,19 @@ use std::{
sync::Arc,
convert::TryFrom,
thread::sleep,
collections::HashSet,
};

use sp_core::offchain::OffchainStorage;
use futures::Future;
use log::error;
use sc_network::{PeerId, Multiaddr, NetworkStateInfo};
use sc_network::{config::identity, PeerId, Multiaddr, NetworkStateInfo};
use codec::{Encode, Decode};
use sp_core::offchain::{
Externalities as OffchainExt, HttpRequestId, Timestamp, HttpRequestStatus, HttpError,
OpaqueNetworkState, OpaquePeerId, OpaqueMultiaddr, StorageKind,
};
use sp_core::NodePublicKey;
pub use sp_offchain::STORAGE_PREFIX;
pub use http::SharedClient;

Expand Down Expand Up @@ -180,6 +182,33 @@ impl<Storage: OffchainStorage> OffchainExt for Api<Storage> {
) -> Result<usize, HttpError> {
self.http.response_read_body(request_id, buffer, deadline)
}

fn get_node_public_key(&mut self) -> Result<NodePublicKey, ()> {
let public_key = self.network_state.local_public_key();
match public_key {
identity::PublicKey::Ed25519(public) =>
Ok(public.encode().into()),
_ => Err(()),
}
}

fn set_reserved_nodes(&mut self, nodes: Vec<NodePublicKey>, reserved_only: bool) {
let peer_ids: HashSet<PeerId> = nodes.iter()
.filter_map(|node|
match node {
NodePublicKey::Ed25519(public) =>
identity::ed25519::PublicKey::decode(&public.0).ok()
}
)
.map(|public|
identity::PublicKey::Ed25519(public).into_peer_id()
)
.collect();

let peerset = self.network_state.peerset();
peerset.set_reserved_peers(peer_ids);
peerset.set_reserved_only(reserved_only);
}
}

/// Information about the local node's network state.
Expand Down Expand Up @@ -292,7 +321,7 @@ mod tests {
use super::*;
use std::{convert::{TryFrom, TryInto}, time::SystemTime};
use sc_client_db::offchain::LocalStorage;
use sc_network::PeerId;
use sc_peerset::{Peerset, PeersetConfig, PeersetHandle};

struct MockNetworkStateInfo();

Expand All @@ -304,6 +333,20 @@ mod tests {
fn local_peer_id(&self) -> PeerId {
PeerId::random()
}

fn local_public_key(&self) -> identity::PublicKey {
identity::Keypair::generate_ed25519().public()
}

fn peerset(&self) -> PeersetHandle {
Peerset::from_config(PeersetConfig {
in_peers: 25,
out_peers: 25,
bootnodes: vec![],
reserved_only: false,
priority_groups: vec![],
}).1
}
}

fn offchain_api() -> (Api<LocalStorage>, AsyncApi) {
Expand Down Expand Up @@ -429,4 +472,14 @@ mod tests {
// then
assert_ne!(seed, [0; 32]);
}

#[test]
fn should_get_node_public_key() {
// given
let mut api = offchain_api().0;
let node_public_key = api.get_node_public_key();

// then
assert!(node_public_key.is_ok());
}
}
17 changes: 16 additions & 1 deletion client/offchain/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,8 @@ pub async fn notification_future<Client, Storage, Block, Spawner>(
mod tests {
use super::*;
use std::sync::Arc;
use sc_network::{Multiaddr, PeerId};
use sc_network::{Multiaddr, PeerId, config::identity};
use sc_peerset::{Peerset, PeersetConfig, PeersetHandle};
use substrate_test_runtime_client::{TestClient, runtime::Block};
use sc_transaction_pool::{BasicPool, FullChainApi};
use sp_transaction_pool::{TransactionPool, InPoolTransaction};
Expand All @@ -223,6 +224,20 @@ mod tests {
fn local_peer_id(&self) -> PeerId {
PeerId::random()
}

fn local_public_key(&self) -> identity::PublicKey {
identity::Keypair::generate_ed25519().public()
}

fn peerset(&self) -> PeersetHandle {
Peerset::from_config(PeersetConfig {
in_peers: 25,
out_peers: 25,
bootnodes: vec![],
reserved_only: false,
priority_groups: vec![],
}).1
}
}

struct TestPool(
Expand Down
12 changes: 12 additions & 0 deletions client/peerset/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const FORGET_AFTER: Duration = Duration::from_secs(3600);
enum Action {
AddReservedPeer(PeerId),
RemoveReservedPeer(PeerId),
SetReservedPeers(HashSet<PeerId>),
SetReservedOnly(bool),
ReportPeer(PeerId, ReputationChange),
SetPriorityGroup(String, HashSet<PeerId>),
Expand Down Expand Up @@ -102,6 +103,11 @@ impl PeersetHandle {
pub fn set_reserved_only(&self, reserved: bool) {
let _ = self.tx.unbounded_send(Action::SetReservedOnly(reserved));
}

/// Set reserved peers to the new set.
pub fn set_reserved_peers(&self, peer_ids: HashSet<PeerId>) {
let _ = self.tx.unbounded_send(Action::SetReservedPeers(peer_ids));
}

/// Reports an adjustment to the reputation of the given peer.
pub fn report_peer(&self, peer_id: PeerId, score_diff: ReputationChange) {
Expand Down Expand Up @@ -246,6 +252,10 @@ impl Peerset {
fn on_remove_reserved_peer(&mut self, peer_id: PeerId) {
self.on_remove_from_priority_group(RESERVED_NODES, peer_id);
}

fn on_set_reserved_peers(&mut self, peer_ids: HashSet<PeerId>) {
self.on_set_priority_group(RESERVED_NODES, peer_ids);
}

fn on_set_reserved_only(&mut self, reserved_only: bool) {
self.reserved_only = reserved_only;
Expand Down Expand Up @@ -655,6 +665,8 @@ impl Stream for Peerset {
self.on_add_reserved_peer(peer_id),
Action::RemoveReservedPeer(peer_id) =>
self.on_remove_reserved_peer(peer_id),
Action::SetReservedPeers(peer_ids) =>
self.on_set_reserved_peers(peer_ids),
Action::SetReservedOnly(reserved) =>
self.on_set_reserved_only(reserved),
Action::ReportPeer(peer_id, score_diff) =>
Expand Down
36 changes: 36 additions & 0 deletions frame/node-authorization/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
[package]
name = "pallet-node-authorization"
version = "2.0.0-rc6"
authors = ["Parity Technologies <[email protected]>"]
edition = "2018"
license = "Apache-2.0"
homepage = "https://substrate.dev"
repository = "https://github.com/paritytech/substrate/"
description = "FRAME pallet for node authorization"

[package.metadata.docs.rs]
targets = ["x86_64-unknown-linux-gnu"]

[dependencies]
serde = { version = "1.0.101", optional = true }
codec = { package = "parity-scale-codec", version = "1.3.4", default-features = false, features = ["derive"] }
frame-support = { version = "2.0.0-rc6", default-features = false, path = "../support" }
frame-system = { version = "2.0.0-rc6", default-features = false, path = "../system" }
sp-core = { version = "2.0.0-rc6", default-features = false, path = "../../primitives/core" }
sp-io = { version = "2.0.0-rc6", default-features = false, path = "../../primitives/io" }
sp-std = { version = "2.0.0-rc6", default-features = false, path = "../../primitives/std" }

[dev-dependencies]
sp-runtime = { version = "2.0.0-rc6", default-features = false, path = "../../primitives/runtime" }

[features]
default = ["std"]
std = [
"serde",
"codec/std",
"frame-support/std",
"frame-system/std",
"sp-core/std",
"sp-io/std",
"sp-std/std",
]
Loading