feat: add lazy total stake calculation#300
Merged
Conversation
ntn-x2
reviewed
Nov 17, 2021
Contributor
Author
|
/bench runtime pallet parachain-staking |
|
Benchmark Runtime Pallet for branch "wf-1701-lazy-total-stake" with command cargo run --quiet --release -p kilt-parachain --features=runtime-benchmarks -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=parachain-staking --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=pallets/parachain-staking/src/default_weights.rs --template=.maintain/weight-template.hbs Results |
…hmarks -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=parachain-staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=pallets/parachain-staking/src/default_weights.rs --template=.maintain/weight-template.hbs
Contributor
Author
|
/bench runtime spiritnet-runtime parachain-staking |
|
Benchmark Runtime Pallet for branch "wf-1701-lazy-total-stake" with command cargo run --quiet --release -p kilt-parachain --features=runtime-benchmarks -- benchmark --chain=spiritnet-dev --steps=50 --repeat=20 --pallet=parachain-staking --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./runtimes/spiritnet/src/weights/parachain_staking.rs --template=.maintain/runtime-weight-template.hbs Results |
weichweich
reviewed
Nov 18, 2021
…hmarks -- benchmark --chain=spiritnet-dev --steps=50 --repeat=20 --pallet=parachain-staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./runtimes/spiritnet/src/weights/parachain_staking.rs --template=.maintain/runtime-weight-template.hbs
weichweich
reviewed
Nov 18, 2021
Contributor
weichweich
left a comment
weichweich
left a comment
There was a problem hiding this comment.
I think update_top_candidates is going to be more complicated?
Contributor
Author
|
Some parts of the weight complexity doc comments in |
Contributor
Yes we have the benchmarks and that should be fine |
weichweich
suggested changes
Dec 8, 2021
Contributor
weichweich
left a comment
weichweich
left a comment
There was a problem hiding this comment.
I think the try_insert_replace method is very confusing and doesn't work as you intended, because it doesn't replace a value if it is equal.
In general, i would be interested in the latest benchmarks ;)
In general2, i would love to see the complexity of this pallet reduced. It's unbelievable complex. I also have no idea how.
Comment on lines
+2093
to
+2094
| // case 2: candidate newly joins TopCandidates and either replaces | ||
| // another candidate in the list or updates their own stake |
Contributor
There was a problem hiding this comment.
I don't understand the last part with or updates their own stake. isn't this method always called when stake is updated?
Contributor
Author
There was a problem hiding this comment.
isn't this method always called when stake is updated?
This method yes, but case 2 only applies if the candidate was not part of TopCandidates with the old stake (case 1) or did not stake at all previously.
Contributor
There was a problem hiding this comment.
I think i still didn't understand. 😂
case 1: he was in the top candidates and the delegator stake changed, but the self stake stayed the same
case 2: He was not part of top candidates || he was part of the list and changed their own stake
Contributor
Author
There was a problem hiding this comment.
Case 1: Candidate was in TopCandidates with old stake.
Case 2: Not Case 1 && Candidate becomes member of TopCandidates with new stake.
Both cases are independent of who/what triggered the total stake change.
However I just realized that case 1 is incomplete and incorrect: Currently I update the TotalCollatorStake if the candidate was a collator with old stake and don' t check their position with new stake. This only works if we can assume the candidate still is a collator 🙈
Comment on lines
+2107
to
+2108
| // candidate either pushed out the least staked collator or updated their own | ||
| // stake |
Contributor
There was a problem hiding this comment.
if the value is already in the set it will net get replaced. So the self replace case doesn't work. I think.
Contributor
Author
There was a problem hiding this comment.
Yes, but the value is of type Stake and not Balance or AccountId. Thus, by increasing the staked amount, the candidate can replace their own entry in the TopCandidates list. Otherwise, the candidate can replace any other candidate with less stake.
Contributor
Author
There was a problem hiding this comment.
Yes, but the value is of type
Stakeand notBalanceorAccountId. Thus, by increasing the staked amount, the candidate can replace their own entry in the TopCandidates list. Otherwise, the candidate can replace any other candidate with less stake.
I revert this statement. You are right.
Comment on lines
+2129
to
+2134
| // only trigger event if candidate and drop_out differ | ||
| if let Some(Stake { owner: drop_id, .. }) = drop_out { | ||
| if drop_id != candidate { | ||
| Self::deposit_event(Event::LeftTopCandidates(drop_id)); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This should always be the case. Let me cite the try_insert_replace doc.
/// Returns
/// * Ok(Some(old_element)) if the new element was added and an old element
/// had to be removed.
/// * Ok(None) if the element was added without removing an element.
/// * Err(true) if the set is full and the new element has a lower rank than
/// the lowest element in the set.
/// * Err(false) if the element is already in the set.
Contributor
Author
There was a problem hiding this comment.
You are mistaking membership with matching AccountIds when in fact, the Stake::<T> { amount: BalanceOf<T>, owner: AccountIdOf<T> } would have to match, see above.
Contributor
Author
There was a problem hiding this comment.
You are mistaking membership with matching
AccountIds when in fact, theStake::<T> { amount: BalanceOf<T>, owner: AccountIdOf<T> }would have to match, see above.
I revert this statement. You are right, two Stakes are considered equal in an OrderedSet if the AccountId matches.
ntn-x2
reviewed
Dec 8, 2021
Contributor
ntn-x2
left a comment
ntn-x2
left a comment
There was a problem hiding this comment.
Some questions about benchmarks and weights, with regard to the usefulness of knowing the # of delegators for a collator, in most (but not all) cases.
Contributor
|
To answer @weichweich point2 about complexity, I think it would help if we can have all the storage reads within the extrinsic, and then have all the auxiliary functions work as a function of the input, without performing additional logic. Splitting responsibilities in several smaller pure functions, and accessing all the storage in the extrinsic. Those functions are then, in my opinion, more easily testable. Nevertheless, this is food for thought for the future. |
Co-authored-by: Albrecht <albrecht@kilt.io>
Contributor
Author
I am honestly unsure whether this reduce the complexity of this pallet because this will add a LOT of duplicate code. But it's definitely something worth trying out in the future. |
ntn-x2
approved these changes
Dec 10, 2021
Contributor
ntn-x2
left a comment
ntn-x2
left a comment
There was a problem hiding this comment.
I like the new approach better, as it is a bit easier to follow now (praised be the Rust pattern matching 🙏). I would say it looks good to me, also considering the additional test cases added 👍. I do have few comments about clarity tho to make it a bit easier to go back to this code in 6 months time.
| // get displacer | ||
| let (add_collators, add_delegators) = Self::get_top_candidate_stake_at(&top_candidates, i) | ||
| // shouldn't be possible to fail, but we handle it gracefully | ||
| .unwrap_or((new_self, new_delegators)); |
Contributor
There was a problem hiding this comment.
I think we can fail if top_candidates < max_selected_candidates, right? Kind of an out-of-bound error. So I think that returning (new_self, new_delegators) (as is already the case) is the right default here.
Contributor
Author
There was a problem hiding this comment.
Even that should be covered because our replacement index is min(top_candidates.len(), max_selected_candidates) - 1. It can only fail if the candidate at the replacement index does not have an entry in CandidatePool which should be impossible.
weichweich
approved these changes
Dec 13, 2021
Comment on lines
2546
to
2554
| /// # <weight> | ||
| /// Weight: O(D) where D is the number of delegators of the collator | ||
| /// candidate bounded by `MaxDelegatorsPerCollator` | ||
| /// Weight: O(D + U) where D is the number of delegators of the collator | ||
| /// candidate bounded by `MaxDelegatorsPerCollator` and U is the | ||
| /// number of locked unstaking requests bounded by `MaxUnstakeRequests`. | ||
| /// - Reads: BlockNumber, D * DelegatorState, D * Unstaking | ||
| /// - Writes: D * DelegatorState, (D + 1) * Unstaking | ||
| /// - Kills: CollatorState, DelegatorState for all delegators which only | ||
| /// - Kills: CandidatePool, DelegatorState for all delegators which only | ||
| /// delegated to the candidate | ||
| /// # </weight> |
Contributor
There was a problem hiding this comment.
I thought we wanted to remove these?
Contributor
Author
There was a problem hiding this comment.
Yes. The changes here were made before this decision.
Contributor
Author
|
/bench runtime pallet parachain-staking |
|
Benchmark Runtime Pallet for branch "wf-1701-lazy-total-stake" with command cargo run --quiet --release -p kilt-parachain --features=runtime-benchmarks -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=parachain-staking --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=pallets/parachain-staking/src/default_weights.rs --template=.maintain/weight-template.hbs Results |
…hmarks -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=parachain-staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=pallets/parachain-staking/src/default_weights.rs --template=.maintain/weight-template.hbs
Contributor
Author
|
/bench runtime spiritnet-runtime parachain-staking |
|
Benchmark Runtime Pallet for branch "wf-1701-lazy-total-stake" with command cargo run --quiet --release -p kilt-parachain --features=runtime-benchmarks -- benchmark --chain=spiritnet-dev --steps=50 --repeat=20 --pallet=parachain-staking --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./runtimes/spiritnet/src/weights/parachain_staking.rs --template=.maintain/runtime-weight-template.hbs Results |
…hmarks -- benchmark --chain=spiritnet-dev --steps=50 --repeat=20 --pallet=parachain-staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./runtimes/spiritnet/src/weights/parachain_staking.rs --template=.maintain/runtime-weight-template.hbs
Contributor
Author
|
/bench runtime peregrine-runtime parachain-staking |
|
Error running benchmark: wf-1701-lazy-total-stake stdoutCaught exception in benchmarkRuntime: TypeError: Cannot read properties of undefined (reading 'benchCommand') |
Contributor
Author
|
/bench runtime peregrine parachain-staking |
|
Benchmark Runtime Substrate Pallet for branch "wf-1701-lazy-total-stake" with command cargo run --quiet --release -p kilt-parachain --features=runtime-benchmarks -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=parachain-staking --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./runtimes/peregrine/src/weights/parachain_staking.rs --template=.maintain/runtime-weight-template.hbs Results |
…hmarks -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=parachain-staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./runtimes/peregrine/src/weights/parachain_staking.rs --template=.maintain/runtime-weight-template.hbs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes KILTProtocol/ticket#1701
TotalCollatorStakestorage by removing the iteration over all selected candidates (except forinit_leave_candidateandforce_remove_candidateTODO
Checklist:
array[3]useget(3), ...)