This repository has been archived by the owner on Nov 6, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Bad blocks RPC + reporting #9433
Merged
+375
−59
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
c96ee74
Bad blocks RPC.
tomusdrw eaf9928
Return bad blocks via RPC.
tomusdrw b22388d
Fix test.
tomusdrw e67f5ce
Merge branch 'master' into td-badblocks
tomusdrw 6d2bd28
More verbose bad block message.
tomusdrw aa46cd9
Expose via CLI.
tomusdrw 1686698
Remove stray whitespace.
tomusdrw 395ec14
Remove stray newline.
tomusdrw 023da5b
Fix tests.
tomusdrw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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)>>, | ||
} | ||
|
||
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() | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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::{ | ||
|
@@ -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. | ||
|
@@ -254,6 +258,7 @@ impl Importer { | |
miner, | ||
ancient_verifier: AncientVerifier::new(engine.clone()), | ||
engine, | ||
bad_blocks: Default::default(), | ||
}) | ||
} | ||
|
||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
}, | ||
} | ||
} | ||
|
||
|
@@ -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), | ||
} | ||
} | ||
} | ||
|
||
|
@@ -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)?; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.