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

Bad blocks RPC + reporting #9433

Merged
merged 9 commits into from
Sep 8, 2018
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
81 changes: 81 additions & 0 deletions ethcore/src/client/bad_blocks.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// Copyright 2015-2018 Parity Technologies (UK) Ltd.
// This file is part of Parity.

// Parity 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.

// Parity 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 Parity. If not, see <http://www.gnu.org/licenses/>.

//! Stores recently seen bad blocks.

use bytes::{Bytes, ToPretty};
use ethereum_types::H256;
use itertools::Itertools;
use memory_cache::MemoryLruCache;
use parking_lot::RwLock;
use verification::queue::kind::blocks::Unverified;

/// Recently seen bad blocks.
pub struct BadBlocks {
last_blocks: RwLock<MemoryLruCache<H256, (Unverified, String)>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

How about to add the raw_block data (Vec) into the tuple also, to be able to save "undecodable" BadBlocks? Is that makes any sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I though about it actually. It doesn't allow us to return detailed block in the RPC response though. I think currently it makes sense to have something with the same results as geth to start detecting issues asap.

That said, feel free to log it as a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also thought about this and checked the geth implementation and it seems they also don't store block rlp's that aren't decodable.

}

impl Default for BadBlocks {
fn default() -> Self {
BadBlocks {
last_blocks: RwLock::new(MemoryLruCache::new(8 * 1024 * 1024)),
}
}
}

impl BadBlocks {
/// Reports given RLP as invalid block.
pub fn report(&self, raw: Bytes, message: String) {
match Unverified::from_rlp(raw) {
Ok(unverified) => {
error!(
target: "client",
"\nBad block detected: {}\nRLP: {}\nHeader: {:?}\nUncles: {}\nTransactions:{}\n",
message,
unverified.bytes.to_hex(),
unverified.header,
unverified.uncles
.iter()
.enumerate()
.map(|(index, uncle)| format!("[Uncle {}] {:?}", index, uncle))
.join("\n"),
unverified.transactions
.iter()
.enumerate()
.map(|(index, tx)| format!("[Tx {}] {:?}", index, tx))
.join("\n"),
);
self.last_blocks.write().insert(unverified.header.hash(), (unverified, message));
},
Err(err) => {
error!(target: "client", "Bad undecodable block detected: {}\n{:?}", message, err);
},
}
}

/// Returns a list of recently detected bad blocks with error descriptions.
pub fn bad_blocks(&self) -> Vec<(Unverified, String)> {
self.last_blocks.read()
.backstore()
.iter()
.map(|(_k, (unverified, message))| (
Unverified::from_rlp(unverified.bytes.clone())
.expect("Bytes coming from UnverifiedBlock so decodable; qed"),
message.clone(),
))
.collect()
}
}
58 changes: 42 additions & 16 deletions ethcore/src/client/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,15 @@ use client::{
RegistryInfo, ReopenBlock, PrepareOpenBlock, ScheduleInfo, ImportSealedBlock,
BroadcastProposalBlock, ImportBlock, StateOrBlock, StateInfo, StateClient, Call,
AccountData, BlockChain as BlockChainTrait, BlockProducer, SealedBlockImporter,
ClientIoMessage
ClientIoMessage,
};
use client::{
BlockId, TransactionId, UncleId, TraceId, ClientConfig, BlockChainClient,
TraceFilter, CallAnalytics, BlockImportError, Mode,
ChainNotify, ChainRoute, PruningInfo, ProvingBlockChainClient, EngineInfo, ChainMessageType,
IoClient,
IoClient, BadBlocks,
};
use client::bad_blocks;
use encoded;
use engines::{EthEngine, EpochTransition, ForkChoice};
use error::{
Expand Down Expand Up @@ -163,6 +164,9 @@ struct Importer {

/// Ethereum engine to be used during import
pub engine: Arc<EthEngine>,

/// A lru cache of recently detected bad blocks
pub bad_blocks: bad_blocks::BadBlocks,
}

/// Blockchain database client backed by a persistent database. Owns and manages a blockchain and a block queue.
Expand Down Expand Up @@ -254,6 +258,7 @@ impl Importer {
miner,
ancient_verifier: AncientVerifier::new(engine.clone()),
engine,
bad_blocks: Default::default(),
})
}

