Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.
Merged
24 changes: 12 additions & 12 deletions ethcore/client-traits/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,8 +412,8 @@ pub trait StateClient {
/// Type representing chain state
type State: StateInfo;

/// Get a copy of the best block's state.
fn latest_state(&self) -> Self::State;
/// Get a copy of the best block's state and header.
fn latest_state_and_header(&self) -> (Self::State, Header);
Copy link
Copy Markdown
Collaborator

@niklasad1 niklasad1 Oct 11, 2019

Choose a reason for hiding this comment

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

I'm not a big fan of methods with several responsibilities. Why not a separate method fn best_header/latest_header()?

Is this some kind of optimization that I don't understand?

Also, seems that we call this method a couple of this without caring about the header. I think the header type is quite big and might carry some cost to return by value when it's not needed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Looking up latest state first looks up latest header, so when we need both we:

  • look up the header
  • use the header to fetch the state
  • toss away the header
  • look up the header again

So while I agree with you that it's generally best to have methods do a single thing, I think returning both pieces of data is warranted here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it was made to ensure atomicity of the operation - i.e. that the we don't import the block in the meantime and the state does not match header.


/// Attempt to get a copy of a specific block's final state.
///
Expand Down Expand Up @@ -487,17 +487,17 @@ pub trait ChainNotify: Send + Sync {

/// Provides a method for importing/exporting blocks
pub trait ImportExportBlocks {
/// Export blocks to destination, with the given from, to and format argument.
/// destination could be a file or stdout.
/// If the format is hex, each block is written on a new line.
/// For binary exports, all block data is written to the same line.
/// Export blocks to destination, with the given from, to and format argument.
/// destination could be a file or stdout.
/// If the format is hex, each block is written on a new line.
/// For binary exports, all block data is written to the same line.
fn export_blocks<'a>(
&self,
destination: Box<dyn std::io::Write + 'a>,
from: BlockId,
to: BlockId,
format: Option<DataFormat>
) -> Result<(), String>;
&self,
destination: Box<dyn std::io::Write + 'a>,
from: BlockId,
to: BlockId,
format: Option<DataFormat>
) -> Result<(), String>;

/// Import blocks from destination, with the given format argument
/// Source could be a file or stdout.
Expand Down
22 changes: 12 additions & 10 deletions ethcore/src/client/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1033,15 +1033,16 @@ impl Client {
}

/// Get a copy of the best block's state.
pub fn latest_state(&self) -> State<StateDB> {
pub fn latest_state_and_header(&self) -> (State<StateDB>, Header) {
let header = self.best_block_header();
State::from_existing(
let state = State::from_existing(
self.state_db.read().boxed_clone_canon(&header.hash()),
*header.state_root(),
self.engine.account_start_nonce(header.number()),
self.factories.clone()
)
.expect("State root of best block header always valid.")
.expect("State root of best block header always valid.");
(state, header)
}

/// Attempt to get a copy of a specific block's final state.
Expand All @@ -1051,9 +1052,9 @@ impl Client {
/// is unknown.
pub fn state_at(&self, id: BlockId) -> Option<State<StateDB>> {
// fast path for latest state.
match id.clone() {
BlockId::Latest => return Some(self.latest_state()),
_ => {},
if let BlockId::Latest = id {
let (state, _) = self.latest_state_and_header();
return Some(state)
}

let block_number = match self.block_number(id) {
Expand Down Expand Up @@ -1087,8 +1088,9 @@ impl Client {
}

/// Get a copy of the best block's state.
pub fn state(&self) -> Box<dyn StateInfo> {
Box::new(self.latest_state()) as Box<_>
pub fn state(&self) -> impl StateInfo {
let (state, _) = self.latest_state_and_header();
state
}

/// Get info on the cache.
Expand Down Expand Up @@ -1476,8 +1478,8 @@ impl ImportBlock for Client {
impl StateClient for Client {
type State = State<::state_db::StateDB>;

fn latest_state(&self) -> Self::State {
Client::latest_state(self)
fn latest_state_and_header(&self) -> (Self::State, Header) {
Client::latest_state_and_header(self)
}

fn state_at(&self, id: BlockId) -> Option<Self::State> {
Expand Down
7 changes: 5 additions & 2 deletions ethcore/src/miner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,16 @@ use types::{
receipt::RichReceipt,
};

use block::SealedBlock;
use call_contract::CallContract;
use registrar::RegistrarClient;
use client::{BlockProducer, SealedBlockImporter};
use client_traits::{BlockChain, ChainInfo, AccountData, Nonce, ScheduleInfo};
use account_state::state::StateInfo;

use crate::{
block::SealedBlock,
client::{BlockProducer, SealedBlockImporter},
};

/// Provides methods to verify incoming external transactions
pub trait TransactionVerifierClient: Send + Sync
// Required for ServiceTransactionChecker
Expand Down
4 changes: 2 additions & 2 deletions ethcore/src/test_helpers/test_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -644,8 +644,8 @@ impl StateInfo for TestState {
impl StateClient for TestBlockChainClient {
type State = TestState;

fn latest_state(&self) -> Self::State {
TestState
fn latest_state_and_header(&self) -> (Self::State, Header) {
(TestState, self.best_block_header())
}

fn state_at(&self, _id: BlockId) -> Option<Self::State> {
Expand Down
1 change: 1 addition & 0 deletions ethcore/src/tests/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use std::str::{FromStr, from_utf8};
use std::sync::Arc;

use account_state::state::StateInfo;
use ethereum_types::{U256, Address};
use ethkey::KeyPair;
use hash::keccak;
Expand Down
87 changes: 55 additions & 32 deletions rpc/src/v1/impls/eth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ use types::{
BlockNumber as EthBlockNumber,
client_types::StateResult,
encoded,
header::Header,
ids::{BlockId, TransactionId, UncleId},
filter::Filter as EthcoreFilter,
transaction::{SignedTransaction, LocalizedTransaction},
Expand Down Expand Up @@ -182,10 +183,11 @@ pub fn base_logs<C, M, T: StateInfo + 'static> (client: &C, miner: &M, filter: F
Box::new(future::ok(logs))
}

impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> EthClient<C, SN, S, M, EM> where
impl<C, SN: ?Sized, S: ?Sized, M, EM, T> EthClient<C, SN, S, M, EM> where
C: miner::BlockChainClient + BlockChainClient + StateClient<State=T> + Call<State=T> + EngineInfo,
SN: SnapshotService,
S: SyncProvider,
T: StateInfo + 'static,
M: MinerService<State=T>,
EM: ExternalMinerService {

Expand Down Expand Up @@ -439,6 +441,10 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> EthClient<C, SN, S
Ok(Some(block))
}

/// Get state for the given block number. Returns either the State or a block from which state
/// can be retrieved.
/// Note: When passing `BlockNumber::Pending` we fall back to the state of the current best block
Comment thread
dvdplm marked this conversation as resolved.
/// if no state found for the best pending block.
fn get_state(&self, number: BlockNumber) -> StateOrBlock {
match number {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash).into(),
Expand All @@ -451,11 +457,33 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> EthClient<C, SN, S
self.miner
.pending_state(info.best_block_number)
.map(|s| Box::new(s) as Box<dyn StateInfo>)
.unwrap_or(Box::new(self.client.latest_state()) as Box<dyn StateInfo>)
.unwrap_or_else(|| {
warn!("Asked for best pending state, but none found. Falling back to latest state");
let (state, _) = self.client.latest_state_and_header();
Box::new(state) as Box<dyn StateInfo>
})
.into()
}
}
}

/// Get the state and header of best pending block. On failure, fall back to the best imported
/// blocks state&header.
fn pending_state_and_header_with_fallback(&self) -> (T, Header) {
let best_block_number = self.client.chain_info().best_block_number;
let (maybe_state, maybe_header) =
self.miner.pending_state(best_block_number).map_or_else(|| (None, None),|s| {
(Some(s), self.miner.pending_block_header(best_block_number))
});

match (maybe_state, maybe_header) {
(Some(state), Some(header)) => (state, header),
_ => {
warn!("Falling back to \"Latest\"");
self.client.latest_state_and_header()
}
}
}
}

pub fn pending_logs<M>(miner: &M, best_block: EthBlockNumber, filter: &EthcoreFilter) -> Vec<Log> where M: MinerService {
Expand Down Expand Up @@ -647,12 +675,13 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<
let num = num.unwrap_or_default();

try_bf!(check_known(&*self.client, num.clone()));
let res = match self.client.storage_at(&address, &BigEndianHash::from_uint(&position), self.get_state(num)) {
Some(s) => Ok(s),
None => Err(errors::state_pruned()),
};
let storage = self.client.storage_at(
&address,
&BigEndianHash::from_uint(&position),
self.get_state(num)
).ok_or_else(errors::state_pruned);

Box::new(future::done(res))
Box::new(future::done(storage))
}

fn transaction_count(&self, address: H160, num: Option<BlockNumber>) -> BoxFuture<U256> {
Expand Down Expand Up @@ -937,26 +966,26 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<
let num = num.unwrap_or_default();
try_bf!(check_known(&*self.client, num.clone()));

let (mut state, header) = if num == BlockNumber::Pending {
let info = self.client.chain_info();
let state = try_bf!(self.miner.pending_state(info.best_block_number).ok_or_else(errors::state_pruned));
let header = try_bf!(self.miner.pending_block_header(info.best_block_number).ok_or_else(errors::state_pruned));

(state, header)
} else {
let id = match num {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(num) => BlockId::Number(num),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,
BlockNumber::Pending => unreachable!(), // Already covered
};
let (mut state, header) =
if num == BlockNumber::Pending {
self.pending_state_and_header_with_fallback()
} else {
let id = match num {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(num) => BlockId::Number(num),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,
BlockNumber::Pending => unreachable!(), // Already covered
};

let state = try_bf!(self.client.state_at(id).ok_or_else(errors::state_pruned));
let header = try_bf!(self.client.block_header(id).ok_or_else(errors::state_pruned).and_then(|h| h.decode().map_err(errors::decode)));
let state = try_bf!(self.client.state_at(id).ok_or_else(errors::state_pruned));
let header = try_bf!(
self.client.block_header(id).ok_or_else(errors::state_pruned)
.and_then(|h| h.decode().map_err(errors::decode))
);

(state, header)
};
(state, header)
};

let result = self.client.call(&signed, Default::default(), &mut state, &header);

Expand All @@ -978,13 +1007,7 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<
let num = num.unwrap_or_default();

let (state, header) = if num == BlockNumber::Pending {
let info = self.client.chain_info();
let state = try_bf!(self.miner.pending_state(info.best_block_number)
.ok_or_else(errors::state_pruned));
let header = try_bf!(self.miner.pending_block_header(info.best_block_number)
.ok_or_else(errors::state_pruned));

(state, header)
self.pending_state_and_header_with_fallback()
} else {
let id = match num {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
Expand Down
4 changes: 2 additions & 2 deletions rpc/src/v1/tests/helpers/miner_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@ impl StateClient for TestMinerService {
// State will not be used by test client anyway, since all methods that accept state are mocked
type State = TestState;

fn latest_state(&self) -> Self::State {
TestState
fn latest_state_and_header(&self) -> (Self::State, Header) {
(TestState, Header::default())
}

fn state_at(&self, _id: BlockId) -> Option<Self::State> {
Expand Down
74 changes: 74 additions & 0 deletions rpc/src/v1/tests/mocked/eth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,43 @@ fn rpc_eth_call_latest() {
assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned()));
}

#[test]
fn rpc_eth_call_pending() {
let tester = EthTester::default();
tester.client.set_execution_result(Ok(Executed {
exception: None,
gas: U256::zero(),
gas_used: U256::from(0xff30),
refunded: U256::from(0x5),
cumulative_gas_used: U256::zero(),
logs: vec![],
contracts_created: vec![],
output: vec![0x12, 0x34, 0xff],
trace: vec![],
vm_trace: None,
state_diff: None,
}));

let request = r#"{
"jsonrpc": "2.0",
"method": "eth_call",
"params": [{
"from": "0xb60e8dd61c5d32be8058bb8eb970870f07233155",
"to": "0xd46e8dd67c5d32be8058bb8eb970870f07244567",
"gas": "0x76c0",
"gasPrice": "0x9184e72a000",
"value": "0x9184e72a",
"data": "0xd46e8dd67c5d32be8d46e8dd67c5d32be8058bb8eb970870f072445675058bb8eb970870f072445675"
},
"pending"],
"id": 1
}"#;
// Falls back to "Latest" and gives the same result.
let response = r#"{"jsonrpc":"2.0","result":"0x1234ff","id":1}"#;

assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned()));
}

#[test]
fn rpc_eth_call() {
let tester = EthTester::default();
Expand Down Expand Up @@ -770,6 +807,43 @@ fn rpc_eth_estimate_gas() {
assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned()));
}

#[test]
fn rpc_eth_estimate_gas_pending() {
let tester = EthTester::default();
tester.client.set_execution_result(Ok(Executed {
exception: None,
gas: U256::zero(),
gas_used: U256::from(0xff30),
refunded: U256::from(0x5),
cumulative_gas_used: U256::zero(),
logs: vec![],
contracts_created: vec![],
output: vec![0x12, 0x34, 0xff],
trace: vec![],
vm_trace: None,
state_diff: None,
}));

let request = r#"{
"jsonrpc": "2.0",
"method": "eth_estimateGas",
"params": [{
"from": "0xb60e8dd61c5d32be8058bb8eb970870f07233155",
"to": "0xd46e8dd67c5d32be8058bb8eb970870f07244567",
"gas": "0x76c0",
"gasPrice": "0x9184e72a000",
"value": "0x9184e72a",
"data": "0xd46e8dd67c5d32be8d46e8dd67c5d32be8058bb8eb970870f072445675058bb8eb970870f072445675"
},
"pending"],
"id": 1
}"#;
// Falls back to "Latest" so the result is the same
let response = r#"{"jsonrpc":"2.0","result":"0x5208","id":1}"#;

assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned()));
}

#[test]
fn rpc_eth_estimate_gas_default_block() {
let tester = EthTester::default();
Expand Down