This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Fix clear prefix check to avoid erasing child trie roots. #7848
Conversation
This file contains 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
cheme
added
A0-please_review
cheme
added
A3-in_progress
|
Will do 👍 |
cheme
added
A0-please_review
bkchr
approved these changes
Jan 12, 2021
| @@ -1022,6 +1023,24 @@ mod tests { | |||
| ext.child_storage_hash(child_info, &[30]), | |||
| Some(Blake2Hasher::hash(&[31]).as_ref().to_vec()), | |||
| ); | |||
|
|
|||
| // check clear prefix is ignored on conflict | |||
| use sp_core::storage::well_known_keys; | |||
There was a problem hiding this comment.
I would prefer to make this an independent test.
primitives/storage/src/lib.rs
Outdated
Comment on lines
184
to
185
| /// Whether a prefix potentially contains child storage keys. | ||
| pub fn can_contain_child_storage_key(prefix: &[u8]) -> bool { |
There was a problem hiding this comment.
Suggested change
| /// Whether a prefix potentially contains child storage keys. | |
| pub fn can_contain_child_storage_key(prefix: &[u8]) -> bool { | |
| /// Returns if the given `key` starts with [`CHILD_STORAGE_KEY_PREFIX`] or collides with it. | |
| pub fn starts_with_child_storage_key(key: &[u8]) -> bool { |
gui1117
approved these changes
Jan 12, 2021
|
I would not bother to skip it, accessing root read only seems fine to me. |
|
Yes I agree |
|
bot merge |
|
Trying merge. |
ghost
merged commit
drahnr
pushed a commit
that referenced
this pull request
Jan 13, 2021
* Fix clear prefix check to avoid erasing child trie roots. * Renaming and extend existing test with check. * last nitpicks.
drahnr
added a commit
that referenced
this pull request
Jan 13, 2021
* make helper error types generics * avoid From<io::Error> dep in runner helper logic * slip of the pen, bump futures to 0.3.9 * more generics * generic var spaces Co-authored-by: Andronik Ordian <[email protected]> * network-gossip: add metric for number of local messages (#7871) * network-gossip: add metric for number of local messages * grandpa: fix GossipEngine missing metrics registry parameter * network-gossip: increase known messages cache size * network-gossip: fix tests * grandpa: remove unnecessary clone Co-authored-by: Max Inden <[email protected]> * network-gossip: count registered and expired messages separately * network-gossip: add comment on known messages cache size * network-gossip: extend comment with cache size in memory Co-authored-by: Max Inden <[email protected]> * Clean-up pass in network/src/protocol.rs (#7889) * Remove statistics system * Remove ContextData struct * Remove next_request_id * Some TryFrom nit-picking * Use constants for peer sets * contracts: Don't read the previous value when overwriting a storage item (#7879) * Add `len` function that can return the length of a storage item efficiently * Make use of the new len function in contracts * Fix benchmarks * cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs * Remove unused imports Co-authored-by: Parity Benchmarking Bot <[email protected]> * Fix clear prefix check to avoid erasing child trie roots. (#7848) * Fix clear prefix check to avoid erasing child trie roots. * Renaming and extend existing test with check. * last nitpicks. * use follow paths to std standarad components * line width Co-authored-by: Bernhard Schuster <[email protected]> Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: André Silva <[email protected]> Co-authored-by: Max Inden <[email protected]> Co-authored-by: Pierre Krieger <[email protected]> Co-authored-by: Alexander Theißen <[email protected]> Co-authored-by: Parity Benchmarking Bot <[email protected]> Co-authored-by: cheme <[email protected]>
This pull request was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Checks on child storage root for clear prefix is incorrect, this changes it.
An alternative implementation would be to allow such clear prefix and skip the child storage root prefix from being deleted.
This is possibly breaking consensus but I know of no pallet that can delete a prefix containing child storage root, so from my point of view it should be fine to use.