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

Fixes required for Cumulus #485

Merged
bkchr merged 3 commits into from
Oct 21, 2019
Merged

Fixes required for Cumulus #485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
05875eb
Collator node need to register all gossip validators as well
bkchr Oct 18, 2019
67a22a2
Make sure that parachain authorities are only written once at genesis
bkchr Oct 18, 2019
7827524
Fix test
bkchr Oct 19, 2019
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
19 changes: 15 additions & 4 deletions runtime/src/parachains.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ const WATERMARK_QUEUE_SIZE: usize = 20000;
decl_storage! {
trait Store for Module<T: Trait> as Parachains {
/// All authorities' keys at the moment.
pub Authorities get(authorities) config(authorities): Vec<ValidatorId>;
pub Authorities get(authorities): Vec<ValidatorId>;
/// The parachains registered at present.
pub Code get(parachain_code): map ParaId => Option<Vec<u8>>;
/// The heads of the parachains registered at present.
Expand Down Expand Up @@ -187,6 +187,10 @@ decl_storage! {
/// None if not yet updated.
pub DidUpdate: Option<Vec<ParaId>>;
}
add_extra_genesis {
config(authorities): Vec<ValidatorId>;
build(|config| Module::<T>::initialize_authorities(&config.authorities))
}
}

decl_module! {
Expand Down Expand Up @@ -814,6 +818,13 @@ impl<T: Trait> Module<T> {
})
}

fn initialize_authorities(authorities: &[ValidatorId]) {
if !authorities.is_empty() {
assert!(Authorities::get().is_empty(), "Authorities are already initialized!");
Authorities::put(authorities);
}
}

/*
// TODO: Consider integrating if needed. (https://github.com/paritytech/polkadot/issues/223)
/// Extract the parachain heads from the block.
Expand All @@ -837,14 +848,14 @@ impl<T: Trait> session::OneSessionHandler<T::AccountId> for Module<T> {
fn on_genesis_session<'a, I: 'a>(validators: I)
where I: Iterator<Item=(&'a T::AccountId, Self::Key)>
{
<Self as Store>::Authorities::put(&validators.map(|(_, key)| key).collect::<Vec<_>>())
Self::initialize_authorities(&validators.map(|(_, key)| key).collect::<Vec<_>>());
}

fn on_new_session<'a, I: 'a>(changed: bool, validators: I, _queued: I)
where I: Iterator<Item=(&'a T::AccountId, Self::Key)>
{
if changed {
Self::on_genesis_session(validators)
<Self as Store>::Authorities::put(validators.map(|(_, key)| key).collect::<Vec<_>>());
}
}

Expand Down Expand Up @@ -1139,7 +1150,7 @@ mod tests {

GenesisConfig {
authorities: authorities.clone(),
}.assimilate_storage(&mut t).unwrap();
}.assimilate_storage::<Test>(&mut t).unwrap();

registrar::GenesisConfig::<Test> {
parachains,
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/registrar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,7 @@ mod tests {

parachains::GenesisConfig {
authorities: authorities.clone(),
}.assimilate_storage(&mut t).unwrap();
}.assimilate_storage::<Test>(&mut t).unwrap();

GenesisConfig::<Test> {
parachains,
Expand Down
9 changes: 1 addition & 8 deletions service/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,8 @@ pub fn new_full(config: Configuration<CustomConfiguration, GenesisConfig>)
{
use substrate_network::DhtEvent;

let is_authority = config.roles.is_authority();
let is_collator = config.custom.collating_for.is_some();
let is_authority = config.roles.is_authority() && !is_collator;
Copy link
Contributor

@gui1117 gui1117 Oct 18, 2019

Choose a reason for hiding this comment

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

I don't know if this breaks some assumption in the code though

Copy link
Member Author

@bkchr bkchr Oct 18, 2019

Choose a reason for hiding this comment

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

What kind of assumptions? As collator we are currently a full node and this line expresses this.

Copy link
Contributor

@gui1117 gui1117 Oct 18, 2019

Choose a reason for hiding this comment

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

ok I should have put this comment on the if is_collator then return.

maybe there is more logic wrong there, like network_gossip register two times is this correct ?

polkadot/service/src/lib.rs

Lines 208 to 211 in 4a983a8

let gossip_validator = network_gossip::register_validator(
service.network(),
(is_known, client.clone()),
);

polkadot/service/src/lib.rs

Line 280 in 4a983a8

network_gossip::register_non_authority_validator(service.network());

Copy link
Member Author

@bkchr bkchr Oct 19, 2019

Choose a reason for hiding this comment

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

This is the stuff I need :) Otherwise the node always disconnects, because of unknown gossip messages.

Copy link
Contributor

@gui1117 gui1117 Oct 21, 2019

Choose a reason for hiding this comment

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

I still don't understand why in case of a collator we register two times: one time as validator and another time as non_authority_validator

Copy link
Member Author

@bkchr bkchr Oct 21, 2019

Choose a reason for hiding this comment

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

This is not only done for collators, also for full nodes.
I think this probably can be moved into the if.

Copy link
Member Author

@bkchr bkchr Oct 21, 2019

Choose a reason for hiding this comment

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

@andresilva confirmed that this is a bug, as you highlighted. We will open another pr to fix this.

let force_authoring = config.force_authoring;
let max_block_data_size = config.custom.max_block_data_size;
let db_path = config.database_path.clone();
Expand All @@ -169,13 +169,6 @@ pub fn new_full(config: Configuration<CustomConfiguration, GenesisConfig>)
let (block_import, link_half, babe_link) = import_setup.take()
.expect("Link Half and Block Import are present for Full Services or setup failed before. qed");

if is_collator {
info!(
"The node cannot start as an authority because it is also configured to run as a collator."
);
return Ok(service);
}

let client = service.client();
let known_oracle = client.clone();
let select_chain = if let Some(select_chain) = service.select_chain() {
Expand Down