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

election-provider-multi-phase: Add extrinsic to challenge signed submissions #11099

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
9 changes: 9 additions & 0 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -571,8 +571,10 @@ parameter_types! {

// signed config
pub const SignedRewardBase: Balance = 1 * DOLLARS;
pub const ChallengeRewardBase: Balance = 1 * CENTS;
pub const SignedDepositBase: Balance = 1 * DOLLARS;
pub const SignedDepositByte: Balance = 1 * CENTS;
pub const MinimumSlashableAmount: Balance = 1 * DOLLARS;

pub SolutionImprovementThreshold: Perbill = Perbill::from_rational(1u32, 10_000);

Expand Down Expand Up @@ -670,6 +672,7 @@ impl pallet_election_provider_multi_phase::Config for Runtime {
type MinerTxPriority = MultiPhaseUnsignedPriority;
type SignedMaxSubmissions = ConstU32<10>;
type SignedRewardBase = SignedRewardBase;
type ChallengeRewardBase = ChallengeRewardBase;
type SignedDepositBase = SignedDepositBase;
type SignedDepositByte = SignedDepositByte;
type SignedDepositWeight = ();
Expand All @@ -685,7 +688,13 @@ impl pallet_election_provider_multi_phase::Config for Runtime {
type MaxElectableTargets = ConstU16<{ u16::MAX }>;
type MaxElectingVoters = MaxElectingVoters;
type BenchmarkingConfig = ElectionProviderBenchmarkConfig;
// BagsList allows a practically unbounded count of nominators to participate in NPoS elections.
// To ensure we respect memory limits when using the BagsList this must be set to a number of
// voters we know can fit into a single vec allocation.
type VoterSnapshotPerBlock = ConstU32<10_000>;
type MinimumSlashableAmount = MinimumSlashableAmount;
type WeightInfo = pallet_election_provider_multi_phase::weights::SubstrateWeight<Self>;

}

parameter_types! {
Expand Down
53 changes: 52 additions & 1 deletion frame/election-provider-multi-phase/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,10 @@ pub mod pallet {
#[pallet::constant]
type SignedRewardBase: Get<BalanceOf<Self>>;

// Base reward for a challenger
#[pallet::constant]
type ChallengeRewardBase: Get<BalanceOf<Self>>;
Copy link
Contributor

@kianenigma kianenigma Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reward of the challenger should be the deposit of the challengee. @emostov wdyt? trying to avoid adding more and more configs.

Copy link
Contributor

@emostov emostov Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reward needs to be less than the deposit so we don't encourage users to spam the system by submitting a bogus solution, then challenging it and claiming back their own solution and claiming back their deposit.

To achieve this we can have a config that is something like ChallengeDepositDiff, e.g. if the deposit is 100 and ChallengeDepositDiff is 5, then reward base would be 95

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. The reward can just be a Perbill of the deposit then? I think this is slightly more intuitive than a diff.


/// Base deposit for a signed solution.
#[pallet::constant]
type SignedDepositBase: Get<BalanceOf<Self>>;
Expand Down Expand Up @@ -708,6 +712,11 @@ pub mod pallet {

/// The weight of the pallet.
type WeightInfo: WeightInfo;

/// The minimum amount a solution challenger must have to execute
/// `challange_submission`
#[pallet::constant]
type MinimumSlashableAmount: Get<BalanceOf<Self>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, I think the slashable deposit of the challenger should be dynamic and should be simply the same as the amount of the deposit of the solution that you are going after.

}

#[pallet::hooks]
Expand Down Expand Up @@ -850,7 +859,7 @@ pub mod pallet {
impl<T: Config> Pallet<T> {
/// Submit a solution for the unsigned phase.
///
/// The dispatch origin fo this call must be __none__.
/// The dispatch origin for this call must be __none__.
///
/// This submission is checked on the fly. Moreover, this unsigned solution is only
/// validated when submitted to the pool from the **local** node. Effectively, this means
Expand Down Expand Up @@ -1055,6 +1064,44 @@ pub mod pallet {
<QueuedSolution<T>>::put(solution);
Ok(())
}

#[pallet::weight(100)]
pub fn challenge_solution(origin: OriginFor<T>, index: u32) -> DispatchResult {
let who = ensure_signed(origin)?;
// TODO: Create custom error
ensure!(
T::Currency::can_slash(&who, T::MinimumSlashableAmount::get()),
<Error<T>>::CallNotAllowed
);
// TODO: Add an `else` for a None result
if let Some(submission) = SignedSubmissionsMap::<T>::get(index) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue: do not use the storage maps directly, see get_submission in SignedSubmissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd have to make get_submission public

match Self::feasibility_check(
submission.raw_solution.clone(),
ElectionCompute::Signed,
) {
Ok(solution) => {
let _ = T::Currency::slash(&who, T::MinimumSlashableAmount::get());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like Kian mention, T::MinimumSlashableAmount::get() should be replaced with the deposit for the solution

CheckedSolutions::<T>::insert(submission, solution);
Ok(())
},
Err(_error) => {
T::Currency::slash_reserved(&who, submission.deposit);
SignedSubmissionsMap::<T>::remove(index);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As pointed out before, this is still not complete. You need to remove the item from all 2 corresponding storage items. All in all, you should never use these storage items directly. Instead, you should only work toward creating clear abstractions in the SignedSubmissions struct wrapper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the newly created method pop still applies here?

let reward = {
let call = Call::challenge_solution::<T> { index };
let call_fee =
T::EstimateCallFee::estimate_call_fee(&call, None.into());
T::ChallengeRewardBase::get().saturating_add(call_fee)
};
let positive_imbalance = T::Currency::deposit_creating(&who, reward);
T::RewardHandler::on_unbalanced(positive_imbalance);
Err(Error::<T>::CallNotAllowed.into())
},
}
} else {
return Err(Error::<T>::CallNotAllowed.into())
}
}
}

#[pallet::event]
Expand Down Expand Up @@ -1240,6 +1287,10 @@ pub mod pallet {
pub type SignedSubmissionsMap<T: Config> =
StorageMap<_, Twox64Concat, u32, SignedSubmissionOf<T>, OptionQuery>;

#[pallet::storage]
pub type CheckedSolutions<T: Config> =
StorageMap<_, Twox64Concat, SignedSubmissionOf<T>, ReadySolution<T::AccountId>>;

// `SignedSubmissions` items end here.

/// The minimum score that each 'untrusted' solution must attain in order to be considered
Expand Down
7 changes: 6 additions & 1 deletion frame/election-provider-multi-phase/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ parameter_types! {
pub static UnsignedPhase: BlockNumber = 5;
pub static SignedMaxSubmissions: u32 = 5;
pub static SignedDepositBase: Balance = 5;
pub static ChallengeRewardBase: Balance = 6;
pub static SignedDepositByte: Balance = 0;
pub static SignedDepositWeight: Balance = 0;
pub static SignedRewardBase: Balance = 7;
Expand All @@ -272,7 +273,9 @@ parameter_types! {
pub static MaxElectableTargets: TargetIndex = TargetIndex::max_value();

pub static EpochLength: u64 = 30;
pub static OnChainFallback: bool = true;

pub static OnChianFallback: bool = true;
pub static MinimumSlashableAmount: Balance = 3;
}

pub struct OnChainSeqPhragmen;
Expand Down Expand Up @@ -419,6 +422,7 @@ impl crate::Config for Runtime {
type MinerMaxLength = MinerMaxLength;
type MinerTxPriority = MinerTxPriority;
type SignedRewardBase = SignedRewardBase;
type ChallengeRewardBase = ChallengeRewardBase;
type SignedDepositBase = SignedDepositBase;
type SignedDepositByte = ();
type SignedDepositWeight = ();
Expand All @@ -436,6 +440,7 @@ impl crate::Config for Runtime {
type MaxElectingVoters = MaxElectingVoters;
type MaxElectableTargets = MaxElectableTargets;
type Solver = SequentialPhragmen<AccountId, SolutionAccuracyOf<Runtime>, Balancing>;
type MinimumSlashableAmount = MinimumSlashableAmount;
}

impl<LocalCall> frame_system::offchain::SendTransactionTypes<LocalCall> for Runtime
Expand Down