Expand Down Expand Up @@ -291,22 +296,27 @@ impl Importer {
continue;
}

if let Ok(closed_block) = self.check_and_lock_block(block, client) {
if self.engine.is_proposal(&header) {
self.block_queue.mark_as_good(&[hash]);
proposed_blocks.push(bytes);
} else {
imported_blocks.push(hash);
let raw = block.bytes.clone();
match self.check_and_lock_block(block, client) {
Ok(closed_block) => {
if self.engine.is_proposal(&header) {
self.block_queue.mark_as_good(&[hash]);
proposed_blocks.push(bytes);
} else {
imported_blocks.push(hash);

let transactions_len = closed_block.transactions().len();
let transactions_len = closed_block.transactions().len();

let route = self.commit_block(closed_block, &header, encoded::Block::new(bytes), client);
import_results.push(route);
let route = self.commit_block(closed_block, &header, encoded::Block::new(bytes), client);
import_results.push(route);

client.report.write().accrue_block(&header, transactions_len);
}
} else {
invalid_blocks.insert(header.hash());
client.report.write().accrue_block(&header, transactions_len);
}
},
Err(err) => {
self.bad_blocks.report(raw, format!("{:?}", err));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this configurable? Such as only enable bad blocks reporting when the debug namespace RPC is enabled, or just use another cli flag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the whole point is to have it running by default, so that anyone (that have seen such message) can be asked to get bad blocks from RPC and then report them for the analysis.

invalid_blocks.insert(header.hash());
},
}
}

Expand Down Expand Up @@ -1390,12 +1400,22 @@ impl ImportBlock for Client {
if self.chain.read().is_known(&unverified.hash()) {
bail!(BlockImportErrorKind::Import(ImportErrorKind::AlreadyInChain));
}

let status = self.block_status(BlockId::Hash(unverified.parent_hash()));
if status == BlockStatus::Unknown || status == BlockStatus::Pending {
bail!(BlockImportErrorKind::Block(BlockError::UnknownParent(unverified.parent_hash())));
}

Ok(self.importer.block_queue.import(unverified)?)
let raw = unverified.bytes.clone();
match self.importer.block_queue.import(unverified).map_err(Into::into) {
Ok(res) => Ok(res),
// we only care about block errors (not import errors)
Err(BlockImportError(BlockImportErrorKind::Block(err), _))=> {
self.importer.bad_blocks.report(raw, format!("{:?}", err));
bail!(BlockImportErrorKind::Block(err))
},
Err(e) => Err(e),
}
}
}

Expand Down Expand Up @@ -1532,6 +1552,12 @@ impl EngineInfo for Client {
}
}

impl BadBlocks for Client {
fn bad_blocks(&self) -> Vec<(Unverified, String)> {
self.importer.bad_blocks.bad_blocks()
}
}

