From 6ac4db05b5b6465e793e7f56ba26a727f9d8ca50 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Fri, 10 Sep 2021 17:07:48 +0200 Subject: [PATCH 1/4] update lowest unbaked --- frame/democracy/src/lib.rs | 24 +++++++----- frame/democracy/src/tests.rs | 2 +- frame/democracy/src/tests/cancellation.rs | 4 ++ frame/democracy/src/tests/scheduling.rs | 45 +++++++++++++++++++++++ 4 files changed, 65 insertions(+), 10 deletions(-) diff --git a/frame/democracy/src/lib.rs b/frame/democracy/src/lib.rs index 473ac964692cf..a571c3f035ab9 100644 --- a/frame/democracy/src/lib.rs +++ b/frame/democracy/src/lib.rs @@ -619,10 +619,7 @@ pub mod pallet { impl Hooks> for Pallet { /// Weight: see `begin_block` fn on_initialize(n: T::BlockNumber) -> Weight { - Self::begin_block(n).unwrap_or_else(|e| { - sp_runtime::print(e); - 0 - }) + Self::begin_block(n) } } @@ -1688,7 +1685,7 @@ impl Pallet { now: T::BlockNumber, index: ReferendumIndex, status: ReferendumStatus>, - ) -> Result { + ) -> bool { let total_issuance = T::Currency::total_issuance(); let approved = status.threshold.approved(status.tally, total_issuance); @@ -1725,7 +1722,7 @@ impl Pallet { Self::deposit_event(Event::::NotPassed(index)); } - Ok(approved) + approved } /// Current era is ending; we should finish up any proposals. @@ -1739,7 +1736,7 @@ impl Pallet { /// - Db writes: `PublicProps`, `account`, `ReferendumCount`, `DepositOf`, `ReferendumInfoOf` /// - Db reads per R: `DepositOf`, `ReferendumInfoOf` /// # - fn begin_block(now: T::BlockNumber) -> Result { + fn begin_block(now: T::BlockNumber) -> Weight { let max_block_weight = T::BlockWeights::get().max_block; let mut weight = 0; @@ -1757,12 +1754,21 @@ impl Pallet { weight = weight.saturating_add(T::WeightInfo::on_initialize_base(r)); // tally up votes for any expiring referenda. for (index, info) in Self::maturing_referenda_at_inner(now, next..last).into_iter() { - let approved = Self::bake_referendum(now, index, info)?; + let approved = Self::bake_referendum(now, index, info); ReferendumInfoOf::::insert(index, ReferendumInfo::Finished { end: now, approved }); weight = max_block_weight; } - Ok(weight) + >::mutate(|ref_index| { + while *ref_index < last && + Self::referendum_info(*ref_index) + .map_or(true, |info| matches!(info, ReferendumInfo::Finished { .. })) + { + *ref_index += 1 + } + }); + + weight } /// Reads the length of account in DepositOf without getting the complete value in the runtime. diff --git a/frame/democracy/src/tests.rs b/frame/democracy/src/tests.rs index 9a5e47c89ac77..dee593c9927e7 100644 --- a/frame/democracy/src/tests.rs +++ b/frame/democracy/src/tests.rs @@ -263,7 +263,7 @@ fn propose_set_balance_and_note(who: u64, value: u64, delay: u64) -> DispatchRes fn next_block() { System::set_block_number(System::block_number() + 1); Scheduler::on_initialize(System::block_number()); - assert!(Democracy::begin_block(System::block_number()).is_ok()); + Democracy::begin_block(System::block_number()); } fn fast_forward_to(n: u64) { diff --git a/frame/democracy/src/tests/cancellation.rs b/frame/democracy/src/tests/cancellation.rs index c2bd725ce934a..83822bf51829f 100644 --- a/frame/democracy/src/tests/cancellation.rs +++ b/frame/democracy/src/tests/cancellation.rs @@ -30,10 +30,14 @@ fn cancel_referendum_should_work() { ); assert_ok!(Democracy::vote(Origin::signed(1), r, aye(1))); assert_ok!(Democracy::cancel_referendum(Origin::root(), r.into())); + assert_eq!(Democracy::lowest_unbaked(), 0); next_block(); + next_block(); + assert_eq!(Democracy::lowest_unbaked(), 1); + assert_eq!(Democracy::lowest_unbaked(), Democracy::referendum_count()); assert_eq!(Balances::free_balance(42), 0); }); } diff --git a/frame/democracy/src/tests/scheduling.rs b/frame/democracy/src/tests/scheduling.rs index 06b492bc6093c..55e470654b82b 100644 --- a/frame/democracy/src/tests/scheduling.rs +++ b/frame/democracy/src/tests/scheduling.rs @@ -30,8 +30,10 @@ fn simple_passing_should_work() { ); assert_ok!(Democracy::vote(Origin::signed(1), r, aye(1))); assert_eq!(tally(r), Tally { ayes: 1, nays: 0, turnout: 10 }); + assert_eq!(Democracy::lowest_unbaked(), 0); next_block(); next_block(); + assert_eq!(Democracy::lowest_unbaked(), 1); assert_eq!(Balances::free_balance(42), 2); }); } @@ -110,3 +112,46 @@ fn delayed_enactment_should_work() { assert_eq!(Balances::free_balance(42), 2); }); } + +#[test] +fn lowest_unbaked_should_be_sensible() { + new_test_ext().execute_with(|| { + let r1 = Democracy::inject_referendum( + 3, + set_balance_proposal_hash_and_note(1), + VoteThreshold::SuperMajorityApprove, + 0, + ); + let r2 = Democracy::inject_referendum( + 2, + set_balance_proposal_hash_and_note(2), + VoteThreshold::SuperMajorityApprove, + 0, + ); + let r3 = Democracy::inject_referendum( + 10, + set_balance_proposal_hash_and_note(3), + VoteThreshold::SuperMajorityApprove, + 0, + ); + assert_ok!(Democracy::vote(Origin::signed(1), r1, aye(1))); + assert_ok!(Democracy::vote(Origin::signed(1), r2, aye(1))); + // r3 is canceled + assert_ok!(Democracy::cancel_referendum(Origin::root(), r3.into())); + assert_eq!(Democracy::lowest_unbaked(), 0); + + next_block(); + + // r2 is approved + assert_eq!(Balances::free_balance(42), 2); + assert_eq!(Democracy::lowest_unbaked(), 0); + + next_block(); + + // r1 is approved + assert_eq!(Balances::free_balance(42), 1); + assert_eq!(Democracy::lowest_unbaked(), 3); + assert_eq!(Democracy::lowest_unbaked(), Democracy::referendum_count()); + }); +} + From 3caa8b2f68c31a9dde8d52bca6d785c1a3c56f09 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Sat, 11 Sep 2021 22:01:14 +0200 Subject: [PATCH 2/4] fix format --- frame/democracy/src/tests/scheduling.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/democracy/src/tests/scheduling.rs b/frame/democracy/src/tests/scheduling.rs index 55e470654b82b..5c857a632b97b 100644 --- a/frame/democracy/src/tests/scheduling.rs +++ b/frame/democracy/src/tests/scheduling.rs @@ -154,4 +154,3 @@ fn lowest_unbaked_should_be_sensible() { assert_eq!(Democracy::lowest_unbaked(), Democracy::referendum_count()); }); } - From 19c7d2f268598f8a90e44d6b64d02f24f01c40ea Mon Sep 17 00:00:00 2001 From: thiolliere Date: Mon, 27 Sep 2021 18:07:02 +0200 Subject: [PATCH 3/4] add note --- frame/democracy/src/lib.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/frame/democracy/src/lib.rs b/frame/democracy/src/lib.rs index a571c3f035ab9..ae3f97c4dca4f 100644 --- a/frame/democracy/src/lib.rs +++ b/frame/democracy/src/lib.rs @@ -1759,6 +1759,15 @@ impl Pallet { weight = max_block_weight; } + // Notes: + // * We don't consider the lowest unbaked to be the last maturing in case some refendum + // have longer voting period than others. + // * The iteration here shouldn't trigger any storage read that are not in cache, due to + // `maturing_referenda_at_inner` having already read them. + // * We shouldn't iterate more than `LaunchPeriod/VotingPeriod + 1` times because the + // number of unbaked referendum is bounded by this number. In case those number have + // changed in a runtime upgrade the formula should be adjusted but the bound should still + // be sensible. >::mutate(|ref_index| { while *ref_index < last && Self::referendum_info(*ref_index) From 27ad7f385670905ee46e5e1ec6331fcb573230bf Mon Sep 17 00:00:00 2001 From: thiolliere Date: Thu, 7 Oct 2021 17:13:21 +0200 Subject: [PATCH 4/4] fmt --- frame/democracy/src/lib.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/frame/democracy/src/lib.rs b/frame/democracy/src/lib.rs index 66a53d990a5bd..9f9ba47f7a8f2 100644 --- a/frame/democracy/src/lib.rs +++ b/frame/democracy/src/lib.rs @@ -1754,14 +1754,13 @@ impl Pallet { } // Notes: - // * We don't consider the lowest unbaked to be the last maturing in case some refendum - // have longer voting period than others. + // * We don't consider the lowest unbaked to be the last maturing in case some refendum have + // longer voting period than others. // * The iteration here shouldn't trigger any storage read that are not in cache, due to // `maturing_referenda_at_inner` having already read them. - // * We shouldn't iterate more than `LaunchPeriod/VotingPeriod + 1` times because the - // number of unbaked referendum is bounded by this number. In case those number have - // changed in a runtime upgrade the formula should be adjusted but the bound should still - // be sensible. + // * We shouldn't iterate more than `LaunchPeriod/VotingPeriod + 1` times because the number + // of unbaked referendum is bounded by this number. In case those number have changed in a + // runtime upgrade the formula should be adjusted but the bound should still be sensible. >::mutate(|ref_index| { while *ref_index < last && Self::referendum_info(*ref_index)