Bring substrate-demo up to date by arkpar · Pull Request #658 · paritytech/substrate

arkpar · 2018-09-04T14:29:38Z

Quite a bit of the consensus code is still copy-pasted from polkadot

Offline validator detection should be moved to substrate
Some shared code in runtime/checked_block.rs

Demo extrinsics still use Address instead of AccounId.

Currently it can sync krumme-lanke.json but I expect this to change as the network protocol grows to be incompatible. Protocol id should be different at least.

dvdplm · 2018-09-04T14:41:39Z

There's a toolchain problem it seems. :/

tomusdrw · 2018-09-04T14:55:03Z

demo/consensus/src/evaluation.rs

+			description("Proposal exceeded the maximum size."),
+			display(
+				"Proposal exceeded the maximum size of {} by {} bytes.",
+				MAX_TRANSACTIONS_SIZE, MAX_TRANSACTIONS_SIZE.saturating_sub(*size)


Should be size.saturating_sub(MAX_SIZE), since max size is exceeded.

rphmeier · 2018-09-04T15:09:39Z

demo/network/src/lib.rs

+
+//! Substrate Demo-specific network implementation.
+//!
+//! This manages gossip of consensus messages for BFT and for parachain statements,


there are no parachains in demo

rphmeier · 2018-09-04T15:09:50Z

demo/network/src/lib.rs

+use substrate_network::StatusMessage as GenericFullStatus;
+
+/// Demo protocol id.
+pub const PROTOCOL_ID: ::substrate_network::ProtocolId = *b"dot";


change this?

I've left it as is for now so that substrate-demo could sync to the Krumme Lanke network. I'll make it configurable through the spec file in the following PR.

pepyakin · 2018-09-04T14:42:09Z

demo/network/src/consensus.rs

+				},
+				Ok(Async::Ready(None)) => return Ok(Async::Ready(())),
+				Ok(Async::NotReady) => return Ok(Async::NotReady),
+				Err(e) => debug!(target: "demo-network", "Error getting consensus message: {:?}", e),


Should we propagate the error here? Otherwise it seems that it should lead to the busy loop

pepyakin · 2018-09-04T14:47:21Z

demo/network/src/lib.rs

