Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Implements a variable deposit base calculation for EPM signed submissions #13983

Open
wants to merge 63 commits into
base: master
Choose a base branch
from
Open

Implements a variable deposit base calculation for EPM signed submissions #13983

wants to merge 63 commits into from

Conversation

gpestana
Copy link
Contributor

@gpestana gpestana commented Apr 23, 2023 •
edited
Loading

This PR implements a generic BaseDeposit calculation for signed submissions, based on the size of the submission queue.

It adds a new associated type to EPM's config, type SignedDepositBase, that implements Convert<usize, BalanceOf<T>>, which is used to calculate the base deposit for signed submissions based on the size of the signed submissions queue.

struct GeometricDepositBase<Balance, Fixed, Inc> implements the convert trait so that the deposit value increases as a geometric progression. The deposit base is calculated by deposit_base = fixed_deposit_base * (1 + increase_factor)^n, where n is the term of the progression (i.e. the number of signed submissions in the queue). Fixed and Inc generic params are getters for Balance and IncreaseFactor to compute the geometric progression. If IncreaseFactor = 0, then the signed deposit is constant and equal to Fixed regardless of the size of the queue.

polkadot companion: paritytech/polkadot#7140

Closes https://github.com/paritytech/srlabs_findings/issues/189

@gpestana gpestana added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C3-medium PR touches the given topic and has a medium impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. T1-runtime This PR/Issue is related to the topic “runtime”. labels Apr 23, 2023
@gpestana gpestana self-assigned this Apr 23, 2023
@gpestana gpestana requested a review from a team April 23, 2023 21:32
@gpestana gpestana marked this pull request as draft April 23, 2023 21:32
@gpestana gpestana mentioned this pull request Apr 27, 2023
Open
@gpestana gpestana marked this pull request as ready for review April 27, 2023 10:29
@gpestana gpestana added D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited C1-low PR touches the given topic and has a low impact on builders. A0-please_review Pull request needs code review. and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. C3-medium PR touches the given topic and has a medium impact on builders. A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 27, 2023
Ank4n
Ank4n reviewed May 1, 2023
View reviewed changes
frame/staking/src/lib.rs Outdated Show resolved Hide resolved
Ank4n
Ank4n reviewed May 1, 2023
View reviewed changes
bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved
@gpestana
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@gpestana gpestana requested a review from kianenigma May 17, 2023 11:12
Ank4n
Ank4n approved these changes May 21, 2023
View reviewed changes
Copy link
Contributor

@Ank4n Ank4n left a comment

Choose a reason for hiding this comment

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

Couple of nits, otherwise LGTM!

@gpestana @Ank4n
Update frame/election-provider-multi-phase/src/signed.rs
99622a5 
Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>
@kianenigma
Copy link
Contributor

Moreover I think this is not the case in the code as it stands now yet:

It adds a new associated type to EPM's config type BaseDeposit that implements Convert<usize, Balance>, which is used to calculate the base deposit for signed submissions based on the size of the signed submissions queue.

kianenigma
kianenigma reviewed Jun 8, 2023
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

I think this is mainly done, but needs to be brought up to date before a final review.

bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved
@kianenigma kianenigma mentioned this pull request Jun 8, 2023
Merged
@gpestana gpestana requested review from a team and kianenigma June 9, 2023 11:42
@gpestana
Copy link
Contributor Author

gpestana commented Jun 9, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Jun 9, 2023 •
edited
Loading

@gpestana https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2965106 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 9-9b73b719-c72e-4e0a-a380-9251ec5eea62 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jun 9, 2023

@gpestana Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2965106 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2965106/artifacts/download.

@gpestana gpestana requested a review from kianenigma June 12, 2023 16:29
@kianenigma
Copy link
Contributor

can be merged once conflict is resolved.

@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Aug 10, 2023
@gpestana gpestana mentioned this pull request Sep 13, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kianenigma kianenigma kianenigma approved these changes

@Ank4n Ank4n Ank4n approved these changes

@rossbulat rossbulat Awaiting requested review from rossbulat

At least 2 approving reviews are required to merge this pull request.

Assignees

@gpestana gpestana

Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
(Nominated) Proof of Stake
Status: ✂️ In progress.
Security Audit (PRs) - SRLabs
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.