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

Improve call, and usage in pallet utility #9418

Merged
12 commits merged into from
Aug 7, 2021
10 changes: 10 additions & 0 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1679,4 +1679,14 @@ mod tests {

is_submit_signed_transaction::<Runtime>();
}

#[test]
fn call_size() {
gui1117 marked this conversation as resolved.
Show resolved Hide resolved
assert!(
core::mem::size_of::<Call>() <= 200,
Copy link
Member

Choose a reason for hiding this comment

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

200 is an arbitrary number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes IIRC it is the same number as clippy lint

Copy link
Contributor

Choose a reason for hiding this comment

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

what clippy lint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"size of Call is more than 200 bytes: some calls have too big arguments, use Box to reduce the
size of Call.
If the limit is too strong, maybe consider increase the limit to 300.",
);
}
}
3 changes: 2 additions & 1 deletion frame/babe/src/equivocation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,8 @@ where
) -> DispatchResult {
use frame_system::offchain::SubmitTransaction;

let call = Call::report_equivocation_unsigned(equivocation_proof, key_owner_proof);
let call =
Call::report_equivocation_unsigned(Box::new(equivocation_proof), key_owner_proof);

match SubmitTransaction::<T, Call<T>>::submit_unsigned_transaction(call.into()) {
Ok(()) => log::info!(
Expand Down
8 changes: 4 additions & 4 deletions frame/babe/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,12 +349,12 @@ pub mod pallet {
))]
pub fn report_equivocation(
origin: OriginFor<T>,
equivocation_proof: EquivocationProof<T::Header>,
equivocation_proof: Box<EquivocationProof<T::Header>>,
key_owner_proof: T::KeyOwnerProof,
) -> DispatchResultWithPostInfo {
let reporter = ensure_signed(origin)?;

Self::do_report_equivocation(Some(reporter), equivocation_proof, key_owner_proof)
Self::do_report_equivocation(Some(reporter), *equivocation_proof, key_owner_proof)
}

/// Report authority equivocation/misbehavior. This method will verify
Expand All @@ -370,14 +370,14 @@ pub mod pallet {
))]
pub fn report_equivocation_unsigned(
origin: OriginFor<T>,
equivocation_proof: EquivocationProof<T::Header>,
equivocation_proof: Box<EquivocationProof<T::Header>>,
key_owner_proof: T::KeyOwnerProof,
) -> DispatchResultWithPostInfo {
ensure_none(origin)?;

Self::do_report_equivocation(
T::HandleEquivocation::block_author(),
equivocation_proof,
*equivocation_proof,
key_owner_proof,
)
}
Expand Down
57 changes: 39 additions & 18 deletions frame/babe/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -440,8 +440,12 @@ fn report_equivocation_current_session_works() {
let key_owner_proof = Historical::prove(key).unwrap();

// report the equivocation
Babe::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof)
.unwrap();
Babe::report_equivocation_unsigned(
Origin::none(),
Box::new(equivocation_proof),
key_owner_proof,
)
.unwrap();

// start a new era so that the results of the offence report
// are applied at era end
Expand Down Expand Up @@ -508,8 +512,12 @@ fn report_equivocation_old_session_works() {
assert_eq!(Staking::slashable_balance_of(&offending_validator_id), 10_000);

// report the equivocation
Babe::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof)
.unwrap();
Babe::report_equivocation_unsigned(
Origin::none(),
Box::new(equivocation_proof),
key_owner_proof,
)
.unwrap();

