Add simple collator election mechanism #2960

georgepisaltu · 2023-08-01T17:24:27Z

This PR adds the ability to bid for collator slots even after the max number of collators have already registered. This eliminates the first come, first served mechanism that was in place before.

Key changes:

added increase_bond/decrease_bond extrinsics to allow registered candidates to adjust their bonds in order to dynamically control their bids
added take_candidate_slot extrinsic to try to replace an already existing candidate by bidding more than them
candidates are now lazily sorted through the bags-list pallet, where the top DesiredCandidates out of MaxCandidates candidates in the list will be selected by the session pallet as collators

I'm not sure whether we should allow decreasing the candidate bids. The mechanic is partially in place already through leave_intent, but it's not quite the same thing. I'm also open to any rename suggestions.

I made adjustments to the parachain template runtime to test things out, but will go ahead and fix the rest of them once we reach consensus on the overall approach.

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

joepetrowski

Looks like you also need to impl SortedListProvider, unless I'm missing it somewhere? But I don't see a default impl in Substrate.

pallets/collator-selection/src/lib.rs

pallets/collator-selection/src/mock.rs

joepetrowski · 2023-08-02T17:24:17Z

parachain-template/runtime/src/lib.rs

@@ -486,6 +504,7 @@ construct_runtime!(
 		Session: pallet_session = 22,
 		Aura: pallet_aura = 23,
 		AuraExt: cumulus_pallet_aura_ext = 24,
+		CandidateList: pallet_bags_list::<Instance1> = 25,


Implied

Suggested change

CandidateList: pallet_bags_list::<Instance1> = 25,

CandidateList: pallet_bags_list = 25,

joepetrowski · 2023-08-02T17:25:54Z

pallets/collator-selection/src/lib.rs

+
+			Self::deposit_event(Event::CandidateRemoved { account_id: target });
+			Self::deposit_event(Event::CandidateAdded { account_id: who, deposit });
+			Ok(Some(T::WeightInfo::take_candidate_slot(length as u32)).into())


Should do the same for the other functions: refund weight by providing the actual length.

Good catch, I forgot to do this for increase_bond/decrease_bond.

muharem · 2023-08-03T14:04:15Z

pallets/collator-selection/src/lib.rs

+					if candidate_info.deposit < <CandidacyBond<T>>::get() {
+						return Err(Error::<T>::OnRemoveInsufficientFunds.into())
+					}
+					T::Currency::unreserve(&who, bond);


would be good to migrate to new fungible trait from Currency
may be in different PR
here about new traits - paritytech/substrate#12951

muharem · 2023-08-03T14:12:44Z

pallets/collator-selection/src/lib.rs

@@ -289,6 +300,16 @@ pub mod pallet {
 		NoAssociatedValidatorId,
 		/// Validator ID is not yet registered.
 		ValidatorNotRegistered,
+		/// Some doc.
+		OnInsert,


more idiomatic way within our libs to name the errors, is not describing on what occasion an error has happened, but what has happened, FailedToUpdateCandidateList for example

Yes, the names are just placeholders right now. Same with doc comments. I'll write some proper ones once the other comments are addressed.

muharem · 2023-08-03T14:18:08Z

pallets/collator-selection/src/lib.rs

+			});
+			Ok(())
+		}
+


it would be easier to review with the docs telling what this meant for
who is target, who is origin?

The caller of this extrinsic wants to register themselves in the candidate list by taking the spot of another candidate target. The caller's deposit must be greater than the deposit of the candidate it's trying to replace for this to work.

I'll get some proper documentation on these functions in a future iteration.

muharem · 2023-08-03T14:28:55Z

