Crowdloan max contributors limit to avoid unbounded work#1591
Crowdloan max contributors limit to avoid unbounded work#1591sam0x17 merged 15 commits intodevnet-readyfrom
Conversation
l0r1s
added
the
skip-cargo-audit
pallets/crowdloan/src/lib.rs
Outdated
|
|
||
| /// A map of crowdloan ids to their contributors count. | ||
| #[pallet::storage] | ||
| pub type ContributorsCount<T: Config> = |
There was a problem hiding this comment.
Why did you choose a separate storage map instead of the field within the crowdload structure? We already have the "raised" computational field. The new storage map increases the DB operations number.
There was a problem hiding this comment.
At first I wanted to replace Contributions with CountedStorageMap but I decided to have a simple independent map, but you are are right it would probably make everything easier to put this in the crowdloan directly but in this case we still need a migration or we may have decoding issues.
|
|
||
| use crate::*; | ||
|
|
||
| pub fn migrate_add_contributors_count<T: Config>() -> Weight { |
There was a problem hiding this comment.
Could we get away without the migration since the crowdloan code didn't get to mainnet?
There was a problem hiding this comment.
Probably, there is only 1 crowdloan on the testnet at the moment.
There was a problem hiding this comment.
It's likely dependent on the next release date.
pallets/crowdloan/src/lib.rs
Outdated
| // Clear refunded contributors | ||
| for contributor in refunded_contributors { | ||
| Contributions::<T>::remove(crowdloan_id, &contributor); | ||
| Self::decrement_contributor_count(crowdloan_id); |
There was a problem hiding this comment.
Suboptimal operation: we could insert the final value in the map.
l0r1s
requested review from
JohnReedV,
ales-otf,
gztensor,
open-junius,
sam0x17 and
shamil-gadelshin
3 participants
Description
Added a contributors limit (500) to avoid unbounded iterations when distributing dividends (being implemented for subnet leasing) and also keep track of the contributors count for each crowdloan to enforce this rule and make it easier to benchmark.
A migration to migrate existing crowdloan count (which is only on devnet atm).
Related Issue(s)
Type of Change
Breaking Change
If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.
Checklist
cargo fmtandcargo clippyto ensure my code is formatted and linted correctlyScreenshots (if applicable)
Please include any relevant screenshots or GIFs that demonstrate the changes made.
Additional Notes
Please provide any additional information or context that may be helpful for reviewers.