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

removed without_storage_info from pallet-collective #14585

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

Conversation

muraca
Copy link
Contributor

@muraca muraca commented Jul 16, 2023

Closes paritytech/polkadot-sdk#167

  • Proposals was removed
  • ProposalOf is now a CountedStorageMap to handle the MaxProposals bound
  • ProposalOf now stores an encoded Proposal in a BoundedVec that contains at most 4 megabytes
  • Voting is now bounded
  • Members is now bounded

Cumulus companion: cumulus#2884 here

Polkadot address: 12poSUQPtcF1HUPQGY3zZu2P8emuW9YnsPduA4XG3oCEfJVp

Signed-off-by: muraca <mmuraca247@gmail.com>
@muraca
Copy link
Contributor Author

muraca commented Jul 16, 2023

@ggwpez could you please run benchmarks?

@juangirini
Copy link
Contributor

bot bench $ pallet dev pallet_collective

@command-bot
Copy link

command-bot bot commented Jul 17, 2023

@juangirini https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3198277 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_collective. 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 3-a09aeb1d-329f-4044-8110-364b21872a93 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jul 17, 2023

@juangirini Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_collective has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3198277 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3198277/artifacts/download.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3203848

Signed-off-by: muraca <mmuraca247@gmail.com>
pub type ProposalOf<T: Config<I>, I: 'static = ()> =
StorageMap<_, Identity, T::Hash, <T as Config<I>>::Proposal, OptionQuery>;
CountedStorageMap<_, Identity, T::Hash, BoundedVec<u8, ConstU32<MAX_SIZE>>, OptionQuery>;
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if this was a missunderstanding. We still need to use the preimage pallet, otherwise it is a re-implementation of it #12070. This is not really an unbound lookup, but it wastes massive PoV since the benchmarking will always assume the worst-case of 4 MiB.
Anyway I think you deserve a tip for this already, even if it does not merge 🙈 @juangirini

I think this map should be ProposalHash -> Bounded<Proposal>. Does it make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

@muraca if you want to be tipped please add your Kusaman or Polkadot address in the PR comment with this format
"[Polkadot | Kusama] address: xxxxx"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the benchmarking will always assume the worst-case of 4 MiB.

@ggwpez I took a look at the benchmarks and weight functions before implementing this, and I saw that weights depend on the length bound passed as input parameter in the extrinsics.
If you are positive that this wouldn't work with the PoV size, or that even if it's bounded we don't like to re-implement preimages, a thumbs up is enough for me to proceed and re-add preimages.

I really really dislike the ProposalHash -> Bounded<Proposal> approach, I feel it just wastes storage space. Alternatively, I could use the note method to store the preimage and generate a hash to put in a Bounded::Lookup variant when I need to interact with preimages to request or delete from storage.

@paritytech-ci paritytech-ci requested a review from a team July 17, 2023 22:44
@juangirini
Copy link
Contributor

/tip small

@substrate-tip-bot
Copy link

@juangirini A small (20 DOT) tip was successfully submitted for @muraca (12poSUQPtcF1HUPQGY3zZu2P8emuW9YnsPduA4XG3oCEfJVp on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/referenda tip

@stale
Copy link

stale bot commented Aug 18, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added A3-stale and removed A3-stale labels Aug 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collective pallet - remove without_storage_info
4 participants