impl BlockChainClient for Client {
fn replay(&self, id: TransactionId, analytics: CallAnalytics) -> Result<Executed, CallError> {
let address = self.transaction_address(id).ok_or(CallError::TransactionNotFound)?;
Expand Down
3 changes: 2 additions & 1 deletion ethcore/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
//! Blockchain database client.

mod ancient_import;
mod bad_blocks;
mod client;
mod config;
#[cfg(any(test, feature = "test-helpers"))]
Expand All @@ -36,7 +37,7 @@ pub use self::test_client::{TestBlockChainClient, EachBlockWith};
pub use self::chain_notify::{ChainNotify, ChainRoute, ChainRouteType, ChainMessageType};
pub use self::traits::{
Nonce, Balance, ChainInfo, BlockInfo, ReopenBlock, PrepareOpenBlock, CallContract, TransactionInfo, RegistryInfo, ScheduleInfo, ImportSealedBlock, BroadcastProposalBlock, ImportBlock,
StateOrBlock, StateClient, Call, EngineInfo, AccountData, BlockChain, BlockProducer, SealedBlockImporter
StateOrBlock, StateClient, Call, EngineInfo, AccountData, BlockChain, BlockProducer, SealedBlockImporter, BadBlocks,
};
pub use state::StateInfo;
pub use self::traits::{BlockChainClient, EngineClient, ProvingBlockChainClient, IoClient};
Expand Down
16 changes: 15 additions & 1 deletion ethcore/src/client/test_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ use client::{
PrepareOpenBlock, BlockChainClient, BlockChainInfo, BlockStatus, BlockId, Mode,
TransactionId, UncleId, TraceId, TraceFilter, LastHashes, CallAnalytics, BlockImportError,
ProvingBlockChainClient, ScheduleInfo, ImportSealedBlock, BroadcastProposalBlock, ImportBlock, StateOrBlock,
Call, StateClient, EngineInfo, AccountData, BlockChain, BlockProducer, SealedBlockImporter, IoClient
Call, StateClient, EngineInfo, AccountData, BlockChain, BlockProducer, SealedBlockImporter, IoClient,
BadBlocks,
};
use db::{NUM_COLUMNS, COL_STATE};
use header::{Header as BlockHeader, BlockNumber};
Expand Down Expand Up @@ -615,6 +616,19 @@ impl EngineInfo for TestBlockChainClient {
}
}

impl BadBlocks for TestBlockChainClient {
fn bad_blocks(&self) -> Vec<(Unverified, String)> {
vec![
(Unverified {
header: Default::default(),
transactions: vec![],
uncles: vec![],
bytes: vec![1, 2, 3],
}, "Invalid block".into())
]
}
}

impl BlockChainClient for TestBlockChainClient {
fn replay(&self, _id: TransactionId, _analytics: CallAnalytics) -> Result<Executed, CallError> {
self.execution_result.read().clone().unwrap()
Expand Down
8 changes: 7 additions & 1 deletion ethcore/src/client/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,9 +211,15 @@ pub trait IoClient: Sync + Send {
fn queue_consensus_message(&self, message: Bytes);
}

/// Provides recently seen bad blocks.
pub trait BadBlocks {
/// Returns a list of blocks that were recently not imported because they were invalid.
fn bad_blocks(&self) -> Vec<(Unverified, String)>;
}

/// Blockchain database client. Owns and manages a blockchain and a block queue.
pub trait BlockChainClient : Sync + Send + AccountData + BlockChain + CallContract + RegistryInfo + ImportBlock
+ IoClient {
+ IoClient + BadBlocks {
/// Look up the block number for the given block ID.
fn block_number(&self, id: BlockId) -> Option<BlockNumber>;

Expand Down
2 changes: 1 addition & 1 deletion parity/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ usage! {

ARG arg_jsonrpc_apis: (String) = "web3,eth,pubsub,net,parity,private,parity_pubsub,traces,rpc,shh,shh_pubsub", or |c: &Config| c.rpc.as_ref()?.apis.as_ref().map(|vec| vec.join(",")),
"--jsonrpc-apis=[APIS]",
"Specify the APIs available through the HTTP JSON-RPC interface using a comma-delimited list of API names. Possible names are: all, safe, web3, net, eth, pubsub, personal, signer, parity, parity_pubsub, parity_accounts, parity_set, traces, rpc, secretstore, shh, shh_pubsub. You can also disable a specific API by putting '-' in the front, example: all,-personal. 'safe' enables the following APIs: web3, net, eth, pubsub, parity, parity_pubsub, traces, rpc, shh, shh_pubsub",
"Specify the APIs available through the HTTP JSON-RPC interface using a comma-delimited list of API names. Possible names are: all, safe, debug, web3, net, eth, pubsub, personal, signer, parity, parity_pubsub, parity_accounts, parity_set, traces, rpc, secretstore, shh, shh_pubsub. You can also disable a specific API by putting '-' in the front, example: all,-personal. 'safe' enables the following APIs: web3, net, eth, pubsub, parity, parity_pubsub, traces, rpc, shh, shh_pubsub",

ARG arg_jsonrpc_hosts: (String) = "none", or |c: &Config| c.rpc.as_ref()?.hosts.as_ref().map(|vec| vec.join(",")),
"--jsonrpc-hosts=[HOSTS]",
Expand Down
Loading