Introduce interface for relay chain interaction #835

skunert · 2021-12-06T12:42:42Z

In this PR, I introduce an interface that exposes methods for interaction with the relay chain, as discussed in #545.
These changes are only refactoring and have no impact on functionality.
Outline of the changes:

Introduce trait RelayChainInterface
Implement RelayChainInterface for RelayChainLocal, a status-quo implementation that uses a full node in the same process
replace all occurrences of polkadot client and backend with RelayChainLocal

…ontents` to interface

Remove polkadot-client fromcollator crate.

Introduce enum as return type for block checking

bkchr · 2021-12-21T08:29:22Z

client/consensus/aura/src/lib.rs

@@ -252,13 +244,12 @@ pub struct BuildAuraConsensusParams<PF, BI, RBackend, CIDP, Client, BS, SO> {
 /// Build the [`AuraConsensus`].
 ///
 /// Returns a boxed [`ParachainConsensus`].
-pub fn build_aura_consensus<P, Block, PF, BI, RBackend, CIDP, Client, SO, BS, Error>(
+pub fn build_aura_consensus<P, Block, PF, BI, CIDP, Client, SO, BS, Error, RCInterface>(


The new function is essentially the same. Aka we should remove this one build_aura_consensus function and maybe rename the new function to build.

However, the new function could also use the BuildAuraConsensusParams.

bkchr · 2021-12-21T08:31:32Z

client/consensus/aura/src/lib.rs

 	create_inherent_data_providers: Arc<CIDP>,
-	relay_chain_client: Arc<RClient>,
-	relay_chain_backend: Arc<RBackend>,
+	relay_chain_interface: RCInterface,


Do we actually use this anywhere? I could not find any usage?

bkchr · 2021-12-21T08:34:18Z

client/relay-chain-interface/src/lib.rs

+use sp_core::sp_std::collections::btree_map::BTreeMap;
+use sp_state_machine::StorageValue;
+
+pub enum BlockCheckResult {


bkchr · 2021-12-21T08:36:58Z

client/relay-chain-local/src/lib.rs

+	}
+
+	fn is_major_syncing(&self) -> bool {
+		let mut network = self.sync_oracle.lock().unwrap();


No unwraps in the code. Either use expect with a proof or remove.

Here you can just replace std::Mutex with parking_lot::Mutex. Parkinglot mutex isn't poisened.

bkchr · 2021-12-21T08:37:27Z

client/relay-chain-local/Cargo.toml

+sc-telemetry = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sc-tracing = { git = "https://github.com/paritytech/substrate", branch = "master" }
+
+futures = { version = "0.3.1", features = ["compat"] }


Suggested change

futures = { version = "0.3.1", features = ["compat"] }

Not used?

bkchr · 2021-12-21T09:32:37Z

client/relay-chain-interface/src/lib.rs

+
+/// Should be used for all interaction with the relay chain in cumulus.
+pub trait RelayChainInterface: Send + Sync {
+	fn get_storage_by_key(&self, block_id: &BlockId, key: &[u8]) -> Option<StorageValue>;


Docs?

Also weird that we return None here for when the key doesn't exist or there was an error.

bkchr · 2021-12-21T09:32:48Z

client/relay-chain-interface/src/lib.rs

+	fn validators(&self, block_id: &BlockId) -> Result<Vec<ValidatorId>, ApiError>;
+
+	fn block_status(&self, block_id: BlockId) -> Result<BlockStatus, sp_blockchain::Error>;
+
+	fn best_block_hash(&self) -> PHash;


Docs please

bkchr · 2021-12-21T09:39:19Z

client/relay-chain-interface/src/lib.rs

+	/// Check if block is in chain. If it is, we return BlockCheckResult::InChain.
+	/// If it is not in the chain, we return BlockCheckResult::NotFound with a listener that can be used to wait on the block.
+	fn check_block_in_chain(


Part of the docs should go to the BlockCheckResult type.

However, I'm not really happy with the naming here.

Maybe something like wait_for_block and return a future that either resolves directly or waits for the block to be imported.

client/relay-chain-interface/Cargo.toml

bkchr · 2021-12-21T09:41:56Z

client/network/src/lib.rs

-/// the builder gets access to this concrete instance.
-struct BlockAnnounceValidatorBuilder<Block, B> {
+/// Builds a [`BlockAnnounceValidator`] for a parachain.
+struct BlockAnnounceValidatorBuilder<Block, RCInterface> {


Still relevant.

Improve get_storage_value method with error handling

skunert · 2021-12-22T14:26:12Z

client/relay-chain-local/src/lib.rs

+}
+
+// Helper function to check if a block is in chain.
+pub fn check_block_in_chain<Client>(


This helper function came back to solve an issue which the lock guard that was not Send in the async function. Also makes this reusable in the test.

bkchr

Looks good! Just some last small nitpicks.

bkchr · 2021-12-22T15:27:50Z

primitives/parachain-inherent/src/client_side.rs

 		.map_err(|e| {
 			tracing::error!(
 				target: LOG_TARGET,
-				error = ?e,
-				"Cannot obtain the hrmp ingress channel index."
+				relay_parent = ?relay_parent_block_id,


Suggested change

relay_parent = ?relay_parent_block_id,

relay_parent = ?relay_parent,

bkchr · 2021-12-22T15:28:01Z

primitives/parachain-inherent/src/client_side.rs

-				error = ?e,
-				"Cannot obtain the hrmp ingress channel index."
+				relay_parent = ?relay_parent_block_id,
+				error = ?e, "Cannot obtain the hrmp ingress channel."


Suggested change

error = ?e, "Cannot obtain the hrmp ingress channel."

error = ?e,

"Cannot obtain the hrmp ingress channel.",

bkchr · 2021-12-22T15:44:01Z

client/network/src/lib.rs

@@ -408,88 +379,14 @@ where
 /// Build a block announce validator instance.
 ///
 /// Returns a boxed [`BlockAnnounceValidator`].
-pub fn build_block_announce_validator<Block: BlockT, B>(
-	relay_chain_client: polkadot_client::Client,
+pub fn build_block_announce_validator<Block: BlockT, RCInterface>(


Function is rather useless now, please remove.

bkchr · 2021-12-22T15:46:14Z

client/consensus/aura/src/lib.rs

+			block_proposal_slot_portion,
+			max_block_proposal_slot_portion,
+		}: BuildAuraConsensusParams<PF, BI, CIDP, Client, BS, SO>,
+	) -> Box<dyn ParachainConsensus<B>>


Suggested change

) -> Box<dyn ParachainConsensus<B>>

) -> Self

We don't need to box anymore. This was just a short coming of the old implementation with all the ExecuteWithClient stuff etc, that is now solved.

Changes related to: paritytech/cumulus#835

* Bump Cumulus, Polkadot, and Substrate Also bumps some other depenencies * Remove duplicate `polkadot` dependency * Update `service.rs` Changes related to: paritytech/cumulus#835 * Update `command.rs` * Add `AddressGenerator` config type From: paritytech/substrate#10521 * Allow Root to execute overweight XCMP messages From: paritytech/cumulus#799 * Add `header` argument to `collect_collation_info` From: paritytech/cumulus#882 * Update Cumulus and friends again * Add Fork ID to genesis config paritytech/cumulus#870 * Add `state_version` field paritytech/substrate#9732 * Add `MaxConsumers` config parameter paritytech/substrate#10382 * Update Substrate compatibility note in README

skunert and others added 30 commits October 15, 2021 10:11

Move retrieve_dmq_contents and `retrieve_all_inbound_hrmp_channel_c…

