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

Allow default block parameter to be blockHash #10932

Merged
merged 3 commits into from
Aug 6, 2019
Merged

Allow default block parameter to be blockHash #10932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8a4ced5
allow default block parameter to be blockHash
seunlanlege Aug 1, 2019
ef2367f
requireCanonical
seunlanlege Aug 5, 2019
c2d29c2
block check takes precedence over canon check
seunlanlege Aug 5, 2019
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
4 changes: 4 additions & 0 deletions ethcore/src/client/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1759,6 +1759,10 @@ impl BlockChainClient for Client {
self.config.spec_name.clone()
}

fn chain(&self) -> Arc<BlockProvider> {
self.chain.read().clone()
}

fn set_spec_name(&self, new_spec_name: String) -> Result<(), ()> {
trace!(target: "mode", "Client::set_spec_name({:?})", new_spec_name);
if !self.enabled.load(AtomicOrdering::Relaxed) {
Expand Down
5 changes: 5 additions & 0 deletions ethcore/src/client/test_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use std::str::FromStr;
use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering as AtomicOrder};
use std::sync::Arc;
use std::collections::{HashMap, BTreeMap};
use blockchain::BlockProvider;
use std::mem;

use blockchain::{TreeRoute, BlockReceipts};
Expand Down Expand Up @@ -703,6 +704,10 @@ impl BlockChainClient for TestBlockChainClient {
}
}

fn chain(&self) -> Arc<BlockProvider> {
unimplemented!()
}

fn list_accounts(&self, _id: BlockId, _after: Option<&Address>, _count: u64) -> Option<Vec<Address>> {
None
}
Expand Down
4 changes: 3 additions & 1 deletion ethcore/src/client/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
use std::collections::BTreeMap;
use std::sync::Arc;

use blockchain::{BlockReceipts, TreeRoute};
use blockchain::{BlockReceipts, TreeRoute, BlockProvider};
use bytes::Bytes;
use call_contract::{CallContract, RegistryInfo};
use ethcore_miner::pool::VerifiedTransaction;
Expand Down Expand Up @@ -233,6 +233,8 @@ pub trait BlockChainClient : Sync + Send + AccountData + BlockChain + CallContra
.expect("code will return Some if given BlockId::Latest; qed")
}

fn chain(&self) -> Arc<BlockProvider>;
Copy link
Contributor

@afck afck Aug 9, 2019

Choose a reason for hiding this comment

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

That triggered another compiler warning for me:

warning: missing documentation for a trait method
   --> ethcore/src/client/traits.rs:236:2
    |
236 |     fn chain(&self) -> Arc<BlockProvider>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
note: lint level defined here
   --> ethcore/src/lib.rs:17:9
    |
17  | #![warn(missing_docs, unused_extern_crates)]

Copy link
Collaborator

@dvdplm dvdplm Aug 9, 2019

Choose a reason for hiding this comment

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

Yeah, missed this during review. I added docs in another PR so it'll sort itself out soon enough.


/// Get block queue information.
fn queue_info(&self) -> BlockQueueInfo;

Expand Down
10 changes: 10 additions & 0 deletions rpc/src/v1/helpers/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,3 +609,13 @@ pub fn require_experimental(allow_experimental_rpcs: bool, eip: &str) -> Result<
})
}
}

