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

use CountedMap in pallet-bags-list#10179

Merged
paritytech-processbot[bot] merged 5 commits intomasterfrom
kiz-use-counted-map
Nov 10, 2021
Merged

use CountedMap in pallet-bags-list#10179
paritytech-processbot[bot] merged 5 commits intomasterfrom
kiz-use-counted-map

Conversation

@kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Nov 4, 2021

No description provided.

@kianenigma kianenigma added 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. labels Nov 4, 2021
@kianenigma kianenigma mentioned this pull request Nov 4, 2021
Closed
KiChjang
KiChjang reviewed Nov 5, 2021
View reviewed changes
@kianenigma @KiChjang
Update frame/bags-list/src/list/mod.rs
6f42e81 
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
kianenigma
kianenigma commented Nov 6, 2021
View reviewed changes
frame/support/src/storage/types/counted_map.rs
CounterFor::<Prefix>::set(0u32);
<Self as MapWrapper>::Map::remove_all(None);
pub fn remove_all(maybe_limit: Option<u32>) {
let leftover = Self::count().saturating_sub(maybe_limit.unwrap_or_else(Bounded::max_value));
Copy link
Contributor Author

@kianenigma kianenigma Nov 6, 2021

Choose a reason for hiding this comment

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

this is the only logical change here.

Copy link
Contributor

@gui1117 gui1117 Nov 10, 2021

Choose a reason for hiding this comment

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

this is not correct, the documentation of remove_all for the storage map is indeed missing, but underneath it calls sp-io::storage::clear_prefix
Which deletes all keys in the overlay and up to limit in the backend.

Thus we cannot keep an accurate leftover here, we need to change sp-io::storage::clear_prefix to return here the number of key deleted in the overlay.

Copy link
Contributor

@KiChjang KiChjang Nov 11, 2021

Choose a reason for hiding this comment

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

@kianenigma wat happens nao?

Copy link
Contributor

@gui1117 gui1117 Nov 12, 2021

Choose a reason for hiding this comment

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

follow up is #10231

shawntabrizi
shawntabrizi approved these changes Nov 6, 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.

we need to double check the storage prefix is the same?

@kianenigma
Copy link
Contributor Author

kianenigma commented Nov 10, 2021

we need to double check the storage prefix is the same?

Code points to the prefix being CouterFor, added a test for it as well.

@kianenigma
Copy link
Contributor Author

kianenigma commented Nov 10, 2021

Let's see if this bad boy is still alive:

@kianenigma
Copy link
Contributor Author

kianenigma commented Nov 10, 2021

/try-runtime queue RUST_LOG=runtime=debug --execution Native --no-spec-name-check on-runtime-upgrade live --uri ws://kusama

@command-bot
Copy link

command-bot bot commented Nov 10, 2021 •
edited
Loading

Preparing try-runtime command for branch: "kiz-use-counted-map". Comment will be updated.
Executing cargo run --quiet --features=try-runtime try-runtime --execution Native --no-spec-name-check on-runtime-upgrade live --uri ws://try-runtime-kusama-node-0:9944
The logs for this command should be available on Grafana for the data source loki.parity-chains and query {container=~"try-runtime"}

@command-bot
Copy link

command-bot bot commented Nov 10, 2021

@kianenigma Results are ready for cargo run --quiet --features=try-runtime try-runtime --execution Native --no-spec-name-check on-runtime-upgrade live --uri ws://try-runtime-kusama-node-0:9944

Output 
try-runtime-bot took 6 minutes, 13 seconds, 799 milliseconds (from 2021-11-10T08:49:38.514Z to 2021-11-10T08:55:52.313Z server time) for cargo run --quiet --features=try-runtime try-runtime --execution Native --no-spec-name-check on-runtime-upgrade live --uri ws://try-runtime-kusama-node-0:9944
            Error: LLVM ERROR: IO failure on output stream: No space left on device
error: could not compile `impl-trait-for-tuples`
LLVM ERROR: IO failure on output stream: No space left on device
error: build failed

@kianenigma
Copy link
Contributor Author

kianenigma commented Nov 10, 2021

anyways, locall I get:

2021-11-10 09:46:07.016  INFO main runtime::bags_list: [10031084] 👜checked bags-list prefix to be correct and have 6753 nodes

@kianenigma
Copy link
Contributor Author

kianenigma commented Nov 10, 2021

bot merge

@paritytech-processbot
Copy link

paritytech-processbot bot commented Nov 10, 2021

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 6ec26df into master Nov 10, 2021
@paritytech-processbot paritytech-processbot bot deleted the kiz-use-counted-map branch November 10, 2021 09:33
gui1117
gui1117 reviewed Nov 10, 2021
View reviewed changes
frame/bags-list/src/list/mod.rs
let pre = crate::ListNodes::<T>::count();
crate::ListNodes::<T>::remove_all(maybe_count);
if let Some(count) = maybe_count {
crate::CounterForListNodes::<T>::mutate(|items| *items - count);
Copy link
Contributor

@gui1117 gui1117 Nov 10, 2021

Choose a reason for hiding this comment

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

this previous code also doesn't seems correct, it is not ensure that count items are removed.

ordian added a commit that referenced this pull request Nov 12, 2021
@ordian
Merge branch 'master' into 3x8_update_nightly_fix_fmt
5cf1cc8 
* master: (27 commits)
  Bump rustversion from 1.0.4 to 1.0.5 (#10243)
  Kill the light client, CHTs and change tries. (#10080)
  tuple to struct event variants (#10206)
  Bump thiserror from 1.0.26 to 1.0.30 (#10240)
  Warn about usage of pallet collective set members call. (#10156)
  Bump git2 from 0.13.22 to 0.13.23 (#10238)
  Add group name in task metrics  (#10196)
  Bump proc-macro-crate from 1.0.0 to 1.1.0 (#10237)
  Bump parity-util-mem from 0.10.0 to 0.10.2 (#10236)
  Bump substrate-bip39 from 0.4.2 to 0.4.4 (#10213)
  Upgrade jsonrpsee to v0.4.1 (#10022)
  expose substrate-cli service (#10229)
  Intend to reactivate cargo-unleash check (#10167)
  CI: build docs with deps (#9884)
  use CountedMap in pallet-bags-list (#10179)
  Move all example pallets under `examples` folder. (#10215)
  Upgrade wasm builder (#10226)
  upgrade ss58-registry with additional networks. (#10224)
  move wiki -> docs (#10225)
  new remote-ext mode: (#10192)
  ...
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
@kianenigma @KiChjang @grishasobol
use CountedMap in pallet-bags-list (paritytech#10179)
9cc77c4 
* use CountedMap in pallet-bags-list

* Fix build

* Update frame/bags-list/src/list/mod.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* add a check as well

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@kianenigma @KiChjang @ark0f
use CountedMap in pallet-bags-list (paritytech#10179)
90bffea 
* use CountedMap in pallet-bags-list

* Fix build

* Update frame/bags-list/src/list/mod.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* add a check as well

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@kianenigma @KiChjang @ark0f
use CountedMap in pallet-bags-list (paritytech#10179)
90bffea 
* use CountedMap in pallet-bags-list

* Fix build

* Update frame/bags-list/src/list/mod.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* add a check as well

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

Reviewers

3 more reviewers

@KiChjang KiChjang KiChjang approved these changes

@gui1117 gui1117 gui1117 left review comments

@shawntabrizi shawntabrizi shawntabrizi approved these changes

Reviewers whose approvals may not affect merge requirements

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.

4 participants

@kianenigma @shawntabrizi @KiChjang @gui1117