Update lowest unbaked storage. #9750
Conversation
gui1117
added
A0-please_review
shawntabrizi
added
D5-nicetohaveaudit ⚠️
| @@ -1757,12 +1754,21 @@ impl<T: Config> Pallet<T> { | |||
| 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::<T>::insert(index, ReferendumInfo::Finished { end: now, approved }); | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thank you for explaining. Given this info, I like keeping your current approach
|
I think the code looks ok, but wondering if 1) it can be a tad more efficient and if there are some other issues with skipping baking proposals if for some reason begin block is not called. More here: #9750 (comment) |
|
bot merge |
|
bot merge |
Fix #9739
Currently I expect the storage to be stuck at a low value. This PR fix it, it also clean for function which never returns error.