Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question: way the calls in pallet-parachain-staking need the count? #1917

Closed
atenjin opened this issue Oct 30, 2022 · 6 comments
Closed

Question: way the calls in pallet-parachain-staking need the count? #1917

atenjin opened this issue Oct 30, 2022 · 6 comments

Comments

@atenjin
Copy link

atenjin commented Oct 30, 2022

when I reading the code for the pallet-parachain-staking, I'm very confused for way the operation calls for the candidate should provide the count like:

the call join_candidates, it need the candidate user to provide the candidate_count.
https://github.com/PureStake/moonbeam/blob/e332702ed0426aef440eacfc4db27db3736eee88/pallets/parachain-staking/src/lib.rs#L910-L925

the call schedule_leave_candidates, it also requires candidate to provide the candidate_count
https://github.com/PureStake/moonbeam/blob/e332702ed0426aef440eacfc4db27db3736eee88/pallets/parachain-staking/src/lib.rs#L960-L971

and the call execute_leave_candidates, it needs candidate_delegation_count
https://github.com/PureStake/moonbeam/blob/e332702ed0426aef440eacfc4db27db3736eee88/pallets/parachain-staking/src/lib.rs#L988-L998

But I'm very confused for this, for the count, is just used to check the Vec len in the runtime, not insert to other storage, or we see using it in other logic.(like the line L919, L969, L996) However the check is no meaningful for every one can get the current value from state, so anyone can provide the right number.

I have no idea why candidate should provide this one? If the count is designed to use in other logic like as the index sequence for the candidate or the logic, I can understand. But this one is just used for the checking for the candidates count, I do not know why you need the check?

Is this any other design for this? Thanks

@boundless-forest
Copy link
Contributor

I think the design goal here is a more accurate dispatch call weight, and you can see this value used in.

#[pallet::weight(<T as Config>::WeightInfo::execute_leave_candidates(*candidate_delegation_count))]

@atenjin
Copy link
Author

atenjin commented Oct 31, 2022

I think the design goal here is a more accurate dispatch call weight, and you can see this value used in.

#[pallet::weight(<T as Config>::WeightInfo::execute_leave_candidates(*candidate_delegation_count))]

I do not agree with this... The weight to measure the value is based on the write and read for the storage plus the calculation cost. However the storage which is related to the count is just a Vec, which means the logic just read one storage. Though if the Vec is larger later, but at the weight part it just a little change. So I do not think let the count to join the calculate for the weight is meaningful

@ghost
Copy link

ghost commented Oct 31, 2022

hmmm... I think what AsceticBear said is right, looking at the weight functions of join_candidate and execute_leave_candidates, each additional candidate count value has a significant impact on the weight calculated.

And these values are automatically generated from Substrate benchmark framework.

@4meta5
Copy link
Contributor

4meta5 commented Nov 7, 2022

But I'm very confused for this, for the count, is just used to check the Vec len in the runtime, not insert to other storage, or we see using it in other logic.(like the line L919, L969, L996) However the check is no meaningful for every one can get the current value from state, so anyone can provide the right number.

@atenjin This is how substrate weight hints are supposed to work. The user must provide them because looking them up in storage would incur an additional cost and the weight for the call must be computed without doing that lookup. The call fails as you noted if the weight hint is incorrect, but the hint still needs to be passed as input so that it doesn't have to be looked up when charging the fees.

@atenjin
Copy link
Author

atenjin commented Nov 7, 2022

I see... But I still think it's not useful... The more cast for weight is read/write times for different state storage, for now the count just presents the Vec length, and the length in your logic is limit by the const value MaxXXX... The effect from the "one storage with different length" VS "read/write different storage" is very different. The prev one is more small, and the later one is the thing what weight should consider.

@4meta5
Copy link
Contributor

4meta5 commented Nov 7, 2022

Are you looking at the weights.rs file? This show that the cost of these extrinsics (measured by benchmarking) depends directly on the number of delegations for the given candidate and/or delegator. Using the max for these inputs would overcharge most users for most operations.

I think there is confusion between enforcing an upper bound on the cost and charging an accurate weight. The weight hint exists to achieve the latter goal, the former is achieved by using the MaxXXX.

@4meta5 4meta5 closed this as completed Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants