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

Add method with_default_block_announcer to ServiceBuilder #5797

Merged
2 commits merged into from
Apr 27, 2020
Merged

Add method with_default_block_announcer to ServiceBuilder #5797

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bfc6a0a
Initial commit
cecton Apr 27, 2020
79de59f
Add method with_default_block_announcer to ServiceBuilder
cecton Apr 27, 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
51 changes: 48 additions & 3 deletions client/service/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ use sc_client_api::{
use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedSender};
use sc_client::{Client, ClientConfig};
use sc_chain_spec::get_extension;
use sp_consensus::import_queue::ImportQueue;
use sp_consensus::{
block_validation::{BlockAnnounceValidator, DefaultBlockAnnounceValidator},
import_queue::ImportQueue,
};
use futures::{
Future, FutureExt, StreamExt,
future::ready,
Expand Down Expand Up @@ -93,6 +96,7 @@ pub struct ServiceBuilder<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp,
remote_backend: Option<Arc<dyn RemoteBlockchain<TBl>>>,
marker: PhantomData<(TBl, TRtApi)>,
background_tasks: Vec<(&'static str, BackgroundTask)>,
block_announce_validator_builder: Option<Box<dyn FnOnce(Arc<TCl>) -> Box<dyn BlockAnnounceValidator<TBl> + Send>>>,
}

/// Full client type.
Expand Down Expand Up @@ -261,6 +265,7 @@ impl ServiceBuilder<(), (), (), (), (), (), (), (), (), (), ()> {
rpc_extensions: Default::default(),
remote_backend: None,
background_tasks: Default::default(),
block_announce_validator_builder: None,
marker: PhantomData,
})
}
Expand Down Expand Up @@ -344,6 +349,7 @@ impl ServiceBuilder<(), (), (), (), (), (), (), (), (), (), ()> {
rpc_extensions: Default::default(),
remote_backend: Some(remote_blockchain),
background_tasks: Default::default(),
block_announce_validator_builder: None,
marker: PhantomData,
})
}
Expand Down Expand Up @@ -417,6 +423,7 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
rpc_extensions: self.rpc_extensions,
remote_backend: self.remote_backend,
background_tasks: self.background_tasks,
block_announce_validator_builder: self.block_announce_validator_builder,
marker: self.marker,
})
}
Expand Down Expand Up @@ -460,6 +467,7 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
rpc_extensions: self.rpc_extensions,
remote_backend: self.remote_backend,
background_tasks: self.background_tasks,
block_announce_validator_builder: self.block_announce_validator_builder,
marker: self.marker,
})
}
Expand Down Expand Up @@ -498,6 +506,7 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
rpc_extensions: self.rpc_extensions,
remote_backend: self.remote_backend,
background_tasks: self.background_tasks,
block_announce_validator_builder: self.block_announce_validator_builder,
marker: self.marker,
})
}
Expand Down Expand Up @@ -560,6 +569,7 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
rpc_extensions: self.rpc_extensions,
remote_backend: self.remote_backend,
background_tasks: self.background_tasks,
block_announce_validator_builder: self.block_announce_validator_builder,
marker: self.marker,
})
}
Expand Down Expand Up @@ -622,6 +632,7 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
rpc_extensions: self.rpc_extensions,
remote_backend: self.remote_backend,
background_tasks: self.background_tasks,
block_announce_validator_builder: self.block_announce_validator_builder,
marker: self.marker,
})
}
Expand Down Expand Up @@ -650,6 +661,36 @@ impl<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp, TExPool, TRpc, Backend>
rpc_extensions,
remote_backend: self.remote_backend,
background_tasks: self.background_tasks,
block_announce_validator_builder: self.block_announce_validator_builder,
marker: self.marker,
})
}

/// Defines the `BlockAnnounceValidator` to use. `DefaultBlockAnnounceValidator` will be used by
/// default.
pub fn with_block_announce_validator(
self,
block_announce_validator_builder:
impl FnOnce(Arc<TCl>) -> Box<dyn BlockAnnounceValidator<TBl> + Send> + 'static,
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 think this should actually return a Result. That's how it's done in all the others and I guess it's for the same reasons than sc-cli's SubstrateCli having a lot of Result everywhere too

Copy link
Contributor

Choose a reason for hiding this comment

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

The type is pretty complex here, maybe traitify this and then implement the trait for every FnOnce() -> ... ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also why isn't this just a generic type on ServiceBuidler level with static new function instead of a builder? Is there some dynamic state we expect to be transferred with the closure?

fn with_block_announce_validator<TBAV: BlockAnnounceValidator<TCl, TBl>>(self) -> ServiceBuilder<TBAV, TBl, ...> {
 ... // copy over the fields.
}

trait BlockAnnounceValidator<TCl, B> {
   fn new(client: Arc<TCl>) -> Self;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. There are 2 reasons for that:

  1. In my use case for cumulus (see issue in description) we want to use a custom validator JustifiedBlockAnnounceValidator and as you can see the arguments for new() is actually something custom. I assume this can be personalized by the parachain as they want

  2. On substrate you can clearly see that this has been done with this intention because the function new is not in the trait.

Copy link
Contributor

Choose a reason for hiding this comment

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

To stay consistent with the existing API, I'd keep it this way for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that "staying consistent with the existing API" meant changing it to return a Result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Changing the closure to return a Result? Yes that's what I meant)

) -> Result<ServiceBuilder<TBl, TRtApi, TCl, TFchr, TSc, TImpQu, TFprb, TFpp,
TExPool, TRpc, Backend>, Error>
where TSc: Clone, TFchr: Clone {
Ok(ServiceBuilder {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

...and the Result here makes no sense if the closure in input is not called right away. @gnunicorn what do you think I should do?

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 think for consistency with the current implementation of the servicebuilder it is best to call the closure directly and use a Result in the closure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this isn't pretty, but okay for now. And as we want to move to deferring calls (see #4587 ) we need to refactor this things alltogether soon anyways.

config: self.config,
client: self.client,
backend: self.backend,
task_manager: self.task_manager,
keystore: self.keystore,
fetcher: self.fetcher,
select_chain: self.select_chain,
import_queue: self.import_queue,
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,
remote_backend: self.remote_backend,
background_tasks: self.background_tasks,
block_announce_validator_builder: Some(Box::new(block_announce_validator_builder)),
marker: self.marker,
})
}
Expand Down Expand Up @@ -761,6 +802,7 @@ ServiceBuilder<
rpc_extensions,
remote_backend,
background_tasks,
block_announce_validator_builder,
} = self;

sp_session::generate_initial_session_keys(
Expand Down Expand Up @@ -809,8 +851,11 @@ ServiceBuilder<
sc_network::config::ProtocolId::from(protocol_id_full)
};

let block_announce_validator =
Box::new(sp_consensus::block_validation::DefaultBlockAnnounceValidator::new(client.clone()));
let block_announce_validator = if let Some(f) = block_announce_validator_builder {
f(client.clone())
} else {
Box::new(DefaultBlockAnnounceValidator::new(client.clone()))
};

let network_params = sc_network::config::Params {
role: config.role.clone(),
Expand Down