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()); diff --git a/polkadot/node/core/approval-voting/src/lib.rs b/polkadot/node/core/approval-voting/src/lib.rs index 7cea22d1a6a7..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); + + 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 d891af01c3ab..238e2c3706ea 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) { @@ -213,8 +214,15 @@ impl ApprovalEntry { self.tranches.len() - 1 }, }; - - self.tranches[idx].assignments.push((validator_index, tick_now)); + // 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); } 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