benchmark: proposals: minor fixes and add pending_execution benchmark branch

Cargo.lock

@@ -4064,7 +4064,7 @@ dependencies = [
 
 [[package]]
 name = "pallet-proposals-discussion"
-version = "4.0.0"
+version = "4.0.1"
 dependencies = [
  "frame-benchmarking",
  "frame-support",

runtime-modules/proposals/codex/src/tests/mock/mod.rs

@@ -86,7 +86,7 @@ parameter_types! {
     pub const LockId: LockIdentifier = [2; 8];
 }
 
-pub struct MockEngineWeight;
+pub struct MockProposalsEngineWeight;
 
 impl proposals_engine::Trait for Test {
     type Event = ();
@@ -102,10 +102,10 @@ impl proposals_engine::Trait for Test {
     type MaxActiveProposalLimit = MaxActiveProposalLimit;
     type DispatchableCallCode = crate::Call<Test>;
     type ProposalObserver = crate::Module<Test>;
-    type WeightInfo = MockEngineWeight;
+    type WeightInfo = MockProposalsEngineWeight;
 }
 
-impl proposals_engine::WeightInfo for MockEngineWeight {
+impl proposals_engine::WeightInfo for MockProposalsEngineWeight {
     fn vote(_: u32) -> Weight {
         0
     }
@@ -122,6 +122,10 @@ impl proposals_engine::WeightInfo for MockEngineWeight {
         0
     }
 
+    fn on_initialize_pending_execution_decode_fails(_: u32) -> Weight {
+        0
+    }
+
     fn on_initialize_approved_pending_constitutionality(_: u32) -> Weight {
         0
     }
@@ -165,7 +169,7 @@ parameter_types! {
     pub const MaxWhiteListSize: u32 = 20;
 }
 
-pub struct MockProposalWeight;
+pub struct MockProposalsDiscussionWeight;
 
 impl proposals_discussion::Trait for Test {
     type Event = ();
@@ -174,11 +178,11 @@ impl proposals_discussion::Trait for Test {
     type ThreadId = u64;
     type PostId = u64;
     type MaxWhiteListSize = MaxWhiteListSize;
-    type WeightInfo = MockProposalWeight;
+    type WeightInfo = MockProposalsDiscussionWeight;
 }
 
-impl proposals_discussion::WeightInfo for MockProposalWeight {
-    fn add_post(_: u32, _: u32) -> Weight {
+impl proposals_discussion::WeightInfo for MockProposalsDiscussionWeight {
+    fn add_post(_: u32) -> Weight {
         0
     }
 

runtime-modules/proposals/discussion/src/lib.rs

@@ -68,8 +68,10 @@ pub use types::ThreadMode;
 
 type MemberId<T> = <T as membership::Trait>::MemberId;
 
+/// Proposals discussion WeightInfo.
+/// Note: This was auto generated through the benchmark CLI using the `--weight-trait` flag
 pub trait WeightInfo {
-    fn add_post(i: u32, j: u32) -> Weight;
+    fn add_post(i: u32) -> Weight; // Note: since parameter doesn't affect weight it's discarded
     fn update_post() -> Weight; // Note: since parameter doesn't affect weight it's discarded
     fn change_thread_mode(i: u32) -> Weight;
 }
@@ -184,7 +186,6 @@ decl_module! {
         /// Adds a post with author origin check.
         #[weight = <T as Trait>::WeightInfo::add_post(
             T::MaxWhiteListSize::get(),
-            _text.len().try_into().unwrap()
         )]
         pub fn add_post(
             origin,

runtime-modules/proposals/discussion/src/tests/mock.rs

@@ -92,7 +92,7 @@ impl crate::Trait for Test {
 }
 
 impl WeightInfo for () {
-    fn add_post(_: u32, _: u32) -> Weight {
+    fn add_post(_: u32) -> Weight {
         0
     }
 

runtime-modules/proposals/engine/src/benchmarking.rs

@@ -4,7 +4,7 @@ use crate::Module as ProposalsEngine;
 use balances::Module as Balances;
 use core::convert::TryInto;
 use frame_benchmarking::{account, benchmarks};
-use frame_support::traits::{Currency, OnInitialize};
+use frame_support::traits::{Currency, OnFinalize, OnInitialize};
 use frame_system::EventRecord;
 use frame_system::Module as System;
 use frame_system::RawOrigin;
@@ -91,12 +91,13 @@ fn create_proposal<T: Trait>(
     id: u32,
     proposal_number: u32,
     constitutionality: u32,
+    grace_period: u32,
 ) -> (T::AccountId, T::MemberId, T::ProposalId) {
     let (account_id, member_id) = member_funded_account::<T>("member", id);
 
     let proposal_parameters = ProposalParameters {
         voting_period: T::BlockNumber::from(1),
-        grace_period: Zero::zero(),
+        grace_period: T::BlockNumber::from(grace_period),
         approval_quorum_percentage: 1,
         approval_threshold_percentage: 1,
         slashing_quorum_percentage: 0,
@@ -161,6 +162,7 @@ fn create_multiple_finalized_proposals<T: Trait + governance::council::Trait>(
     constitutionality: u32,
     vote_kind: VoteKind,
     total_voters: u32,
+    grace_period: u32,
 ) -> (Vec<T::AccountId>, Vec<T::ProposalId>) {
     let mut voters = Vec::new();
     for i in 0..total_voters {
@@ -180,7 +182,7 @@ fn create_multiple_finalized_proposals<T: Trait + governance::council::Trait>(
     let mut proposals = Vec::new();
     for id in total_voters..number_of_proposals + total_voters {
         let (proposer_account_id, _, proposal_id) =
-            create_proposal::<T>(id, id - total_voters + 1, constitutionality);
+            create_proposal::<T>(id, id - total_voters + 1, constitutionality, grace_period);
         proposers.push(proposer_account_id);
         proposals.push(proposal_id);
 
@@ -212,7 +214,7 @@ benchmarks! {
     vote {
         let i in 0 .. MAX_BYTES;
 
-        let (_, _, proposal_id) = create_proposal::<T>(0, 1, 0);
+        let (_, _, proposal_id) = create_proposal::<T>(0, 1, 0, 0);
 
         let (account_voter_id, member_voter_id) = member_funded_account::<T>("voter", 1);
 
@@ -254,7 +256,7 @@ benchmarks! {
     cancel_proposal {
         let i in 1 .. T::MaxLocks::get();
 
-        let (account_id, member_id, proposal_id) = create_proposal::<T>(0, 1, 0);
+        let (account_id, member_id, proposal_id) = create_proposal::<T>(0, 1, 0, 0);
 
         for lock_number in 1 .. i {
             let (locked_account_id, _) = member_funded_account::<T>("locked_member", lock_number);
@@ -284,7 +286,7 @@ benchmarks! {
     }
 
     veto_proposal {
-        let (account_id, _, proposal_id) = create_proposal::<T>(0, 1, 0);
+        let (account_id, _, proposal_id) = create_proposal::<T>(0, 1, 0, 0);
     }: _ (RawOrigin::Root, proposal_id)
     verify {
         assert!(!Proposals::<T>::contains_key(proposal_id), "Proposal still in storage");
@@ -318,8 +320,10 @@ benchmarks! {
             i,
             0,
             VoteKind::Approve,
-            1
+            1,
+            0,
         );
+
     }: { ProposalsEngine::<T>::on_initialize(System::<T>::block_number().into()) }
     verify {
         for proposer_account_id in proposers {
@@ -330,6 +334,90 @@ benchmarks! {
             );
         }
 
+        assert_eq!(
+            ProposalsEngine::<T>::active_proposal_count(),
+            0,
+            "Proposals should no longer be active"
+        );
+
+        for proposal_id in proposals.iter() {
+            assert!(
+                !Proposals::<T>::contains_key(proposal_id),
+                "Proposals should've been removed"
+            );
+
+            assert!(
+                !DispatchableCallCode::<T>::contains_key(proposal_id),
+                "Dispatchable code should've been removed"
+            );
+        }
+
+        if cfg!(test) {
+            for proposal_id in proposals.iter() {
+                assert_in_events::<T>(
+                    RawEvent::ProposalExecuted(
+                        proposal_id.clone(),
+                        ExecutionStatus::failed_execution("Not enough data to fill buffer")).into()
+                );
+            }
+        }
+    }
+
+    on_initialize_pending_execution_decode_fails {
+        let i in 1 .. T::MaxActiveProposalLimit::get();
+
+        let (proposers, proposals) = create_multiple_finalized_proposals::<T>(
+            i,
+            0,
+            VoteKind::Approve,
+            1,
+            1,
+        );
+
+        let mut current_block_number = System::<T>::block_number();
+
+        System::<T>::on_finalize(current_block_number);
+        System::<T>::on_finalize(current_block_number);
+
+        current_block_number += One::one();
+
+        System::<T>::on_initialize(current_block_number);
+        ProposalsEngine::<T>::on_initialize(current_block_number);
+
+        assert_eq!(
+            ProposalsEngine::<T>::active_proposal_count(),
+            i,
+            "Proposals should still be active"
+        );
+
+        for proposal_id in proposals.iter() {
+            assert!(
+                Proposals::<T>::contains_key(proposal_id),
+                "All proposals should still be stored"
+            );
+
+            assert!(
+                DispatchableCallCode::<T>::contains_key(proposal_id),
+                "All dispatchable call code should still be stored"
+            );
+        }
+
+    }: { ProposalsEngine::<T>::on_initialize(current_block_number) }
+    verify {
+        for proposer_account_id in proposers {
+            assert_eq!(
+                T::StakingHandler::current_stake(&proposer_account_id),
+                Zero::zero(),
+                "Should've unlocked all stake"
+            );
+        }
+
+        assert_eq!(ProposalsEngine::<T>::active_proposal_count(), 0, "Proposals should no longer be active");
+        for proposal_id in proposals.iter() {
+            assert!(!Proposals::<T>::contains_key(proposal_id), "Proposals should've been removed");
+            assert!(!DispatchableCallCode::<T>::contains_key(proposal_id), "Dispatchable code should've been removed");
+        }
+
         if cfg!(test) {
             for proposal_id in proposals.iter() {
                 assert_in_events::<T>(
@@ -348,7 +436,8 @@ benchmarks! {
             i,
             2,
             VoteKind::Approve,
-            1
+            1,
+            0,
         );
 
     }: { ProposalsEngine::<T>::on_initialize(System::<T>::block_number().into()) }
@@ -388,6 +477,7 @@ benchmarks! {
             0,
             VoteKind::Reject,
             max(T::TotalVotersCounter::total_voters_count(), 1),
+            0,
         );
     }: { ProposalsEngine::<T>::on_initialize(System::<T>::block_number().into()) }
     verify {
@@ -432,6 +522,7 @@ benchmarks! {
             0,
             VoteKind::Slash,
             max(T::TotalVotersCounter::total_voters_count(), 1),
+            0,
         );
     }: { ProposalsEngine::<T>::on_initialize(System::<T>::block_number().into()) }
     verify {
@@ -517,6 +608,13 @@ mod tests {
         });
     }
 
+    #[test]
+    fn test_on_initialize_pending_execution_decode_fails() {
+        initial_test_ext().execute_with(|| {
+            assert_ok!(test_benchmark_on_initialize_pending_execution_decode_fails::<Test>());
+        });
+    }
+
     #[test]
     fn test_on_initialize_rejected() {
         initial_test_ext().execute_with(|| {

runtime-modules/proposals/engine/src/lib.rs

@@ -140,12 +140,14 @@ use sp_std::vec::Vec;
 use common::origin::ActorOriginValidator;
 use membership::staking_handler::StakingHandler;
 
-// proposals_engine
+/// Proposals engine WeightInfo.
+/// Note: This was auto generated through the benchmark CLI using the `--weight-trait` flag
 pub trait WeightInfo {
     fn vote(i: u32) -> Weight;
     fn cancel_proposal(i: u32) -> Weight;
     fn veto_proposal() -> Weight;
     fn on_initialize_immediate_execution_decode_fails(i: u32) -> Weight;
+    fn on_initialize_pending_execution_decode_fails(i: u32) -> Weight;
     fn on_initialize_approved_pending_constitutionality(i: u32) -> Weight;
     fn on_initialize_rejected(i: u32) -> Weight;
     fn on_initialize_slashed(i: u32) -> Weight;
@@ -372,21 +374,50 @@ decl_module! {
         /// Block Initialization. Perform voting period check, vote result tally, approved proposals
         /// grace period checks, and proposal execution.
         fn on_initialize() -> Weight {
+            // `process_proposal` returns the weight of the executed proposals. The weight of the
+            // executed proposals doesn't include any access to the store or calculation that
+            // `on_initialize` does. Therefore, to get the total weight of `on_initialize` we need
+            // to add the weight of the execution of `on_intialize` to the weight returned by
+            // `process_proposal`.
+            // To be safe, we use the worst possible case for `on_initialize`, meaning that there
+            // are as many proposals active as possible and they all take the worst possible branch.
+
             let max_active_proposals = T::MaxActiveProposalLimit::get();
 
-            <T as Trait>::WeightInfo::on_initialize_immediate_execution_decode_fails(max_active_proposals)
-                .max(
-                    <T as Trait>
-                        ::WeightInfo
-                        ::on_initialize_approved_pending_constitutionality(max_active_proposals)
-                )
-                .max(
-                    <T as Trait>::WeightInfo::on_initialize_rejected(max_active_proposals)
-                )
-                .max(
-                    <T as Trait>::WeightInfo::on_initialize_slashed(max_active_proposals)
-                )
-                .saturating_add(Self::process_proposals())
+            // Weight when all the proposals are immediatly approved and executed
+            let immediate_execution_branch_weight =
+                <T as Trait>::WeightInfo::
+                on_initialize_immediate_execution_decode_fails(max_active_proposals);
+
+            let pending_execution_branch_weight =
+                <T as Trait>::WeightInfo::
+                on_initialize_pending_execution_decode_fails(max_active_proposals);
+
+            // Weight when all the proposals are approved and pending constitutionality
+            let approved_pending_constitutionality_branch_weight =
+                <T as Trait>::WeightInfo::
+                on_initialize_approved_pending_constitutionality(max_active_proposals);
+
+            // Weight when all proposals are rejected
+            let rejected_branch_weight =
+                <T as Trait>::WeightInfo::on_initialize_rejected(max_active_proposals);
+
+            // Weight when all proposals are slashed
+            let slashed_branch_weight =
+                <T as Trait>::WeightInfo::on_initialize_slashed(max_active_proposals);
+
+            // Weight of the executed proposals
+            let executed_proposals_weight = Self::process_proposals();
+
+            // Maximum Weight of all possible worst case scenarios
+            let maximum_branch_weight = immediate_execution_branch_weight
+                .max(pending_execution_branch_weight)
+                .max(approved_pending_constitutionality_branch_weight)
+                .max(rejected_branch_weight)
+                .max(slashed_branch_weight);
+
+            // total_weight = executed_proposals_weight + maximum_branch_weight
+            executed_proposals_weight.saturating_add(maximum_branch_weight)
         }
 
         /// Vote extrinsic. Conditions:  origin must allow votes.
@@ -729,8 +760,7 @@ impl<T: Trait> Module<T> {
 
             // immediately execute proposal if it ready for execution or save it for the future otherwise.
             if finalized_proposal.is_ready_for_execution(now) {
-                executed_weight =
-                    executed_weight.saturating_add(Self::execute_proposal(proposal_id));
+                executed_weight = Self::execute_proposal(proposal_id);
             } else {
                 <Proposals<T>>::insert(proposal_id, finalized_proposal);
             }

runtime-modules/proposals/engine/src/tests/mock/mod.rs

@@ -123,6 +123,10 @@ impl crate::WeightInfo for () {
         0
     }
 
+    fn on_initialize_pending_execution_decode_fails(_: u32) -> Weight {
+        0
+    }
+
     fn on_initialize_approved_pending_constitutionality(_: u32) -> Weight {
         0
     }

runtime/src/weights/proposals_discussion.rs

@@ -7,20 +7,20 @@ use frame_support::weights::{constants::RocksDbWeight as DbWeight, Weight};
 
 pub struct WeightInfo;
 impl proposals_discussion::WeightInfo for WeightInfo {
-    fn add_post(i: u32, j: u32) -> Weight {
-        (10_881_426_000 as Weight)
-            .saturating_add((66_823_000 as Weight).saturating_mul(i as Weight))
-            .saturating_add((1_000 as Weight).saturating_mul(j as Weight))
+    // WARNING! Some components were not used: ["j"]
+    fn add_post(i: u32) -> Weight {
+        (361_189_000 as Weight)
+            .saturating_add((508_000 as Weight).saturating_mul(i as Weight))
             .saturating_add(DbWeight::get().reads(4 as Weight))
             .saturating_add(DbWeight::get().writes(2 as Weight))
     }
     // WARNING! Some components were not used: ["j"]
     fn update_post() -> Weight {
-        (7_759_302_000 as Weight).saturating_add(DbWeight::get().reads(3 as Weight))
+        (231_487_000 as Weight).saturating_add(DbWeight::get().reads(3 as Weight))
     }
     fn change_thread_mode(i: u32) -> Weight {
-        (11_577_573_000 as Weight)
-            .saturating_add((69_266_000 as Weight).saturating_mul(i as Weight))
+        (379_400_000 as Weight)
+            .saturating_add((1_244_000 as Weight).saturating_mul(i as Weight))
             .saturating_add(DbWeight::get().reads(3 as Weight))
             .saturating_add(DbWeight::get().writes(1 as Weight))
     }

runtime/src/weights/proposals_engine.rs

@@ -8,48 +8,56 @@ use frame_support::weights::{constants::RocksDbWeight as DbWeight, Weight};
 pub struct WeightInfo;
 impl proposals_engine::WeightInfo for WeightInfo {
     fn vote(i: u32) -> Weight {
-        (10_707_315_000 as Weight)
-            .saturating_add((1_075_000 as Weight).saturating_mul(i as Weight))
+        (375_240_000 as Weight)
+            .saturating_add((35_000 as Weight).saturating_mul(i as Weight))
             .saturating_add(DbWeight::get().reads(4 as Weight))
             .saturating_add(DbWeight::get().writes(2 as Weight))
     }
     fn cancel_proposal(i: u32) -> Weight {
-        (27_737_659_000 as Weight)
-            .saturating_add((38_762_000 as Weight).saturating_mul(i as Weight))
+        (874_300_000 as Weight)
+            .saturating_add((1_713_000 as Weight).saturating_mul(i as Weight))
             .saturating_add(DbWeight::get().reads(5 as Weight))
             .saturating_add(DbWeight::get().writes(8 as Weight))
     }
     fn veto_proposal() -> Weight {
-        (13_460_881_000 as Weight)
+        (404_254_000 as Weight)
             .saturating_add(DbWeight::get().reads(4 as Weight))
             .saturating_add(DbWeight::get().writes(8 as Weight))
     }
     fn on_initialize_immediate_execution_decode_fails(i: u32) -> Weight {
-        (845_226_000 as Weight)
-            .saturating_add((18_757_457_000 as Weight).saturating_mul(i as Weight))
+        (22_531_000 as Weight)
+            .saturating_add((578_486_000 as Weight).saturating_mul(i as Weight))
             .saturating_add(DbWeight::get().reads(3 as Weight))
             .saturating_add(DbWeight::get().reads((4 as Weight).saturating_mul(i as Weight)))
             .saturating_add(DbWeight::get().writes(2 as Weight))
             .saturating_add(DbWeight::get().writes((7 as Weight).saturating_mul(i as Weight)))
     }
+    fn on_initialize_pending_execution_decode_fails(i: u32) -> Weight {
+        (31_944_000 as Weight)
+            .saturating_add((274_852_000 as Weight).saturating_mul(i as Weight))
+            .saturating_add(DbWeight::get().reads(2 as Weight))
+            .saturating_add(DbWeight::get().reads((2 as Weight).saturating_mul(i as Weight)))
+            .saturating_add(DbWeight::get().writes(2 as Weight))
+            .saturating_add(DbWeight::get().writes((5 as Weight).saturating_mul(i as Weight)))
+    }
     fn on_initialize_approved_pending_constitutionality(i: u32) -> Weight {
-        (1_194_100_000 as Weight)
-            .saturating_add((8_103_583_000 as Weight).saturating_mul(i as Weight))
+        (50_422_000 as Weight)
+            .saturating_add((250_210_000 as Weight).saturating_mul(i as Weight))
             .saturating_add(DbWeight::get().reads(2 as Weight))
             .saturating_add(DbWeight::get().reads((1 as Weight).saturating_mul(i as Weight)))
             .saturating_add(DbWeight::get().writes((1 as Weight).saturating_mul(i as Weight)))
     }
     fn on_initialize_rejected(i: u32) -> Weight {
-        (751_079_000 as Weight)
-            .saturating_add((27_189_652_000 as Weight).saturating_mul(i as Weight))
+        (0 as Weight)
+            .saturating_add((884_947_000 as Weight).saturating_mul(i as Weight))
             .saturating_add(DbWeight::get().reads(3 as Weight))
             .saturating_add(DbWeight::get().reads((3 as Weight).saturating_mul(i as Weight)))
             .saturating_add(DbWeight::get().writes(2 as Weight))
             .saturating_add(DbWeight::get().writes((7 as Weight).saturating_mul(i as Weight)))
     }
     fn on_initialize_slashed(i: u32) -> Weight {
-        (1_297_364_000 as Weight)
-            .saturating_add((20_346_415_000 as Weight).saturating_mul(i as Weight))
+        (24_867_000 as Weight)
+            .saturating_add((628_899_000 as Weight).saturating_mul(i as Weight))
             .saturating_add(DbWeight::get().reads(3 as Weight))
             .saturating_add(DbWeight::get().reads((3 as Weight).saturating_mul(i as Weight)))
             .saturating_add(DbWeight::get().writes(2 as Weight))

scripts/generate-weights.sh

@@ -3,12 +3,12 @@
 # Executes and replaces all benchmarks with the new weights
 
 echo "Benchmarking proposals_discussion..."
-./target/debug/joystream-node benchmark --pallet=proposals_discussion --extrinsic=* --chain=dev --steps=50 --repeat=20 --execution=wasm --output=. > /dev/null
+./target/release/joystream-node benchmark --pallet=proposals_discussion --extrinsic=* --chain=dev --steps=50 --repeat=20 --execution=wasm --output=. > /dev/null
 mv proposals_discussion.rs runtime/src/weights/
 echo "proposals_discussion benchmarked"
 
 
 echo "Benchmarking proposals_engine..."
-./target/debug/joystream-node benchmark --pallet=proposals_engine --extrinsic=* --chain=dev --steps=50 --repeat=20 --execution=wasm --output=. > /dev/null
+./target/release/joystream-node benchmark --pallet=proposals_engine --extrinsic=* --chain=dev --steps=50 --repeat=20 --execution=wasm --output=. > /dev/null
 mv proposals_engine.rs runtime/src/weights/
 echo "proposals_engine benchmarked"