From 85be345ecc203848d3c0b3ab268cd5a06c75cf69 Mon Sep 17 00:00:00 2001 From: alindima Date: Fri, 8 Nov 2024 16:40:45 +0200 Subject: [PATCH 1/5] fix prospective-parachains best backable chain reversion bug --- .../src/fragment_chain/mod.rs | 2 +- .../src/fragment_chain/tests.rs | 63 ++++++++++++++++++- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs b/polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs index 265d1498ee96..ded0a3ab73b2 100644 --- a/polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs +++ b/polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs @@ -630,7 +630,7 @@ impl BackedChain { ) -> impl Iterator + 'a { let mut found_index = None; for index in 0..self.chain.len() { - let node = &self.chain[0]; + let node = &self.chain[index]; if found_index.is_some() { self.by_parent_head.remove(&node.parent_head_data_hash); diff --git a/polkadot/node/core/prospective-parachains/src/fragment_chain/tests.rs b/polkadot/node/core/prospective-parachains/src/fragment_chain/tests.rs index 2f8a5525570c..624dd74132c1 100644 --- a/polkadot/node/core/prospective-parachains/src/fragment_chain/tests.rs +++ b/polkadot/node/core/prospective-parachains/src/fragment_chain/tests.rs @@ -1165,8 +1165,9 @@ fn test_populate_and_check_potential() { Err(Error::CandidateAlreadyKnown) ); - // Simulate a best chain reorg by backing a2. + // Simulate some best chain reorgs. { + // Back A2. The reversion should happen right at the root. let mut chain = chain.clone(); chain.candidate_backed(&candidate_a2_hash); assert_eq!(chain.best_chain_vec(), vec![candidate_a2_hash, candidate_b2_hash]); @@ -1185,6 +1186,66 @@ fn test_populate_and_check_potential() { chain.can_add_candidate_as_potential(&candidate_a_entry), Err(Error::ForkChoiceRule(_)) ); + + // Simulate a more complex chain reorg. + // A2 points to B2, which is backed. + // A2 has underneath a subtree A2 -> B2 -> C3 and A2 -> B2 -> C4. B2 and C3 are backed. C4 + // is kept because it has a lower candidate hash than C3. Backing C4 will cause a chain + // reorg. + + // Candidate C3. + let (pvd_c3, candidate_c3) = make_committed_candidate( + para_id, + relay_parent_y_info.hash, + relay_parent_y_info.number, + vec![0xb4].into(), + vec![0xc2].into(), + relay_parent_y_info.number, + ); + let candidate_c3_hash = candidate_c3.hash(); + let candidate_c3_entry = + CandidateEntry::new(candidate_c3_hash, candidate_c3, pvd_c3, CandidateState::Seconded) + .unwrap(); + + // Candidate C4. + let (pvd_c4, candidate_c4) = make_committed_candidate( + para_id, + relay_parent_y_info.hash, + relay_parent_y_info.number, + vec![0xb4].into(), + vec![0xc3].into(), + relay_parent_y_info.number, + ); + let candidate_c4_hash = candidate_c4.hash(); + // C4 should have a lower candidate hash than C3. + assert_eq!(fork_selection_rule(&candidate_c4_hash, &candidate_c3_hash), Ordering::Less); + let candidate_c4_entry = + CandidateEntry::new(candidate_c4_hash, candidate_c4, pvd_c4, CandidateState::Seconded) + .unwrap(); + + let mut storage = storage.clone(); + storage.add_candidate_entry(candidate_c3_entry).unwrap(); + storage.add_candidate_entry(candidate_c4_entry).unwrap(); + let mut chain = populate_chain_from_previous_storage(&scope, &storage); + chain.candidate_backed(&candidate_a2_hash); + chain.candidate_backed(&candidate_c3_hash); + + assert_eq!( + chain.best_chain_vec(), + vec![candidate_a2_hash, candidate_b2_hash, candidate_c3_hash] + ); + + // Backing C4 will cause a reorg. + chain.candidate_backed(&candidate_c4_hash); + assert_eq!( + chain.best_chain_vec(), + vec![candidate_a2_hash, candidate_b2_hash, candidate_c4_hash] + ); + + assert_eq!( + chain.unconnected().map(|c| c.candidate_hash).collect::>(), + [candidate_f_hash].into_iter().collect() + ); } // Candidate F has an invalid hrmp watermark. however, it was not checked beforehand as we don't From 85b047d6ef6959420307461eb2592881ef7e128c Mon Sep 17 00:00:00 2001 From: GitHub Action Date: Fri, 8 Nov 2024 22:38:57 +0000 Subject: [PATCH 2/5] Update from bkchr running command 'prdoc --audience node_dev' --- prdoc/pr_6417.prdoc | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 prdoc/pr_6417.prdoc diff --git a/prdoc/pr_6417.prdoc b/prdoc/pr_6417.prdoc new file mode 100644 index 000000000000..fccd49102e90 --- /dev/null +++ b/prdoc/pr_6417.prdoc @@ -0,0 +1,10 @@ +title: fix prospective-parachains best backable chain reversion bug +doc: +- audience: Node Dev + description: |- + Kudos to @EclesioMeloJunior for noticing it + + Also added a regression test for it. The existing unit test was exercising only the case where the full chain is reverted +crates: +- name: polkadot-node-core-prospective-parachains + bump: major From b5dbd0036f696c5641fce4b66ff7aaa16ed2facd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 8 Nov 2024 23:52:32 +0100 Subject: [PATCH 3/5] Update prdoc/pr_6417.prdoc --- prdoc/pr_6417.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_6417.prdoc b/prdoc/pr_6417.prdoc index fccd49102e90..5744e2c580a4 100644 --- a/prdoc/pr_6417.prdoc +++ b/prdoc/pr_6417.prdoc @@ -7,4 +7,4 @@ doc: Also added a regression test for it. The existing unit test was exercising only the case where the full chain is reverted crates: - name: polkadot-node-core-prospective-parachains - bump: major + bump: patch From 6c495749b5a58e6bfb9c0406f75ade675e498807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 8 Nov 2024 23:52:37 +0100 Subject: [PATCH 4/5] Update prdoc/pr_6417.prdoc --- prdoc/pr_6417.prdoc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/prdoc/pr_6417.prdoc b/prdoc/pr_6417.prdoc index 5744e2c580a4..2c899f76cce7 100644 --- a/prdoc/pr_6417.prdoc +++ b/prdoc/pr_6417.prdoc @@ -1,10 +1,5 @@ title: fix prospective-parachains best backable chain reversion bug doc: -- audience: Node Dev - description: |- - Kudos to @EclesioMeloJunior for noticing it - - Also added a regression test for it. The existing unit test was exercising only the case where the full chain is reverted crates: - name: polkadot-node-core-prospective-parachains bump: patch From d93f2612a869b417fb46dfa435a8e725d224f705 Mon Sep 17 00:00:00 2001 From: alindima Date: Mon, 11 Nov 2024 09:48:00 +0200 Subject: [PATCH 5/5] fix prdoc --- prdoc/pr_6417.prdoc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/prdoc/pr_6417.prdoc b/prdoc/pr_6417.prdoc index 2c899f76cce7..dfbc8c0d311b 100644 --- a/prdoc/pr_6417.prdoc +++ b/prdoc/pr_6417.prdoc @@ -1,5 +1,9 @@ title: fix prospective-parachains best backable chain reversion bug doc: + - audience: Node Dev + description: | + Fixes a bug in the prospective-parachains subsystem that prevented proper best backable chain reorg. + crates: - name: polkadot-node-core-prospective-parachains bump: patch