+	/// Note new consensus session.
+	fn new_consensus(&mut self, hash: Hash) {
+		let old_consensus = self.live_consensus.take();
+		self.live_consensus = Some(hash);


can we rename hash → parent_hash here?

demo/network/src/consensus.rs

pepyakin · 2018-09-04T14:58:26Z

demo/runtime/wasm/build.sh

 set -e

-cargo +nightly build --target=wasm32-unknown-unknown --release
+cargo +nightly-2018-08-27 build --target=wasm32-unknown-unknown --release


I think this should have not been committed.

gnunicorn

Not really have enough knowledge about what the code is supposed to do, so its mostly style, dependency and comment comments

gnunicorn · 2018-09-06T09:59:08Z

demo/Cargo.toml

+error-chain = "0.12"
+demo-cli = { path = "cli" }
+futures = "0.1"
+ctrlc = { git = "https://github.com/paritytech/rust-ctrlc.git" }


Shouldn't we use the official crate rather than a two year old fork? Especially considering the only change added in the fork (SIGINT and SIGTERM support) is supported in the official crate for a while by now?

gnunicorn · 2018-09-06T09:59:46Z

demo/api/src/lib.rs

@@ -0,0 +1,155 @@
+// Copyright 2017 Parity Technologies (UK) Ltd.


gnunicorn · 2018-09-06T10:05:02Z

demo/consensus/src/error.rs

@@ -0,0 +1,55 @@
+// Copyright 2017 Parity Technologies (UK) Ltd.


gnunicorn · 2018-09-06T10:05:15Z

demo/consensus/src/evaluation.rs

@@ -0,0 +1,96 @@
+// Copyright 2017 Parity Technologies (UK) Ltd.
+// This file is part of Polkadot.


-Polkadot+Substrate Demo

gnunicorn · 2018-09-06T10:08:43Z

demo/consensus/src/lib.rs

@@ -0,0 +1,446 @@
+// Copyright 2017 Parity Technologies (UK) Ltd.


gnunicorn · 2018-09-06T10:16:39Z

demo/network/src/lib.rs

@@ -0,0 +1,117 @@
+// Copyright 2017 Parity Technologies (UK) Ltd.


gnunicorn · 2018-09-06T10:17:33Z

demo/runtime/src/checked_block.rs

@@ -0,0 +1,94 @@
+// Copyright 2017 Parity Technologies (UK) Ltd.


gnunicorn · 2018-09-06T10:18:25Z

demo/service/src/chain_spec.rs

@@ -0,0 +1,196 @@
+// Copyright 2017 Parity Technologies (UK) Ltd.


gnunicorn · 2018-09-06T10:19:14Z

demo/service/src/chain_spec.rs

+			minimum_deposit: 10,
+		}),
+		council: Some(CouncilConfig {
+			active_council: endowed_accounts.iter().filter(|a| initial_authorities.iter().find(|&b| a.0 == b.0).is_none()).map(|a| (a.clone(), 1000000)).collect(),


please wrap.

gnunicorn · 2018-09-06T10:19:29Z

demo/service/src/lib.rs

@@ -0,0 +1,214 @@
+// Copyright 2017 Parity Technologies (UK) Ltd.


gavofyork · 2018-09-07T17:51:29Z

demo/consensus/src/error.rs

+	}
+
+	errors {
+		InvalidDutyRosterLength(expected: usize, got: usize) {


don't think this is needed, is it?

gavofyork · 2018-09-07T17:52:16Z

demo/consensus/src/lib.rs

+			.expect("blocks are defined to serialize to substrate blocks correctly; qed");
+
+		assert!(evaluation::evaluate_initial(
+				&substrate_block,


gavofyork · 2018-09-07T17:52:48Z

demo/consensus/src/lib.rs

+				timestamp,
+				&self.parent_hash,
+				self.parent_number,
+				).is_ok());


two extra tabs

gavofyork · 2018-09-07T17:54:12Z

demo/network/src/consensus.rs

+	fn start_send(&mut self, message: bft::Communication<Block>)
+		-> ::futures::StartSend<bft::Communication<Block>, E>
+	{
+		let network_message = net::LocalizedBftMessage {


would be nice to keep anything bft-specific in the substrate library

gavofyork · 2018-09-07T17:55:10Z

demo/service/src/lib.rs

+
+		let consensus_net = ConsensusNetwork::new(service.network(), client.clone());
+		Some(consensus::Service::new(
+				client.clone(),


double indent unneeded

gavofyork · 2018-09-07T17:55:48Z

substrate/client/src/client.rs

+				"initialise_block",
+				&header.encode(),
+				execution_manager()
+				)?;


closing paren goes on opening paren's line

gavofyork · 2018-09-07T17:55:53Z

substrate/client/src/client.rs

+				function,
+				input,
+				execution_manager()
+				))?;


gavofyork · 2018-09-07T17:58:34Z

yey for the direction :)

would really like to shift all networking logic into substrate libraries with just an extension API that can be used should protocol additions be required like for polkadot.

i guess this can be done in subsequent PRs though.

rphmeier · 2018-09-07T22:48:12Z

We want something like a substrate-network-rhododendron crate for dealing with rhododendron networking messages etc. We will want other consensus protocols which have their own messages so it's best not to make the existing ones too first-class in substrate. consensus networking crates will need to be extensible enough to also support things like enabling polkadot sub-agreement messages to be sent via the same channels.

gavofyork · 2018-09-10T14:44:56Z

@arkpar needs resolving...

* Add optional block number in RPC Higher fee for registering node (paritytech#655) Support optional number for more RPC A few more Complete support for optional number in RPC Nits . Remove useless best_number() * Use BlockHash instead of BlockNumber in PRC * Nit

arkpar added 2 commits September 4, 2018 16:08

Updating substrate-demo

ab710bf

Consenus fixes

a5604d6

arkpar added the A0-please_review Pull request needs code review. label Sep 4, 2018

Reverted toolchain change

d5a0270

tomusdrw reviewed Sep 4, 2018

View reviewed changes

rphmeier reviewed Sep 4, 2018

View reviewed changes

pepyakin reviewed Sep 4, 2018

View reviewed changes

arkpar added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Sep 4, 2018

arkpar added 7 commits September 4, 2018 18:10

Merge branch 'master' of github.com:paritytech/substrate into a-demo

6a5ea57

Adjusted timeout formula

eab11ad

Simplfied proposal creation

04eb184

Merge branch 'master' of github.com:paritytech/substrate into a-demo

91041c3

Fixed tests

67107bf

Merge branch 'master' of github.com:paritytech/substrate into a-demo

8b7a8bb

Fixed a few small issues

f3e9299

arkpar added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 5, 2018

Merge branch 'master' of github.com:paritytech/substrate into a-demo

b3a96ad

gnunicorn reviewed Sep 6, 2018

View reviewed changes

arkpar added 6 commits September 6, 2018 12:25

2017->2018

dbf6d0c

Merge branch 'master' of github.com:paritytech/substrate into a-demo

3e61f36

Style

7d8ef2c

More style

7353474

Renamed demo executable to substrate

946e5e4

Merge branch 'master' of github.com:paritytech/substrate into a-demo

a02f2b7

gavofyork reviewed Sep 7, 2018

View reviewed changes

demo/consensus/src/lib.rs Outdated

timestamp,

&self.parent_hash,

self.parent_number,

).is_ok());

Copy link

Member

gavofyork Sep 7, 2018

Choose a reason for hiding this comment

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

two extra tabs

gavofyork reviewed Sep 7, 2018

View reviewed changes

substrate/client/src/client.rs Outdated

function,

input,

execution_manager()

))?;

