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

Fix pallet bags list and doc #10231

Merged
merged 14 commits into from
Dec 3, 2021
Merged

Fix pallet bags list and doc #10231

merged 14 commits into from
Dec 3, 2021

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Nov 10, 2021 •
edited
Loading

with current sp-io storage API we can't know how much we deleted when calling sp_io::storage::clear_prefix.

This information was not in the doc of remove_all function in frame-support., this is fixed.
This was misused in pallet-bags-list, this is fixed too.

polkadot companion: paritytech/polkadot#4306
skip check-dependent-cumulus

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Nov 10, 2021
@gui1117 gui1117 added 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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Nov 10, 2021
shawntabrizi
shawntabrizi reviewed Nov 10, 2021
View reviewed changes
frame/support/src/storage/types/map.rs
Comment on lines +237 to +238
/// Remove all values of the storage in the overlay and up to `limit` in the backend.
///
Copy link
Member

Choose a reason for hiding this comment

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

Its just a crazy api. This is absolutely not an "expected" behavior, and we shouldn't really expose such weird things in this way...

We need apis like this which are both bounded and behave in expected ways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I agree, IMO we should at least have sp_io::storage::clear_prefix return the exact number of value removed (in both overlay and backend).

shawntabrizi
shawntabrizi approved these changes Nov 10, 2021
View reviewed changes
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

seems like we need this right away, but hopefully this is not the final state of things

@gui1117 gui1117 mentioned this pull request Nov 12, 2021
Merged
bkchr
bkchr reviewed Nov 12, 2021
View reviewed changes
frame/election-provider-support/src/lib.rs Outdated
///
/// This function should never be called in production settings because it can lead to an
/// unbounded amount of storage accesses.
fn clear();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call this unsafe_clear then? To make the user aware of the problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also in pallet-bags-list when implementing the function SortedListProvider::regenerate it calls clear inside. Should we also prepend unsafe to regenerate ? WDYT @kianenigma

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes we should

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in fede886

@xlc
Copy link
Contributor

xlc commented Nov 16, 2021

Um... Does this means remove_all is unsafe to be used by parachains? Do we need to re-audit all the usage of it?

@gui1117
Copy link
Contributor Author

gui1117 commented Nov 17, 2021

Um... Does this means remove_all is unsafe to be used by parachains? Do we need to re-audit all the usage of it?

remove_all for counted storage map doesn't allow to give bounds on the number of value to remove from backend, thus in case you remove too many keys, this can go beyond PoV size.

Also benchmarks should already count the number of storage access in general.

@xlc
Copy link
Contributor

xlc commented Nov 17, 2021

Ok. So as long as we correctly benchmark all the calls using remove_all we should be mostly fine.

This was referenced Nov 17, 2021
Closed
Merged
@gui1117
Copy link
Contributor Author

gui1117 commented Nov 17, 2021

bot merge

@paritytech-processbot
Copy link

Error:
Error: When trying to meet the "Project Owners" approval requirements: this pull request does not belong to a project defined in Process.json.

Approval by "Project Owners" is only attempted if other means defined in the criteria for merge are not satisfied first.

The following errors might have affected the outcome of this attempt:

  • 'Batch: Availability and Validity' does not match any projects in polkadot's Process.json
  • 'Batch: Codebase Restructure' does not match any projects in polkadot's Process.json

Merge failed. Check out the criteria for merge. If you're not meeting the approval count, check if the approvers are members of substrateteamleads or core-devs.

@gui1117
Copy link
Contributor Author

gui1117 commented Dec 1, 2021

companion needs a review

@gui1117 gui1117 mentioned this pull request Dec 2, 2021
Merged
@shawntabrizi
Copy link
Member

reminder on this one

@gui1117
Copy link
Contributor Author

gui1117 commented Dec 3, 2021

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Companion paritytech/polkadot#4306 has error: Checks failed for bfccb973507ebc8d3a28a514e6a8f38073216fdb

@gui1117 gui1117 mentioned this pull request Dec 3, 2021
Open
@gui1117
Copy link
Contributor Author

gui1117 commented Dec 3, 2021

bot merge

@paritytech-processbot paritytech-processbot bot merged commit b6d489e into master Dec 3, 2021
@paritytech-processbot paritytech-processbot bot deleted the gui-fix-bags-list branch December 3, 2021 05:58
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
@gui1117 @KiChjang
@shawntabrizi @grishasobol
Fix pallet bags list and doc (paritytech#10231)
431bb7a 
* fix bags list

* improve doc

* doc

* inner doc

* fix test

* Update docs in frame/election-provider-support/src/lib.rs

* fix staking impl

* prepend unsafe to clear and regenerate

* fix test

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
AurevoirXavier added a commit to darwinia-network/darwinia-common that referenced this pull request Sep 8, 2022
@AurevoirXavier
Companion of paritytech/substrate#10231, paritytech/substrate#10233
ecb22b3
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@gui1117 @KiChjang
@shawntabrizi @ark0f
Fix pallet bags list and doc (paritytech#10231)
2c5577b 
* fix bags list

* improve doc

* doc

* inner doc

* fix test

* Update docs in frame/election-provider-support/src/lib.rs

* fix staking impl

* prepend unsafe to clear and regenerate

* fix test

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KiChjang KiChjang KiChjang left review comments

@bkchr bkchr bkchr left review comments

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@kianenigma kianenigma kianenigma approved these changes

@emostov emostov emostov approved these changes

Assignees
No one assigned
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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gui1117 @xlc @shawntabrizi @KiChjang @kianenigma @bkchr @emostov