c3f77d1

…ontents` to interface

Add cumulus-client-collator to polkadot-parachains package

6260554

Remove more direct references to clients

830f8e0

Remove more direct references to clients

387b32f

Move interface to its own crate

b178df1

Remove more direct references to clients

5b4b9d7

Remove more direct references to clients

cc66c2f

revert network changes

91c6a5c

Merge branch 'master' into relay-chain-interface

8d991cf

Implement relaychaininterface for Arc<dyn RelayChainInterface>

ea57405

Remove unused code. Clean up imports.

d7a7ffb

Remove polkadot-client fromcollator crate.

Extract cumulu-client-network usages of polkadot-client

1c14ea5

Change edition to string in rustfmt

2ffa0c3

Change BlockchainEvent parameters

026832d

Remove polkadot-client dependency from cumulus-network

6350c8a

Modify tests to compile again

0fabdf0

Merge branch 'master' into relay-chain-interface

1a810d4

Cleanup

3d0af0c

Implement BlockchainEvents for TestApi

1033b8b

Remove references to backend from BlockAnnounceValidator

d40f328

Extract get_state to RelayChainInterface

a938b70

Remove backend references from consensus structs

9c865fe

Move network to relay-chain-interface

054d22d

Move network to RelayChainInterface

14ec0a1

Remove TestApi and add DummyRelayChainInterface

5e42e77

Remove network from BlockAnnounceValidator

06815ee

Remove references to full node from service.rs

ce860cf

Move StartCollator and StartConsensus to RelayChainInterface

f8c8325

Replace more references to full_node

b1b3814

Clean up and fix overseer handle

4b37786

skunert added 4 commits December 20, 2021 12:35

Remove polkadot-service dependency from relay-chain-interface

4e6fbad

Introduce enum as return type for block checking

Fix test

4768ad9

Merge branch 'master' into relay-chain-interface

9ec6cf8

Move relay-chain-local to dev dependency in network

dd64161

skunert added the B0-silent Changes should not be mentioned in any release notes label Dec 20, 2021

Remove unused dependency

b1b07b5

bkchr reviewed Dec 21, 2021

View reviewed changes

skunert added 6 commits December 21, 2021 13:58

Remove unused relay-chain reference in AuraConsensus

9ccc9bf

Improve get_storage_value method with error handling

Remove unused builders, move wait_on_block to relaychain

481d7e9

Remove WaitOnRelayChain and add docs

67ce2f2

Remove unused dep

6f7094a

Merge branch 'master' into relay-chain-interface

d3599c2

Format

83c977f

skunert requested a review from bkchr December 22, 2021 10:53

Remove unused dependencies, fix lock issue

2f51a3e

skunert commented Dec 22, 2021

View reviewed changes

bkchr approved these changes Dec 22, 2021

View reviewed changes

skunert added 3 commits December 22, 2021 17:16

Fix log output

aebfcda

Remove build_block_announce_validator function

117ba4c

Merge branch 'master' into relay-chain-interface

f3af41d

skunert merged commit 7b42df1 into master Dec 22, 2021

skunert deleted the relay-chain-interface branch December 22, 2021 18:02

ordian mentioned this pull request Dec 22, 2021

companion for polkadot#4545 (session-info v2) #872

Closed

skunert mentioned this pull request Jan 5, 2022

Fix rococo-parachain-runtime build #887

Merged

HCastano added a commit to paritytech/canvas that referenced this pull request Jan 8, 2022

Update service.rs

70b4c05

Changes related to: paritytech/cumulus#835

ghzlatarev mentioned this pull request Feb 2, 2022

[manta] Bump upstream to v0.9.16 Manta-Network/Manta#377

Merged

26 tasks

hussein-aitlahcen mentioned this pull request Feb 4, 2022

Upgrade to polkadot-v0.9.16 ComposableFi/composable#567

Merged

Garandor mentioned this pull request Feb 8, 2022

Expose more ports in dockerfile Manta-Network/Manta#373

Merged

12 tasks

skunert mentioned this pull request Nov 2, 2022

Member request polkadot-fellows/seeding#55

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce interface for relay chain interaction #835

Introduce interface for relay chain interaction #835

skunert commented Dec 6, 2021

bkchr Dec 21, 2021

bkchr Dec 21, 2021

bkchr Dec 21, 2021

bkchr Dec 21, 2021

bkchr Dec 21, 2021

bkchr Dec 21, 2021

bkchr Dec 21, 2021

bkchr Dec 21, 2021

bkchr Dec 21, 2021

skunert Dec 22, 2021

bkchr left a comment

bkchr Dec 22, 2021

bkchr Dec 22, 2021

bkchr Dec 22, 2021

bkchr Dec 22, 2021

	relay_parent = ?relay_parent_block_id,
	relay_parent = ?relay_parent,

	error = ?e, "Cannot obtain the hrmp ingress channel."
	error = ?e,
	"Cannot obtain the hrmp ingress channel.",

Introduce interface for relay chain interaction #835

Introduce interface for relay chain interaction #835

Conversation

skunert commented Dec 6, 2021

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

bkchr 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