Ensure that table router is always built #952

bkchr · 2020-03-30T22:16:20Z

This pr ensures that the table router is always build, aka the future is
resolved. This is important, as the table router internally spawns tasks
to handle gossip messages. Handling gossip messages is not only required
on parachain validators, but also on relay chain validators to receive collations.

Tests are added to ensure that the assumptions hold.

Requires: paritytech/substrate#5448

This pr ensures that the table router is always build, aka the future is resolved. This is important, as the table router internally spawns tasks to handle gossip messages. Handling gossip messages is not only required on parachain validators, but also on relay chain validators to receive collations. Tests are added to ensure that the assumptions hold.

rphmeier · 2020-03-31T01:12:59Z

validation/src/validation_service/mod.rs

@@ -236,6 +232,57 @@ impl<C, N, P, SC, SP> ServiceBuilder<C, N, P, SC, SP> where
 	}
 }

+/// Abstraction over `collation_fetch`.
+pub trait CollationFetch {


CollationFetch is already a wrapper around the Collators abstraction. I say we only need one or the other

This doesn't need to be pub, does it? The user of the trait is pub(crate) only

rphmeier · 2020-03-31T01:13:55Z

validation/src/validation_service/mod.rs

-		let availability_store = self.availability_store.clone();
+// launch parachain work asynchronously.
+async fn launch_work<Client>(
+	collation_fetch: impl CollationFetch,


this function has way too many parameters. what about refactoring it to a struct *Params?

rphmeier · 2020-03-31T01:27:45Z

Was the table router not always built before? launch_work was invoked before and was spawning a future that drove build_router to completion

bkchr · 2020-03-31T07:59:19Z

Launch work was only called when the duty of the local validator is some parachain. However we need to build the router also for relay chain validators. Otherwise we don't spawn the task that handles gossip messages and thus the relay chain validator doesn't receive the candidate message.

(I debugged why parachains don't work anymore and found multiple bugs. This one, the other pr about updating the parachain header and I will need to open another one to fix the last issue)

…n-service

rphmeier · 2020-03-31T15:11:28Z

validation/src/validation_service/mod.rs

+	Client::Api: ParachainHost<Block, Error = sp_blockchain::Error>,
+{
+	// fetch a local collation from connected collators.
+	let (collation_info, full_output) = match collation_fetch.collation_fetch(


One simplification would be to accept the collation_fetch future as an argument rather than the CollationFetch trait. then this function doesn't need all those parameters

bkchr · 2020-03-31T20:38:59Z

@rphmeier I changed the implementation a little bit, not sure if that makes you more happy :)

rphmeier · 2020-04-01T18:40:44Z

validation/src/validation_service/mod.rs

 	<N::TableRouter as TableRouter>::SendLocalCollation: Send,
 	N::BuildTableRouter: Unpin + Send + 'static,
 	SP: Spawn + Send + 'static,
+	CFB: Fn() -> CF + Send,
+	CF: FnOnce(ParaId, Hash, Arc<P>, Option<u64>, usize) -> CFF + Send + 'static,


This code is less readable than before. It's not clear what all of those parameters are from the signature

rphmeier · 2020-04-01T18:42:41Z

validation/src/validation_service/mod.rs

+	}
+
+	#[test]
+	fn router_is_build_on_relay_chain_validator() {


Suggested change

fn router_is_build_on_relay_chain_validator() {

fn router_is_built_on_relay_chain_validator() {

baut vs. gebaut

rphmeier · 2020-04-01T18:45:33Z

validation/src/validation_service/mod.rs

+}
+
+// launch parachain work asynchronously.
+async fn launch_work<CF, Error>(


👍 that this is rewritten as async

This reverts commit a6c1943.

This reverts commit b989c30.

bkchr · 2020-04-03T10:41:50Z

@rphmeier comments addressed!

f43c92430 Fix account derivation in CLI (#952) 9ac07e733 Add backbone configuration of cargo-spellcheck (#924) 2761c3fef Message dispatch support multiple instances (#942) 801c99f3d Add Wococo<>Rococo Header Relayer (#925) 21f490514 Remove Westend<>Rococo header sync (#940) 06235f162 do not panic if pallet is not yet initialized (#937) a13ee0bc3 Bump Substrate (#939) f8680cbfc jsonrpsee alpha6 (#938) 6163bcbf4 reonnect to failed client in on-demand relay background task (#936) 14e82bea3 Do not spawn additional task for on-demand relays (#933) b1557b882 Relay at least one header for every source chain session (#923) 9420649c1 Remove deprecated Runtime Header APIs (#932) 9627011e1 Update README.md (#931) 7b736b9cc Truncate output in logs. (#930) faad06e39 Make sure that relayers have dates in logs. (#927) 077345351 Update dump-logs script. (#928) c2d56b2e9 Add pruning to bechmarks & update weights. (#918) a30c51dc9 Add properties to Chain Spec (#917) d691c73e9 Fix issue with on-demand headers relay not starting (#921) 8ee55c1e1 Fix image publishing. (#922) f51fb59d0 Prefix in relay loops logs (#920) git-subtree-dir: bridges git-subtree-split: f43c924301c227d29ec161f6815d9bac458a211d

b2099c5c Bump Substrate to `b094edaf` (#958) 3f037094 Bump endowment amounts on Rialto and Millau (#957) b21fd07c Bump Substrate WASM builder (#947) 30ccd07c Bump Substrate to `ec180313` (#955) a7422ab1 Upgrade to GitHub-native Dependabot (#945) ed20ef34 Move pallet-bridge-dispatch types to primitives (#948) 2070c4d6 Endow accounts and add `bridgeIds` to chainspec. (#951) f43c9243 Fix account derivation in CLI (#952) 9ac07e73 Add backbone configuration of cargo-spellcheck (#924) 2761c3fe Message dispatch support multiple instances (#942) 801c99f3 Add Wococo<>Rococo Header Relayer (#925) 21f49051 Remove Westend<>Rococo header sync (#940) 06235f16 do not panic if pallet is not yet initialized (#937) a13ee0bc Bump Substrate (#939) f8680cbf jsonrpsee alpha6 (#938) 6163bcbf reonnect to failed client in on-demand relay background task (#936) 14e82bea Do not spawn additional task for on-demand relays (#933) b1557b88 Relay at least one header for every source chain session (#923) 9420649c Remove deprecated Runtime Header APIs (#932) 9627011e Update README.md (#931) 7b736b9c Truncate output in logs. (#930) faad06e3 Make sure that relayers have dates in logs. (#927) 07734535 Update dump-logs script. (#928) c2d56b2e Add pruning to bechmarks & update weights. (#918) a30c51dc Add properties to Chain Spec (#917) d691c73e Fix issue with on-demand headers relay not starting (#921) 8ee55c1e Fix image publishing. (#922) f51fb59d Prefix in relay loops logs (#920) git-subtree-dir: bridges git-subtree-split: b2099c5c0baf569e2ec7228507b6e4f3972143cc

bkchr added the A0-please_review Pull request needs code review. label Mar 30, 2020

bkchr requested a review from rphmeier March 30, 2020 22:16

paritytech-ci added the B2-breaksapi label Mar 30, 2020

rphmeier changed the title ~~Ensure that table router is always build~~ Ensure that table router is always built Mar 31, 2020

rphmeier reviewed Mar 31, 2020

View reviewed changes

bkchr added 3 commits March 31, 2020 13:57

Merge remote-tracking branch 'origin/master' into bkchr-fix-validatio…

165dbd1

…n-service

Merge remote-tracking branch 'origin/master' into bkchr-fix-validatio…

653ac0a

…n-service

Fix compilation

ade2ecd

rphmeier reviewed Mar 31, 2020

View reviewed changes

bkchr added 2 commits March 31, 2020 22:36

Switch to closures

b989c30

Remove empty line

a6c1943

rphmeier reviewed Apr 1, 2020

View reviewed changes

bkchr added 4 commits April 3, 2020 12:03

Revert "Remove empty line"

0edf4b4

This reverts commit a6c1943.

Revert "Switch to closures"

d2bd87d

This reverts commit b989c30.

Hybrid approach

b0068ea

Rename test

01cac5b

Make trait crate local

ecc081a

rphmeier approved these changes Apr 3, 2020

View reviewed changes

rphmeier merged commit ec11d7e into master Apr 3, 2020

rphmeier deleted the bkchr-fix-validation-service branch April 3, 2020 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that table router is always built #952

Ensure that table router is always built #952

bkchr commented Mar 30, 2020

rphmeier Mar 31, 2020

rphmeier Apr 3, 2020

rphmeier Mar 31, 2020 •

edited

Loading

rphmeier commented Mar 31, 2020

bkchr commented Mar 31, 2020

rphmeier Mar 31, 2020

bkchr commented Mar 31, 2020

rphmeier Apr 1, 2020

rphmeier Apr 1, 2020

rphmeier Apr 1, 2020

rphmeier Apr 1, 2020

bkchr commented Apr 3, 2020

	fn router_is_build_on_relay_chain_validator() {
	fn router_is_built_on_relay_chain_validator() {

Ensure that table router is always built #952

Ensure that table router is always built #952

Conversation

bkchr commented Mar 30, 2020

rphmeier Mar 31, 2020

Choose a reason for hiding this comment

rphmeier Apr 3, 2020

Choose a reason for hiding this comment

rphmeier Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

rphmeier commented Mar 31, 2020

bkchr commented Mar 31, 2020

rphmeier Mar 31, 2020

Choose a reason for hiding this comment

bkchr commented Mar 31, 2020

rphmeier Apr 1, 2020

Choose a reason for hiding this comment

rphmeier Apr 1, 2020

Choose a reason for hiding this comment

rphmeier Apr 1, 2020

Choose a reason for hiding this comment

rphmeier Apr 1, 2020

Choose a reason for hiding this comment

bkchr commented Apr 3, 2020

rphmeier Mar 31, 2020 •

edited

Loading