pallets/collator-selection/src/lib.rs

 			collators
 		}

 		/// Kicks out candidates that did not produce a block in the kick threshold and refunds
 		/// their deposits.
 		pub fn kick_stale_candidates(
 			candidates: BoundedVec<CandidateInfo<T::AccountId, BalanceOf<T>>, T::MaxCandidates>,
-		) -> BoundedVec<T::AccountId, T::MaxCandidates> {
+		) -> usize {


would be nice to document what is returned value

muharem · 2023-08-03T14:44:27Z

pallets/collator-selection/src/lib.rs

@@ -142,6 +143,8 @@ pub mod pallet {
 		/// Maximum number of invulnerables.
 		type MaxInvulnerables: Get<u32>;

+		type CandidateList: SortedListProvider<Self::AccountId, Score = BalanceOf<Self>>;


can you explain why we using the bags-list?
the number of Candidates is not more than desired_candidates (can be more if desired_candidates changed to the less value), so the Candidates list is quite small.
Candidates list stored as a value, the whole list is being read usually from the storage.
is this more efficient than maintaining a sorted list of Candidates within the pallet?

There can be at most MaxCandidates candidates, out of which the top N (where N is DesiredCandidates) are selected to be collators when sessions begin/end. The list could be small, it depends on the config, but the idea behind using bags-list was to have it lazily sorted, as in people take a slot in the list and then they use bags-list extrinsics to place their entry higher on the list, thus sorting it themselves and paying for it. This moves the sorting logic off-chain as the candidates registering have to watch the list and do the calls if/when they see fit. It would also give an incentive to only the top N candidates to be in a sorted order, since if you're below the N threshold, it doesn't matter if your entry is out of order or not since you're not gonna get selected for collation. This is also true for all the entries above the threshold, since it doesn't matter what spot of the top N you got, you're still going to be selected. This reduces the sorting burden on the list and also makes the candidates responsible for their place in the list.

bags-list will perform additional reads and inserts on insert/decrease/increase etc. On decrease and increase it will run the same rebag as it does with the extrinsic you mentioning. So I am not sure what exactly you optimizing with it.
what the BagThresholds you planning to set?
I think for this case, when MaxCandidates wont be more than 50 in near future, its not necessary.

If the bags list is counter productive in terms of performance and resources since the number of candidates is potentially small, I'd suggest implementing a simpler SortedListProvider that allows parachain devs to decide whether to use that implementation or an instance of bags-lists.

In any case, it would be interesting to compare benchmarks with both implementations and see what's the threshold in terms of number of candidates (and corresponding thresholds) where the bags list approach starts to make sense (if it's not always better -- I'm unsure).

bags-list will perform additional reads and inserts on insert/decrease/increase etc. On decrease and increase it will run the same rebag as it does with the extrinsic you mentioning. So I am not sure what exactly you optimizing with it.
what the BagThresholds you planning to set?

I was planning to have a single universal bag - so BagThresholds = [] - and on_increase and on_decrease would be noops for that configuration since rebag would do nothing, but they would be needed if the user chose a different SortedListProvider implementation. The on_insert operation for bags-list is very simple, while on_remove is a bit costly indeed, since it would need to traverse the list.

I think for this case, when MaxCandidates wont be more than 50 in near future, its not necessary.

I thought it was supposed to be about 100 in the near future with the potential to grow even more, but I'm not sure, I might be misremembering.

In any case, it would be interesting to compare benchmarks with both implementations and see what's the threshold in terms of number of candidates (and corresponding thresholds) where the bags list approach starts to make sense (if it's not always better -- I'm unsure).

I'm in favor of implementing a simpler SortedListProvider and comparing the two.

I implemented both the simple list as a separate pallet as well as integrated within collator-selection. I ran the benchmarks on my machine and these are the results in terms of weight:
register_as_candidate: bags-list outperforms for MaxCandidates > 55
increase_bond: bags-list outperforms for MaxCandidates > 10
decrease_bond: bags-list outperforms for MaxCandidates > 6
take_candidate_slot: bags-list outperforms for MaxCandidates > 160
leave_intent always: more expensive on bags-list by about 50%
new_session always: more expensive on bags-list by about 30%

There is no major difference between the simple sorted list pallet vs list kept in the storage of collator-selection, aside from increase_bond which starts to be better for the integrated list for MaxCandidates > 13.

The bags-list benchmarks do not include the put_in_front_of call costs because those are separate and are supposed to be called less often than the other extrinsics presented here, as in not every time a bond changes.

I didn't get to run the bags-list pallet benchmarks locally to get an idea of how much those would cost, but each put_in_front_of scales linearly with MaxCandidates. Given the small MaxCandidates values we're working with, bags-list is not ideal. That said, the simple list as a separate pallet seems to be the way to go for now, since it allows for customization down the line and the weights are very similar with the integrated version.

I'll link the branches used for the benchmarks below:

bags-list version

simple list separate pallet

simple list within collator-selection

@georgepisaltu why you need two lists (Candidates and CandidateList) for the simple list within collator-selection solution?
here you even do an additional read for very candidate - https://github.com/georgepisaltu/cumulus/blob/b0ca1b615336d83ea210a26950efc4833ab5367e/pallets/collator-selection/src/lib.rs#L823

you can have only one Candidate<T: Config> = StorageValue<_, BoundedVec<(AccountId, Deposit), T::MaxCandidates> for that solution.

with that we wont event need those new three dispatchables. we need only one more or register_as_candidate can both. we can only rename it (keeping the same index) to imply that its not only register but also reset bond.

why you need two lists (Candidates and CandidateList) for the simple list within collator-selection solution?

The purpose of the Candidates map is to allow fast access to the deposits. Even in the solution where the list was within the collator-selection pallet storage, there are still places where we look for an account's deposit and we have to iterate the list if we don't have the map. However, with these small numbers it's probably counterproductive to load a 10-sized map into memory just for that. As a bonus, it works well with the SortedListProvider/ScoreProvider abstraction we have in place. I'm not too worried about the "extra" map since it uses the Identity hasher, so it should be cheaper than a standard one.

with that we wont event need those new three dispatchables. we need only one more or register_as_candidate can both. we can only rename it (keeping the same index) to imply that its not only register but also reset bond.

The plan right now is to merge increase_bond and decrease_bond into a update_bond. A function like update_bond is necessary to allow an auction to take place and I like the idea of these functions being as intuitive and simple as possible, so I'm against having bid update mechanics in the function used to register.

Anyway, I implemented a simpler version without the map, as you suggested, and I got the benchmarks comparing the simple list in a separate pallet with the mapless implementation (I'll refer to it as "simplest" from this point on):
register_as_candidate: always more expensive with the pallet version by about 40%
increase_bond: simplest version outperforms for MaxCandidates > 10
decrease_bond: same base weight, increases by about 1/30 of the base weight for each extra candidate for the pallet version
take_candidate_slot: always more expensive with the pallet version by about 35%
leave_intent: always more expensive with the pallet version by about 20%
new_session: always more expensive with the pallet version by about 25%
Also, the simplest version also uses about half the reads/writes on the database compared to the simple pallet version.

Also, I managed to get the benchmarks for the bags-list put_in_front_of calls, which would be additional weight incurred by users in the bags-list version. However, the catch with that is that if we use an empty BagThresholds, there is only a single universal bag, so all rebags are basically noops (they still have to fetch the thresholds from storage but the cost is reflected in the previous benchmarks I posted anyway). This means that the optimal way to use the bags-list version would be to register as a candidate, observe the list off-chain and do calls to update_bond as necessary to have a top-DesiredCandidates bond in place, so before the new session is about to begin and the collators are being selected (after the dust is settled and the bidding is done), if you are a top-DesiredCandidates candidate, call put_in_front_of so that you can claim your collator spot. This means that the amount of swaps done on the list becomes DesiredCandidates, a fraction of MaxCandidates by definition, and it's easy to see how it would scale if the numbers were greater, but as I understand, MaxCandidates will probably be a small number. put_in_front_of is about as expensive as a leave_intent, which means it's inexpensive for a candidate to call this to claim their spot.

That being said, I think for the problem we're trying to solve right now we should go with the simple list version, but in a separate pallet while keeping the SortedListProvider abstraction. The reasons for this are:

there isn't a significant difference between the separate pallet version and the list inside collator-selection pallet storage (see benches above)

easy to upgrade in the future

works for other users of the collator-selection pallet who may want to have lots of collators, even if we plan on having small lists of candidates

Main downside of this approach is the extra complexity in the code because of the new simple-list pallet which needs to be tested/maintained.

I assumed that other users may want to use the collator-selection pallet with MaxCandidates values greater than 50-100, which seems to be the cut-off point for bags-list. If I'm wrong about that, and, AFAIK, we don't really plan to raise that over 20 any time soon, then it's probably better to go with the simplest possible version, a list kept inside the collator-selection pallet storage.

@muharem @joepetrowski @gpestana let me know what you think

I would go for the simpler solution since we don't expect needing more than 100 candidates at any time. Having a list and a map also adds overhead of needing to maintain some invariants like have one-to-one correspondence of elements and keys.

About the solution of removing the map, BoundedVecs should generally be at their limit in usage, while general storage should use maps to facilitate O(1) operations. Perhaps a good solution is to store candidates in a map, but only pass a (full) BoundedVec to Session for the next set of validators? I'm not sure the costs of computing that on session change vs. just maintaining candidates in a vec though.

Agree with collapsing increase/decrease into update.

I don't think we should optimize for benchmarks. We don't expect these functions to be called very often. The one we do need to keep an eye on is the new_session call since that is called in on_initialize and needs to stay within block building bounds.

georgepisaltu · 2023-08-04T09:40:05Z

pallets/collator-selection/src/lib.rs

@@ -142,6 +143,8 @@ pub mod pallet {
 		/// Maximum number of invulnerables.
 		type MaxInvulnerables: Get<u32>;

+		type CandidateList: SortedListProvider<Self::AccountId, Score = BalanceOf<Self>>;


There can be at most MaxCandidates candidates, out of which the top N (where N is DesiredCandidates) are selected to be collators when sessions begin/end. The list could be small, it depends on the config, but the idea behind using bags-list was to have it lazily sorted, as in people take a slot in the list and then they use bags-list extrinsics to place their entry higher on the list, thus sorting it themselves and paying for it. This moves the sorting logic off-chain as the candidates registering have to watch the list and do the calls if/when they see fit. It would also give an incentive to only the top N candidates to be in a sorted order, since if you're below the N threshold, it doesn't matter if your entry is out of order or not since you're not gonna get selected for collation. This is also true for all the entries above the threshold, since it doesn't matter what spot of the top N you got, you're still going to be selected. This reduces the sorting burden on the list and also makes the candidates responsible for their place in the list.

pallets/collator-selection/src/lib.rs

georgepisaltu · 2023-08-04T09:41:19Z

pallets/collator-selection/src/lib.rs

@@ -289,6 +300,16 @@ pub mod pallet {
 		NoAssociatedValidatorId,
 		/// Validator ID is not yet registered.
 		ValidatorNotRegistered,
+		/// Some doc.
+		OnInsert,


Yes, the names are just placeholders right now. Same with doc comments. I'll write some proper ones once the other comments are addressed.

georgepisaltu · 2023-08-04T09:43:34Z

pallets/collator-selection/src/lib.rs

+			});
+			Ok(())
+		}
+


The caller of this extrinsic wants to register themselves in the candidate list by taking the spot of another candidate target. The caller's deposit must be greater than the deposit of the candidate it's trying to replace for this to work.

I'll get some proper documentation on these functions in a future iteration.

georgepisaltu · 2023-08-04T09:44:03Z

pallets/collator-selection/src/lib.rs

 			collators
 		}

 		/// Kicks out candidates that did not produce a block in the kick threshold and refunds
 		/// their deposits.
 		pub fn kick_stale_candidates(
 			candidates: BoundedVec<CandidateInfo<T::AccountId, BalanceOf<T>>, T::MaxCandidates>,
-		) -> BoundedVec<T::AccountId, T::MaxCandidates> {
+		) -> usize {


pallets/collator-selection/src/tests.rs

pallets/collator-selection/src/lib.rs

gpestana · 2023-08-07T21:11:17Z

pallets/collator-selection/src/lib.rs

@@ -142,6 +143,8 @@ pub mod pallet {
 		/// Maximum number of invulnerables.
 		type MaxInvulnerables: Get<u32>;

+		type CandidateList: SortedListProvider<Self::AccountId, Score = BalanceOf<Self>>;


If the bags list is counter productive in terms of performance and resources since the number of candidates is potentially small, I'd suggest implementing a simpler SortedListProvider that allows parachain devs to decide whether to use that implementation or an instance of bags-lists.

In any case, it would be interesting to compare benchmarks with both implementations and see what's the threshold in terms of number of candidates (and corresponding thresholds) where the bags list approach starts to make sense (if it's not always better -- I'm unsure).

gpestana · 2023-08-07T21:14:46Z

pallets/collator-selection/src/lib.rs

+			let who = ensure_signed(origin)?;
+
+			let new_bond =
+				<Candidates<T>>::try_mutate(|candidates| -> Result<BalanceOf<T>, DispatchError> {


Maybe a map keyed by account Id would suit better the candidates use case to avoid this loop.

gpestana · 2023-08-07T21:20:45Z

pallets/collator-selection/src/lib.rs

+		///
+		/// Todo
+		#[pallet::call_index(7)]
+		#[pallet::weight(T::WeightInfo::increase_bond(T::MaxCandidates::get()))]


If we keep the candidates in a vec, perhaps we should do weight refund here using the DispatchWeightWithPostInfo, as the worst case estimate may be overestimating and fees will be too high. See more here and maybe some helpful pointers here

gpestana · 2023-08-07T21:22:11Z

pallets/collator-selection/src/lib.rs

+		///
+		/// Todo
+		#[pallet::call_index(8)]
+		#[pallet::weight(T::WeightInfo::decrease_bond(T::MaxCandidates::get()))]


Same comment regarding weights as in increase_bond.

gpestana · 2023-08-07T21:27:55Z

pallets/collator-selection/src/lib.rs

+			let who = ensure_signed(origin)?;
+
+			let new_bond =
+				<Candidates<T>>::try_mutate(|candidates| -> Result<BalanceOf<T>, DispatchError> {


I have a suggestion for you here: the implementation of the increase and decrease bond extrinsics are very similar. I'd suggest refactoring out most it it's mutation logic into a Self::update_bond(accountID, imbalance), where the imbalance is an enum that can take positive and negative imbalance (to add or remove bond).

The pallet balances implements a similar logic and, perhaps more relevant, I've implemented something similar in this PR, which also calls into a SortedListProvoder::on_increase/decrease paritytech/substrate#14620

I think it's a good idea.

gpestana · 2023-08-07T21:34:22Z

pallets/collator-selection/src/lib.rs

+				Error::<T>::ValidatorNotRegistered
+			);
+
+			ensure!(deposit >= Self::candidacy_bond(), Error::<T>::InsufficientBond);


I'd move this check before fetching the validator key, in the spirit of verifying first and read/write later. The storage transactional ensures that all the writes are rolled back if the extrinsic fails, but the node may spend unnecessary cycles reading/writing something that will be eventually rolled back due to this check.

gpestana · 2023-08-07T21:35:50Z

pallets/collator-selection/src/lib.rs

+
+			ensure!(deposit >= Self::candidacy_bond(), Error::<T>::InsufficientBond);
+
+			<Candidates<T>>::try_mutate(|candidates| -> Result<(), DispatchError> {


Yes, I start to think that Candidates should be refactored as a storage map instead of a vec.

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

paritytech-cicd-pr · 2023-08-09T11:44:00Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3352264

georgepisaltu added 9 commits August 1, 2023 19:56

Introduce bags-list pallet to collator selection

b477394

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Add increase/decrease bond and benches

a218cda

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

UT and some bench fixes

bd4064f

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Work on session UTs

54d8e58

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Benchmarks and some refactoring

637112b

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Add extrinsic for buying slot

6fc4026

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Add UT for buy_slot

9955611

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Fix register candidate UT

54dd1c5

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Rename buy_slot to take_candidate_slot

5d928f5

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

georgepisaltu requested review from gilescope, gpestana, bkchr, muharem and joepetrowski August 1, 2023 17:24

georgepisaltu self-assigned this Aug 1, 2023

georgepisaltu added T5-parachains This PR/Issue is related to Parachains. T1-runtime This PR/Issue is related to the topic “runtime”. and removed T5-parachains This PR/Issue is related to Parachains. labels Aug 1, 2023

joepetrowski reviewed Aug 2, 2023

View reviewed changes

muharem reviewed Aug 3, 2023

View reviewed changes

georgepisaltu commented Aug 4, 2023

View reviewed changes

gpestana suggested changes Aug 7, 2023

View reviewed changes

georgepisaltu added 4 commits August 8, 2023 10:37

Renames and minor refactoring

ca21855

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Refund weight for bid extrinsics

5f89bc3

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Collapse take_candidate_slot events

3e5494a

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

Move Candidates to StorageMap

7526910

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>

georgepisaltu mentioned this pull request Aug 31, 2023

Add simple collator election mechanism paritytech/polkadot-sdk#1340

Merged

5 tasks

	CandidateList: pallet_bags_list::<Instance1> = 25,
	CandidateList: pallet_bags_list = 25,


		ensure!(deposit >= Self::candidacy_bond(), Error::<T>::InsufficientBond);

		<Candidates<T>>::try_mutate(\|candidates\| -> Result<(), DispatchError> {

Add simple collator election mechanism #2960

Are you sure you want to change the base?

Add simple collator election mechanism #2960

Conversation

georgepisaltu commented Aug 1, 2023

joepetrowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muharem Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muharem Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muharem Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgepisaltu Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented Aug 9, 2023

muharem Aug 3, 2023 •

edited

Loading

muharem Aug 7, 2023 •

edited

Loading

muharem Aug 22, 2023 •

edited

Loading

georgepisaltu Aug 23, 2023 •

edited

Loading