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

Logic for checking Substrate proofs from within runtime module. #3783

Merged
merged 1 commit into from
Oct 10, 2019
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
3 changes: 3 additions & 0 deletions Cargo.lock

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

4 changes: 4 additions & 0 deletions srml/bridge/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,18 @@ edition = "2018"

[dependencies]
codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false }
hash-db = { version = "0.15.2", default-features = false }
session = { package = "srml-session", path = "../session", default-features = false, features = ["historical"] }
serde = { version = "1.0", optional = true }
sr-primitives = { path = "../../core/sr-primitives", default-features = false }
support = { package = "srml-support", path = "../support", default-features = false }
system = { package = "srml-system", path = "../system", default-features = false }
trie = { package = "substrate-trie", path = "../../core/trie", default-features = false }

[dev-dependencies]
primitives = { package = "substrate-primitives", path = "../../core/primitives" }
runtime-io = { package = "sr-io", path = "../../core/sr-io", default-features = false }
state-machine = { package = "substrate-state-machine", path = "../../core/state-machine" }

[features]
default = ["std"]
Expand All @@ -27,5 +30,6 @@ std = [
"sr-primitives/std",
"support/std",
"system/std",
"trie/std",
"runtime-io/std",
]
25 changes: 25 additions & 0 deletions srml/bridge/src/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Copyright 2017-2019 Parity Technologies (UK) Ltd.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2017-2019 Parity Technologies (UK) Ltd.
// Copyright 2019 Parity Technologies (UK) Ltd.

// This file is part of Substrate.

// Substrate is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// Substrate is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Substrate. If not, see <http://www.gnu.org/licenses/>.

//! Common error types for the srml-bridge crate.

#[derive(PartialEq)]
#[cfg_attr(feature = "std", derive(Debug))]
pub enum Error {
StorageRootMismatch,
StorageValueUnavailable,
}

8 changes: 6 additions & 2 deletions srml/bridge/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
// Ensure we're `no_std` when compiling for Wasm.
#![cfg_attr(not(feature = "std"), no_std)]

mod error;
mod storage_proof;

