Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Revert "Trim compact solution for length during preparation" #8613

Closed
gui1117 wants to merge 1 commit into from
Closed

Revert "Trim compact solution for length during preparation" #8613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions bin/node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,11 +517,6 @@ parameter_types! {
.get(DispatchClass::Normal)
.max_extrinsic.expect("Normal extrinsics have a weight limit configured; qed")
.saturating_sub(BlockExecutionWeight::get());
// Solution can occupy 90% of normal block size
pub MinerMaxLength: u32 = Perbill::from_rational(9u32, 10) *
*RuntimeBlockLength::get()
.max
.get(DispatchClass::Normal);
}

sp_npos_elections::generate_solution_type!(
Expand All @@ -544,7 +539,6 @@ impl pallet_election_provider_multi_phase::Config for Runtime {
type SolutionImprovementThreshold = SolutionImprovementThreshold;
type MinerMaxIterations = MinerMaxIterations;
type MinerMaxWeight = MinerMaxWeight;
type MinerMaxLength = MinerMaxLength;
type MinerTxPriority = MultiPhaseUnsignedPriority;
type DataProvider = Staking;
type OnChainAccuracy = Perbill;
Expand Down
9 changes: 1 addition & 8 deletions frame/election-provider-multi-phase/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,19 +534,12 @@ pub mod pallet {
/// Maximum number of iteration of balancing that will be executed in the embedded miner of
/// the pallet.
type MinerMaxIterations: Get<u32>;

/// Maximum weight that the miner should consume.
///
/// The miner will ensure that the total weight of the unsigned solution will not exceed
/// this value, based on [`WeightInfo::submit_unsigned`].
/// this values, based on [`WeightInfo::submit_unsigned`].
type MinerMaxWeight: Get<Weight>;

/// Maximum length (bytes) that the mined solution should consume.
///
/// The miner will ensure that the total length of the unsigned solution will not exceed
/// this value.
type MinerMaxLength: Get<u32>;

/// Something that will provide the election data.
type DataProvider: ElectionDataProvider<Self::AccountId, Self::BlockNumber>;

Expand Down
2 changes: 0 additions & 2 deletions frame/election-provider-multi-phase/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,6 @@ parameter_types! {
pub static MinerTxPriority: u64 = 100;
pub static SolutionImprovementThreshold: Perbill = Perbill::zero();
pub static MinerMaxWeight: Weight = BlockWeights::get().max_block;
pub static MinerMaxLength: u32 = 256;
pub static MockWeightInfo: bool = false;


Expand Down Expand Up @@ -278,7 +277,6 @@ impl crate::Config for Runtime {
type SolutionImprovementThreshold = SolutionImprovementThreshold;
type MinerMaxIterations = MinerMaxIterations;
type MinerMaxWeight = MinerMaxWeight;
type MinerMaxLength = MinerMaxLength;
type MinerTxPriority = MinerTxPriority;
type DataProvider = StakingMock;
type WeightInfo = DualMockWeightInfo;
Expand Down
170 changes: 3 additions & 167 deletions frame/election-provider-multi-phase/src/unsigned.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@ pub enum MinerError {
PreDispatchChecksFailed,
/// The solution generated from the miner is not feasible.
Feasibility(FeasibilityError),
/// There are no more voters to remove to trim the solution.
NoMoreVoters,
}

impl From<sp_npos_elections::Error> for MinerError {
Expand Down Expand Up @@ -179,13 +177,8 @@ impl<T: Config> Pallet<T> {
maximum_allowed_voters,
);

// trim length and weight
let compact = Self::trim_compact_weight(maximum_allowed_voters, compact, &voter_index)?;
let compact = Self::trim_compact_length(
T::MinerMaxLength::get(),
compact,
&voter_index,
)?;
// trim weight.
let compact = Self::trim_compact(maximum_allowed_voters, compact, &voter_index)?;

// re-calc score.
let winners = sp_npos_elections::to_without_backing(winners);
Expand Down Expand Up @@ -228,7 +221,7 @@ impl<T: Config> Pallet<T> {
///
/// Indeed, the score must be computed **after** this step. If this step reduces the score too
/// much or remove a winner, then the solution must be discarded **after** this step.
pub fn trim_compact_weight<FN>(
pub fn trim_compact<FN>(
maximum_allowed_voters: u32,
mut compact: CompactOf<T>,
voter_index: FN,
Expand Down Expand Up @@ -274,50 +267,6 @@ impl<T: Config> Pallet<T> {
}
}

/// Greedily reduce the size of the solution to fit into the block w.r.t length.
///
/// The length of the solution is largely a function of the number of voters. The number of
/// winners cannot be changed. Thus, to reduce the solution size, we need to strip voters.
///
/// Note that this solution is already computed, and winners are elected based on the merit of
/// the total stake in the system. Nevertheless, some of the voters may be removed here.
///
/// Sometimes, removing a voter can cause a validator to also be implicitly removed, if
/// that voter was the only backer of that winner. In such cases, this solution is invalid, which
/// will be caught prior to submission.
///
/// The score must be computed **after** this step. If this step reduces the score too much,
/// then the solution must be discarded.
pub fn trim_compact_length(
max_allowed_length: u32,
mut compact: CompactOf<T>,
voter_index: impl Fn(&T::AccountId) -> Option<CompactVoterIndexOf<T>>,
) -> Result<CompactOf<T>, MinerError> {
// short-circuit to avoid getting the voters if possible
// this involves a redundant encoding, but that should hopefully be relatively cheap
if (compact.encoded_size().saturated_into::<u32>()) <= max_allowed_length {
return Ok(compact);
}

// grab all voters and sort them by least stake.
let RoundSnapshot { voters, .. } =
Self::snapshot().ok_or(MinerError::SnapshotUnAvailable)?;
let mut voters_sorted = voters
.into_iter()
.map(|(who, stake, _)| (who.clone(), stake))
.collect::<Vec<_>>();
voters_sorted.sort_by_key(|(_, y)| *y);
voters_sorted.reverse();

while compact.encoded_size() > max_allowed_length.saturated_into() {
let (smallest_stake_voter, _) = voters_sorted.pop().ok_or(MinerError::NoMoreVoters)?;
let index = voter_index(&smallest_stake_voter).ok_or(MinerError::SnapshotUnAvailable)?;
compact.remove_voter(index);
}

Ok(compact)
}

/// Find the maximum `len` that a compact can have in order to fit into the block weight.
///
/// This only returns a value between zero and `size.nominators`.
Expand Down Expand Up @@ -557,7 +506,6 @@ mod tests {
Call, *,
};
use frame_support::{dispatch::Dispatchable, traits::OffchainWorker};
use helpers::voter_index_fn_linear;
use mock::Call as OuterCall;
use frame_election_provider_support::Assignment;
use sp_runtime::{traits::ValidateUnsigned, PerU16};
Expand Down Expand Up @@ -941,116 +889,4 @@ mod tests {
assert!(matches!(call, OuterCall::MultiPhase(Call::submit_unsigned(_, _))));
})
}

#[test]
fn trim_compact_length_does_not_modify_when_short_enough() {
let mut ext = ExtBuilder::default().build();
ext.execute_with(|| {
roll_to(25);

// given
let RoundSnapshot { voters, ..} = MultiPhase::snapshot().unwrap();
let RawSolution { mut compact, .. } = raw_solution();
let encoded_len = compact.encode().len() as u32;
let compact_clone = compact.clone();

// when
assert!(encoded_len < <Runtime as Config>::MinerMaxLength::get());

// then
compact = MultiPhase::trim_compact_length(
encoded_len,
compact,
voter_index_fn_linear::<Runtime>(&voters),
).unwrap();
assert_eq!(compact, compact_clone);
});
}

#[test]
fn trim_compact_length_modifies_when_too_long() {
let mut ext = ExtBuilder::default().build();
ext.execute_with(|| {
roll_to(25);

let RoundSnapshot { voters, ..} =
MultiPhase::snapshot().unwrap();

let RawSolution { mut compact, .. } = raw_solution();
let encoded_len = compact.encoded_size() as u32;
let compact_clone = compact.clone();

compact = MultiPhase::trim_compact_length(
encoded_len - 1,
compact,
voter_index_fn_linear::<Runtime>(&voters),
).unwrap();

assert_ne!(compact, compact_clone);
assert!((compact.encoded_size() as u32) < encoded_len);
});
}

#[test]
fn trim_compact_length_trims_lowest_stake() {
let mut ext = ExtBuilder::default().build();
ext.execute_with(|| {
roll_to(25);

let RoundSnapshot { voters, ..} =
MultiPhase::snapshot().unwrap();

let RawSolution { mut compact, .. } = raw_solution();
let encoded_len = compact.encoded_size() as u32;
let voter_count = compact.voter_count();
let min_stake_voter = voters.iter()
.map(|(id, weight, _)| (weight, id))
.min()
.map(|(_, id)| id)
.unwrap();


compact = MultiPhase::trim_compact_length(
encoded_len - 1,
compact,
voter_index_fn_linear::<Runtime>(&voters),
).unwrap();

assert_eq!(compact.voter_count(), voter_count - 1, "we must have removed exactly 1 voter");

let assignments = compact.into_assignment(
|voter| Some(voter as AccountId),
|target| Some(target as AccountId),
).unwrap();
assert!(
assignments.iter()
.all(|Assignment{ who, ..}| who != min_stake_voter),
"min_stake_voter must no longer be in the set of voters",
);
});
}

// all the other solution-generation functions end up delegating to `mine_solution`, so if we
// demonstrate that `mine_solution` solutions are all trimmed to an acceptable length, then
// we know that higher-level functions will all also have short-enough solutions.
#[test]
fn mine_solution_solutions_always_within_acceptable_length() {
let mut ext = ExtBuilder::default().build();
ext.execute_with(|| {
roll_to(25);

// how long would the default solution be?
let solution = MultiPhase::mine_solution(0).unwrap();
let max_length = <Runtime as Config>::MinerMaxLength::get();
let solution_size = solution.0.compact.encoded_size();
assert!(solution_size <= max_length as usize);

// now set the max size to less than the actual size and regenerate
<Runtime as Config>::MinerMaxLength::set(solution_size as u32 - 1);
let solution = MultiPhase::mine_solution(0).unwrap();
let max_length = <Runtime as Config>::MinerMaxLength::get();
let solution_size = solution.0.compact.encoded_size();
assert!(solution_size <= max_length as usize);
});
}
}