/// returns an error for when require_canonical was specified and
pub fn invalid_input() -> Error {
Error {
// UNSUPPORTED_REQUEST shares the same error code for EIP-1898
code: ErrorCode::ServerError(codes::UNSUPPORTED_REQUEST),
message: "Invalid input".into(),
data: None
}
}
1 change: 1 addition & 0 deletions rpc/src/v1/helpers/light_fetch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ where
// (they don't have state) we can safely fallback to `Latest`.
let id = match num.unwrap_or_default() {
BlockNumber::Num(n) => BlockId::Number(n),
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,
BlockNumber::Pending => {
Expand Down
23 changes: 21 additions & 2 deletions rpc/src/v1/impls/eth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> EthClient<C, SN, S

BlockNumberOrId::Number(num) => {
let id = match num {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Latest => BlockId::Latest,
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Num(n) => BlockId::Number(n),
Expand Down Expand Up @@ -433,10 +434,10 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> EthClient<C, SN, S

fn get_state(&self, number: BlockNumber) -> StateOrBlock {
match number {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash).into(),
BlockNumber::Num(num) => BlockId::Number(num).into(),
BlockNumber::Earliest => BlockId::Earliest.into(),
BlockNumber::Latest => BlockId::Latest.into(),

BlockNumber::Pending => {
let info = self.client.chain_info();

Expand Down Expand Up @@ -472,10 +473,22 @@ fn check_known<C>(client: &C, number: BlockNumber) -> Result<()> where C: BlockC

let id = match number {
BlockNumber::Pending => return Ok(()),

BlockNumber::Num(n) => BlockId::Number(n),
BlockNumber::Latest => BlockId::Latest,
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Hash { hash, require_canonical } => {
// block check takes precedence over canon check.
match client.block_status(BlockId::Hash(hash.clone())) {
BlockStatus::InChain => {},
_ => return Err(errors::unknown_block()),
};

if require_canonical && !client.chain().is_canon(&hash) {
return Err(errors::invalid_input())
}

return Ok(())
}
};

match client.block_status(id) {
Expand Down Expand Up @@ -589,6 +602,7 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<

let num = num.unwrap_or_default();
let id = match num {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(n) => BlockId::Number(n),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,
Expand Down Expand Up @@ -762,6 +776,7 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<

fn transaction_by_block_number_and_index(&self, num: BlockNumber, index: Index) -> BoxFuture<Option<Transaction>> {
let block_id = match num {
BlockNumber::Hash { hash, .. } => PendingOrBlock::Block(BlockId::Hash(hash)),
BlockNumber::Latest => PendingOrBlock::Block(BlockId::Latest),
BlockNumber::Earliest => PendingOrBlock::Block(BlockId::Earliest),
BlockNumber::Num(num) => PendingOrBlock::Block(BlockId::Number(num)),
Expand Down Expand Up @@ -798,6 +813,7 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<

fn uncle_by_block_number_and_index(&self, num: BlockNumber, index: Index) -> BoxFuture<Option<RichBlock>> {
let id = match num {
BlockNumber::Hash { hash, .. } => PendingUncleId { id: PendingOrBlock::Block(BlockId::Hash(hash)), position: index.value() },
BlockNumber::Latest => PendingUncleId { id: PendingOrBlock::Block(BlockId::Latest), position: index.value() },
BlockNumber::Earliest => PendingUncleId { id: PendingOrBlock::Block(BlockId::Earliest), position: index.value() },
BlockNumber::Num(num) => PendingUncleId { id: PendingOrBlock::Block(BlockId::Number(num)), position: index.value() },
Expand Down Expand Up @@ -914,6 +930,7 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<
let signed = try_bf!(fake_sign::sign_call(request));

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();
Expand All @@ -923,6 +940,7 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<
(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,
Expand Down Expand Up @@ -964,6 +982,7 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<
(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,
Expand Down
3 changes: 3 additions & 0 deletions rpc/src/v1/impls/parity.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ impl<C, M, U, S> Parity for ParityClient<C, M, U> where
(header.encoded(), None)
} else {
let id = match number {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(num) => BlockId::Number(num),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,
Expand Down Expand Up @@ -381,6 +382,7 @@ impl<C, M, U, S> Parity for ParityClient<C, M, U> where
.collect()
))
},
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(num) => BlockId::Number(num),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,
Expand Down Expand Up @@ -412,6 +414,7 @@ impl<C, M, U, S> Parity for ParityClient<C, M, U> where
(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,
Expand Down
4 changes: 4 additions & 0 deletions rpc/src/v1/impls/traces.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ impl<C, S> Traces for TracesClient<C> where
let signed = fake_sign::sign_call(request)?;

let id = match block {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(num) => BlockId::Number(num),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,
Expand Down Expand Up @@ -122,6 +123,7 @@ impl<C, S> Traces for TracesClient<C> where
.collect::<Result<Vec<_>>>()?;

let id = match block {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(num) => BlockId::Number(num),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,
Expand All @@ -144,6 +146,7 @@ impl<C, S> Traces for TracesClient<C> where
let signed = SignedTransaction::new(tx).map_err(errors::transaction)?;

let id = match block {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(num) => BlockId::Number(num),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,
Expand All @@ -167,6 +170,7 @@ impl<C, S> Traces for TracesClient<C> where

fn replay_block_transactions(&self, block_number: BlockNumber, flags: TraceOptions) -> Result<Vec<TraceResultsWithTransactionHash>> {
let id = match block_number {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(num) => BlockId::Number(num),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,
Expand Down
99 changes: 91 additions & 8 deletions rpc/src/v1/types/block_number.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,20 @@

use std::fmt;
use serde::{Deserialize, Deserializer, Serialize, Serializer};
use serde::de::{Error, Visitor};
use serde::de::{Error, Visitor, MapAccess};
use ethcore::client::BlockId;
use ethereum_types::H256;

/// Represents rpc api block number param.
#[derive(Debug, PartialEq, Clone, Hash, Eq)]
pub enum BlockNumber {
/// Hash
Hash {
/// block hash
hash: H256,
/// only return blocks part of the canon chain
require_canonical: bool,
},
/// Number
Num(u64),
/// Latest block
Expand Down Expand Up @@ -68,6 +76,7 @@ impl LightBlockNumber for BlockNumber {
// Since light clients don't produce pending blocks
// (they don't have state) we can safely fallback to `Latest`.
match self {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(n) => BlockId::Number(n),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,
Expand All @@ -82,6 +91,9 @@ impl LightBlockNumber for BlockNumber {
impl Serialize for BlockNumber {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> where S: Serializer {
match *self {
BlockNumber::Hash{ hash, require_canonical } => serializer.serialize_str(
&format!("{{ 'hash': '{}', 'requireCanonical': '{}' }}", hash, require_canonical)
),
BlockNumber::Num(ref x) => serializer.serialize_str(&format!("0x{:x}", x)),
BlockNumber::Latest => serializer.serialize_str("latest"),
BlockNumber::Earliest => serializer.serialize_str("earliest"),
Expand All @@ -99,6 +111,54 @@ impl<'a> Visitor<'a> for BlockNumberVisitor {
write!(formatter, "a block number or 'latest', 'earliest' or 'pending'")
}

fn visit_map<V>(self, mut visitor: V) -> Result<Self::Value, V::Error> where V: MapAccess<'a> {
let (mut require_canonical, mut block_number, mut block_hash) = (false, None::<u64>, None::<H256>);

loop {
let key_str: Option<String> = visitor.next_key()?;

match key_str {
Some(key) => match key.as_str() {
"blockNumber" => {
let value: String = visitor.next_value()?;
if value.starts_with("0x") {
let number = u64::from_str_radix(&value[2..], 16).map_err(|e| {
Error::custom(format!("Invalid block number: {}", e))
})?;

block_number = Some(number);
break;
} else {
return Err(Error::custom("Invalid block number: missing 0x prefix".to_string()))
}
}
"blockHash" => {
block_hash = Some(visitor.next_value()?);
}
"requireCanonical" => {
require_canonical = visitor.next_value()?;
}
key => {
return Err(Error::custom(format!("Unknown key: {}", key)))
}
}
None => {
break
}
};
}

if let Some(number) = block_number {
return Ok(BlockNumber::Num(number))
}

if let Some(hash) = block_hash {
return Ok(BlockNumber::Hash { hash, require_canonical })
}

return Err(Error::custom("Invalid input"))
}

fn visit_str<E>(self, value: &str) -> Result<Self::Value, E> where E: Error {
match value {
"latest" => Ok(BlockNumber::Latest),
Expand All @@ -107,7 +167,9 @@ impl<'a> Visitor<'a> for BlockNumberVisitor {
_ if value.starts_with("0x") => u64::from_str_radix(&value[2..], 16).map(BlockNumber::Num).map_err(|e| {
Error::custom(format!("Invalid block number: {}", e))
}),
_ => Err(Error::custom("Invalid block number: missing 0x prefix".to_string())),
_ => {
Err(Error::custom("Invalid block number: missing 0x prefix".to_string()))
},
}
}

Expand All @@ -119,10 +181,10 @@ impl<'a> Visitor<'a> for BlockNumberVisitor {
/// Converts `BlockNumber` to `BlockId`, panics on `BlockNumber::Pending`
pub fn block_number_to_id(number: BlockNumber) -> BlockId {
match number {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(num) => BlockId::Number(num),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,

BlockNumber::Pending => panic!("`BlockNumber::Pending` should be handled manually")
}
}
Expand All @@ -131,19 +193,40 @@ pub fn block_number_to_id(number: BlockNumber) -> BlockId {
mod tests {
use ethcore::client::BlockId;
use super::*;
use std::str::FromStr;
use serde_json;

#[test]
fn block_number_deserialization() {
let s = r#"["0xa", "latest", "earliest", "pending"]"#;
let s = r#"[
"0xa",
"latest",
"earliest",
"pending",
{"blockNumber": "0xa"},
{"blockHash": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347"},
{"blockHash": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347", "requireCanonical": true}
]"#;
let deserialized: Vec<BlockNumber> = serde_json::from_str(s).unwrap();
assert_eq!(deserialized, vec![BlockNumber::Num(10), BlockNumber::Latest, BlockNumber::Earliest, BlockNumber::Pending])

assert_eq!(
deserialized,
vec![
BlockNumber::Num(10),
BlockNumber::Latest,
BlockNumber::Earliest,
BlockNumber::Pending,
BlockNumber::Num(10),
BlockNumber::Hash { hash: H256::from_str("1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347").unwrap(), require_canonical: false },
BlockNumber::Hash { hash: H256::from_str("1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347").unwrap(), require_canonical: true }
]
)
}

#[test]
fn should_not_deserialize_decimal() {
let s = r#""10""#;
assert!(serde_json::from_str::<BlockNumber>(s).is_err());
fn should_not_deserialize() {
let s = r#"[{}, "10"]"#;
Copy link
Member

@ordian ordian Aug 5, 2019

Choose a reason for hiding this comment

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

Do we reject requireCanonical with BlockNumber or any other invalid combinations?

Copy link
Member Author

@seunlanlege seunlanlege Aug 5, 2019

Choose a reason for hiding this comment

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

Currently, the code does not reject if used with blockNumber instead it is ignored. Do you think we should reject?

Copy link
Member

@ordian ordian Aug 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think if we derive Serialize/Deserialize (seems possible with serde attributes) instead of manually implementing it, it would be automatically supported. This can be done in a separate PR though.

assert!(serde_json::from_str::<Vec<BlockNumber>>(s).is_err());
}

#[test]
Expand Down
1 change: 1 addition & 0 deletions rpc/src/v1/types/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ impl Filter {
}

let num_to_id = |num| match num {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(n) => BlockId::Number(n),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest | BlockNumber::Pending => BlockId::Latest,
Expand Down
1 change: 1 addition & 0 deletions rpc/src/v1/types/trace_filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ pub struct TraceFilter {
impl Into<client::TraceFilter> for TraceFilter {
fn into(self) -> client::TraceFilter {
let num_to_id = |num| match num {
BlockNumber::Hash { hash, .. } => BlockId::Hash(hash),
BlockNumber::Num(n) => BlockId::Number(n),
BlockNumber::Earliest => BlockId::Earliest,
BlockNumber::Latest => BlockId::Latest,
Expand Down