Rewrite the BasiQueue using channels by gterzian · Pull Request #1327 · paritytech/substrate

gterzian · 2018-12-24T04:54:55Z

Fix #1325
Fix #1482

parity-cla-bot · 2018-12-24T04:54:58Z

It looks like @gterzian signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

gterzian · 2018-12-24T05:00:34Z

This is currently written on top of #1282, which makes this PR look bigger than it is...

The main change is https://github.com/paritytech/substrate/pull/1327/files#diff-fdedd07ae8ae881a0907be42dd80a840

gterzian · 2018-12-28T03:48:27Z

This is, I think, currently blocked on #1326 the locks in the Protocol and ChainSync code)

gterzian · 2019-02-14T10:57:59Z

Ok I fixed an error introduced in the rebase, and tested on charred-cherry(which seems to work on master again), which seemed to work fine...

andresilva

Just some minor grumbles.

andresilva · 2019-02-14T09:54:44Z

+
+	fn handle_import_justification(&self, who: Origin, hash: B::Hash, number: NumberFor<B>, justification: Justification) {
+		let success = self.justification_import.as_ref().map(|justification_import| {
+			justification_import.import_justification(hash, number, justification).is_ok()


Let's log this error here under debug.

andresilva · 2019-02-15T12:09:46Z


 /// The Aura import queue type.
-pub type AuraImportQueue<B, C, E> = BasicQueue<B, AuraVerifier<C, E>>;
+pub type AuraImportQueue<B> = BasicQueue<B>;


Maybe we can remove this type, since the BasicQueue type is now an opaque interface that will be used by everyone (regardless of underlying verifier).

The problem with this is that the AuraImportQueue is exported and used outside of the crate, while BasicQueue is private(and probably should be). Perhaps AuraImportQueue can be replaced by a ImportQueue, however I think that's going to require boxing it 😄 , and will also require some additional work since I would be very surprised if it didn't require changing some of the code using it. Perhaps we can look into this as a follow-up?

Didn't think about that. Sure that's reasonable. 👍

andresilva · 2019-02-15T12:14:08Z

+		let _ = self
+			.sender
+			.send(BlockImportMsg::Start(link))
+			.expect("1. self is holding a sender to the Importer, 2. Importer should handle messages while there are senders around");


We usually write these proofs in the form:
"self is holding a sender to the Importer; Importer should handle messages while there are senders around; qed"

andresilva · 2019-02-15T12:16:10Z

+	ImportBlocks(BlockOrigin, Vec<IncomingBlock<B>>),
+	Imported(
+		Vec<(
+			Result<BlockImportResult<<<B as BlockT>::Header as HeaderT>::Number>, BlockImportError>,


Suggested change

Result<BlockImportResult<<<B as BlockT>::Header as HeaderT>::Number>, BlockImportError>,

Result<BlockImportResult<NumberFor<Block>>, BlockImportError>,

andresilva · 2019-02-15T12:17:53Z

-impl<B: BlockT> AsyncImportQueueData<B> {
-	/// Instantiate a new async import queue data.
-	pub fn new() -> Self {
+impl<B: BlockT> BasicQueue<B> {


Can you add a general description here about how the queue works? e.g. how messages are dispatched to a worker, and blocks are imported on a different worker.

andresilva · 2019-02-15T12:22:25Z

+				Some(link) => link,
+				None => {
+					trace!(target:"sync", "Received import result for {} while import-queue has no link", hash);
+					return true


Suggested change

return true

return true;

andresilva · 2019-02-15T12:26:06Z

+		let mut imported = 0;
+
+		let blocks_range = match (
+			blocks


This formatting is weird, it should be fine the keep the expression on one line like:
blocks.first().and_then(|b| b.header.as_ref().map(|h| h.number())),

andresilva · 2019-02-15T12:30:59Z

+			let was_ok = import_result.is_ok();
+			results.push((import_result, block.hash));
+			if was_ok {
+				imported = imported + 1;


Suggested change

imported = imported + 1;

imported += 1;

andresilva · 2019-02-15T12:33:41Z

 	/// Tell protocol to request justification for a block.
 	RequestJustification(B::Hash, NumberFor<B>),
+	/// Inform protocol whether a justification was successfully imported.
+	JustificationImportResut(B::Hash, NumberFor<B>, bool),


Suggested change

JustificationImportResut(B::Hash, NumberFor<B>, bool),

JustificationImportResult(B::Hash, NumberFor<B>, bool),

andresilva · 2019-02-15T12:40:15Z

 	let mut net = TestNet::new(2);
 	net.peer(1).push_blocks(500, false);
 	net.sync();
-	// Wait for peer 0 to import blocks received over the network.


Why were these removed? (I'm not familiar with this test in particular)

I think it's a result of this change: https://github.com/paritytech/substrate/pull/1327/files/8113ac554afb9ed430b565e4569d9bc695c06a55#diff-d19df74b5b495aae3892f8f92a5f174aR213

It appears unnecessary to have the extra wait in the test...

andresilva · 2019-02-15T16:22:05Z

 			justification_import.import_justification(hash, number, justification).is_ok()
 		}).unwrap_or(false);
+		if !success {
+			debug!("Justification import failed for hash: {:?} number: {:?} coming from node: {:?}", hash, number, who);


Can we move this to inside the map above so we can log the error itself?

* use channels to implement basic import queue * async justification import * better conditional for is_done in tests * reword the test for presence of link * fix conditional * trace instead of panic when no link present * reword expectations when sending to importers * fix * debug justification import error * update expectations * use NumberFor * nits * add general description * move error handling into closure

networking: Pause piece provider when not connected to DSN.

gterzian added the A3-in_progress Pull request is in progress. No review needed at this stage. label Dec 24, 2018

gterzian force-pushed the try_using_channels_2 branch 5 times, most recently from c1b8903 to 6095ffb Compare December 26, 2018 10:50

gterzian mentioned this pull request Dec 31, 2018

Rewrite network protocol/service to use channels #1340

Merged

gterzian force-pushed the try_using_channels_2 branch 2 times, most recently from b6d3dac to 979ce9d Compare January 2, 2019 11:47

gterzian force-pushed the try_using_channels_2 branch 6 times, most recently from 33e94d6 to 34ae605 Compare January 12, 2019 14:25

gterzian mentioned this pull request Jan 21, 2019

Move justification import to the import queue #1482

Closed

gterzian force-pushed the try_using_channels_2 branch 7 times, most recently from 81e058f to 6f897a9 Compare January 24, 2019 12:14

gterzian changed the title ~~[WIP] Rewrite the BasiQueue using channels~~ Rewrite the BasiQueue using channels Jan 24, 2019

gterzian force-pushed the try_using_channels_2 branch from 6f897a9 to b4c4f56 Compare January 24, 2019 12:35

gterzian force-pushed the try_using_channels_2 branch 2 times, most recently from c05df23 to 17d10a5 Compare February 6, 2019 14:10

andresilva suggested changes Feb 15, 2019

View reviewed changes

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

andresilva reviewed Feb 15, 2019

View reviewed changes

gterzian added 13 commits February 16, 2019 00:37

use channels to implement basic import queue

36682fb

async justification import

b1025ad

better conditional for is_done in tests

95b7230

reword the test for presence of link

dd3ddd8

fix conditional

c711292

trace instead of panic when no link present

161eb2d

reword expectations when sending to importers

d0571db

fix

accc3ca

debug justification import error

f37b471

update expectations

625b497

use NumberFor

4174f15

nits

8468806

add general description

dfd2e22

gterzian force-pushed the try_using_channels_2 branch from eefe1ee to dfd2e22 Compare February 15, 2019 16:38

move error handling into closure

00f47b8

gterzian removed the A3-needsresolving label Feb 15, 2019

andresilva approved these changes Feb 15, 2019

View reviewed changes

andresilva added A8-looksgood and removed A5-grumble labels Feb 15, 2019

gavofyork merged commit ed2faf4 into paritytech:master Feb 17, 2019

gterzian deleted the try_using_channels_2 branch February 18, 2019 04:15

gterzian mentioned this pull request Feb 18, 2019

Parallelize block import from verification #1816

Closed

liuchengxu pushed a commit to liuchengxu/substrate that referenced this pull request May 31, 2023

Pause piece provider when not connected to DSN. (paritytech#1327)

06d6fa2

networking: Pause piece provider when not connected to DSN.

	Result<BlockImportResult<<<B as BlockT>::Header as HeaderT>::Number>, BlockImportError>,
	Result<BlockImportResult<NumberFor<Block>>, BlockImportError>,

	JustificationImportResut(B::Hash, NumberFor<B>, bool),
	JustificationImportResult(B::Hash, NumberFor<B>, bool),

Uh oh!

Conversation

gterzian commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parity-cla-bot commented Dec 24, 2018

Uh oh!

gterzian commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gterzian commented Dec 28, 2018

Uh oh!

gterzian commented Feb 14, 2019

Uh oh!

andresilva 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gterzian commented Dec 24, 2018 •

edited

Loading

gterzian commented Dec 24, 2018 •

edited

Loading