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

[MOON-2552] fix: only add PoV weight of Self::mint_and_compound() the first time it is called #2459

Closed
wants to merge 5 commits into from

Conversation

grw-ms
Copy link
Contributor

@grw-ms grw-ms commented Aug 29, 2023

alternative to #2458

What does it do?

add already_called_for_delegator: bool argument to delegation_bond_more_without_event() and mint_and_compound, skip adding PoV size if it is true

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@grw-ms grw-ms changed the title fix: only add PoV weight of Self::mint_and_compound() the first time it is called [MOON-2552] fix: only add PoV weight of Self::mint_and_compound() the first time it is called Aug 29, 2023
pallets/parachain-staking/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Alan Sapede <alan.sapede@gmail.com>
@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2023

Coverage generated "Tue Aug 29 12:57:22 UTC 2023":
https://d3ifz9vhxc2wtb.cloudfront.net/pulls/2459/html/index.html

Master coverage: 87.39%
Pull coverage:

// returned will be an over-estimate since the read was already performed on the first
// call and subsequent calls do not increase PoV size further.
if already_called_for_delegator {
actual_weight.set_proof_size(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seem's wrong to me as the only overlap is on the storage item DelegationScheduledRequests, it's better to apply x = 0 instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not going to work.
The way the benchmark is done is that it doesn't properly use the topDelegation storage properly and rely on the DelegationScheduledRequests argument passed, which is already 0 is most of the cases.

If we want to use this method of applying x = 0, then we need to review the weight (around 5kb for x = 0) of the benchmark, as this adds up to 1.5Mb with an empty storage (while in reality a full storage only goes to 150kb)

Co-authored-by: tmpolaczyk <44604217+tmpolaczyk@users.noreply.github.com>
@grw-ms grw-ms force-pushed the grw/moon-2552-empty-blocks-alternate branch from e966e54 to 05226ff Compare August 29, 2023 12:08
Co-authored-by: Éloïs <c@elo.tf>
@grw-ms grw-ms force-pushed the grw/moon-2552-empty-blocks-alternate branch from b3133b5 to 173701a Compare August 29, 2023 12:19
@grw-ms grw-ms added the A0-pleasereview Pull request needs code review. label Aug 29, 2023
@nbaztec nbaztec added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. breaking Needs to be mentioned in breaking changes and removed A0-pleasereview Pull request needs code review. labels Aug 29, 2023
@noandrea
Copy link
Collaborator

superseded by #2461

@noandrea noandrea closed this Aug 29, 2023
@grw-ms grw-ms deleted the grw/moon-2552-empty-blocks-alternate branch September 5, 2023 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants