diff --git a/prdoc/pr_8585.prdoc b/prdoc/pr_8585.prdoc new file mode 100644 index 0000000000000..4eb7f25b0a935 --- /dev/null +++ b/prdoc/pr_8585.prdoc @@ -0,0 +1,14 @@ +title: fix epmb solution duplicate issue + add remote mining apparatus to epm +doc: +- audience: Runtime Dev + description: | + Prevents the NPoS election process from accepting duplicate voters and targets. +crates: +- name: pallet-election-provider-multi-phase + bump: minor +- name: frame-election-provider-solution-type + bump: patch +- name: frame-election-provider-support + bump: patch +- name: sp-npos-elections + bump: minor diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index 1b43385bd6bc4..3566d870f597d 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -469,9 +469,9 @@ where /// These are stored together because they are often accessed together. #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default, TypeInfo)] #[scale_info(skip_type_params(T))] -pub struct RoundSnapshot { +pub struct RoundSnapshot { /// All of the voters. - pub voters: Vec, + pub voters: Vec, /// All of the targets. pub targets: Vec, } diff --git a/substrate/frame/election-provider-multi-phase/src/unsigned.rs b/substrate/frame/election-provider-multi-phase/src/unsigned.rs index 191131ed3acc3..451f22b502849 100644 --- a/substrate/frame/election-provider-multi-phase/src/unsigned.rs +++ b/substrate/frame/election-provider-multi-phase/src/unsigned.rs @@ -551,6 +551,13 @@ impl Miner { let is_trimmed = TrimmingStatus { weight: weight_trimmed, length: length_trimmed }; + log_no_system!( + debug, + "feasible solution mined: trimmed? {:?}, score: {:?}, encoded size: {:?}", + is_trimmed, + score, + solution.encoded_size() + ); Ok((solution, score, size, is_trimmed)) } diff --git a/substrate/frame/election-provider-support/solution-type/src/single_page.rs b/substrate/frame/election-provider-support/solution-type/src/single_page.rs index c921be34b3430..e64b99b073bae 100644 --- a/substrate/frame/election-provider-support/solution-type/src/single_page.rs +++ b/substrate/frame/election-provider-support/solution-type/src/single_page.rs @@ -153,9 +153,9 @@ pub(crate) fn generate(def: crate::SolutionDef) -> Result { voter_at: impl Fn(Self::VoterIndex) -> Option, target_at: impl Fn(Self::TargetIndex) -> Option, ) -> Result<_fepsp::Vec<_feps::Assignment>, _feps::Error> { - let mut #assignment_name: _fepsp::Vec<_feps::Assignment> = Default::default(); + let mut #assignment_name: _fepsp::BTreeMap> = Default::default(); #into_impl - Ok(#assignment_name) + Ok(#assignment_name.into_values().collect()) } fn voter_count(&self) -> usize { @@ -426,13 +426,18 @@ pub(crate) fn into_impl( let into_impl_single = { let name = vote_field(1); quote!( - for (voter_index, target_index) in self.#name { - #assignments.push(_feps::Assignment { - who: voter_at(voter_index).or_invalid_index()?, - distribution: vec![ - (target_at(target_index).or_invalid_index()?, #per_thing::one()) - ], - }) + for (voter_index, target_index) in self.#name {; + if #assignments.contains_key(&voter_index) { + return Err(_feps::Error::DuplicateVoter); + } else { + #assignments.insert( + voter_index, + _feps::Assignment { + who: voter_at(voter_index).or_invalid_index()?, + distribution: vec![(target_at(target_index).or_invalid_index()?, #per_thing::one())], + } + ); + } } ) }; @@ -442,10 +447,21 @@ pub(crate) fn into_impl( let name = vote_field(c); quote!( for (voter_index, inners, t_last_idx) in self.#name { + if #assignments.contains_key(&voter_index) { + return Err(_feps::Error::DuplicateVoter); + } + + let mut targets_seen = _fepsp::BTreeSet::new(); + let mut sum = #per_thing::zero(); let mut inners_parsed = inners .iter() .map(|(ref t_idx, p)| { + if targets_seen.contains(t_idx) { + return Err(_feps::Error::DuplicateTarget); + } else { + targets_seen.insert(t_idx); + } sum = _fepsp::sp_arithmetic::traits::Saturating::saturating_add(sum, *p); let target = target_at(*t_idx).or_invalid_index()?; Ok((target, *p)) @@ -456,6 +472,13 @@ pub(crate) fn into_impl( return Err(_feps::Error::SolutionWeightOverflow); } + // check that the last target index is also unique. + if targets_seen.contains(&t_last_idx) { + return Err(_feps::Error::DuplicateTarget); + } else { + // no need to insert, we are done. + } + // defensive only. Since Percent doesn't have `Sub`. let p_last = _fepsp::sp_arithmetic::traits::Saturating::saturating_sub( #per_thing::one(), @@ -464,10 +487,13 @@ pub(crate) fn into_impl( inners_parsed.push((target_at(t_last_idx).or_invalid_index()?, p_last)); - #assignments.push(_feps::Assignment { - who: voter_at(voter_index).or_invalid_index()?, - distribution: inners_parsed, - }); + #assignments.insert( + voter_index, + _feps::Assignment { + who: voter_at(voter_index).or_invalid_index()?, + distribution: inners_parsed, + } + ); } ) }) diff --git a/substrate/frame/election-provider-support/src/lib.rs b/substrate/frame/election-provider-support/src/lib.rs index ba081aa533ffd..617b060f3e99a 100644 --- a/substrate/frame/election-provider-support/src/lib.rs +++ b/substrate/frame/election-provider-support/src/lib.rs @@ -205,7 +205,10 @@ use sp_runtime::TryRuntimeError; // re-export for the solution macro, with the dependencies of the macro. #[doc(hidden)] pub mod private { - pub use alloc::{collections::btree_set::BTreeSet, vec::Vec}; + pub use alloc::{ + collections::{btree_map::BTreeMap, btree_set::BTreeSet}, + vec::Vec, + }; pub use codec; pub use scale_info; pub use sp_arithmetic; diff --git a/substrate/frame/election-provider-support/src/tests.rs b/substrate/frame/election-provider-support/src/tests.rs index 6e3deb9e38346..849017ee2a1d7 100644 --- a/substrate/frame/election-provider-support/src/tests.rs +++ b/substrate/frame/election-provider-support/src/tests.rs @@ -244,6 +244,65 @@ mod solution_type { ); } + #[test] + fn prevents_target_duplicate_into_assignment() { + let voter_at = |a: u32| -> Option { Some(a as AccountId) }; + let target_at = |a: u16| -> Option { Some(a as AccountId) }; + + // case 1: duplicate target in votes2. + let solution = TestSolution { votes2: vec![(0, [(1, p(50))], 1)], ..Default::default() }; + assert_eq!( + solution.into_assignment(&voter_at, &target_at).unwrap_err(), + NposError::DuplicateTarget, + ); + + // case 2: duplicate target in votes3. + let solution = + TestSolution { votes3: vec![(0, [(1, p(25)), (2, p(50))], 1)], ..Default::default() }; + assert_eq!( + solution.into_assignment(&voter_at, &target_at).unwrap_err(), + NposError::DuplicateTarget, + ); + } + + #[test] + fn prevents_voter_duplicate_into_assignment() { + let voter_at = |a: u32| -> Option { Some(a as AccountId) }; + let target_at = |a: u16| -> Option { Some(a as AccountId) }; + + // case 1: there is a duplicate among two different fields + let solution = TestSolution { + // voter index 0 is present here + votes1: vec![(0, 0), (1, 0)], + // voter index 0 is also present here + votes2: vec![(0, [(1, p(50))], 2)], + ..Default::default() + }; + + assert_eq!( + solution.into_assignment(&voter_at, &target_at).unwrap_err(), + NposError::DuplicateVoter, + ); + + // case 2: there is a duplicate in the same field + let solution = TestSolution { votes1: vec![(0, 0), (0, 1)], ..Default::default() }; + assert_eq!( + solution.into_assignment(&voter_at, &target_at).unwrap_err(), + NposError::DuplicateVoter, + ); + + // case 2.1: there is a duplicate in the same fieild, a bit more complex + let solution = TestSolution { + votes1: vec![(0, 0)], + votes2: vec![(1, [(1, p(50))], 2), (1, [(3, p(50))], 4)], + ..Default::default() + }; + assert_eq!( + solution.into_assignment(&voter_at, &target_at).unwrap_err(), + NposError::DuplicateVoter, + ); + } + #[test] fn from_and_into_assignment_works() { let voters = vec![2 as AccountId, 4, 1, 5, 3]; diff --git a/substrate/primitives/npos-elections/src/lib.rs b/substrate/primitives/npos-elections/src/lib.rs index f5a8ccb4351a4..53cb087c1a4cf 100644 --- a/substrate/primitives/npos-elections/src/lib.rs +++ b/substrate/primitives/npos-elections/src/lib.rs @@ -127,6 +127,10 @@ pub enum Error { InvalidSupportEdge, /// The number of voters is bigger than the `MaxVoters` bound. TooManyVoters, + /// A duplicate voter was detected. + DuplicateVoter, + /// A duplicate target was detected. + DuplicateTarget, } /// A type which is used in the API of this crate as a numeric weight of a vote, most often the