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

start working on building the real overseer #1795

Merged
54 commits merged into from
Oct 28, 2020
Merged

start working on building the real overseer #1795

54 commits merged into from
Oct 28, 2020

Conversation

coriolinus
Copy link
Contributor

@coriolinus coriolinus commented Oct 8, 2020

No description provided.

Unfortunately, this fails to compile right now due to an upstream
failure to compile which is probably brought on by a recent upgrade
to rustc v1.47.
@coriolinus coriolinus added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Oct 8, 2020
node/service/src/lib.rs Outdated Show resolved Hide resolved
node/service/src/lib.rs Outdated Show resolved Hide resolved
node/service/src/lib.rs Outdated Show resolved Hide resolved
@ordian
Copy link
Member

ordian commented Oct 12, 2020

I think impl HeaderBackend for Arc<Client> is indeed missing. We could work around that by accepting Arc<RuntimeClient> like here (and in many other places) and/or implement that directly in substrate if possible.

It's not straightforwardly obvious that this is the best way to handle
the case when there is no authority discovery service, but it seems
to be the best option available at the moment.
node/service/src/lib.rs Outdated Show resolved Hide resolved
ordian and others added 5 commits October 15, 2020 15:15
for debugging purposes, added this to node/subsystem-util/src/lib.rs:472-476:

```rust
Some(registry) => Self::try_register(registry).map_err(|err| {
	eprintln!("PrometheusError calling {}::register: {:?}", std::any::type_name::<Self>(), err);
	err
}),
```

That pointed out where the registration was failing, which led to
this fix. The test still doesn't pass, but it now fails in a new
and different way!
detailed logging determined that using the `Box::new` style of
future generation, the `self.run` method was never being called,
leading to dropped receivers / closed senders for those subsystems,
causing the overseer to shut down immediately.

This is not the final fix needed to get things working properly,
but it's a good start.
Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

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

Looks fine to me

@coriolinus
Copy link
Contributor Author

Note: polkadod_proposer_block_constructed_count (grafana) has not reported any values since this branch was put onto that node. My expectation was that it would continue to produce blocks, but empty ones.

@coriolinus
Copy link
Contributor Author

Meanwhile, looking at the logs (kibana), it looks like the node is up and running, but it's a bit hard to read because it looks like it's hard-emitting terminal color codes which Kibana isn't stripping.

Comment on lines 23 to 24
#[cfg(feature = "full-node")]
use std::str::FromStr;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[cfg(feature = "full-node")]
use std::str::FromStr;

This makes no sense... The browser node will not have the full-node feature activated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes from f3fd10f; @montekki showed that it reduced warnings when building the browser target.

cli/src/cli.rs Outdated
Comment on lines 108 to 113

/// Set when this node is a collator.
///
/// When enabled, enables authority discovery.
#[structopt(long = "collator")]
pub is_collator: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Set when this node is a collator.
///
/// When enabled, enables authority discovery.
#[structopt(long = "collator")]
pub is_collator: bool,

We don't need a cli flag for this. How should this even work? What would do the collating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -19,6 +19,8 @@ polkadot-node-subsystem-util = { path = "../../subsystem-util" }
polkadot-overseer = { path = "../../overseer" }
polkadot-primitives = { path = "../../../primitives" }

sc-service = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sc-service = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
sc-service = { git = "https://github.com/paritytech/substrate", branch = "master" }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That comes from a4642e1 which fixes this CI failure.

Comment on lines 972 to 974
.map(|r| if let Err(e) = r {
log::error!(target: "availabilitystore", "Subsystem exited with an error {:?}", e);
}).boxed();
Copy link
Member

Choose a reason for hiding this comment

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