use codec::{Encode, Decode};
use sr_primitives::traits::{Header, Member};
use support::{
Expand Down Expand Up @@ -148,7 +151,7 @@ mod tests {

use primitives::{H256, Blake2Hasher};
use sr_primitives::{
Perbill, traits::IdentityLookup, testing::Header, generic::Digest
Perbill, traits::{Header as HeaderT, IdentityLookup}, testing::Header, generic::Digest,
};
use support::{assert_ok, impl_outer_origin, parameter_types};
use runtime_io::with_externalities;
Expand Down Expand Up @@ -223,6 +226,7 @@ mod tests {
extrinsics_root: H256::default(),
digest: Digest::default(),
};
let test_hash = test_header.hash();

with_externalities(&mut new_test_ext(), || {
assert_eq!(MockBridge::num_bridges(), 0);
Expand All @@ -242,7 +246,7 @@ mod tests {
MockBridge::tracked_bridges(1),
Some(BridgeInfo {
last_finalized_block_number: 42,
last_finalized_block_hash: test_header.hash(), // FIXME: This is broken
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know the details but is this now unbroken?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is now fixed in this PR. Was a compilation issue.

Copy link
Contributor

@HCastano HCastano Oct 10, 2019

Choose a reason for hiding this comment

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

testing::Header doesn't have a hash() method available, which is why it was broken. But yeah, it's fixed now

Copy link
Contributor

Choose a reason for hiding this comment

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

My comment was due to the fact that only looking at the local scope of the code, I only identified that test_header.hash(); now has a variable and no logic has changed and I was like hmm.. how does that solve the problem? 😬

last_finalized_block_hash: test_hash,
last_finalized_state_root: dummy_state_root,
current_validator_set: vec![1, 2, 3],
}));
Expand Down
103 changes: 103 additions & 0 deletions srml/bridge/src/storage_proof.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// Copyright 2017-2019 Parity Technologies (UK) Ltd.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2017-2019 Parity Technologies (UK) Ltd.
// Copyright 2019 Parity Technologies (UK) Ltd.

// This file is part of Substrate.

// Substrate is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// Substrate is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Substrate. If not, see <http://www.gnu.org/licenses/>.

//! Logic for checking Substrate storage proofs.

use hash_db::{Hasher, HashDB, EMPTY_PREFIX};
use trie::{MemoryDB, Trie, trie_types::TrieDB};

use crate::error::Error;

/// This struct is used to read storage values from a subset of a Merklized database. The "proof"
/// is a subset of the nodes in the Merkle structure of the database, so that it provides
/// authentication against a known Merkle root as well as the values in the database themselves.
pub struct StorageProofChecker<H>
where H: Hasher
{
root: H::Out,
db: MemoryDB<H>,
}

impl<H> StorageProofChecker<H>
where H: Hasher
{
/// Constructs a new storage proof checker.
///
/// This returns an error if the given proof is invalid with respect to the given root.
pub fn new(root: H::Out, proof: Vec<Vec<u8>>) -> Result<Self, Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This API does look a bit strange to me that you build a new instance of checked and via the constructor you also give it the task that it has to do (i.e. proof to check). If it woks for you then it's okay but I was expecting new to actually return a new instance of something that can check and a .check to actually check.

Copy link
Contributor

@kianenigma kianenigma Oct 9, 2019

Choose a reason for hiding this comment

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

Though looking at the test I see it more clear of how you intend to use it. Just a thought. It seems like this struct is StateProofCheckerAndReader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... I'm also not thrilled about this API, but it's kind of a direct consequence of the proof format. See #3782.

StateProofCheckerAndReader is somewhat more accurate of a description, but in substrate-state-machine, it's just called check_read_proof usually so ¯_(ツ)_/¯.

let mut db = MemoryDB::default();
for item in proof {
db.insert(EMPTY_PREFIX, &item);
}
let checker = StorageProofChecker {
root,
db,
};
// Return error if trie would be invalid.
let _ = checker.trie()?;
Ok(checker)
}

/// Reads a value from the available subset of storage. If the value cannot be read due to an
/// incomplete or otherwise invalid proof, this returns an error.
pub fn read_value(&self, key: &[u8]) -> Result<Option<Vec<u8>>, Error> {
self.trie()?
.get(key)
.map(|value| value.map(|value| value.into_vec()))
.map_err(|_| Error::StorageValueUnavailable)
}

fn trie(&self) -> Result<TrieDB<H>, Error> {
TrieDB::new(&self.db, &self.root)
.map_err(|_| Error::StorageRootMismatch)
}
}

#[cfg(test)]
mod tests {
use super::*;

use primitives::{Blake2Hasher, H256};
use state_machine::{prove_read, backend::{Backend, InMemory}};
// use trie::{PrefixedMemoryDB, TrieDBMut};

#[test]
fn storage_proof_check() {
// construct storage proof
let backend = <InMemory<Blake2Hasher>>::from(vec![
(None, b"key1".to_vec(), Some(b"value1".to_vec())),
(None, b"key2".to_vec(), Some(b"value2".to_vec())),
(None, b"key3".to_vec(), Some(b"value3".to_vec())),
// Value is too big to fit in a branch node
(None, b"key11".to_vec(), Some(vec![0u8; 32])),
]);
let root = backend.storage_root(std::iter::empty()).0;
let proof = prove_read(backend, &[&b"key1"[..], &b"key2"[..], &b"key22"[..]]).unwrap();

// check proof in runtime
let checker = <StorageProofChecker<Blake2Hasher>>::new(root, proof.clone()).unwrap();
assert_eq!(checker.read_value(b"key1"), Ok(Some(b"value1".to_vec())));
assert_eq!(checker.read_value(b"key2"), Ok(Some(b"value2".to_vec())));
assert_eq!(checker.read_value(b"key11111"), Err(Error::StorageValueUnavailable));
assert_eq!(checker.read_value(b"key22"), Ok(None));

// checking proof against invalid commitment fails
assert_eq!(
<StorageProofChecker<Blake2Hasher>>::new(H256::random(), proof).err(),
Some(Error::StorageRootMismatch)
);
}
}