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

runtime/parachains: Keep track of included candidates and historical babe vrfs in shared module #3558

Closed

Conversation

Lldenaurois
Copy link
Contributor

@Lldenaurois Lldenaurois commented Aug 2, 2021

This PR attempts to take the first step in adding incentive structures to the Approval Voting system, c.f. #3463

Closes #3469

The goal is three-fold:

  1. Keep track of all historical CandidateHashes
  2. Maintain the CandidateHash -> CoreIndex mapping for some historical CandidateHashes
  3. Prune CandidateHashes for a historical session over the course of an ongoing session

Specifically, we add a StorageDoubleMap to the shared module. This StorageDoubleMap maintains the (SessionIndex, CandidateHash) -> Coreindex mapping (Note that we also include the BlockNumber in the value of the Map).

The additional indirection of maintaining the Session allows to implement more coherent pruning logic.

Pruning is currently simplified in that we prune a fixed number of Candidates at a time. This is achieved by prefix-query for a particular SessionIndex in the DoubleMap, thereby pruning CandidateHashes over time until a SessionIndex is exhausted.

Closes #3470

In addition, this PR also adds a StorageDoubleMap to the shared module which keeps track of historical Babe VRFs for the purpose of comparing these VRFs against the Approval Assignments that are issued throughout the Approval Voting process.

The pruning algorithm for Historical VRFs is modified from the above pruning logic for candidates. Within every invocation of the pruning function, we remove one block's worth of historical VRFs for a single pruneable session. However, if all Candidates for a specfic Session have been exhausted, then all Historical VRFs across all blocks in that session are pruned.

The parameter for how many candidates to prune per invocation of the pruning function should be more adaptive in order to ensure we prune all blocks (and historical VRFs) in a session approximately as quickly as we prune all candidates in a session.

@Lldenaurois Lldenaurois added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Aug 2, 2021
session: SessionIndex,
candidate_hash: CandidateHash,
included_in: T::BlockNumber,
revert_to: T::BlockNumber,
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not exactly revert_to, though, because I think we revert to included_in - 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about we rename it to revert_tail, since it's the tail of the chain that gets removed in reversion

Copy link
Contributor

Choose a reason for hiding this comment

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

not an expert thus can't give good terminology advice, but seems like included_in was a beteter name, when there's a clear document saying we revert to included_in - 1 when ....

@Lldenaurois Lldenaurois changed the title runtime/parachains: Keep track of included candidates in module runtime/parachains: Keep track of included candidates and historical babe vrfs in shared module Aug 10, 2021
@Lldenaurois Lldenaurois changed the title runtime/parachains: Keep track of included candidates and historical babe vrfs in shared module runtime/parachains: Keep track of included candidates and historical babe vrfs in shared module Aug 10, 2021
@Lldenaurois Lldenaurois marked this pull request as ready for review August 10, 2021 19:37
@kianenigma kianenigma self-requested a review August 18, 2021 16:11
@@ -565,7 +553,9 @@ impl<T: Config> Pallet<T> {
dispute.concluded_at = Some(now);
<Disputes<T>>::insert(session_index, candidate_hash, &dispute);

if <Included<T>>::contains_key(&session_index, &candidate_hash) {
let is_local =
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not clear why you want this variable.

if <Included<T>>::contains_key(&session_index, &candidate_hash) {
let is_local =
<shared::Pallet<T>>::is_included_candidate(&session_index, &candidate_hash);
if is_local {
// Local disputes don't count towards spam.

weight += T::DbWeight::get().reads_writes(1, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

please use saturating math here.

// removed over the course of many block inclusions.
shared::Pallet::<T>::mark_session_pruneable(to_prune);
// TODO(ladi): remove this call, currently allows unit tests to pass. Need to
// figure out how to invoke paras_inherent::enter in run_to_block.
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't you call this in your run_to_block? enter is a dispatchable and is public

CoreIndex(0),
) {
Pallet::<Test>::process_included(session, candidate_hash.clone(), revert_to);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what should happen in else?

/// transitioning to a new session.
#[pallet::storage]
#[pallet::getter(fn pruneable_sessions)]
pub(super) type PruneableSessions<T: Config> = StorageMap<_, Twox64Concat, SessionIndex, ()>;
Copy link
Contributor

Choose a reason for hiding this comment

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

@thiolliere if this pattern is legit, it would be easy to abstract it behind a StorageSet.

included_in: T::BlockNumber,
core_index: CoreIndex,
) -> Option<T::BlockNumber> {
if included_in.is_zero() {
Copy link
Contributor

Choose a reason for hiding this comment

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

when does this happen?

<IncludedCandidates<T>>::get(session, candidate_hash)
}

#[cfg(test)]
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this code not in the mod tests?

@rphmeier rphmeier closed this Apr 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Track all historical BABE VRFs in the shared module Track included candidates and cores in the shared module
3 participants