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

Update lowest unbaked storage. #9750

Merged
merged 7 commits into from
Oct 18, 2021
Merged

Update lowest unbaked storage. #9750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6ac4db0
update lowest unbaked
gui1117 Sep 10, 2021
3caa8b2
fix format
gui1117 Sep 11, 2021
19c7d2f
add note
gui1117 Sep 27, 2021
fd57506
Merge remote-tracking branch 'origin/master' into gui-update-lowest-u…
gui1117 Sep 27, 2021
27ad7f3
fmt
gui1117 Oct 7, 2021
2936658
Merge remote-tracking branch 'origin/master' into gui-update-lowest-u…
gui1117 Oct 7, 2021
4fc6f85
Merge remote-tracking branch 'origin/master' into gui-update-lowest-u…
gui1117 Oct 18, 2021
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
32 changes: 23 additions & 9 deletions frame/democracy/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,10 +613,7 @@ pub mod pallet {
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
/// 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)
}
}

Expand Down Expand Up @@ -1682,7 +1679,7 @@ impl<T: Config> Pallet<T> {
now: T::BlockNumber,
index: ReferendumIndex,
status: ReferendumStatus<T::BlockNumber, T::Hash, BalanceOf<T>>,
) -> Result<bool, DispatchError> {
) -> bool {
let total_issuance = T::Currency::total_issuance();
let approved = status.threshold.approved(status.tally, total_issuance);

Expand Down Expand Up @@ -1719,7 +1716,7 @@ impl<T: Config> Pallet<T> {
Self::deposit_event(Event::<T>::NotPassed(index));
}

Ok(approved)
approved
}

/// Current era is ending; we should finish up any proposals.
Expand All @@ -1734,7 +1731,7 @@ impl<T: Config> Pallet<T> {
/// - Db writes: `PublicProps`, `account`, `ReferendumCount`, `DepositOf`, `ReferendumInfoOf`
/// - Db reads per R: `DepositOf`, `ReferendumInfoOf`
/// # </weight>
fn begin_block(now: T::BlockNumber) -> Result<Weight, DispatchError> {
fn begin_block(now: T::BlockNumber) -> Weight {
let max_block_weight = T::BlockWeights::get().max_block;
let mut weight = 0;

Expand All @@ -1758,12 +1755,29 @@ impl<T: Config> Pallet<T> {

// 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::<T>::insert(index, ReferendumInfo::Finished { end: now, approved });
Copy link
Contributor

@emostov emostov Sep 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Would it be clearer <LowestUnbaked<T>>::mutate(|ref_index| { *ref_index = index + 1}) right here?

The only issue I see is if we miss calling this begin_block for some previous block and there are some referendum that get skipped over in maturing_referenda_at_inner because they end at less than n, but have not yet finished. I think the below logic would then just get stuck on referenda that end at less than n but have not yet finished. (And the below loop just seems like an unnecessary constant increase in iterations + storage reads)

To handle the skipped block case we could change maturing_referenda_at_inner to filter like .filter(|(_, status)| status.end <= n) (currently it is .filter(|(_, status)| status.end == n)).

(haven't read the whole pallet yet though, so may be missing something)

Copy link
Contributor Author

@gui1117 gui1117 Sep 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

this is true only if referendum are maturing in order of their index. I expected some referendum to mature later than some other with bigger index. In this case we can't do your proposal.
EDIT: it safer in case of runtime upgrade increase some voting period

I agree we could do the filter status.end <= n, or log an error because it should never happen in practice and if it happens it is not a big issue.

(And the below loop just seems like an unnecessary constant increase in iterations + storage reads)

there shouldn't be any additional (not in cache) storage reads, all storage reads should read from cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

there shouldn't be any additional (not in cache) storage reads, all storage reads should read from cache.

Just too clarify, you want to keep the second iteration because the cache hit cost is minimal and I assume you think it is more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no I'm not against refactoring maturing_referenda_at_inner to avoid this second iteration. But the suggestion you propose seems a bit more error prone, due to the fact that if any referendum of an index ends after another referendum of an index bigger then the lowest unbaked is invalid. This could maybe happen when the constant VotingPeriod is changed with a runtime upgrade.

So either we refactor, or we keep this second iteration seems to be the best options

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for explaining. Given this info, I like keeping your current approach

weight = max_block_weight;
}

Ok(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.
<LowestUnbaked<T>>::mutate(|ref_index| {
while *ref_index < last &&
Self::referendum_info(*ref_index)
.map_or(true, |info| matches!(info, ReferendumInfo::Finished { .. }))
emostov marked this conversation as resolved.
Show resolved Hide resolved
{
*ref_index += 1
}
});

weight
}

/// Reads the length of account in DepositOf without getting the complete value in the runtime.
Expand Down
2 changes: 1 addition & 1 deletion frame/democracy/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,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) {
Expand Down
4 changes: 4 additions & 0 deletions frame/democracy/src/tests/cancellation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
Expand Down
44 changes: 44 additions & 0 deletions frame/democracy/src/tests/scheduling.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
Expand Down Expand Up @@ -110,3 +112,45 @@ 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());
});
}