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

babe: treat epoch_authorship RPC method as unsafe #6069

Merged
gavofyork merged 13 commits into from
May 21, 2020
Merged

babe: treat epoch_authorship RPC method as unsafe #6069

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
285234d
service: pass DenyUnsafe to rpc extensions
andresilva May 18, 2020
d3730f1
node: add DenyUnsafe to rpc full node dependencies
andresilva May 18, 2020
d6b56d7
client: fix whitespace in rpc policy file
andresilva May 18, 2020
27bcb4b
babe: treat epochAuthorship rpc method as unsafe
andresilva May 18, 2020
78c1b9c
babe: add test for unsafe rpc method
andresilva May 18, 2020
229d32f
babe: rename babe rpc handler
andresilva May 18, 2020
a083f24
service: traitify rpc extension builder
andresilva May 20, 2020
09ded54
service: make the rpc extensions builder api non-breaking
andresilva May 20, 2020
824545b
service: revert changes from light node rpc extensions builder
andresilva May 20, 2020
0ffb971
node: remove unnecessary type in service creation
andresilva May 20, 2020
8298d0a
service: cleanup with_rpc_extensions implementation
andresilva May 20, 2020
d515e33
service: add missing docs to RpcExtensionBuilder
andresilva May 20, 2020
6d8ba08
Merge branch 'master' into andre/babe-unsafe-rpc
andresilva May 20, 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
Next Next commit
service: pass DenyUnsafe to rpc extensions
  • Loading branch information