Weird formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -740,30 +851,35 @@ pub fn build_light(config: Configuration) -> Result<(TaskManager, RpcHandlers),
pub fn build_full(
config: Configuration,
authority_discovery_disabled: bool,
is_collator: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Please make this an enum, as I have described it:

enum IsCollator {
    Yes,
    No,
}

Comment on lines +571 to +575
let forward = forward.fuse();
let overseer_fut = overseer.run().fuse();

pin_mut!(overseer_fut);
pin_mut!(forward);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let forward = forward.fuse();
let overseer_fut = overseer.run().fuse();
pin_mut!(overseer_fut);
pin_mut!(forward);
let mut forward = forward.fuse();
let mut overseer_fut = overseer.run().fuse();

Should do the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope:

error[E0277]: `from_generator::GenFuture<[static generator@forward_events::{closure#0} for<'r, 's, 't0, 't1, 't2, 't3, 't4, 't5, 't6, 't7, 't8, 't9, 't10> {ResumeTy, Arc<sc_service::client::Client<sc_client_db::Backend<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>, LocalCallExecutor<sc_client_db::Backend<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>, NativeExecutor<Executor>>, sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>, RuntimeApi>>, OverseerHandler, sp_utils::mpsc::inner::TracingUnboundedReceiver<FinalityNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>, sp_utils::mpsc::inner::TracingUnboundedReceiver<BlockImportNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>, Next<'r, sp_utils::mpsc::inner::TracingUnboundedReceiver<FinalityNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>>, Next<'s, sp_utils::mpsc::inner::TracingUnboundedReceiver<BlockImportNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>>, [closure@forward_events::{closure#0}::{closure#0}], futures::future::PollFn<[closure@forward_events::{closure#0}::{closure#0}]>, (), forward_events::{closure#0}::__PrivResult<std::option::Option<FinalityNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>, std::option::Option<BlockImportNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>>, std::option::Option<FinalityNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>, FinalityNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>, &'t8 mut OverseerHandler, BlockInfo, impl futures::Future, std::option::Option<BlockImportNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>, BlockImportNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>, impl futures::Future}]>` cannot be unpinned
   --> node/service/src/lib.rs:591:5
    |
591 |                   select! {
    |  _________________^
592 | |                     _ = forward => break,
593 | |                     _ = overseer_fut => break,
594 | |                     complete => break,
595 | |                 }
    | |_________________^ within `futures_util::future::future::fuse::_::__Fuse<'_, impl futures::Future>`, the trait `Unpin` is not implemented for `from_generator::GenFuture<[static generator@forward_events::{closure#0} for<'r, 's, 't0, 't1, 't2, 't3, 't4, 't5, 't6, 't7, 't8, 't9, 't10> {ResumeTy, Arc<sc_service::client::Client<sc_client_db::Backend<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>, LocalCallExecutor<sc_client_db::Backend<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>, NativeExecutor<Executor>>, sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>, RuntimeApi>>, OverseerHandler, sp_utils::mpsc::inner::TracingUnboundedReceiver<FinalityNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>, sp_utils::mpsc::inner::TracingUnboundedReceiver<BlockImportNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>, Next<'r, sp_utils::mpsc::inner::TracingUnboundedReceiver<FinalityNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>>, Next<'s, sp_utils::mpsc::inner::TracingUnboundedReceiver<BlockImportNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>>, [closure@forward_events::{closure#0}::{closure#0}], futures::future::PollFn<[closure@forward_events::{closure#0}::{closure#0}]>, (), forward_events::{closure#0}::__PrivResult<std::option::Option<FinalityNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>, std::option::Option<BlockImportNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>>, std::option::Option<FinalityNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>, FinalityNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>, &'t8 mut OverseerHandler, BlockInfo, impl futures::Future, std::option::Option<BlockImportNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>>, BlockImportNotification<sp_runtime::generic::Block<sp_runtime::generic::Header<u32, BlakeTwo256>, OpaqueExtrinsic>>, impl futures::Future}]>`
    | 
   ::: /home/coriolinus/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/futures-util-0.3.5/src/async_await/mod.rs:43:24
    |
43  |   pub fn assert_unpin<T: Unpin>(_: &T) {}
    |                          ----- required by this bound in `futures_util::async_await::assert_unpin`
    |
    = note: required because it appears within the type `impl futures::Future`
    = note: required because it appears within the type `impl futures::Future`
    = note: required because it appears within the type `std::option::Option<impl futures::Future>`
    = note: required because it appears within the type `futures_util::future::future::fuse::_::__Fuse<'_, impl futures::Future>`
    = note: required because of the requirements on the impl of `Unpin` for `futures::future::Fuse<impl futures::Future>`
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: `from_generator::GenFuture<[static generator@Overseer::<S>::run::{closure#0} for<'r, 's, 't0, 't1, 't2, 't3, 't4, 't5, 't6, 't7, 't8, 't9, 't10, 't11, 't12, 't13, 't14, 't15, 't16, 't17, 't18, 't19, 't20, 't21> {ResumeTy, Overseer<SpawnTaskHandle>, Vec<(H256, u32)>, ActiveLeavesUpdate, &'r mut Overseer<SpawnTaskHandle>, OverseerSignal, impl futures::Future, (), &'t0 mut futures::channel::mpsc::Receiver<polkadot_overseer::Event>, futures::channel::mpsc::Receiver<polkadot_overseer::Event>, Next<'t1, futures::channel::mpsc::Receiver<polkadot_overseer::Event>>, &'t2 mut Next<'t3, futures::channel::mpsc::Receiver<polkadot_overseer::Event>>, futures_util::async_await::poll::PollOnce<&'t4 mut Next<'t5, futures::channel::mpsc::Receiver<polkadot_overseer::Event>>>, Poll<std::option::Option<polkadot_overseer::Event>>, polkadot_overseer::Event, polkadot_node_subsystem::messages::AllMessages, impl futures::Future, impl futures::Future, BlockInfo, impl futures::Future, impl futures::Future, &'t9 mut streamunordered::StreamUnordered<futures::channel::mpsc::Receiver<polkadot_overseer::ToOverseer>>, streamunordered::StreamUnordered<futures::channel::mpsc::Receiver<polkadot_overseer::ToOverseer>>, Next<'t10, streamunordered::StreamUnordered<futures::channel::mpsc::Receiver<polkadot_overseer::ToOverseer>>>, &'t11 mut Next<'t12, streamunordered::StreamUnordered<futures::channel::mpsc::Receiver<polkadot_overseer::ToOverseer>>>, futures_util::async_await::poll::PollOnce<&'t13 mut Next<'t14, streamunordered::StreamUnordered<futures::channel::mpsc::Receiver<polkadot_overseer::ToOverseer>>>>, Poll<std::option::Option<(streamunordered::StreamYield<futures::channel::mpsc::Receiver<polkadot_overseer::ToOverseer>>, usize)>>, polkadot_overseer::ToOverseer, &'t15 mut FuturesUnordered<Pin<Box<(dyn futures::Future<Output = sc_service::Result<(), SubsystemError>> + std::marker::Send + 't16)>>>, FuturesUnordered<Pin<Box<(dyn futures::Future<Output = sc_service::Result<(), SubsystemError>> + std::marker::Send + 't17)>>>, Next<'t18, FuturesUnordered<Pin<Box<(dyn futures::Future<Output = sc_service::Result<(), SubsystemError>> + std::marker::Send + 't19)>>>>, futures_util::async_await::poll::PollOnce<Next<'t20, FuturesUnordered<Pin<Box<(dyn futures::Future<Output = sc_service::Result<(), SubsystemError>> + std::marker::Send + 't21)>>>>>, Poll<std::option::Option<sc_service::Result<(), SubsystemError>>>, sc_service::Result<(), SubsystemError>, futures_util::async_await::pending::PendingOnce}]>` cannot be unpinned
   --> node/service/src/lib.rs:591:5
    |
591 |                   select! {
    |  _________________^
592 | |                     _ = forward => break,
593 | |                     _ = overseer_fut => break,
594 | |                     complete => break,
595 | |                 }
    | |_________________^ within `futures_util::future::future::fuse::_::__Fuse<'_, impl futures::Future>`, the trait `Unpin` is not implemented for `from_generator::GenFuture<[static generator@Overseer::<S>::run::{closure#0} for<'r, 's, 't0, 't1, 't2, 't3, 't4, 't5, 't6, 't7, 't8, 't9, 't10, 't11, 't12, 't13, 't14, 't15, 't16, 't17, 't18, 't19, 't20, 't21> {ResumeTy, Overseer<SpawnTaskHandle>, Vec<(H256, u32)>, ActiveLeavesUpdate, &'r mut Overseer<SpawnTaskHandle>, OverseerSignal, impl futures::Future, (), &'t0 mut futures::channel::mpsc::Receiver<polkadot_overseer::Event>, futures::channel::mpsc::Receiver<polkadot_overseer::Event>, Next<'t1, futures::channel::mpsc::Receiver<polkadot_overseer::Event>>, &'t2 mut Next<'t3, futures::channel::mpsc::Receiver<polkadot_overseer::Event>>, futures_util::async_await::poll::PollOnce<&'t4 mut Next<'t5, futures::channel::mpsc::Receiver<polkadot_overseer::Event>>>, Poll<std::option::Option<polkadot_overseer::Event>>, polkadot_overseer::Event, polkadot_node_subsystem::messages::AllMessages, impl futures::Future, impl futures::Future, BlockInfo, impl futures::Future, impl futures::Future, &'t9 mut streamunordered::StreamUnordered<futures::channel::mpsc::Receiver<polkadot_overseer::ToOverseer>>, streamunordered::StreamUnordered<futures::channel::mpsc::Receiver<polkadot_overseer::ToOverseer>>, Next<'t10, streamunordered::StreamUnordered<futures::channel::mpsc::Receiver<polkadot_overseer::ToOverseer>>>, &'t11 mut Next<'t12, streamunordered::StreamUnordered<futures::channel::mpsc::Receiver<polkadot_overseer::ToOverseer>>>, futures_util::async_await::poll::PollOnce<&'t13 mut Next<'t14, streamunordered::StreamUnordered<futures::channel::mpsc::Receiver<polkadot_overseer::ToOverseer>>>>, Poll<std::option::Option<(streamunordered::StreamYield<futures::channel::mpsc::Receiver<polkadot_overseer::ToOverseer>>, usize)>>, polkadot_overseer::ToOverseer, &'t15 mut FuturesUnordered<Pin<Box<(dyn futures::Future<Output = sc_service::Result<(), SubsystemError>> + std::marker::Send + 't16)>>>, FuturesUnordered<Pin<Box<(dyn futures::Future<Output = sc_service::Result<(), SubsystemError>> + std::marker::Send + 't17)>>>, Next<'t18, FuturesUnordered<Pin<Box<(dyn futures::Future<Output = sc_service::Result<(), SubsystemError>> + std::marker::Send + 't19)>>>>, futures_util::async_await::poll::PollOnce<Next<'t20, FuturesUnordered<Pin<Box<(dyn futures::Future<Output = sc_service::Result<(), SubsystemError>> + std::marker::Send + 't21)>>>>>, Poll<std::option::Option<sc_service::Result<(), SubsystemError>>>, sc_service::Result<(), SubsystemError>, futures_util::async_await::pending::PendingOnce}]>`
    | 
   ::: /home/coriolinus/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/futures-util-0.3.5/src/async_await/mod.rs:43:24
    |
43  |   pub fn assert_unpin<T: Unpin>(_: &T) {}
    |                          ----- required by this bound in `futures_util::async_await::assert_unpin`
    |
    = note: required because it appears within the type `impl futures::Future`
    = note: required because it appears within the type `impl futures::Future`
    = note: required because it appears within the type `std::option::Option<impl futures::Future>`
    = note: required because it appears within the type `futures_util::future::future::fuse::_::__Fuse<'_, impl futures::Future>`
    = note: required because of the requirements on the impl of `Unpin` for `futures::future::Fuse<impl futures::Future>`
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

pin_mut!(overseer_fut);
pin_mut!(forward);

loop {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we loop here if we end anyway the first time any future resolves?

This loop can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@coriolinus
Copy link
Contributor Author

Block production: it appears that the burnin node is in fact producing blocks. We have not broken block production; we have broken metrics collection related to block production.

@coriolinus
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Oct 28, 2020

Waiting for commit status.

@ghost ghost merged commit 798f781 into master Oct 28, 2020
@ghost ghost deleted the prgn-real-overseer branch October 28, 2020 10:26
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants