bounded_vec: Avoid unnecessary allocation when decoding by mrcnski · Pull Request #713 · paritytech/parity-common

mrcnski · 2023-01-26T11:42:14Z

Needed for paritytech/polkadot#6603. 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.

bounded-collections/src/bounded_vec.rs

bkchr

% what @ggwpez said.

KiChjang

Also probably requires a version bump for substrate to pick up the changes.

mrcnski · 2023-01-27T09:02:12Z

Also probably requires a version bump for substrate to pick up the changes.

Should I do that in this PR?

Also, I'll try to get to BoundedBTreeMap and BoundedBTreeSet soon, haven't had a chance yet.

ggwpez

Should I do that in this PR?

You can also do it in another PR, just thought it would fit in here 😄

bkchr · 2023-01-27T21:49:13Z

As the crate is currently lacking test, I would be happy if this pr could start with this ;) We could ensure that we don't consume more data than the compact length.

mrcnski · 2023-01-28T17:50:46Z

@bkchr There are tests, including for that case -- see too_big_vec_fail_to_decode and too_big_fail_to_decode. Or do you mean we should check whether unnecessary memory is consumed (allocated)? I wasn't sure how to do that, or if such a test was necessary.

mrcnski · 2023-01-28T17:56:04Z

This PR is already approved, but would like a quick re-approval from someone (to sanity check the last two commits). :)

ggwpez · 2023-01-28T19:21:45Z

I wasn't sure how to do that, or if such a test was necessary.

Dump idea: Just use u32::MAX as Bound and 1MiB as value, then try to decode that. Should fail to allocate if the implementation is wrong.

bkchr · 2023-01-28T19:27:18Z

I wasn't sure how to do that, or if such a test was necessary.

let data = something.encode();
let data_input = &mut &data[..];

Something::decode(data_input).unwrap_err();
assert_eq!(data_input.len(), data.len() - Compact::<u32>(data.len() as u32).compact_len());

This should do it. Also some easy tests ensuring that Vec etc encodings are the same as the bounded impls.

There are tests, including for that case -- see too_big_vec_fail_to_decode and too_big_fail_to_decode

Sorry 🙈

ggwpez · 2023-01-28T19:32:43Z

bounded-collections/src/bounded_btree_map.rs

 			return Err("BoundedBTreeMap exceeds its limit".into())
 		}
+		input.descend_ref()?;
+		let inner = Result::from_iter((0..len).map(|_| Decode::decode(input)))?;


No idea if rust handles this in the most efficient way, otherwise we should maybe pre-allocate as optimization.

I mean this is now discusable with the bound, but for the normal BTreeMap we don't have done this as someone could send you a fake encoded BTreeMap that pretends to be u32::Max in size and then maybe leading to some OOM.

Interesting, would have to compile with optimizations and see. At any rate, I don't want to deviate too much from the standard Decode for the unbounded types, at least in this PR.

Should be fine to merge as is.

Is this just another way of calling collect?

Suggested change

let inner = Result::from_iter((0..len).map(|_| Decode::decode(input)))?;

let inner = (0..len).map(|_| Decode::decode(input)).collect::<Result<Vec<_>, codec::Error>>()?;

Yes, I think it is. http://doc.rust-lang.org/1.66.1/std/result/enum.Result.html#method.from_iter

Currently this impl is the same as the unbounded version, apart from the length check. I think it should be kept the same unless there's a good reason not to.

bounded-collections/src/bounded_btree_set.rs

bkchr

Ty! But let us use the same derives everywhere.

bkchr · 2023-01-29T21:47:58Z

bounded-collections/src/bounded_btree_map.rs

 			return Err("BoundedBTreeMap exceeds its limit".into())
 		}
+		input.descend_ref()?;
+		let inner = Result::from_iter((0..len).map(|_| Decode::decode(input)))?;


Should be fine to merge as is.

bkchr · 2023-01-29T21:49:12Z

bounded-collections/src/bounded_btree_map.rs

 ///
 /// Unlike a standard `BTreeMap`, there is an enforced upper limit to the number of items in the
 /// map. All internal operations ensure this bound is respected.
+#[cfg_attr(feature = "std", derive(Hash))]


Suggested change

#[cfg_attr(feature = "std", derive(Hash))]

