Fix candidate tracking on async backing #675

AndreiEres · 2024-03-05T15:56:03Z

Fixes #673

Major changes

Instead of a single current candidate, we track [included, backed] candidates for each assigned core.
We set the included status based on chain events, which we store in the collector.
Dead code removed

sandreim · 2024-03-05T17:45:37Z

parachain-tracer/src/tracker.rs

@@ -40,7 +40,7 @@ pub struct SubxtTracker {
 	/// A new session index.
 	new_session: Option<u32>,
 	/// Information about current parachain block we track.
-	current_candidate: ParachainBlockInfo,
+	candidates: Vec<ParachainBlockInfo>,


Why do we need a vec of these, which wraps a vec of states. There should be one state per parachain that is tracked.

From parachain consensus perspective we now allow a new candidate to be backed at an occupied core (while the previous candidate is pending avilability).

I'd expect to see this reflected here somehow.

parachain-tracer/src/parachain_block_info.rs

sandreim · 2024-03-06T16:49:22Z

parachain-tracer/src/parachain_block_info.rs

@@ -38,58 +34,55 @@ pub struct ParachainBlockInfo {
 	pub assigned_core: Option<u32>,
 	/// Core occupation status.
 	pub core_occupied: bool,
+	/// The current state.
+	state: Vec<ParachainBlockState>,


Why more objects of state instead of one ?

sandreim · 2024-03-11T15:14:26Z

parachain-tracer/src/tracker.rs

+			if candidate.is_idle() {
+				progress.events.push(ParachainConsensusEvent::SkippedSlot);
+				stats.on_skipped_slot(progress);
+				metrics.on_skipped_slot(progress);


insert continue . we don't need to check next conditions.

sandreim · 2024-03-11T15:14:54Z

parachain-tracer/src/tracker.rs

-				progress.events.push(ParachainConsensusEvent::Backed(candidate_hash));
-				stats.on_backed();
-				metrics.on_backed(self.para_id);
+			if candidate.is_backed() {


There seem to be so many ifs here, a match would be better if we had an enum for the candidate state.

sandreim · 2024-03-11T15:16:45Z

parachain-tracer/src/tracker.rs

 	fn is_just_backed(&self) -> bool {
 		self.last_backed_at_block_number.is_some() &&
 			self.last_backed_at_block_number == self.current_relay_block.map(|v| v.num)
 	}

-	fn is_slow_availability(&self) -> bool {
-		self.current_candidate.core_occupied &&
+	fn is_slow_availability(&self, core: u32) -> bool {


I think it is a good time to make sure we have tests covering all of these functions and ensure they work as expected. The condition below seems brittle.

sandreim · 2024-03-11T15:23:16Z

parachain-tracer/src/tracker.rs

+				let max_bits = self.validators_indices(block_hash, storage).await?.len() as u32;
+				let candidate = self.current_candidate_mut(core).expect("Checked above");
+
+				candidate.current_availability_bits = extract_availability_bits_count(&bitfields, core);


If the candidate was backed at current block, the bits should not be set at all. I'd assume we want to do this only if !is_just_backed, below.

sandreim · 2024-04-17T07:26:57Z

parachain-tracer/src/tracker.rs

+			}
+		}
+		if last_included.is_some() {
+			self.relay_forks.last_mut().expect("must have relay fork").included_candidate = last_included;


Can this expect panic ?

sandreim · 2024-04-17T07:28:27Z

parachain-tracer/src/tracker.rs

+			if let Some(candidate_hash) = candidate.candidate_hash {
+				if let Some(stored_candidate) = storage.candidate(candidate_hash).await {
+					if stored_candidate.is_included() {
+						candidate.set_included();
+						last_included = candidate.candidate_hash;
+					}
+				}
+			}


Suggested change

if let Some(candidate_hash) = candidate.candidate_hash {

if let Some(stored_candidate) = storage.candidate(candidate_hash).await {

if stored_candidate.is_included() {

candidate.set_included();

last_included = candidate.candidate_hash;

}

}

}

let Some(candidate_hash) = candidate.candidate_hash else {

continue

}

let Some(stored_candidate) = storage.candidate(candidate_hash).await else {

continue

}

if stored_candidate.is_included() {

candidate.set_included();

last_included = candidate.candidate_hash;

}

sandreim · 2024-04-17T07:29:32Z

parachain-tracer/src/tracker.rs

+			.map(|v| ParachainBlockInfo::candidate_hash(&v));
+		let mut used_cores = vec![];
+		for candidate_hash in candidate_hashes {
+			if let Some(core) = self.candidate_core(candidate_hash, storage).await {


use let Some(..) to avoid nesting.

sandreim · 2024-04-17T07:31:06Z

parachain-tracer/src/tracker.rs

+					metrics.on_skipped_slot(progress);
+				},
+				ParachainBlockState::Backed =>
+					if let Some(candidate_hash) = candidate.candidate_hash {


Could we remove the option ? Doesn't seem to make sense to have the state backed but no hash.

Yes, removing ParachainBlockState::Idle.

sandreim · 2024-04-17T07:32:11Z

parachain-tracer/src/tracker.rs

+						metrics.on_backed(self.para_id);
+					},
+				ParachainBlockState::PendingAvailability => {
+					if self.is_slow_availability(candidate.assigned_core.expect("must be assigned")) {


When would expect panic ? I'd avoid doing this since it reduces the uptime of the monitor. Let's log errors instead.

It should never panic, because a backed candidate info should include assigned core.

AndreiEres · 2024-04-17T11:04:03Z

Updated, removing Idle state from candidate's state. Used None and Some(block_info). So we removed unnecessary .expect()s.

AndreiEres requested a review from sandreim March 6, 2024 13:54

AndreiEres added 5 commits March 6, 2024 14:57

Update tracking of candidates

63155f0

Fix candidate state reporting

619edee

Remove dead code

d2d4010

Update tests

b8b9524

Remove unused params

8a2e5bb

AndreiEres force-pushed the AndreiEres/async-backing-candidates branch from a67d1e1 to 8a2e5bb Compare March 6, 2024 13:58

Fix clippy errors

3ad75e8

sandreim reviewed Mar 6, 2024

View reviewed changes

AndreiEres added 2 commits March 7, 2024 12:29

Update block info

053981e

Add core mapping

4c58127

AndreiEres changed the title ~~Fix candidate tracking on async backing~~ [WIP] Fix candidate tracking on async backing Mar 7, 2024

AndreiEres added 9 commits March 11, 2024 08:57

Fix errors

8c9528f

Fix tests

7ce439d

Fix clippy errors

fee2fa2

Fix core idx

5f431cd

Update

23a79be

Fix tests

0640070

Fix clippy

ab34722

Add tests

1d4e047

fix clippy

ae52a2c

AndreiEres changed the title ~~[WIP] Fix candidate tracking on async backing~~ Fix candidate tracking on async backing Mar 11, 2024

AndreiEres requested a review from sandreim March 11, 2024 14:17

sandreim reviewed Mar 11, 2024

View reviewed changes

Update

5ab1943

AndreiEres requested a review from sandreim March 26, 2024 13:26

sandreim reviewed Apr 17, 2024

View reviewed changes

AndreiEres added 3 commits April 17, 2024 10:28

Avoid nesting

dac97c0

Use None for skipped slots

9a5d5ae

Fix tests

0465afe

sandreim approved these changes Apr 19, 2024

View reviewed changes

AndreiEres merged commit 5f05614 into master Apr 19, 2024
6 checks passed

AndreiEres deleted the AndreiEres/async-backing-candidates branch April 19, 2024 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix candidate tracking on async backing #675

Fix candidate tracking on async backing #675

AndreiEres commented Mar 5, 2024 •

edited

Loading

sandreim Mar 5, 2024

sandreim Mar 6, 2024

sandreim Mar 11, 2024

sandreim Mar 11, 2024

sandreim Mar 11, 2024

sandreim Mar 11, 2024

sandreim Apr 17, 2024

sandreim Apr 17, 2024

sandreim Apr 17, 2024

sandreim Apr 17, 2024

AndreiEres Apr 17, 2024 •

edited

Loading

sandreim Apr 17, 2024

AndreiEres Apr 17, 2024

AndreiEres commented Apr 17, 2024

Fix candidate tracking on async backing #675

Fix candidate tracking on async backing #675

Conversation

AndreiEres commented Mar 5, 2024 • 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

AndreiEres Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreiEres commented Apr 17, 2024

AndreiEres commented Mar 5, 2024 •

edited

Loading

AndreiEres Apr 17, 2024 •

edited

Loading