This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Use a BoundedVec in ValidationResult#6603
Merged
paritytech-processbot[bot] merged 10 commits intomasterfrom Feb 16, 2023
Merged
Use a BoundedVec in ValidationResult#6603paritytech-processbot[bot] merged 10 commits intomasterfrom
BoundedVec in ValidationResult#6603paritytech-processbot[bot] merged 10 commits intomasterfrom
Conversation
> Use a `BoundedVec` for `upward_messages` and `horizontal_messages` in order to > limit the number of individual messages/memory allocations right at decoding > time. The reason for this is that the `ValidationResult` may contain a code > upgrade (including a full PVF binary), so the total size limit can't be set > too low and this limit will still allow several millions of upward messages, > which will (due to the memory allocator overhead) already have a > non-negligible memory footprint in decoded form.
mrcnski
added
A0-please_review
bkchr
suggested changes
Jan 22, 2023
mrcnski
added a commit
to paritytech/parity-scale-codec
that referenced
this pull request
Jan 23, 2023
This is necessary to implement a custom `Decode` for `BoundedVec`. See: paritytech/polkadot#6603 (comment) > You want to ensure that you don't even start try decoding/allocating when the length of the vector is more than the > allowed maximum. > You first decode length of the vector and then early reject if that is too long.
eskimor
reviewed
Jan 25, 2023
ggwpez
reviewed
Jan 25, 2023
mrcnski
added a commit
to paritytech/parity-scale-codec
that referenced
this pull request
Jan 26, 2023
* Export `decode_vec_with_len` This is necessary to implement a custom `Decode` for `BoundedVec`. See: paritytech/polkadot#6603 (comment) > You want to ensure that you don't even start try decoding/allocating when the length of the vector is more than the > allowed maximum. > You first decode length of the vector and then early reject if that is too long. * Actually export `decode_vec_with_len` * Bump version
bkchr
approved these changes
Jan 26, 2023
eskimor
approved these changes
Jan 29, 2023
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
|
bot merge |
|
Error: Statuses failed for adbe1e4 |
mrcnski
added
the
E5-needs_cumulus_pr
Contributor
Author
|
Can someone please advise me on the cumulus companion? I need to convert the What should happen on error? Is it OK to panic here? Or maybe better to handle it before storing it? |
mrcnski
removed
the
E5-needs_cumulus_pr
koute
reviewed
Feb 16, 2023
runtime/parachains/src/builder.rs
Outdated
| commitments: CandidateCommitments::<u32> { | ||
| upward_messages: Vec::new(), | ||
| horizontal_messages: Vec::new(), | ||
| upward_messages: vec![].try_into().unwrap(), |
Contributor
There was a problem hiding this comment.
Any particular reason why you're using try_into().unwrap() here instead of just Default::default() (which would eliminate the unwrap)?
Contributor
Author
|
bot merge |
|
Error: "Check reviews" status is not passing for paritytech/cumulus#2161 |
Contributor
Author
|
bot merge |
paritytech-processbot
bot
deleted the
mrcnski/boundedvec-validationresult
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PULL REQUEST
Overview
Considerations
BoundedVec<UpwardMessage, ConstU32<MAX_UPWARD_MESSAGE_NUM>>shouldn't be exported as its own type, as there is some duplication.Vec->BoundedVeca breaking change anywhere? I hope not, as the encoding is the same: https://github.com/paritytech/substrate/blob/2dff067e9f7f6f3cc4dbfdaaa97753eccc407689/primitives/core/src/bounded/bounded_vec.rs#L323Related Issue
Closes https://github.com/paritytech/srlabs_findings/issues/257
cumulus companion: paritytech/cumulus#2161