Adds election provider multi-block pallet (wip) #6213
Conversation
gpestana
changed the title
| //! ## Pallet organization and sub-pallets | ||
| //! | ||
| //! This pallet consists of a `core` pallet and multiple sub-pallets. Conceptually, the pallets have | ||
| //! a hierarquical relationship, where the `core` pallet sets and manages shared state between all |
There was a problem hiding this comment.
| //! a hierarquical relationship, where the `core` pallet sets and manages shared state between all | |
| //! a hierarchical relationship, where the `core` pallet sets and manages shared state between all |
| //! Even though the [`frame_election_provider_support::ElectionDataProvider`] and | ||
| //! [`frame_election_provider_support::ElectionProvider`] traits support multi-paged election, this | ||
| //! pallet only supports a single page election flow. Thus, [`Config::Pages`] must be always set to | ||
| //! one, which is asserted by the [`frame_support::traits::Hooks::integrity_test`]. |
There was a problem hiding this comment.
So in short this pallet does not support multi-page election and if one wants to the multi-page election on their runtime, they should use the multi-block pallet, correct?
If it's so it'd be good to add such a mention to the doc.
| //! [`verifier::SolutionDataProvider`], which is used by the [`verifier`] pallet to fetch solution | ||
| //! data to perform the solution verification. | ||
| //! - The [`unsigned`] pallet implements the unsigned phase, where block authors can compute and | ||
| //! submit through inherent paged solutions. This pallet also implements the |
There was a problem hiding this comment.
My understanding, based on https://github.com/paritytech/polkadot-sdk/blob/gpestana/pallet-epm-mb/substrate/frame/election-provider-multi-phase/src/lib.rs#L98, is that this phase is essentially as the signed one but the origins of solutions are different, is it not the case?
This description is a little unclear as-well, thus my remark here.
|
|
||
| ensure!( | ||
| crate::Pallet::<T>::current_phase().is_signed(), | ||
| Error::<T>::NotAcceptingSubmissions |
There was a problem hiding this comment.
Shouldn't there be a different error? e.g. CannotBailAtThisPhase
| // updates metadata release strategy so that all the deposit is burned when the | ||
| // leader's data is cleared. | ||
| metadata.release_strategy = ReleaseStrategy::BurnAll; | ||
| Submissions::<T>::set_metadata(round, &leader, metadata); |
There was a problem hiding this comment.
This will fail because set_metadata checks for the leader to be present in SortedScores but he (that entry) was already removed in line 828 by Submissions::<T>::take_leader_score(round).
| "error fetching the desired targets from the election data provider {}", | ||
| err | ||
| ); | ||
| None |
There was a problem hiding this comment.
Will "None" be ever returned? I wrote a test and it panics at "defensive" already
| if Self::ensure_score_quality(claimed_score).is_err() { | ||
| Self::deposit_event(Event::<T>::VerificationFailed( | ||
| crate::Pallet::<T>::msp(), | ||
| FeasibilityError::ScoreTooLow, |
There was a problem hiding this comment.
Shouldn't it be the exact error returned from ensure_score_quality instead?
| /// better than the minimum score, of no queued solution exists. | ||
| /// 5. Submits the paged solution as an inherent through the [`Call::submit_page_unsigned`] | ||
| /// callable. | ||
| pub fn do_sync_offchain_worker(_now: BlockNumberFor<T>) -> Result<(), OffchainMinerError> { |
There was a problem hiding this comment.
_now parameter is unused. Is it needed here?
| type MaxWinnersPerPage: Get<u32>; | ||
| type MaxBackersPerWinner: Get<u32>; | ||
|
|
||
| type VoterSnapshotPerBlock: Get<u32>; |
There was a problem hiding this comment.
Shouldn't this be called ...PerPage rather?
Maybe stupid question but line 146 with argument all_voter_pages made me think this way: it's a vector with max length equal to T::Pages so each entry in this vector should be "per page" therefore each entry should have at most N MinerVoterOf entities, where N is a cap on these voters "per page".
|
|
||
| if let Status::Ongoing(current_page) = <VerificationStatus<T>>::get() { | ||
| let maybe_page_solution = | ||
| <T::SolutionDataProvider as SolutionDataProvider>::get_paged_solution(current_page); |
There was a problem hiding this comment.
get_paged_solution returns the leader's solution so if it's none (no leader solution) T::SolutionDataProvider::report_result will panic because it does Submissions::<T>::take_leader_score which already is none.
So maybe report_result shouldn't check for the leader's score or there is some other change needed in the logic.
… tests to its own mod
Co-authored-by: Gonçalo Pestana <g6pestana@gmail.com>
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
I will re-open this from another branch. Thank you @gpestana for your contributions here! |
|
Superseded by #7282 |
This PR adds the election provider multi-block pallet. It doesn't necessarily need to be audited and completed in order to merge, assuming that the multi-block types and refactors have been merged and audited (#6034).
Remove from WIP/draft once #6034 is merged/locked.
Todo before merge:
masterafter above