paritytech / parity-common

Collection of crates used in Parity projects
https://www.paritytech.io/
Apache License 2.0
290 stars 218 forks source link

bounded_vec: Avoid unnecessary allocation when decoding #713

Closed mrcnski closed 1 year ago

mrcnski commented 1 year ago

Needed for https://github.com/paritytech/polkadot/pull/6603. See https://github.com/paritytech/polkadot/pull/6603#discussion_r1083461147:

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.

mrcnski commented 1 year ago

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.

bkchr commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

ggwpez commented 1 year ago

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 commented 1 year ago

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 :see_no_evil:

mrcnski commented 1 year ago

@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 commented 1 year ago

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