backing-availability-audit: Move ErasureChunk Proof to BoundedVec #3626

Lldenaurois · 2021-08-11T21:57:30Z

Addresses: https://github.com/paritytech/srlabs_findings/issues/100

This PR moved the proof field in ErasureChunk to a nested set of BoundedVecs.

Lldenaurois · 2021-08-12T02:03:18Z

node/core/av-store/src/lib.rs

@@ -1177,7 +1178,7 @@ fn store_available_data(
 	let erasure_chunks = chunks.iter().zip(branches.map(|(proof, _)| proof)).enumerate().map(
 		|(index, (chunk, proof))| ErasureChunk {
 			chunk: chunk.clone(),
-			proof,
+			proof: Proof::try_from(proof).unwrap(),


Note: I am not sure what the best approach is here.

Since the obtain_chunks function (c.f. here) and branches function (c.f. here returns an AsRef<[u8]> and Vec<Vec> respectively, it may be prudent to modify those functions to return the actual Proof object. However, this is a failable conversion and would need proper error-handling, e.g. modifying the .map above to .filter_map

One option would be to have the erasure-coding return the Proof as oppose to the AsRef<[u8]> and the Vec<Vec<u8>>. But before I go ahead and do that it maybe be best to sync up regarding the proper approach.

Lldenaurois · 2021-08-12T02:05:50Z

node/core/av-store/src/tests.rs

@@ -16,11 +16,14 @@

 use super::*;

+use std::convert::TryFrom;


Since there's no use of this trait anywhere (it seems), I believe it may be incorrect to implement the TryFrom trait as oppose to just making a Method on Proof.

How would it be incorrect?

Lldenaurois · 2021-08-12T02:14:33Z

node/primitives/src/lib.rs

+		let mut out = Vec::new();
+		for element in self.0.iter() {
+			let temp = element.as_vec();
+			let mut element_vec = [0u8; MERKLE_NODE_MAX_SIZE];


This is not the tersest way to store Merkle proofs, but it works. I wanted to have something working quickly in order to get some of the higher-level questions out of the way.

Strangely, the Merkle proofs generated in the polkadot tests seem to always an outer length 2, which I currently don't fully understand. The inner length varies from 3 to 347 and I also don't fully understand why.

Based on how we store these proofs as a Vec<Vec>, we can choose the best way to bound the inner and out vectors.

Lldenaurois · 2021-08-12T02:22:58Z

node/primitives/src/lib.rs

@@ -51,6 +52,9 @@ pub use disputes::{
 	SignedDisputeStatement, UncheckedDisputeMessage, ValidDisputeVote,
 };

+const MERKLE_NODE_MAX_SIZE: usize = 347;
+const MERKLE_PROOF_MAX_DEPTH: usize = 3;


Note that these constants would not work in practice. I've just set them to what I observed being crated in the tests. It is my belief that the depth is no more than 3 (in fact it's not more than 2) throughout all tests because we don't ever create a Merkle Tree with that many elements, i.e. intermediate nodes.

The correct values to use here are:

MERKLE_NODE_MAX_SIZE = 512;
MERKLE_PROOF_MAX_DEPTH = 8;

I just found it interesting that the node max size was uneven. I may be mistaken, but I'm quite certain the max proof node created is 347 bytes.

eskimor · 2021-08-12T09:07:42Z

With the limits in place, how big can a proof become now?

Lldenaurois · 2021-08-12T20:57:02Z

4096 (= 512 * 8) bytes is the maximum size of the Proof with this change, as oppose to the 500KB protocol-level limit.

drahnr

I would prefer the error handling to not be static strings, with the exception of CodecError unless there is a particular requirement to do so.

Besides that, 👍

drahnr · 2021-08-19T07:44:04Z

node/primitives/src/lib.rs

+const MERKLE_NODE_MAX_SIZE: usize = 512;
+const MERKLE_PROOF_MAX_DEPTH: usize = 8;


Adding comments as to why these were chosen as they are would be appreciated.

drahnr · 2021-08-19T07:47:23Z

node/primitives/src/lib.rs

+}
+
+impl TryFrom<Vec<Vec<u8>>> for Proof {
+	type Error = &'static str;


I would argue for a

#[derive(thiserror::Error)] enum Error { #[error("erkle max proof depth exceeded {0} > {} .", MERKLE_PROOF_MAX_DEPTH)] MerkleProofDepthExceeded(depth), // .. }

drahnr · 2021-08-19T07:47:41Z

node/primitives/src/lib.rs

+		let mut out = Vec::new();
+		for element in input.into_iter() {
+			let data: BoundedVec<u8, 1, MERKLE_NODE_MAX_SIZE> =
+				BoundedVec::from_vec(element).map_err(|_| "Merkle node max size exceeded.")?;


drahnr · 2021-08-19T07:48:52Z

node/primitives/src/lib.rs

+				BoundedVec::from_vec(element).map_err(|_| "Merkle node max size exceeded.")?;
+			out.push(data);
+		}
+		Ok(Proof(BoundedVec::from_vec(out).expect("Buffer size is deterined above. QED")))


Suggested change

Ok(Proof(BoundedVec::from_vec(out).expect("Buffer size is deterined above. QED")))

Ok(Proof(BoundedVec::from_vec(out).expect("Buffer size is already checked; qed")))

drahnr · 2021-08-19T07:52:15Z

node/primitives/src/lib.rs

+
+impl Proof {
+	/// This function allows to convert back to the standard nested Vec format
+	pub fn as_vec(&self) -> Vec<Vec<u8>> {


nit/future: Do we truely require a Vec<Vec<_>>? Or could we avoid all those allocations by employing a single BoundedVec<_> and providing a Vec<&[]> or even &[&[]]? Again, not for this PR, a potential future optimization.

node/primitives/src/lib.rs

…_bounded

eskimor · 2021-08-23T08:22:31Z

erasure-coding/src/lib.rs

 				self.current_pos += 1;
-				Some((nodes, chunk.as_ref()))
+				Proof::try_from(nodes).ok().map(|proof| (proof, chunk.as_ref()))


Not sure it is the right behavior to just let the iterator end if the Proof could not be constructed. Can this even happen, without the code having a logic error?

eskimor · 2021-08-23T08:31:36Z

node/network/availability-distribution/src/requester/fetch_task/mod.rs

@@ -363,7 +363,7 @@ impl RunningTask {

 	fn validate_chunk(&self, validator: &AuthorityDiscoveryId, chunk: &ErasureChunk) -> bool {
 		let anticipated_hash =
-			match branch_hash(&self.erasure_root, &chunk.proof, chunk.index.0 as usize) {
+			match branch_hash(&self.erasure_root, &chunk.proof_as_vec(), chunk.index.0 as usize) {


Can't we fix branch_hash in order to get rid of that conversion?

eskimor · 2021-08-23T08:34:31Z

node/primitives/src/lib.rs

+	}
+}
+
+impl Decode for Proof {


I am missing tests, to make sure Encode/Decode works and is compatible to what we had before.

…_bounded

…dVec (#3626)" This reverts commit d4576bc.

* master: backing-availability-audit: Move ErasureChunk Proof to BoundedVec (#3626) Substrate Companion #9575 (#3695) Fill up requests slots via `launch_parallel_requests` (#3681) Bump serde_json from 1.0.64 to 1.0.66 (#3669) substrate #9202 companion: Multiple vesting schedules (#3407) XCM: Introduce versioning to dispatchables' params (#3693) remove dead_code from chain selection test (#3685) Improve MultiLocation conversion functions in xcm-procedural (#3690)

Lldenaurois force-pushed the erasure_chunk_proof_bounded branch 5 times, most recently from e660af4 to 2541526 Compare August 12, 2021 02:19

Lldenaurois commented Aug 12, 2021

View reviewed changes

backing-availability-audit: Move ErasureChunk Proof to BoundedVec

576c48f

Lldenaurois force-pushed the erasure_chunk_proof_bounded branch from 2541526 to 576c48f Compare August 12, 2021 06:19

Lldenaurois added 4 commits August 17, 2021 21:58

WIP

0edfca9

Merge remote-tracking branch 'origin/master' into temp

960ecf1

Touch up

e35d65e

Fix spelling mistake

bc7a996

Lldenaurois marked this pull request as ready for review August 18, 2021 16:04

drahnr suggested changes Aug 19, 2021

View reviewed changes

Address Feedback

7b7bd4b

Lldenaurois added the C1-low PR touches the given topic and has a low impact on builders. label Aug 20, 2021

drahnr approved these changes Aug 20, 2021

View reviewed changes

node/primitives/src/lib.rs Show resolved Hide resolved

node/primitives/src/lib.rs Show resolved Hide resolved

node/primitives/src/lib.rs Show resolved Hide resolved

node/primitives/src/lib.rs Show resolved Hide resolved

Lldenaurois force-pushed the erasure_chunk_proof_bounded branch from 5eb17d3 to bf72823 Compare August 20, 2021 21:23

Merge remote-tracking branch 'origin/master' into erasure_chunk_proof…

f442c43

…_bounded

Lldenaurois force-pushed the erasure_chunk_proof_bounded branch from bf72823 to f442c43 Compare August 22, 2021 20:53

eskimor reviewed Aug 23, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into erasure_chunk_proof…

5a4ad8e

…_bounded

Lldenaurois force-pushed the erasure_chunk_proof_bounded branch from 8ed41b6 to 5a4ad8e Compare August 24, 2021 15:52

Lldenaurois merged commit d4576bc into paritytech:master Aug 24, 2021

Lldenaurois added a commit that referenced this pull request Aug 24, 2021

Revert "backing-availability-audit: Move ErasureChunk Proof to Bounde…

7a5275c

…dVec (#3626)" This reverts commit d4576bc.

Lldenaurois mentioned this pull request Aug 24, 2021

Add tests and modify as_vec implementation #3715

Merged

ordian mentioned this pull request Aug 25, 2021

allow some overhead in MERKLE_NODE_MAX_SIZE #3724

Merged

stze added D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Sep 28, 2021

github-actions bot mentioned this pull request Oct 11, 2021

Update substrate/polkadot/cumulus from v0.9.10 to v0.9.11 moonbeam-foundation/moonbeam#892

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backing-availability-audit: Move ErasureChunk Proof to BoundedVec #3626

backing-availability-audit: Move ErasureChunk Proof to BoundedVec #3626

Lldenaurois commented Aug 11, 2021

Lldenaurois Aug 12, 2021 •

edited

Loading

Lldenaurois Aug 12, 2021

drahnr Aug 19, 2021

Lldenaurois Aug 12, 2021

Lldenaurois Aug 12, 2021

eskimor commented Aug 12, 2021

Lldenaurois commented Aug 12, 2021

drahnr left a comment

drahnr Aug 19, 2021

drahnr Aug 19, 2021

drahnr Aug 19, 2021

drahnr Aug 19, 2021 •

edited

Loading

drahnr Aug 19, 2021

eskimor Aug 23, 2021

eskimor Aug 23, 2021

eskimor Aug 23, 2021

		const MERKLE_NODE_MAX_SIZE: usize = 512;
		const MERKLE_PROOF_MAX_DEPTH: usize = 8;

	Ok(Proof(BoundedVec::from_vec(out).expect("Buffer size is deterined above. QED")))
	Ok(Proof(BoundedVec::from_vec(out).expect("Buffer size is already checked; qed")))

backing-availability-audit: Move ErasureChunk Proof to BoundedVec #3626

backing-availability-audit: Move ErasureChunk Proof to BoundedVec #3626

Conversation

Lldenaurois commented Aug 11, 2021

Lldenaurois Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eskimor commented Aug 12, 2021

Lldenaurois commented Aug 12, 2021

drahnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lldenaurois Aug 12, 2021 •

edited

Loading

drahnr Aug 19, 2021 •

edited

Loading