@andresilva
andresilva committed May 18, 2020
commit 285234d535a0a40e7c5268e166a139741eeb1c75
88 changes: 55 additions & 33 deletions bin/node/cli/src/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,30 +99,45 @@ macro_rules! new_full_start {
import_setup = Some((block_import, grandpa_link, babe_link));
Ok(import_queue)
})?
.with_rpc_extensions(|builder| -> std::result::Result<RpcExtension, _> {
let babe_link = import_setup.as_ref().map(|s| &s.2)
.expect("BabeLink is present for full services or set up failed; qed.");
.with_rpc_extensions(|builder| {
Copy link
Contributor

@tomusdrw tomusdrw May 19, 2020

Choose a reason for hiding this comment

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

We could expect a closure that get's a RpcDependencies object here instead of Builder, which would contain all required stuff and deny_unsafe. That would simplify the code a bit (no extra closure), but would make this method special in a way that it's not invoked right away like other with_* methods.

let grandpa_link = import_setup.as_ref().map(|s| &s.1)
.expect("GRANDPA LinkHalf is present for full services or set up failed; qed.");
let shared_authority_set = grandpa_link.shared_authority_set();

let shared_authority_set = grandpa_link.shared_authority_set().clone();
let shared_voter_state = grandpa::SharedVoterState::empty();
let deps = node_rpc::FullDeps {
client: builder.client().clone(),
pool: builder.pool(),
select_chain: builder.select_chain().cloned()
.expect("SelectChain is present for full services or set up failed; qed."),
babe: node_rpc::BabeDeps {
keystore: builder.keystore(),
babe_config: sc_consensus_babe::BabeLink::config(babe_link).clone(),
shared_epoch_changes: sc_consensus_babe::BabeLink::epoch_changes(babe_link).clone()
},
grandpa: node_rpc::GrandpaDeps {
shared_voter_state: shared_voter_state.clone(),
shared_authority_set: shared_authority_set.clone(),
},
};
rpc_setup = Some((shared_voter_state));
Ok(node_rpc::create_full(deps))

rpc_setup = Some((shared_voter_state.clone()));

let babe_link = import_setup.as_ref().map(|s| &s.2)
.expect("BabeLink is present for full services or set up failed; qed.");

let babe_config = babe_link.config().clone();
let shared_epoch_changes = babe_link.epoch_changes().clone();

let client = builder.client().clone();
let pool = builder.pool().clone();
let select_chain = builder.select_chain().cloned()
.expect("SelectChain is present for full services or set up failed; qed.");
let keystore = builder.keystore().clone();

Ok(move |deny_unsafe| -> RpcExtension {
Copy link
Contributor

@tomusdrw tomusdrw May 19, 2020

Choose a reason for hiding this comment

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

Another way to avoid returning a closure would be to have deny_unsafe be a dynamic object that is lazy initialized when RPC is bound. That might require a Mutex and thread_local cache or some unsafe black magic to avoid huge performance costs on every RPC call though.

Copy link
Contributor Author

@andresilva andresilva May 19, 2020

Choose a reason for hiding this comment

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

But when we're building the service we don't know how many times the given extension will be initialized (http, ws?), and we'd need separate instances of this dynamic object for each instantiation I think.

Copy link
Contributor

@tomusdrw tomusdrw May 19, 2020

Choose a reason for hiding this comment

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

Yeah, that's correct - we do need different parameter depending on the transport the extension will be used for, so this solution is a no-go.

let deps = node_rpc::FullDeps {
client: client.clone(),
pool: pool.clone(),
select_chain: select_chain.clone(),
babe: node_rpc::BabeDeps {
babe_config: babe_config.clone(),
shared_epoch_changes: shared_epoch_changes.clone(),
keystore: keystore.clone(),
},
grandpa: node_rpc::GrandpaDeps {
shared_voter_state: shared_voter_state.clone(),
shared_authority_set: shared_authority_set.clone(),
},
};

node_rpc::create_full(deps)
})
})?;

(builder, import_setup, inherent_data_providers, rpc_setup)
Expand Down Expand Up @@ -366,21 +381,28 @@ pub fn new_light(config: Configuration)
let provider = client as Arc<dyn StorageAndProofProvider<_, _>>;
Ok(Arc::new(GrandpaFinalityProofProvider::new(backend, provider)) as _)
})?
.with_rpc_extensions(|builder,| ->
Result<RpcExtension, _>
{
.with_rpc_extensions(|builder| {
let fetcher = builder.fetcher()
.ok_or_else(|| "Trying to start node RPC without active fetcher")?;
.ok_or_else(|| "Trying to start node RPC without active fetcher")?
.clone();

let remote_blockchain = builder.remote_backend()
.ok_or_else(|| "Trying to start node RPC without active remote blockchain")?;
.ok_or_else(|| "Trying to start node RPC without active remote blockchain")?
.clone();

let client = builder.client().clone();
let pool = builder.pool().clone();

Ok(move |deny_unsafe| -> RpcExtension {
let light_deps = node_rpc::LightDeps {
remote_blockchain: remote_blockchain.clone(),
fetcher: fetcher.clone(),
client: client.clone(),
pool: pool.clone(),
};

let light_deps = node_rpc::LightDeps {
remote_blockchain,
fetcher,
client: builder.client().clone(),
pool: builder.pool(),
};
Ok(node_rpc::create_light(light_deps))
node_rpc::create_light(light_deps)
})
})?
.build()?;

Expand Down
42 changes: 24 additions & 18 deletions client/service/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ pub struct ServiceBuilder<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp,
finality_proof_request_builder: Option<TFprb>,
finality_proof_provider: Option<TFpp>,
transaction_pool: Arc<TExPool>,
rpc_extensions: TRpc,
rpc_extensions_builder: Box<dyn Fn(sc_rpc::DenyUnsafe) -> TRpc + Send>,
remote_backend: Option<Arc<dyn RemoteBlockchain<TBl>>>,
marker: PhantomData<(TBl, TRtApi)>,
block_announce_validator_builder: Option<Box<dyn FnOnce(Arc<TCl>) -> Box<dyn BlockAnnounceValidator<TBl> + Send> + Send>>,
Expand Down Expand Up @@ -310,7 +310,7 @@ impl ServiceBuilder<(), (), (), (), (), (), (), (), (), (), ()> {
finality_proof_request_builder: None,
finality_proof_provider: None,
transaction_pool: Arc::new(()),
rpc_extensions: Default::default(),
rpc_extensions_builder: Box::new(|_| ()),
remote_backend: None,
block_announce_validator_builder: None,
marker: PhantomData,
Expand Down Expand Up @@ -393,7 +393,7 @@ impl ServiceBuilder<(), (), (), (), (), (), (), (), (), (), ()> {
finality_proof_request_builder: None,
finality_proof_provider: None,
transaction_pool: Arc::new(()),
rpc_extensions: Default::default(),
rpc_extensions_builder: Box::new(|_| ()),
remote_backend: Some(remote_blockchain),
block_announce_validator_builder: None,
marker: PhantomData,
Expand Down Expand Up @@ -466,7 +466,7 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
finality_proof_request_builder: self.finality_proof_request_builder,
finality_proof_provider: self.finality_proof_provider,
transaction_pool: self.transaction_pool,
rpc_extensions: self.rpc_extensions,
rpc_extensions_builder: self.rpc_extensions_builder,
remote_backend: self.remote_backend,
block_announce_validator_builder: self.block_announce_validator_builder,
marker: self.marker,
Expand Down Expand Up @@ -511,7 +511,7 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
finality_proof_request_builder: self.finality_proof_request_builder,
finality_proof_provider: self.finality_proof_provider,
transaction_pool: self.transaction_pool,
rpc_extensions: self.rpc_extensions,
rpc_extensions_builder: self.rpc_extensions_builder,
remote_backend: self.remote_backend,
block_announce_validator_builder: self.block_announce_validator_builder,
marker: self.marker,
Expand Down Expand Up @@ -549,7 +549,7 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
finality_proof_request_builder: self.finality_proof_request_builder,
finality_proof_provider,
transaction_pool: self.transaction_pool,
rpc_extensions: self.rpc_extensions,
rpc_extensions_builder: self.rpc_extensions_builder,
remote_backend: self.remote_backend,
block_announce_validator_builder: self.block_announce_validator_builder,
marker: self.marker,
Expand Down Expand Up @@ -615,7 +615,7 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
finality_proof_request_builder: fprb,
finality_proof_provider: self.finality_proof_provider,
transaction_pool: self.transaction_pool,
rpc_extensions: self.rpc_extensions,
rpc_extensions_builder: self.rpc_extensions_builder,
remote_backend: self.remote_backend,
block_announce_validator_builder: self.block_announce_validator_builder,
marker: self.marker,
Expand Down Expand Up @@ -679,21 +679,27 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
finality_proof_request_builder: self.finality_proof_request_builder,
finality_proof_provider: self.finality_proof_provider,
transaction_pool: Arc::new(transaction_pool),
rpc_extensions: self.rpc_extensions,
rpc_extensions_builder: self.rpc_extensions_builder,
remote_backend: self.remote_backend,
block_announce_validator_builder: self.block_announce_validator_builder,
marker: self.marker,
})
}

/// Defines the RPC extensions to use.
pub fn with_rpc_extensions<URpc>(
pub fn with_rpc_extensions<URpcBuilder, URpc>(
self,
rpc_ext_builder: impl FnOnce(&Self) -> Result<URpc, Error>,
) -> Result<ServiceBuilder<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp,
TExPool, URpc, Backend>, Error>
where TSc: Clone, TFchr: Clone {
let rpc_extensions = rpc_ext_builder(&self)?;
rpc_ext_builder: impl FnOnce(&Self) -> Result<URpcBuilder, Error>,
) -> Result<
ServiceBuilder<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, URpc, Backend>,
Error,
>
where
TSc: Clone,
TFchr: Clone,
URpcBuilder: Fn(sc_rpc::DenyUnsafe) -> URpc + Send + 'static,
Copy link
Contributor

@tomusdrw tomusdrw May 19, 2020

Choose a reason for hiding this comment

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

We could consider traitifying Fn(sc_rpc::DenyUnsafe) -> URpc + Send + 'static and implement that trait for both closures and concrete URpc type, so that we could maintain backward compatibility. Not sure if it's worth it though.

Copy link
Contributor Author

@andresilva andresilva May 19, 2020

Choose a reason for hiding this comment

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

This might be a good idea actually, for example in the light client currently we don't care about DenyUnsafe for any rpc extensions. I guess right now anyone who isn't using BABE's RPC wouldn't care that much about it either.

Copy link
Contributor

@Demi-Marie Demi-Marie May 19, 2020

Choose a reason for hiding this comment

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

@tomusdrw how does one do that? Every time I have tried to do that it fails with some obscure lifetime error.

Copy link
Contributor

@tomusdrw tomusdrw May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You mean this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=59c9d81d289603e857e5a9dbb8970486 ?
(updated with usage: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6d9803694d261742b5871b80f5067b33 )

{
let rpc_extensions_builder = Box::new(rpc_ext_builder(&self)?);

Ok(ServiceBuilder {
config: self.config,
Expand All @@ -707,7 +713,7 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
finality_proof_request_builder: self.finality_proof_request_builder,
finality_proof_provider: self.finality_proof_provider,
transaction_pool: self.transaction_pool,
rpc_extensions,
rpc_extensions_builder: rpc_extensions_builder,
remote_backend: self.remote_backend,
block_announce_validator_builder: self.block_announce_validator_builder,
marker: self.marker,
Expand Down Expand Up @@ -735,7 +741,7 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
finality_proof_request_builder: self.finality_proof_request_builder,
finality_proof_provider: self.finality_proof_provider,
transaction_pool: self.transaction_pool,
rpc_extensions: self.rpc_extensions,
rpc_extensions_builder: self.rpc_extensions_builder,
remote_backend: self.remote_backend,
block_announce_validator_builder: Some(Box::new(block_announce_validator_builder)),
marker: self.marker,
Expand Down Expand Up @@ -853,7 +859,7 @@ ServiceBuilder<
finality_proof_request_builder,
finality_proof_provider,
transaction_pool,
rpc_extensions,
rpc_extensions_builder,
remote_backend,
block_announce_validator_builder,
} = self;
Expand Down Expand Up @@ -1159,7 +1165,7 @@ ServiceBuilder<
maybe_offchain_rpc,
author::AuthorApi::to_delegate(author),
system::SystemApi::to_delegate(system),
rpc_extensions.clone(),
rpc_extensions_builder(deny_unsafe),
))
};
let rpc = start_rpc_servers(&config, gen_handler)?;
Expand Down