#[cfg_attr(feature = "std", derive(Hash, serde::Serialize, serde::Deserialize), serde(transparent))]

I'm not sure about deriving these; Deserialize would need a custom impl to add bounds checks, like we do now with BoundedVec. And the custom impl looks annoying to implement, so I'm thinking we don't add it until there's actually a need for it.

@bkchr I'm going to go ahead and merge -- these derives can be a follow-up if we really want them.

bkchr · 2023-01-29T21:49:22Z

bounded-collections/src/bounded_btree_set.rs

 ///
 /// Unlike a standard `BTreeSet`, there is an enforced upper limit to the number of items in the
 /// set. All internal operations ensure this bound is respected.
+#[cfg_attr(feature = "std", derive(Hash))]


Suggested change

#[cfg_attr(feature = "std", derive(Hash))]

#[cfg_attr(feature = "std", derive(Hash, serde::Serialize, serde::Deserialize), serde(transparent))]

bkchr · 2023-01-29T21:49:45Z

bounded-collections/src/bounded_vec.rs

 /// As the name suggests, the length of the queue is always bounded. All internal operations ensure
 /// this bound is respected.
-#[cfg_attr(feature = "std", derive(Serialize), serde(transparent))]
+#[cfg_attr(feature = "std", derive(Hash, Serialize), serde(transparent))]


Suggested change

#[cfg_attr(feature = "std", derive(Hash, Serialize), serde(transparent))]

#[cfg_attr(feature = "std", derive(Hash, serde::Serialize, serde::Deserialize), serde(transparent))]

mrcnski · 2023-02-01T08:12:17Z

@KiChjang Looks like I'm not able to publish 0.1.4, you're the only listed owner. Possible to add paritytech/Core devs as an owner?

KiChjang · 2023-02-02T00:56:26Z

I've added the core-devs team as the owner for the crate on crates.io.

bounded_vec: Avoid unnecessary allocation when decoding

75ec16c

mrcnski added the A0-pleasereview label Jan 26, 2023

mrcnski requested review from KiChjang and bkchr January 26, 2023 11:42

ggwpez reviewed Jan 26, 2023

View reviewed changes

bounded-collections/src/bounded_vec.rs Show resolved Hide resolved

bkchr approved these changes Jan 26, 2023

View reviewed changes

KiChjang approved these changes Jan 27, 2023

View reviewed changes

ggwpez approved these changes Jan 27, 2023

View reviewed changes

Merge branch 'master' into mrcnski/boundedvec-decode-allocation

a0b7f5d

mrcnski added 3 commits January 28, 2023 18:52

Fix unnecessary alloc for other types; add Hash derive

87e5556

bounded-collections: Update version to 0.1.4

7a02d51

Cargo fmt

e89f7bc

ggwpez reviewed Jan 28, 2023

View reviewed changes

ggwpez approved these changes Jan 28, 2023

View reviewed changes

bounded-collections/src/bounded_btree_set.rs Show resolved Hide resolved

bounded-collections: add some missing encoding tests

72dec2a

bkchr approved these changes Jan 29, 2023

View reviewed changes

mrcnski merged commit 07127b6 into master Feb 1, 2023

mrcnski deleted the mrcnski/boundedvec-decode-allocation branch February 1, 2023 08:07

	let inner = Result::from_iter((0..len).map(\|_\| Decode::decode(input)))?;
	let inner = (0..len).map(\|_\| Decode::decode(input)).collect::<Result<Vec<_>, codec::Error>>()?;

	#[cfg_attr(feature = "std", derive(Hash))]
	#[cfg_attr(feature = "std", derive(Hash, serde::Serialize, serde::Deserialize), serde(transparent))]

Conversation

mrcnski commented Jan 26, 2023

Uh oh!

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

KiChjang left a comment

Choose a reason for hiding this comment

Uh oh!

mrcnski commented Jan 27, 2023

Uh oh!

ggwpez left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr commented Jan 27, 2023

Uh oh!

mrcnski commented Jan 28, 2023

Uh oh!

mrcnski commented Jan 28, 2023

Uh oh!

ggwpez commented Jan 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkchr commented Jan 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrcnski commented Feb 1, 2023

Uh oh!

KiChjang commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ggwpez commented Jan 28, 2023 •

edited

Loading

KiChjang commented Feb 2, 2023 •

edited

Loading