Remove blocking sync -> import-queue operations #1818

gterzian · 2019-02-19T05:05:18Z

gterzian · 2019-02-19T05:11:02Z

One of the issue I am running into with the related update to the testnet code, is that I've ended up adding the NetworkSpecialization as a type on TestNetFactory(required because the NetworkLink is now actually stored on a Peer), and the compiler was complaining about the default add_peer implementation in the following way:

   --> core/network/src/test/mod.rs:529:15
    |
529 |             peers.push(peer.clone())
    |                        ^^^^^^^^^^^^ expected associated type, found struct `test::DummySpecialization`
    |
    = note: expected type `std::sync::Arc<test::Peer<_, <Self as test::TestNetFactory>::Specialization>>`
               found type `std::sync::Arc<test::Peer<_, test::DummySpecialization>>`

so I've tried moving the default add_peer method onto TestNet, and then having for example the GrandpaTestnet call self.test_net.add_peer, yet that bring other problems because the Peer type is different between TestNet and GrandpaTestnet.

EDIT: solved, see f0dece3

gterzian · 2019-02-19T12:43:26Z

@andresilva It seems that the sync_justifications is failing, and I can't tell why. I think you wrote it, any ideas what could have caused this? From debugging it appears that the justification import flow works, yet the actual import fails with an error...

Edit: it was me for hadn't applied the change I did to the other testnets, see 5754c89

core/network/src/sync.rs

add specialization to testnet remove add peer default impl on TestNetFactory

rphmeier · 2019-02-23T20:38:29Z

Does this also address #1787 ?

rphmeier · 2019-02-23T20:38:43Z

core/consensus/aura/src/lib.rs

+			let client = Arc::new(test_client::new());
+			let verifier = self.make_verifier(client.clone(), config);
+			let (block_import, justification_import, data) = self.make_block_import(client.clone());
+			let specialization = DummySpecialization {};


braces aren't necessary here, are they? (braced empty struct is considered bad style usually and should only be used in auto-generated code)

I'm getting the below when I remove then:

error[E0423]: expected value, found struct `DummySpecialization` --> core/network/src/test/mod.rs:729:24 | 729 | let specialization = DummySpecialization; | ^^^^^^^^^^^^^^^^^^^ did you mean `DummySpecialization { /* fields */ }`?

probably the error traces back to the struct definition then and we could address it there.

You're right, the empty brackets had to be removed from the struct definition.

rphmeier · 2019-02-23T20:43:08Z

core/consensus/common/src/import_queue.rs

