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

Create opaque struct for StorageProof. #3834

Merged
bkchr merged 1 commit into from
Oct 31, 2019
Merged

Create opaque struct for StorageProof. #3834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

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

4 changes: 2 additions & 2 deletions core/authority-discovery/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
//! 4. Adds the retrieved external addresses as priority nodes to the peerset.

use authority_discovery_primitives::{AuthorityDiscoveryApi, AuthorityId, Signature};
use client::blockchain::HeaderBackend;
use client::{blockchain::HeaderBackend, runtime_api::StorageProof};
use error::{Error, Result};
use futures::{prelude::*, sync::mpsc::Receiver};
use log::{debug, error, log_enabled, warn};
Expand Down Expand Up @@ -528,7 +528,7 @@ mod tests {
unimplemented!("Not required for testing!")
}

fn extract_proof(&mut self) -> Option<Vec<Vec<u8>>> {
fn extract_proof(&mut self) -> Option<StorageProof> {
unimplemented!("Not required for testing!")
}
}
Expand Down
3 changes: 2 additions & 1 deletion core/client/src/block_builder/block_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use sr_primitives::traits::{
Header as HeaderT, Hash, Block as BlockT, One, HashFor, ProvideRuntimeApi, ApiRef, DigestFor,
};
use primitives::{H256, ExecutionContext};
use state_machine::StorageProof;
use crate::blockchain::HeaderBackend;
use crate::runtime_api::{Core, ApiExt};
use crate::error;
Expand Down Expand Up @@ -140,7 +141,7 @@ where
///
/// The proof will be `Some(_)`, if proof recording was enabled while creating
/// the block builder.
pub fn bake_and_extract_proof(mut self) -> error::Result<(Block, Option<Vec<Vec<u8>>>)> {
pub fn bake_and_extract_proof(mut self) -> error::Result<(Block, Option<StorageProof>)> {
self.bake_impl()?;

let proof = self.api.extract_proof();
Expand Down
8 changes: 4 additions & 4 deletions core/client/src/call_executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use sr_primitives::{
};
use state_machine::{
self, OverlayedChanges, Ext, ExecutionManager, StateMachine, ExecutionStrategy,
backend::Backend as _, ChangesTrieTransaction,
backend::Backend as _, ChangesTrieTransaction, StorageProof,
};
use executor::{RuntimeVersion, RuntimeInfo, NativeVersion};
use hash_db::Hasher;
Expand Down Expand Up @@ -127,7 +127,7 @@ where
overlay: &mut OverlayedChanges,
method: &str,
call_data: &[u8]
) -> Result<(Vec<u8>, Vec<Vec<u8>>), error::Error> {
) -> Result<(Vec<u8>, StorageProof), error::Error> {
let trie_state = state.as_trie_backend()
.ok_or_else(||
Box::new(state_machine::ExecutionError::UnableToGenerateProof)
Expand All @@ -145,7 +145,7 @@ where
overlay: &mut OverlayedChanges,
method: &str,
call_data: &[u8]
) -> Result<(Vec<u8>, Vec<Vec<u8>>), error::Error>;
) -> Result<(Vec<u8>, StorageProof), error::Error>;

/// Get runtime version if supported.
fn native_runtime_version(&self) -> Option<&NativeVersion>;
Expand Down Expand Up @@ -377,7 +377,7 @@ where
overlay: &mut OverlayedChanges,
method: &str,
call_data: &[u8]
) -> Result<(Vec<u8>, Vec<Vec<u8>>), error::Error> {
) -> Result<(Vec<u8>, StorageProof), error::Error> {
state_machine::prove_execution_on_trie_backend(
trie_state,
overlay,
Expand Down
6 changes: 3 additions & 3 deletions core/client/src/cht.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use trie;
use primitives::{H256, convert_hash};
use sr_primitives::traits::{Header as HeaderT, SimpleArithmetic, Zero, One};
use state_machine::backend::InMemory as InMemoryState;
use state_machine::{MemoryDB, TrieBackend, Backend as StateBackend,
use state_machine::{MemoryDB, TrieBackend, Backend as StateBackend, StorageProof,
prove_read_on_trie_backend, read_proof_check, read_proof_check_on_proving_backend};

use crate::error::{Error as ClientError, Result as ClientResult};
Expand Down Expand Up @@ -88,7 +88,7 @@ pub fn build_proof<Header, Hasher, BlocksI, HashesI>(
cht_num: Header::Number,
blocks: BlocksI,
hashes: HashesI
) -> ClientResult<Vec<Vec<u8>>>
) -> ClientResult<StorageProof>
where
Header: HeaderT,
Hasher: hash_db::Hasher,
Expand All @@ -114,7 +114,7 @@ pub fn check_proof<Header, Hasher>(
local_root: Header::Hash,
local_number: Header::Number,
remote_hash: Header::Hash,
remote_proof: Vec<Vec<u8>>
remote_proof: StorageProof,
) -> ClientResult<()>
where
Header: HeaderT,
Expand Down
22 changes: 11 additions & 11 deletions core/client/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ use state_machine::{
DBValue, Backend as StateBackend, ChangesTrieAnchorBlockId, ExecutionStrategy, ExecutionManager,
prove_read, prove_child_read, ChangesTrieRootsStorage, ChangesTrieStorage,
ChangesTrieTransaction, ChangesTrieConfigurationRange, key_changes, key_changes_proof,
OverlayedChanges, BackendTrustLevel,
OverlayedChanges, BackendTrustLevel, StorageProof, merge_storage_proofs,
};
use executor::{RuntimeVersion, RuntimeInfo};
use consensus::{
Expand Down Expand Up @@ -421,7 +421,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
}

/// Reads storage value at a given block + key, returning read proof.
pub fn read_proof<I>(&self, id: &BlockId<Block>, keys: I) -> error::Result<Vec<Vec<u8>>> where
pub fn read_proof<I>(&self, id: &BlockId<Block>, keys: I) -> error::Result<StorageProof> where
I: IntoIterator,
I::Item: AsRef<[u8]>,
{
Expand All @@ -437,7 +437,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
id: &BlockId<Block>,
storage_key: &[u8],
keys: I,
) -> error::Result<Vec<Vec<u8>>> where
) -> error::Result<StorageProof> where
I: IntoIterator,
I::Item: AsRef<[u8]>,
{
Expand All @@ -454,14 +454,14 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
id: &BlockId<Block>,
method: &str,
call_data: &[u8]
) -> error::Result<(Vec<u8>, Vec<Vec<u8>>)> {
) -> error::Result<(Vec<u8>, StorageProof)> {
let state = self.state_at(id)?;
let header = self.prepare_environment_block(id)?;
prove_execution(state, header, &self.executor, method, call_data)
}

/// Reads given header and generates CHT-based header proof.
pub fn header_proof(&self, id: &BlockId<Block>) -> error::Result<(Block::Header, Vec<Vec<u8>>)> {
pub fn header_proof(&self, id: &BlockId<Block>) -> error::Result<(Block::Header, StorageProof)> {
self.header_proof_with_cht_size(id, cht::size())
}

Expand All @@ -477,7 +477,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
&self,
id: &BlockId<Block>,
cht_size: NumberFor<Block>,
) -> error::Result<(Block::Header, Vec<Vec<u8>>)> {
) -> error::Result<(Block::Header, StorageProof)> {
let proof_error = || error::Error::Backend(format!("Failed to generate header proof for {:?}", id));
let header = self.backend.blockchain().expect_header(*id)?;
let block_num = *header.number();
Expand Down Expand Up @@ -696,18 +696,18 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
&self,
cht_size: NumberFor<Block>,
blocks: I
) -> error::Result<Vec<Vec<u8>>> {
) -> error::Result<StorageProof> {
// most probably we have touched several changes tries that are parts of the single CHT
// => GroupBy changes tries by CHT number and then gather proof for the whole group at once
let mut proof = HashSet::new();
let mut proofs = Vec::new();

cht::for_each_cht_group::<Block::Header, _, _, _>(cht_size, blocks, |_, cht_num, cht_blocks| {
let cht_proof = self.changes_trie_roots_proof_at_cht(cht_size, cht_num, cht_blocks)?;
proof.extend(cht_proof);
proofs.push(cht_proof);
Ok(())
}, ())?;

Ok(proof.into_iter().collect())
Ok(merge_storage_proofs(proofs))
Copy link
Contributor

@cheme cheme Oct 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I prefer the old version where we did not cat vecs: here we are eating memory. Maybe an iterator version of merge_storage_proofs will get the best of both world.
Actually merge_storage_proofs takes an iterator as input, so there is maybe something todo, does not seems super easy though since it neends to transform for_each_cht_group to an iterator.

Edit: may not be worth addressing.

}

/// Generates CHT-based proof for roots of changes tries at given blocks (that are part of single CHT).
Expand All @@ -716,7 +716,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
cht_size: NumberFor<Block>,
cht_num: NumberFor<Block>,
blocks: Vec<NumberFor<Block>>
) -> error::Result<Vec<Vec<u8>>> {
) -> error::Result<StorageProof> {
let cht_start = cht::start_number(cht_size, cht_num);
let mut current_num = cht_start;
let cht_range = ::std::iter::from_fn(|| {
Expand Down
2 changes: 1 addition & 1 deletion core/client/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ pub use crate::client::{
#[cfg(feature = "std")]
pub use crate::notifications::{StorageEventStream, StorageChangeSet};
#[cfg(feature = "std")]
pub use state_machine::ExecutionStrategy;
pub use state_machine::{ExecutionStrategy, StorageProof};
#[cfg(feature = "std")]
pub use crate::leaves::LeafSet;
#[cfg(feature = "std")]
Expand Down
24 changes: 9 additions & 15 deletions core/client/src/light/call_executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@

//! Methods that light client could use to execute runtime calls.

use std::{
collections::HashSet, sync::Arc, panic::UnwindSafe, result,
cell::RefCell, rc::Rc,
};
use std::{sync::Arc, panic::UnwindSafe, result, cell::RefCell, rc::Rc};

use codec::{Encode, Decode};
use primitives::{
Expand All @@ -31,7 +28,8 @@ use sr_primitives::{
};
use state_machine::{
self, Backend as StateBackend, OverlayedChanges, ExecutionStrategy, create_proof_check_backend,
execution_proof_check_on_trie_backend, ExecutionManager, ChangesTrieTransaction,
execution_proof_check_on_trie_backend, ExecutionManager, ChangesTrieTransaction, StorageProof,
merge_storage_proofs,
};
use hash_db::Hasher;

Expand Down Expand Up @@ -179,7 +177,7 @@ impl<Block, B, Local> CallExecutor<Block, Blake2Hasher> for
_changes: &mut OverlayedChanges,
_method: &str,
_call_data: &[u8]
) -> ClientResult<(Vec<u8>, Vec<Vec<u8>>)> {
) -> ClientResult<(Vec<u8>, StorageProof)> {
Err(ClientError::NotAvailableOnLightClient)
}

Expand All @@ -198,7 +196,7 @@ pub fn prove_execution<Block, S, E>(
executor: &E,
method: &str,
call_data: &[u8],
) -> ClientResult<(Vec<u8>, Vec<Vec<u8>>)>
) -> ClientResult<(Vec<u8>, StorageProof)>
where
Block: BlockT<Hash=H256>,
S: StateBackend<Blake2Hasher>,
Expand All @@ -218,11 +216,7 @@ pub fn prove_execution<Block, S, E>(

// execute method + record execution proof
let (result, exec_proof) = executor.prove_at_trie_state(&trie_state, &mut changes, method, call_data)?;
let total_proof = init_proof.into_iter()
.chain(exec_proof.into_iter())
.collect::<HashSet<_>>()
.into_iter()
.collect();
let total_proof = merge_storage_proofs(vec![init_proof, exec_proof]);
Copy link
Contributor

@cheme cheme Oct 16, 2019

Choose a reason for hiding this comment

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

vec allocation can be avoided here, especially since merge_storage_proofs takes an iterator as input.

Copy link
Contributor Author

@jimpo jimpo Oct 17, 2019

Choose a reason for hiding this comment

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

How exactly do you recommend creating this iterator? I think something like iter::once(init_proof).chain(iter::once(exec_proof)) is a bit clunky.

Copy link
Contributor

@cheme cheme Oct 17, 2019

Choose a reason for hiding this comment

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

I do not have better solution than using 'chain', I know some people prefer using Some(init_proof) rather than iter::once, I prefer once.
Clunkier solution (I tend to like fn_iter but I never try to bench it but it should have no advantage over chain): https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=8c36252915e606e2fbd845ac2618ffdd

Copy link
Contributor Author

@jimpo jimpo Oct 17, 2019

Choose a reason for hiding this comment

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

This really doesn't seem like a good reason to sacrifice readability to me. merge_storage_proofs internally has to create a new HashSet and much larger vector of trie nodes anyway.

Copy link
Contributor

@cheme cheme Oct 17, 2019

Choose a reason for hiding this comment

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

Of course it is just an alloc among lot of others, I think crazy iter expression are rather common in the code base, but not abusing them is probably a good thing.

Regarding the other comment where we grow memory, you also can use a different approach, I know some poeple like this kind of api, I never really use it but it may be good here.

struct ProofMerger(Hashset<StorageProof>);

impl ProofMerger {
  fn add(self, proof: StorageProof) {
    self.0.extends(proof);
    self
  }
  fn merge(self) -> StorageProof {
    StorageProof { trie_nodes: self.0.into_iter().collect() }
  }

}

in this case would look like

let total_proof = ProofMerger(init_proof)
  .add(exec_proof)
  .merge();

Copy link
Contributor

@cheme cheme Oct 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Note that it is really only nitpicking, should not prevent merging this PR for me.


Ok((result, total_proof))
}
Expand All @@ -234,7 +228,7 @@ pub fn prove_execution<Block, S, E>(
pub fn check_execution_proof<Header, E, H>(
executor: &E,
request: &RemoteCallRequest<Header>,
remote_proof: Vec<Vec<u8>>,
remote_proof: StorageProof,
) -> ClientResult<Vec<u8>>
where
Header: HeaderT,
Expand All @@ -258,7 +252,7 @@ pub fn check_execution_proof<Header, E, H>(
fn check_execution_proof_with_make_header<Header, E, H, MakeNextHeader: Fn(&Header) -> Header>(
executor: &E,
request: &RemoteCallRequest<Header>,
remote_proof: Vec<Vec<u8>>,
remote_proof: StorageProof,
make_next_header: MakeNextHeader,
) -> ClientResult<Vec<u8>>
where
Expand Down Expand Up @@ -382,7 +376,7 @@ mod tests {
_overlay: &mut OverlayedChanges,
_method: &str,
_call_data: &[u8]
) -> Result<(Vec<u8>, Vec<Vec<u8>>), ClientError> {
) -> Result<(Vec<u8>, StorageProof), ClientError> {
unreachable!()
}

Expand Down
Loading