Copy link

Member

gavofyork Sep 7, 2018

Choose a reason for hiding this comment

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

same

arkpar added 2 commits September 7, 2018 21:37

Merge branch 'master' of github.com:paritytech/substrate into a-demo

9464a68

Style

c17c8b4

arkpar added 4 commits September 10, 2018 10:18

Merge branch 'master' of github.com:paritytech/substrate into a-demo

a599454

Fixed compilation after merge

f882159

Merge branch 'master' of github.com:paritytech/substrate into a-demo

544bb89

Style

b3295d6

gavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. labels Sep 10, 2018

gavofyork approved these changes Sep 10, 2018

View reviewed changes

Merge branch 'master' of github.com:paritytech/substrate into a-demo

18d67cb

gavofyork merged commit 68137ae into master Sep 10, 2018

gavofyork deleted the a-demo branch September 10, 2018 15:54

		@@ -0,0 +1,155 @@
		// Copyright 2017 Parity Technologies (UK) Ltd.

		@@ -0,0 +1,55 @@
		// Copyright 2017 Parity Technologies (UK) Ltd.

		@@ -0,0 +1,96 @@
		// Copyright 2017 Parity Technologies (UK) Ltd.
		// This file is part of Polkadot.

		@@ -0,0 +1,446 @@
		// Copyright 2017 Parity Technologies (UK) Ltd.

		@@ -0,0 +1,117 @@
		// Copyright 2017 Parity Technologies (UK) Ltd.

		@@ -0,0 +1,94 @@
		// Copyright 2017 Parity Technologies (UK) Ltd.

		@@ -0,0 +1,196 @@
		// Copyright 2017 Parity Technologies (UK) Ltd.

		@@ -0,0 +1,214 @@
		// Copyright 2017 Parity Technologies (UK) Ltd.

Conversation

arkpar commented Sep 4, 2018

Uh oh!

dvdplm commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnunicorn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavofyork commented Sep 7, 2018

Uh oh!

rphmeier commented Sep 7, 2018

Uh oh!

gavofyork commented Sep 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants