Undo v4 changes, move them to vstaging #7103

ghost · 2023-04-19T11:14:57Z

No description provided.

tdimitrov · 2023-05-25T09:33:23Z

primitives/src/runtime_api.rs

@@ -123,7 +123,7 @@ use sp_std::{collections::btree_map::BTreeMap, prelude::*};

 sp_api::decl_runtime_apis! {
 	/// The API for querying the state of parachains on-chain.
-	#[api_version(4)]
+	#[api_version(5)]


This is good. You also need to add (can't leave a comment at the right):

#[changed_in(5)] fn availability_cores() -> Vec<CoreState<H, N>>;

This way you can have two implementations - old and new ones. The intention of changed_in is to change the method signature. I'm not sure if it will work in the case 'same signature, different implementation'.

If this is the case - you can just add another function and handle the right one in the client code. This one breaks the semantics a little but imo it's fine. If pick this approach, you shouldn't touch base version here and just add another method:

#[api_version(5)] fn availability_cores_with_parathreads() -> Vec<CoreState<H, N>>;

tdimitrov · 2023-06-01T13:06:35Z

primitives/src/v4/mod.rs

@@ -819,121 +815,14 @@ pub struct ParathreadEntry {
 	pub retries: u32,
 }

-/// An Assignemnt for a paras going to produce a paras block.
-#[derive(Clone, Encode, Decode, PartialEq, TypeInfo, RuntimeDebug)]


I remember these are actually not part of v4 at the moment and you are undoing previous work done in alex/parathreads_review, right?

Exactly! At the end of the day, we'll need to make sure v4 is just as it is on master. Which I think it is here but will confirm later down the road when we have the final feature branch with a diff to master.

tdimitrov · 2023-06-01T13:15:00Z

node/subsystem-types/src/runtime_client.rs

@@ -252,7 +252,18 @@ where
 		&self,
 		at: Hash,
 	) -> Result<Vec<CoreState<Hash, BlockNumber>>, ApiError> {
-		self.runtime_api().availability_cores(at)
+		let version = self


We discussed somewhere that such transformations in the client are not encouraged and we prefer doing this in the subsystem. Unfortunately I don't remember the exact reasons.

For your particular case I think it's okay though. Depending on the runtime api you call either the new or the old version of the function. And you do this workaround to avoid the changed_in issues with the runtime api.

Which file would this ideally go to then? What exactly is the subsystem again? I'm only aware of the node/client and runtime distinction.

Subsystem is a module (lets say) from the node - e.g. approval-voting, dispute-coordinator etc.
You can put a function in any helpers module (e.g. https://github.com/paritytech/polkadot/blob/dbae30efe080a1d41fe54ef4da8af47614c9ca93/node/subsystem-util/src/runtime/mod.rs) and call it each time you need the availability_cores.

But I'd suggest not to touch it for now.

eskimor · 2023-06-07T10:18:54Z

primitives/src/vstaging/mod.rs

+	pub next_up_on_available: Option<ScheduledCore>,
+	/// The relay-chain block number this began occupying the core at.
+	pub occupied_since: N,
+	/// The relay-chain block this will time-out at, if any.


This is not an Option, so the if any seems odd.

This is a copy and paste from v4. Fixed.

eskimor · 2023-06-07T10:20:12Z

primitives/src/vstaging/mod.rs

+	/// The relay-chain block this will time-out at, if any.
+	pub time_out_at: N,
+	/// If this core is freed by being timed-out, this is the assignment that is next up on this
+	/// core. None if there is nothing queued for this core or there is no possibility of timing


There is always a possibility of timing out. What could be that there is no other assignment queued and the currently occupying assignment already has exhausted its retry counter.

This is a copy and paste from v4. Fixed.

eskimor · 2023-06-07T10:21:45Z

primitives/src/vstaging/mod.rs

+	/// validators has attested to availability on-chain. A 2/3+ majority of `1` bits means that
+	/// this will be available.
+	pub availability: BitVec<u8, bitvec::order::Lsb0>,
+	/// The group assigned to distribute availability pieces of this candidate.


And backing the candidate.

This is a copy and paste from v4. Fixed.

eskimor · 2023-06-07T10:23:47Z

primitives/src/vstaging/mod.rs

+	/// variant.
+	#[codec(index = 1)]
+	Scheduled(ScheduledCore),
+	/// The core is currently free and there is nothing scheduled. This can be the case for parathread


We should start using on-demand parachain.

eskimor · 2023-06-07T10:25:14Z

primitives/src/runtime_api.rs

+		/// Yields information on all availability cores as relevant to the child block.
+		/// Cores are either free or occupied. Free cores can have paras assigned to them.
+		#[api_version(5)]
+		fn availability_cores_on_demand() -> Vec<vstaging::CoreState<H, N>>;


So this only gets us on-demand availability cores? I guess not, from the docs and from my understanding. So where does the name come from?

It's supposed to be the v5 availability_cores call. I thought just calling it availability_cores_v5 would be a bit meaningless but I see how the current name is misleading as well.

eskimor · 2023-06-07T10:42:19Z

runtime/parachains/src/runtime_api_impl/vstaging.rs

+		// actively. We could even time out this block.
+		if time_out_at < current_window {
+			time_out_at
+		} else if time_out_at <= next_rotation {


This code is out of sync with the updated timeout predicate. It is bad that this can even happen, we should be using the same logic. DRY.

Agreed! The issue is that the updated timeout predicate is in the scheduler itself. The runtime API uses its own implementation for reasons unknown to me. Will unify.

cla-bot-2021 · 2023-07-21T19:00:17Z

User @ghost, please sign the CLA here.

Undo v4 changes, move them to vstaging

b6b9f8e

ghost added A3-in_progress Pull request is in progress. No review needed at this stage. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. labels Apr 19, 2023

alexgparity added 3 commits April 19, 2023 14:59

Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging

c76ce40

minor

e2d5869

Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging

fc276d6

ghost force-pushed the alex/parathreads_vstaging branch from 8b03c90 to fc276d6 Compare April 25, 2023 10:05

alexgparity added 3 commits May 19, 2023 07:43

Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging

7baa04f

Fix imports

94d24d5

Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging

f943c1a

tdimitrov reviewed May 25, 2023

View reviewed changes

alexgparity added 12 commits May 29, 2023 11:12

Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging

59f6bc2

include version dispatch on availablity_cores() call

d65d96e

ifmt

cbfa7a1

Fix test-runtime

2471334

fix node runtime api test

678b1d8

Undo RuntimeDebug -> Debug changes in v4

13fa655

runtime-api tests fix

2cf57a8

Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging

4d01e65

Fix removing api version by accident

af57dd0

Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging

b413cc7

Remove naked unwrap()s

6fae5a3

Add availability_cores_on_demand to mock and use that in tests

a665df0

ghost requested review from tdimitrov and s0me0ne-unkn0wn June 1, 2023 12:44

tdimitrov reviewed Jun 1, 2023

View reviewed changes

Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging

42bc527

ghost marked this pull request as ready for review June 2, 2023 17:33

ghost requested review from tdimitrov and eskimor June 2, 2023 17:33

tdimitrov approved these changes Jun 3, 2023

View reviewed changes

paritytech-ci requested review from a team June 3, 2023 18:48

alexgparity added 2 commits June 5, 2023 11:20

Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging

ea8a0c6

Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging

a198b95

eskimor reviewed Jun 7, 2023

View reviewed changes

paritytech-ci requested review from a team June 7, 2023 10:53

alexgparity added 4 commits June 8, 2023 12:35

Address PR comments

918243e

Rename availability_cores_on_demand to ..._vstaging

49ebc7d

pebkac

c49582c

minor refactor

6273880

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Undo v4 changes, move them to vstaging #7103

Undo v4 changes, move them to vstaging #7103

ghost commented Apr 19, 2023

tdimitrov May 25, 2023

tdimitrov Jun 1, 2023

ghost Jun 1, 2023

tdimitrov Jun 1, 2023

ghost Jun 1, 2023

tdimitrov Jun 1, 2023

eskimor Jun 7, 2023

ghost Jun 8, 2023

eskimor Jun 7, 2023

ghost Jun 8, 2023

eskimor Jun 7, 2023

ghost Jun 8, 2023

eskimor Jun 7, 2023

eskimor Jun 7, 2023

ghost Jun 8, 2023

eskimor Jun 7, 2023

ghost Jun 8, 2023

cla-bot-2021 bot commented Jul 21, 2023

Undo v4 changes, move them to vstaging #7103

Are you sure you want to change the base?

Undo v4 changes, move them to vstaging #7103

Conversation

ghost commented Apr 19, 2023

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

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

cla-bot-2021 bot commented Jul 21, 2023