From 24f8fdd45c3cd7b94fce46e49caa803e54008dde Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 20 Dec 2024 10:51:02 +0200 Subject: [PATCH 1/6] Fix importing of duplicate assingmnents Signed-off-by: Alexandru Gheorghe --- polkadot/node/core/approval-voting/src/lib.rs | 2 +- .../core/approval-voting/src/persisted_entries.rs | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/polkadot/node/core/approval-voting/src/lib.rs b/polkadot/node/core/approval-voting/src/lib.rs index 7cea22d1a6a7..6c420681ae28 100644 --- a/polkadot/node/core/approval-voting/src/lib.rs +++ b/polkadot/node/core/approval-voting/src/lib.rs @@ -2681,7 +2681,7 @@ where )), }; is_duplicate &= approval_entry.is_assigned(assignment.validator); - approval_entry.import_assignment(tranche, assignment.validator, tick_now); + approval_entry.import_assignment(tranche, assignment.validator, tick_now, is_duplicate); // We've imported a new assignment, so we need to schedule a wake-up for when that might // no-show. diff --git a/polkadot/node/core/approval-voting/src/persisted_entries.rs b/polkadot/node/core/approval-voting/src/persisted_entries.rs index d891af01c3ab..354ed9a9148a 100644 --- a/polkadot/node/core/approval-voting/src/persisted_entries.rs +++ b/polkadot/node/core/approval-voting/src/persisted_entries.rs @@ -172,7 +172,7 @@ impl ApprovalEntry { }); our.map(|a| { - self.import_assignment(a.tranche(), a.validator_index(), tick_now); + self.import_assignment(a.tranche(), a.validator_index(), tick_now, false); (a.cert().clone(), a.validator_index(), a.tranche()) }) @@ -197,6 +197,7 @@ impl ApprovalEntry { tranche: DelayTranche, validator_index: ValidatorIndex, tick_now: Tick, + is_duplicate: bool, ) { // linear search probably faster than binary. not many tranches typically. let idx = match self.tranches.iter().position(|t| t.tranche >= tranche) { @@ -214,7 +215,14 @@ impl ApprovalEntry { }, }; - self.tranches[idx].assignments.push((validator_index, tick_now)); + if !is_duplicate || + !self.tranches[idx] + .assignments + .iter() + .any(|(validator, _)| validator == &validator_index) + { + self.tranches[idx].assignments.push((validator_index, tick_now)); + } self.assigned_validators.set(validator_index.0 as _, true); } From 443309f2e6bedb05be208cf1fee98f33ebcd928b Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 20 Dec 2024 12:11:06 +0200 Subject: [PATCH 2/6] Fix import duplicate on restart Signed-off-by: Alexandru Gheorghe --- .../approval-voting/src/approval_checking.rs | 78 ++++++++++++------- 1 file changed, 50 insertions(+), 28 deletions(-) diff --git a/polkadot/node/core/approval-voting/src/approval_checking.rs b/polkadot/node/core/approval-voting/src/approval_checking.rs index 3b7262a46826..c7f38619ea1f 100644 --- a/polkadot/node/core/approval-voting/src/approval_checking.rs +++ b/polkadot/node/core/approval-voting/src/approval_checking.rs @@ -712,13 +712,13 @@ mod tests { } .into(); - approval_entry.import_assignment(0, ValidatorIndex(0), block_tick); - approval_entry.import_assignment(0, ValidatorIndex(1), block_tick); + approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, false); + approval_entry.import_assignment(0, ValidatorIndex(1), block_tick, false); - approval_entry.import_assignment(1, ValidatorIndex(2), block_tick + 1); - approval_entry.import_assignment(1, ValidatorIndex(3), block_tick + 1); + approval_entry.import_assignment(1, ValidatorIndex(2), block_tick + 1, false); + approval_entry.import_assignment(1, ValidatorIndex(3), block_tick + 1, false); - approval_entry.import_assignment(2, ValidatorIndex(4), block_tick + 2); + approval_entry.import_assignment(2, ValidatorIndex(4), block_tick + 2, false); let approvals = bitvec![u8, BitOrderLsb0; 1; 5]; @@ -757,8 +757,10 @@ mod tests { } .into(); - approval_entry.import_assignment(0, ValidatorIndex(0), block_tick); - approval_entry.import_assignment(1, ValidatorIndex(2), block_tick); + approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, false); + approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, true); + approval_entry.import_assignment(1, ValidatorIndex(2), block_tick, false); + approval_entry.import_assignment(1, ValidatorIndex(2), block_tick, true); let approvals = bitvec![u8, BitOrderLsb0; 0; 10]; @@ -798,10 +800,10 @@ mod tests { } .into(); - approval_entry.import_assignment(0, ValidatorIndex(0), block_tick); - approval_entry.import_assignment(0, ValidatorIndex(1), block_tick); + approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, false); + approval_entry.import_assignment(0, ValidatorIndex(1), block_tick, false); - approval_entry.import_assignment(1, ValidatorIndex(2), block_tick); + approval_entry.import_assignment(1, ValidatorIndex(2), block_tick, false); let mut approvals = bitvec![u8, BitOrderLsb0; 0; 10]; approvals.set(0, true); @@ -844,11 +846,11 @@ mod tests { } .into(); - approval_entry.import_assignment(0, ValidatorIndex(0), block_tick); - approval_entry.import_assignment(0, ValidatorIndex(1), block_tick); + approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, false); + approval_entry.import_assignment(0, ValidatorIndex(1), block_tick, false); - approval_entry.import_assignment(1, ValidatorIndex(2), block_tick); - approval_entry.import_assignment(1, ValidatorIndex(3), block_tick); + approval_entry.import_assignment(1, ValidatorIndex(2), block_tick, false); + approval_entry.import_assignment(1, ValidatorIndex(3), block_tick, false); let mut approvals = bitvec![u8, BitOrderLsb0; 0; n_validators]; approvals.set(0, true); @@ -913,14 +915,24 @@ mod tests { } .into(); - approval_entry.import_assignment(0, ValidatorIndex(0), block_tick); - approval_entry.import_assignment(0, ValidatorIndex(1), block_tick); + approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, false); + approval_entry.import_assignment(0, ValidatorIndex(1), block_tick, false); - approval_entry.import_assignment(1, ValidatorIndex(2), block_tick + 1); - approval_entry.import_assignment(1, ValidatorIndex(3), block_tick + 1); + approval_entry.import_assignment(1, ValidatorIndex(2), block_tick + 1, false); + approval_entry.import_assignment(1, ValidatorIndex(3), block_tick + 1, false); - approval_entry.import_assignment(2, ValidatorIndex(4), block_tick + no_show_duration + 2); - approval_entry.import_assignment(2, ValidatorIndex(5), block_tick + no_show_duration + 2); + approval_entry.import_assignment( + 2, + ValidatorIndex(4), + block_tick + no_show_duration + 2, + false, + ); + approval_entry.import_assignment( + 2, + ValidatorIndex(5), + block_tick + no_show_duration + 2, + false, + ); let mut approvals = bitvec![u8, BitOrderLsb0; 0; n_validators]; approvals.set(0, true); @@ -1007,14 +1019,24 @@ mod tests { } .into(); - approval_entry.import_assignment(0, ValidatorIndex(0), block_tick); - approval_entry.import_assignment(0, ValidatorIndex(1), block_tick); + approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, false); + approval_entry.import_assignment(0, ValidatorIndex(1), block_tick, false); - approval_entry.import_assignment(1, ValidatorIndex(2), block_tick + 1); - approval_entry.import_assignment(1, ValidatorIndex(3), block_tick + 1); + approval_entry.import_assignment(1, ValidatorIndex(2), block_tick + 1, false); + approval_entry.import_assignment(1, ValidatorIndex(3), block_tick + 1, false); - approval_entry.import_assignment(2, ValidatorIndex(4), block_tick + no_show_duration + 2); - approval_entry.import_assignment(2, ValidatorIndex(5), block_tick + no_show_duration + 2); + approval_entry.import_assignment( + 2, + ValidatorIndex(4), + block_tick + no_show_duration + 2, + false, + ); + approval_entry.import_assignment( + 2, + ValidatorIndex(5), + block_tick + no_show_duration + 2, + false, + ); let mut approvals = bitvec![u8, BitOrderLsb0; 0; n_validators]; approvals.set(0, true); @@ -1066,7 +1088,7 @@ mod tests { }, ); - approval_entry.import_assignment(3, ValidatorIndex(6), block_tick); + approval_entry.import_assignment(3, ValidatorIndex(6), block_tick, false); approvals.set(6, true); let tranche_now = no_show_duration as DelayTranche + 3; @@ -1176,7 +1198,7 @@ mod tests { // Populate the requested tranches. The assignments aren't inspected in // this test. for &t in &test_tranche { - approval_entry.import_assignment(t, ValidatorIndex(0), 0) + approval_entry.import_assignment(t, ValidatorIndex(0), 0, false); } let filled_tranches = filled_tranche_iterator(approval_entry.tranches()); From 4e2788b4361251cfc3aa6a6d4b760baf2f33cf65 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Tue, 7 Jan 2025 13:09:16 +0200 Subject: [PATCH 3/6] Minor fixes Signed-off-by: Alexandru Gheorghe --- polkadot/node/core/approval-voting/src/persisted_entries.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/polkadot/node/core/approval-voting/src/persisted_entries.rs b/polkadot/node/core/approval-voting/src/persisted_entries.rs index 354ed9a9148a..1f3bbae69390 100644 --- a/polkadot/node/core/approval-voting/src/persisted_entries.rs +++ b/polkadot/node/core/approval-voting/src/persisted_entries.rs @@ -214,7 +214,9 @@ impl ApprovalEntry { self.tranches.len() - 1 }, }; - + // We already know already if we seen already an assignment from this validator + // and since this function is on the hot path we can avoid going through tranches + // if the assignment is not a duplicate. if !is_duplicate || !self.tranches[idx] .assignments From a49f7e03a7fbd31f1640a820ce3846881722fe74 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe <49718502+alexggh@users.noreply.github.com> Date: Tue, 7 Jan 2025 13:18:28 +0200 Subject: [PATCH 4/6] Update polkadot/node/core/approval-voting/src/persisted_entries.rs Co-authored-by: ordian --- polkadot/node/core/approval-voting/src/persisted_entries.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/node/core/approval-voting/src/persisted_entries.rs b/polkadot/node/core/approval-voting/src/persisted_entries.rs index 1f3bbae69390..1febe9c1faed 100644 --- a/polkadot/node/core/approval-voting/src/persisted_entries.rs +++ b/polkadot/node/core/approval-voting/src/persisted_entries.rs @@ -214,7 +214,7 @@ impl ApprovalEntry { self.tranches.len() - 1 }, }; - // We already know already if we seen already an assignment from this validator + // We already know if we have seen an assignment from this validator // and since this function is on the hot path we can avoid going through tranches // if the assignment is not a duplicate. if !is_duplicate || From 1a18e5753fb7f5513bbb0266a190efcba62d6c0e Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Mon, 13 Jan 2025 14:51:53 +0200 Subject: [PATCH 5/6] Remove redundant checks Signed-off-by: Alexandru Gheorghe --- polkadot/node/core/approval-voting/src/lib.rs | 11 +++++++++-- .../approval-voting/src/persisted_entries.rs | 16 +++++++--------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/polkadot/node/core/approval-voting/src/lib.rs b/polkadot/node/core/approval-voting/src/lib.rs index 6c420681ae28..eb236f42fca2 100644 --- a/polkadot/node/core/approval-voting/src/lib.rs +++ b/polkadot/node/core/approval-voting/src/lib.rs @@ -2680,8 +2680,15 @@ where Vec::new(), )), }; - is_duplicate &= approval_entry.is_assigned(assignment.validator); - approval_entry.import_assignment(tranche, assignment.validator, tick_now, is_duplicate); + + let is_duplicate_for_candidate = approval_entry.is_assigned(assignment.validator); + is_duplicate &= is_duplicate_for_candidate; + approval_entry.import_assignment( + tranche, + assignment.validator, + tick_now, + is_duplicate_for_candidate, + ); // We've imported a new assignment, so we need to schedule a wake-up for when that might // no-show. diff --git a/polkadot/node/core/approval-voting/src/persisted_entries.rs b/polkadot/node/core/approval-voting/src/persisted_entries.rs index 1febe9c1faed..238e2c3706ea 100644 --- a/polkadot/node/core/approval-voting/src/persisted_entries.rs +++ b/polkadot/node/core/approval-voting/src/persisted_entries.rs @@ -214,15 +214,13 @@ impl ApprovalEntry { self.tranches.len() - 1 }, }; - // We already know if we have seen an assignment from this validator - // and since this function is on the hot path we can avoid going through tranches - // if the assignment is not a duplicate. - if !is_duplicate || - !self.tranches[idx] - .assignments - .iter() - .any(|(validator, _)| validator == &validator_index) - { + // At restart we might have duplicate assignments because approval-distribution is not + // persistent across restarts, so avoid adding duplicates. + // We already know if we have seen an assignment from this validator and since this + // function is on the hot path we can avoid iterating through tranches by using + // !is_duplicate to determine if it is already present in the vector and does not need + // adding. + if !is_duplicate { self.tranches[idx].assignments.push((validator_index, tick_now)); } self.assigned_validators.set(validator_index.0 as _, true); From 8d610cecc662eafb57232e7c1cb59c68ba8b8eb4 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Mon, 13 Jan 2025 17:15:57 +0200 Subject: [PATCH 6/6] Add prdoc Signed-off-by: Alexandru Gheorghe --- prdoc/pr_6971.prdoc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 prdoc/pr_6971.prdoc diff --git a/prdoc/pr_6971.prdoc b/prdoc/pr_6971.prdoc new file mode 100644 index 000000000000..4790d773fee4 --- /dev/null +++ b/prdoc/pr_6971.prdoc @@ -0,0 +1,16 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Make importing of duplicate assignment idempotent + +doc: + - audience: Node Dev + description: | + Normally, approval-voting wouldn't receive duplicate assignments because approval-distribution makes + sure of it, however in the situation where we restart we might receive the same assignment again and + since approval-voting already persisted it we will end up inserting it twice in ApprovalEntry.tranches.assignments + because that's an array. Fix this by inserting only assignments that are not duplicate. + +crates: + - name: polkadot-node-core-approval-voting + bump: minor