// start a new era so that the results of the offence report
// are applied at era end
Expand Down Expand Up @@ -558,7 +566,7 @@ fn report_equivocation_invalid_key_owner_proof() {
assert_err!(
Babe::report_equivocation_unsigned(
Origin::none(),
equivocation_proof.clone(),
Box::new(equivocation_proof.clone()),
key_owner_proof
),
Error::<Test>::InvalidKeyOwnershipProof,
Expand All @@ -576,7 +584,11 @@ fn report_equivocation_invalid_key_owner_proof() {
start_era(2);

assert_err!(
Babe::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof),
Babe::report_equivocation_unsigned(
Origin::none(),
Box::new(equivocation_proof),
key_owner_proof,
),
Error::<Test>::InvalidKeyOwnershipProof,
);
})
Expand Down Expand Up @@ -608,7 +620,7 @@ fn report_equivocation_invalid_equivocation_proof() {
assert_err!(
Babe::report_equivocation_unsigned(
Origin::none(),
equivocation_proof,
Box::new(equivocation_proof),
key_owner_proof.clone(),
),
Error::<Test>::InvalidEquivocationProof,
Expand Down Expand Up @@ -714,8 +726,10 @@ fn report_equivocation_validate_unsigned_prevents_duplicates() {
let key = (sp_consensus_babe::KEY_TYPE, &offending_authority_pair.public());
let key_owner_proof = Historical::prove(key).unwrap();

let inner =
Call::report_equivocation_unsigned(equivocation_proof.clone(), key_owner_proof.clone());
let inner = Call::report_equivocation_unsigned(
Box::new(equivocation_proof.clone()),
key_owner_proof.clone(),
);

// only local/inblock reports are allowed
assert_eq!(
Expand Down Expand Up @@ -746,8 +760,12 @@ fn report_equivocation_validate_unsigned_prevents_duplicates() {
assert_ok!(<Babe as sp_runtime::traits::ValidateUnsigned>::pre_dispatch(&inner));

// we submit the report
Babe::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof)
.unwrap();
Babe::report_equivocation_unsigned(
Origin::none(),
Box::new(equivocation_proof),
key_owner_proof,
)
.unwrap();

// the report should now be considered stale and the transaction is invalid.
// the check for staleness should be done on both `validate_unsigned` and on `pre_dispatch`
Expand Down Expand Up @@ -805,7 +823,7 @@ fn valid_equivocation_reports_dont_pay_fees() {

// check the dispatch info for the call.
let info = Call::<Test>::report_equivocation_unsigned(
equivocation_proof.clone(),
Box::new(equivocation_proof.clone()),
key_owner_proof.clone(),
)
.get_dispatch_info();
Expand All @@ -817,7 +835,7 @@ fn valid_equivocation_reports_dont_pay_fees() {
// report the equivocation.
let post_info = Babe::report_equivocation_unsigned(
Origin::none(),
equivocation_proof.clone(),
Box::new(equivocation_proof.clone()),
key_owner_proof.clone(),
)
.unwrap();
Expand All @@ -829,11 +847,14 @@ fn valid_equivocation_reports_dont_pay_fees() {

// report the equivocation again which is invalid now since it is
// duplicate.
let post_info =
Babe::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof)
.err()
.unwrap()
.post_info;
let post_info = Babe::report_equivocation_unsigned(
Origin::none(),
Box::new(equivocation_proof),
key_owner_proof,
)
.err()
.unwrap()
.post_info;

// the fee is not waived and the original weight is kept.
assert!(post_info.actual_weight.is_none());
Expand Down
17 changes: 13 additions & 4 deletions frame/election-provider-multi-phase/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ use rand::{prelude::SliceRandom, rngs::SmallRng, SeedableRng};
use sp_arithmetic::{per_things::Percent, traits::One};
use sp_npos_elections::IndexAssignment;
use sp_runtime::InnerOf;
use sp_std::convert::{TryFrom, TryInto};
use sp_std::{
boxed::Box,
convert::{TryFrom, TryInto},
};

const SEED: u32 = 999;

Expand Down Expand Up @@ -317,7 +320,7 @@ frame_benchmarking::benchmarks! {
let caller = frame_benchmarking::whitelisted_caller();
T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance() * 10u32.into());

}: _(RawOrigin::Signed(caller), solution, c)
}: _(RawOrigin::Signed(caller), Box::new(solution), c)
verify {
assert!(<MultiPhase<T>>::signed_submissions().len() as u32 == c + 1);
}
Expand All @@ -344,9 +347,15 @@ frame_benchmarking::benchmarks! {

// encode the most significant storage item that needs to be decoded in the dispatch.
let encoded_snapshot = <MultiPhase<T>>::snapshot().unwrap().encode();
let encoded_call = <Call<T>>::submit_unsigned(raw_solution.clone(), witness).encode();
let encoded_call = <Call<T>>::submit_unsigned(Box::new(raw_solution.clone()), witness).encode();
}: {
assert_ok!(<MultiPhase<T>>::submit_unsigned(RawOrigin::None.into(), raw_solution, witness));
assert_ok!(
<MultiPhase<T>>::submit_unsigned(
RawOrigin::None.into(),
Box::new(raw_solution),
witness,
)
);
let _decoded_snap = <RoundSnapshot<T::AccountId> as Decode>::decode(&mut &*encoded_snapshot)
.unwrap();
let _decoded_call = <Call<T> as Decode>::decode(&mut &*encoded_call).unwrap();
Expand Down
11 changes: 6 additions & 5 deletions frame/election-provider-multi-phase/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,7 @@ pub mod pallet {
))]
pub fn submit_unsigned(
origin: OriginFor<T>,
solution: RawSolution<CompactOf<T>>,
solution: Box<RawSolution<CompactOf<T>>>,
witness: SolutionOrSnapshotSize,
) -> DispatchResultWithPostInfo {
ensure_none(origin)?;
Expand All @@ -876,7 +876,7 @@ pub mod pallet {
assert!(targets as u32 == witness.targets, "{}", error_message);

let ready =
Self::feasibility_check(solution, ElectionCompute::Unsigned).expect(error_message);
Self::feasibility_check(*solution, ElectionCompute::Unsigned).expect(error_message);

// Store the newly received solution.
log!(info, "queued unsigned solution with score {:?}", ready.score);
Expand Down Expand Up @@ -947,7 +947,7 @@ pub mod pallet {
#[pallet::weight(T::WeightInfo::submit(*num_signed_submissions))]
pub fn submit(
origin: OriginFor<T>,
solution: RawSolution<CompactOf<T>>,
solution: Box<RawSolution<CompactOf<T>>>,
num_signed_submissions: u32,
) -> DispatchResult {
let who = ensure_signed(origin)?;
Expand Down Expand Up @@ -982,7 +982,8 @@ pub mod pallet {
T::SignedRewardBase::get().saturating_add(call_fee)
};

let submission = SignedSubmission { who: who.clone(), deposit, solution, reward };
let submission =
SignedSubmission { who: who.clone(), deposit, solution: *solution, reward };

// insert the submission if the queue has space or it's better than the weakest
// eject the weakest if the queue was full
Expand Down Expand Up @@ -1927,7 +1928,7 @@ mod tests {
let solution = RawSolution { score: [(5 + s).into(), 0, 0], ..Default::default() };
assert_ok!(MultiPhase::submit(
crate::mock::Origin::signed(99),
solution,
Box::new(solution),
MultiPhase::signed_submissions().len() as u32
));
}
Expand Down
8 changes: 6 additions & 2 deletions frame/election-provider-multi-phase/src/signed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,11 @@ mod tests {
origin: Origin,
solution: RawSolution<CompactOf<Runtime>>,
) -> DispatchResult {
MultiPhase::submit(origin, solution, MultiPhase::signed_submissions().len() as u32)
MultiPhase::submit(
origin,
Box::new(solution),
MultiPhase::signed_submissions().len() as u32,
)
}

#[test]
Expand Down Expand Up @@ -532,7 +536,7 @@ mod tests {

// now try and cheat by passing a lower queue length
assert_noop!(
MultiPhase::submit(Origin::signed(99), solution, 0),
MultiPhase::submit(Origin::signed(99), Box::new(solution), 0),
Error::<Runtime>::SignedInvalidWitness,
);
})
Expand Down
30 changes: 19 additions & 11 deletions frame/election-provider-multi-phase/src/unsigned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use sp_runtime::{
traits::TrailingZeroInput,
DispatchError, SaturatedConversion,
};
use sp_std::{cmp::Ordering, convert::TryFrom, vec::Vec};
use sp_std::{boxed::Box, cmp::Ordering, convert::TryFrom, vec::Vec};

/// Storage key used to store the last block number at which offchain worker ran.
pub(crate) const OFFCHAIN_LAST_BLOCK: &[u8] = b"parity/multi-phase-unsigned-election";
Expand Down Expand Up @@ -208,7 +208,7 @@ impl<T: Config> Pallet<T> {
let (raw_solution, witness) = Self::mine_and_check(iters)?;

let score = raw_solution.score.clone();
let call: Call<T> = Call::submit_unsigned(raw_solution, witness).into();
let call: Call<T> = Call::submit_unsigned(Box::new(raw_solution), witness).into();

log!(
debug,
Expand Down Expand Up @@ -773,7 +773,7 @@ mod tests {
fn validate_unsigned_retracts_wrong_phase() {
ExtBuilder::default().desired_targets(0).build_and_execute(|| {
let solution = RawSolution::<TestCompact> { score: [5, 0, 0], ..Default::default() };
let call = Call::submit_unsigned(solution.clone(), witness());
let call = Call::submit_unsigned(Box::new(solution.clone()), witness());

// initial
assert_eq!(MultiPhase::current_phase(), Phase::Off);
Expand Down Expand Up @@ -842,7 +842,7 @@ mod tests {
assert!(MultiPhase::current_phase().is_unsigned());

let solution = RawSolution::<TestCompact> { score: [5, 0, 0], ..Default::default() };
let call = Call::submit_unsigned(solution.clone(), witness());
let call = Call::submit_unsigned(Box::new(solution.clone()), witness());

// initial
assert!(<MultiPhase as ValidateUnsigned>::validate_unsigned(
Expand Down Expand Up @@ -879,7 +879,7 @@ mod tests {
assert!(MultiPhase::current_phase().is_unsigned());

let solution = RawSolution::<TestCompact> { score: [5, 0, 0], ..Default::default() };
let call = Call::submit_unsigned(solution.clone(), witness());
let call = Call::submit_unsigned(Box::new(solution.clone()), witness());
assert_eq!(solution.compact.unique_targets().len(), 0);

// won't work anymore.
Expand All @@ -905,7 +905,7 @@ mod tests {

let solution =
RawSolution::<TestCompact> { score: [5, 0, 0], ..Default::default() };
let call = Call::submit_unsigned(solution.clone(), witness());
let call = Call::submit_unsigned(Box::new(solution.clone()), witness());

assert_eq!(
<MultiPhase as ValidateUnsigned>::validate_unsigned(
Expand All @@ -931,7 +931,7 @@ mod tests {

// This is in itself an invalid BS solution.
let solution = RawSolution::<TestCompact> { score: [5, 0, 0], ..Default::default() };
let call = Call::submit_unsigned(solution.clone(), witness());
let call = Call::submit_unsigned(Box::new(solution.clone()), witness());
let outer_call: OuterCall = call.into();
let _ = outer_call.dispatch(Origin::none());
})
Expand All @@ -951,7 +951,7 @@ mod tests {
let mut correct_witness = witness();
correct_witness.voters += 1;
correct_witness.targets -= 1;
let call = Call::submit_unsigned(solution.clone(), correct_witness);
let call = Call::submit_unsigned(Box::new(solution.clone()), correct_witness);
let outer_call: OuterCall = call.into();
let _ = outer_call.dispatch(Origin::none());
})
Expand All @@ -972,7 +972,7 @@ mod tests {

// ensure this solution is valid.
assert!(MultiPhase::queued_solution().is_none());
assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, witness));
assert_ok!(MultiPhase::submit_unsigned(Origin::none(), Box::new(solution), witness));
assert!(MultiPhase::queued_solution().is_some());
})
}
Expand Down Expand Up @@ -1054,7 +1054,11 @@ mod tests {
};
let (solution, witness) = MultiPhase::prepare_election_result(result).unwrap();
assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution));
assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, witness));
assert_ok!(MultiPhase::submit_unsigned(
Origin::none(),
Box::new(solution),
witness
));
assert_eq!(MultiPhase::queued_solution().unwrap().score[0], 10);

// trial 1: a solution who's score is only 2, i.e. 20% better in the first element.
Expand Down Expand Up @@ -1096,7 +1100,11 @@ mod tests {

// and it is fine
assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution));
assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, witness));
assert_ok!(MultiPhase::submit_unsigned(
Origin::none(),
Box::new(solution),
witness
));
})
}

Expand Down
Loading