@@ -389,28 +333,11 @@ impl<B: BlockT> BlockImporter<B> {
 			};
 		}
 		if let Some(link) = self.link.as_ref() {
-			link.maintain_sync();
+			link.blocks_processed(hashes, has_error);


fn blocks_processed doesn't maintain sync anymore. Where is that call done?

The call is made at https://github.com/paritytech/substrate/pull/1818/files#diff-9c1b34b73c6680d46fc07cae4591547fR347 as part of handling blocks_processed. I removed the maintain_sync method on Link and replaced it with blocks_processed, which still does the "maintaining sync" as part of it...

gterzian · 2019-02-25T04:20:50Z

Does this also address #1787 ?

@rphmeier yes it does, adding a FIX #1787

bkchr · 2019-02-25T07:58:03Z

core/consensus/aura/src/lib.rs

+			let (block_import, justification_import, data) = self.make_block_import(client.clone());
+			let specialization = DummySpecialization;
+			let peer = create_peer(client, block_import, justification_import, data, verifier, specialization, config);
+			self.mut_peers(|peers| {


Why do you need to clone the peer? And self.peers.push(peer); is much shorter.

Why do you need to clone the peer?

error[E0507]: cannot move out of captured variable in an `Fn` closure --> core/network/src/test/mod.rs:803:15 | 801 | let peer = create_peer(client, block_import, justification_import, data, verifier, specialization, config); | ---- captured outer variable 802 | self.mut_peers(|peers| { 803 | peers.push(peer) | ^^^^ cannot move out of captured variable in an `Fn` closure

self.peers.push(peer); is much shorter.

The self.mut_peers allows composition of testnets, for example the JustificationTestnet below doesn't have self.peers, yet it owns a TestNet and it can access the peers of it via self.mut_peers.

Here we could probably do self.peers since its the TestNet implementation itself..

Then mut_peers should take FnOnce.

core/consensus/common/src/import_queue.rs

bkchr · 2019-02-25T08:02:53Z

core/finality-grandpa/src/tests.rs

@@ -114,6 +113,17 @@ impl TestNetFactory for GrandpaTestNet {
 		(shared_import.clone(), Some(shared_import), Mutex::new(Some(link)))
 	}

+	fn add_peer(&mut self, config: &ProtocolConfig) {


Code duplication. If this is the same implementation, can we not just move this as default implementation into the trait definition?

Since I added NetworkSpecialization as a type on the TestNetFactory, we cannot have a default add_peer method on the factory because it contains let specialization = DummySpecialization;, which requires an implementation of the TesnetFactory trait to specificy type Specialization = DummySpecialization;.

I did remove as much duplication as possible by adding the create_peer function, which is used in each implementation.

Introduce a trait

trait Specialization { fn create() -> Self; }

And then Self::Specialization::create().

I'm not convinced an additional trait is better than some duplication. I also expect most testnet implementations will have their own setup for peer creation and this duplication will prove a non-issue.

You already repeated the same code 4 times.

Sounds better than what I proposed! Nice @andresilva 👍

Ok, you're right the proposed trait is a better solution. It's not that complicated.

You can remove this implementation now :)

@andresilva just seeing your comment, feel free to push your version if it is better than what I just committed...

Nope, yours is fine 👍

bkchr · 2019-02-25T08:06:08Z

core/network/src/sync.rs

+			.unwrap_or_else(|| Zero::zero());
+		self.queue_blocks
+			.extend(new_blocks.iter().map(|b| b.hash.clone()));
+		if new_best_importing_number > self.best_importing_number {


Could be self.best_importing_number = max(self.best_importing_number, new_best_importing_number);

For some reason, this results in a test failure:

---- test::sync::sync_after_fork_works stdout ---- thread 'test::sync::sync_after_fork_works' panicked at 'assertion failed: net.peer(0).client.backend().as_in_memory().blockchain().canon_equals_to(&peer1_chain)', core/network/src/test/sync.rs:149:2 note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

I've also seen this test fail so it's probably some non-determinism being introduced (not sure if by this PR or if it was already like that). Can you try re-running the test to see if it passes?

Yes it seems to fail quite often regardless of the syntax used. I've changed it to max(new_best_importing_number, self.best_importing_number); (note: "Returns the second argument if the comparison determines them to be equal." https://doc.rust-lang.org/std/cmp/fn.max.html)

bkchr · 2019-02-25T08:10:01Z

core/network/src/test/mod.rs

+	data:D,
+	verifier: Arc<V>,
+	specialization: S,
+	config: &ProtocolConfig)


Suggested change

config: &ProtocolConfig)

config: &ProtocolConfig

) -> Arc<Peer<D, S>> {

bkchr · 2019-02-25T08:10:12Z

core/network/src/test/mod.rs

+	verifier: Arc<V>,
+	specialization: S,
+	config: &ProtocolConfig)
+	-> Arc<Peer<D, S>> {


Suggested change

-> Arc<Peer<D, S>> {

bkchr · 2019-02-25T08:10:35Z

core/network/src/test/mod.rs

+		None,
+		tx_pool,
+		specialization,
+	).unwrap();


Please use expect

bkchr · 2019-02-25T08:11:02Z

core/network/src/test/mod.rs

@@ -655,21 +722,32 @@ impl TestNetFactory for TestNet {
 		}
 	}

+	fn add_peer(&mut self, config: &ProtocolConfig) {
+		let client = Arc::new(test_client::new());


Again code duplication.

bkchr · 2019-02-25T08:11:31Z

core/network/src/test/mod.rs

@@ -714,15 +793,26 @@ impl TestNetFactory for JustificationTestNet {
 		self.0.make_verifier(client, config)
 	}

-	fn peer(&self, i: usize) -> &Peer<Self::PeerData> {
+	fn add_peer(&mut self, config: &ProtocolConfig) {
+		let client = Arc::new(test_client::new());


andresilva

LGTM. Just a minor nit.

andresilva · 2019-02-25T10:44:56Z

core/network/src/protocol.rs

@@ -210,8 +210,8 @@ pub enum ProtocolMsg<B: BlockT, S: NetworkSpecialization<B>,> {
 	Peers(Sender<Vec<(NodeIndex, PeerInfo<B>)>>),
 	/// Let protocol know a peer is currenlty clogged.
 	PeerClogged(NodeIndex, Option<Message<B>>),
-	/// Tell protocol to maintain sync.
-	MaintainSync,
+	/// A batch of blocks have been processed, with or without errors.


Suggested change

/// A batch of blocks have been processed, with or without errors.

/// A batch of blocks has been processed, with or without errors.

andresilva · 2019-02-25T10:47:35Z

core/network/src/sync.rs

+			.unwrap_or_else(|| Zero::zero());
+		self.queue_blocks
+			.extend(new_blocks.iter().map(|b| b.hash.clone()));
+		if new_best_importing_number > self.best_importing_number {


I've also seen this test fail so it's probably some non-determinism being introduced (not sure if by this PR or if it was already like that). Can you try re-running the test to see if it passes?

andresilva · 2019-02-25T10:50:06Z

core/finality-grandpa/src/tests.rs

@@ -114,6 +113,17 @@ impl TestNetFactory for GrandpaTestNet {
 		(shared_import.clone(), Some(shared_import), Mutex::new(Some(link)))
 	}

+	fn add_peer(&mut self, config: &ProtocolConfig) {


You can remove this implementation now :)

andresilva · 2019-02-25T10:51:00Z

core/finality-grandpa/src/tests.rs

@@ -114,6 +113,17 @@ impl TestNetFactory for GrandpaTestNet {
 		(shared_import.clone(), Some(shared_import), Mutex::new(Some(link)))
 	}

+	fn add_peer(&mut self, config: &ProtocolConfig) {


Nope, yours is fine 👍

…t_queue_blocks

bkchr

Please fix the merge conflict and then we are good to merge this.

…t_queue_blocks

Restore old behavior of not sleeping in a test.

* remove blocking sync -> import-queue operations add specialization to testnet remove add peer default impl on TestNetFactory * remove empty brackets from dummy specialization * nits * make mut_peers take an fn once * add SpecializationFactory trait in test * remove add_peer imple in grandpa, fix typo * use cmp::max for best importing number comparison * remove import of non-existent create_peer * add sender to start message

gterzian added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 19, 2019

gterzian force-pushed the remove_sync_import_queue_blocks branch from 8fc7ce7 to 728b5cd Compare February 19, 2019 05:05

gterzian changed the title ~~remove blocking sync -> import-queue operations~~ Remove blocking sync -> import-queue operations Feb 19, 2019

gterzian 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 Feb 19, 2019

gterzian requested a review from andresilva February 19, 2019 06:23

gnunicorn added the A3-needsresolving label Feb 20, 2019

gterzian commented Feb 21, 2019

View reviewed changes

core/network/src/sync.rs Outdated Show resolved Hide resolved

gterzian force-pushed the remove_sync_import_queue_blocks branch 2 times, most recently from ae932eb to 978b446 Compare February 21, 2019 14:03

gterzian removed the A3-needsresolving label Feb 21, 2019

remove blocking sync -> import-queue operations

543d75c

add specialization to testnet remove add peer default impl on TestNetFactory

gterzian force-pushed the remove_sync_import_queue_blocks branch from 978b446 to 543d75c Compare February 21, 2019 14:11

rphmeier reviewed Feb 23, 2019

View reviewed changes

rphmeier added A5-grumble and removed A0-please_review Pull request needs code review. labels Feb 23, 2019

remove empty brackets from dummy specialization

d0d209f

bkchr reviewed Feb 25, 2019

View reviewed changes

gterzian added 3 commits February 25, 2019 17:48

nits

8f1dd30

make mut_peers take an fn once

53f4662

add SpecializationFactory trait in test

4ed640d

andresilva suggested changes Feb 25, 2019

View reviewed changes

gterzian added 3 commits February 25, 2019 18:59

remove add_peer imple in grandpa, fix typo

f15cc0e

use cmp::max for best importing number comparison

d981b79

remove import of non-existent create_peer

849a852

Merge remote-tracking branch 'upstream/master' into remove_sync_impor…

ec63452

…t_queue_blocks

andresilva approved these changes Feb 26, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into remove_sync_impor…

c9b6c84

…t_queue_blocks

bkchr previously approved these changes Feb 27, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into remove_sync_impor…

0a742a4

…t_queue_blocks

add sender to start message

8037a59

bkchr approved these changes Feb 28, 2019

View reviewed changes

bkchr merged commit 6f4c995 into paritytech:master Feb 28, 2019

	config: &ProtocolConfig)
	config: &ProtocolConfig
	) -> Arc<Peer<D, S>> {

	/// A batch of blocks have been processed, with or without errors.
	/// A batch of blocks has been processed, with or without errors.

Remove blocking sync -> import-queue operations #1818

Remove blocking sync -> import-queue operations #1818

Conversation

gterzian commented Feb 19, 2019

gterzian commented Feb 19, 2019 • edited Loading

gterzian commented Feb 19, 2019 • edited Loading

rphmeier commented Feb 23, 2019

rphmeier Feb 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gterzian Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gterzian Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

gterzian commented Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

gterzian Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gterzian Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

gterzian commented Feb 19, 2019 •

edited

Loading

gterzian commented Feb 19, 2019 •

edited

Loading

rphmeier Feb 23, 2019 •

edited

Loading

gterzian Feb 25, 2019 •

edited

Loading

gterzian Feb 25, 2019 •

edited

Loading

gterzian commented Feb 25, 2019 •

edited

Loading

gterzian Feb 25, 2019 •

edited

Loading

gterzian Feb 25, 2019 •

edited

Loading