Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 10 additions & 6 deletions erasure-coding/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@
//! f is the maximum number of faulty validators in the system.
//! The data is coded so any f+1 chunks can be used to reconstruct the full data.

use std::convert::TryFrom;

use parity_scale_codec::{Decode, Encode};
use polkadot_node_primitives::AvailableData;
use polkadot_node_primitives::{AvailableData, Proof};
use polkadot_primitives::v0::{self, BlakeTwo256, Hash as H256, HashT};
use sp_core::Blake2Hasher;
use thiserror::Error;
Expand Down Expand Up @@ -245,7 +247,7 @@ impl<'a, I: AsRef<[u8]>> Branches<'a, I> {
}

impl<'a, I: AsRef<[u8]>> Iterator for Branches<'a, I> {
type Item = (Vec<Vec<u8>>, &'a [u8]);
type Item = (Proof, &'a [u8]);

fn next(&mut self) -> Option<Self::Item> {
use trie::Recorder;
Expand All @@ -258,13 +260,12 @@ impl<'a, I: AsRef<[u8]>> Iterator for Branches<'a, I> {

match res.expect("all nodes in trie present; qed") {
Some(_) => {
let nodes = recorder.drain().into_iter().map(|r| r.data).collect();
let nodes: Vec<Vec<u8>> = recorder.drain().into_iter().map(|r| r.data).collect();
let chunk = self.chunks.get(self.current_pos).expect(
"there is a one-to-one mapping of chunks to valid merkle branches; qed",
);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

},
None => None,
}
Expand Down Expand Up @@ -421,7 +422,10 @@ mod tests {
assert_eq!(proofs.len(), 10);

for (i, proof) in proofs.into_iter().enumerate() {
assert_eq!(branch_hash(&root, &proof, i).unwrap(), BlakeTwo256::hash(&chunks[i]));
assert_eq!(
branch_hash(&root, &proof.as_vec(), i).unwrap(),
BlakeTwo256::hash(&chunks[i])
);
}
}
}
17 changes: 11 additions & 6 deletions node/core/av-store/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@

use super::*;

use std::convert::TryFrom;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would it be incorrect?


use assert_matches::assert_matches;
use futures::{channel::oneshot, executor, future, Future};

use parking_lot::Mutex;
use polkadot_node_primitives::{AvailableData, BlockData, PoV};
use polkadot_node_primitives::{AvailableData, BlockData, PoV, Proof};
use polkadot_node_subsystem_test_helpers as test_helpers;
use polkadot_node_subsystem_util::TimeoutExt;
use polkadot_primitives::v1::{
Expand Down Expand Up @@ -287,7 +289,7 @@ fn store_chunk_works() {
let chunk = ErasureChunk {
chunk: vec![1, 2, 3],
index: validator_index,
proof: vec![vec![3, 4, 5]],
proof: Proof::try_from(vec![vec![3, 4, 5]]).unwrap(),
};

// Ensure an entry already exists. In reality this would come from watching
Expand Down Expand Up @@ -333,7 +335,7 @@ fn store_chunk_does_nothing_if_no_entry_already() {
let chunk = ErasureChunk {
chunk: vec![1, 2, 3],
index: validator_index,
proof: vec![vec![3, 4, 5]],
proof: Proof::try_from(vec![vec![3, 4, 5]]).unwrap(),
};

let (tx, rx) = oneshot::channel();
Expand Down Expand Up @@ -441,8 +443,11 @@ fn store_block_works() {
let mut branches = erasure::branches(chunks.as_ref());

let branch = branches.nth(5).unwrap();
let expected_chunk =
ErasureChunk { chunk: branch.1.to_vec(), index: ValidatorIndex(5), proof: branch.0 };
let expected_chunk = ErasureChunk {
chunk: branch.1.to_vec(),
index: ValidatorIndex(5),
proof: Proof::try_from(branch.0).unwrap(),
};

assert_eq!(chunk, expected_chunk);
virtual_overseer
Expand Down Expand Up @@ -545,7 +550,7 @@ fn query_all_chunks_works() {
let chunk = ErasureChunk {
chunk: vec![1, 2, 3],
index: ValidatorIndex(1),
proof: vec![vec![3, 4, 5]],
proof: Proof::try_from(vec![vec![3, 4, 5]]).unwrap(),
};

let (tx, rx) = oneshot::channel();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ok(hash) => hash,
Err(e) => {
tracing::warn!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

use std::collections::HashMap;
use std::{collections::HashMap, convert::TryFrom};

use parity_scale_codec::Encode;

Expand All @@ -29,7 +29,7 @@ use sc_network as network;
use sp_keyring::Sr25519Keyring;

use polkadot_node_network_protocol::request_response::{v1, Recipient};
use polkadot_node_primitives::{BlockData, PoV};
use polkadot_node_primitives::{BlockData, PoV, Proof};
use polkadot_primitives::v1::{CandidateHash, ValidatorIndex};

use super::*;
Expand Down Expand Up @@ -60,7 +60,7 @@ fn task_does_not_accept_invalid_chunk() {
Recipient::Authority(Sr25519Keyring::Alice.public().into()),
ChunkFetchingResponse::Chunk(v1::ChunkResponse {
chunk: vec![1, 2, 3],
proof: vec![vec![9, 8, 2], vec![2, 3, 4]],
proof: Proof::try_from(vec![vec![9, 8, 2], vec![2, 3, 4]]).unwrap(),
}),
);
m
Expand Down Expand Up @@ -170,7 +170,7 @@ fn task_stores_valid_chunk_if_there_is_one() {
Recipient::Authority(Sr25519Keyring::Charlie.public().into()),
ChunkFetchingResponse::Chunk(v1::ChunkResponse {
chunk: vec![1, 2, 3],
proof: vec![vec![9, 8, 2], vec![2, 3, 4]],
proof: Proof::try_from(vec![vec![9, 8, 2], vec![2, 3, 4]]).unwrap(),
}),
);

Expand Down
6 changes: 3 additions & 3 deletions node/network/availability-distribution/src/tests/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@

//! Helper functions and tools to generate mock data useful for testing this subsystem.

use std::sync::Arc;
use std::{convert::TryFrom, sync::Arc};

use sp_keyring::Sr25519Keyring;

use polkadot_erasure_coding::{branches, obtain_chunks_v1 as obtain_chunks};
use polkadot_node_primitives::{AvailableData, BlockData, ErasureChunk, PoV};
use polkadot_node_primitives::{AvailableData, BlockData, ErasureChunk, PoV, Proof};
use polkadot_primitives::v1::{
CandidateCommitments, CandidateDescriptor, CandidateHash, CommittedCandidateReceipt,
GroupIndex, Hash, HeadData, Id as ParaId, OccupiedCore, PersistedValidationData, SessionInfo,
Expand Down Expand Up @@ -139,7 +139,7 @@ pub fn get_valid_chunk_data(pov: PoV) -> (Hash, ErasureChunk) {
.map(|(index, (proof, chunk))| ErasureChunk {
chunk: chunk.to_vec(),
index: ValidatorIndex(index as _),
proof,
proof: Proof::try_from(proof).unwrap(),
})
.next()
.expect("There really should be 10 chunks.");
Expand Down
8 changes: 5 additions & 3 deletions node/network/availability-recovery/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -301,9 +301,11 @@ impl RequestChunksPhase {

let validator_index = chunk.index;

if let Ok(anticipated_hash) =
branch_hash(&params.erasure_root, &chunk.proof, chunk.index.0 as usize)
{
if let Ok(anticipated_hash) = branch_hash(
&params.erasure_root,
&chunk.proof_as_vec(),
chunk.index.0 as usize,
) {
let erasure_chunk_hash = BlakeTwo256::hash(&chunk.chunk);

if erasure_chunk_hash != anticipated_hash {
Expand Down
6 changes: 3 additions & 3 deletions node/network/availability-recovery/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

use std::{sync::Arc, time::Duration};
use std::{convert::TryFrom, sync::Arc, time::Duration};

use assert_matches::assert_matches;
use futures::{executor, future};
Expand All @@ -28,7 +28,7 @@ use super::*;
use sc_network::config::RequestResponseConfig;

use polkadot_erasure_coding::{branches, obtain_chunks_v1 as obtain_chunks};
use polkadot_node_primitives::{BlockData, PoV};
use polkadot_node_primitives::{BlockData, PoV, Proof};
use polkadot_node_subsystem_util::TimeoutExt;
use polkadot_primitives::v1::{AuthorityDiscoveryId, HeadData, PersistedValidationData};
use polkadot_subsystem::{
Expand Down Expand Up @@ -371,7 +371,7 @@ fn derive_erasure_chunks_with_proofs_and_root(
.map(|(index, (proof, chunk))| ErasureChunk {
chunk: chunk.to_vec(),
index: ValidatorIndex(index as _),
proof,
proof: Proof::try_from(proof).unwrap(),
})
.collect::<Vec<ErasureChunk>>();

Expand Down
4 changes: 2 additions & 2 deletions node/network/protocol/src/request_response/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
use parity_scale_codec::{Decode, Encode};

use polkadot_node_primitives::{
AvailableData, DisputeMessage, ErasureChunk, PoV, UncheckedDisputeMessage,
AvailableData, DisputeMessage, ErasureChunk, PoV, Proof, UncheckedDisputeMessage,
};
use polkadot_primitives::v1::{
CandidateHash, CandidateReceipt, CommittedCandidateReceipt, Hash, Id as ParaId, ValidatorIndex,
Expand Down Expand Up @@ -67,7 +67,7 @@ pub struct ChunkResponse {
/// The erasure-encoded chunk of data belonging to the candidate block.
pub chunk: Vec<u8>,
/// Proof for this chunk's branch in the Merkle tree.
pub proof: Vec<Vec<u8>>,
pub proof: Proof,
}

impl From<ErasureChunk> for ChunkResponse {
Expand Down
1 change: 1 addition & 0 deletions node/primitives/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ edition = "2018"
description = "Primitives types for the Node-side"

[dependencies]
bounded-vec = "0.4"
futures = "0.3.15"
polkadot-primitives = { path = "../../primitives" }
polkadot-statement-table = { path = "../../statement-table" }
Expand Down
Loading