From 70db4f503fd41eda51fedcaf38d906323df8ca81 Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 1 May 2018 14:29:39 +0200 Subject: [PATCH 01/17] rlp::decode returns Result --- util/rlp/src/lib.rs | 6 +++--- util/rlp/tests/tests.rs | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/util/rlp/src/lib.rs b/util/rlp/src/lib.rs index a6754e22de9..b416b1c25b0 100644 --- a/util/rlp/src/lib.rs +++ b/util/rlp/src/lib.rs @@ -63,13 +63,13 @@ pub const EMPTY_LIST_RLP: [u8; 1] = [0xC0; 1]; /// /// fn main () { /// let data = vec![0x83, b'c', b'a', b't']; -/// let animal: String = rlp::decode(&data); +/// let animal: String = rlp::decode(&data).expect("could not decode"); /// assert_eq!(animal, "cat".to_owned()); /// } /// ``` -pub fn decode(bytes: &[u8]) -> T where T: Decodable { +pub fn decode(bytes: &[u8]) -> Result where T: Decodable { let rlp = Rlp::new(bytes); - rlp.as_val().expect("trusted rlp should be valid") + rlp.as_val() } pub fn decode_list(bytes: &[u8]) -> Vec where T: Decodable { diff --git a/util/rlp/tests/tests.rs b/util/rlp/tests/tests.rs index 6ff426a7739..041c267667d 100644 --- a/util/rlp/tests/tests.rs +++ b/util/rlp/tests/tests.rs @@ -209,8 +209,10 @@ struct VDTestPair(Vec, Vec) where T: Decodable + fmt::Debug + cmp::Eq; fn run_decode_tests(tests: Vec>) where T: Decodable + fmt::Debug + cmp::Eq { for t in &tests { - let res: T = rlp::decode(&t.1); - assert_eq!(res, t.0); + let res : Result = rlp::decode(&t.1); + assert!(res.is_ok()); + let res = res.unwrap(); + assert_eq!(&res, &t.0); } } From 3b6f5c93eceb032cc6eb9aca65fc201ba65395f5 Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 1 May 2018 14:30:09 +0200 Subject: [PATCH 02/17] Fix journaldb to handle rlp::decode Result --- util/journaldb/src/archivedb.rs | 12 ++++++------ util/journaldb/src/earlymergedb.rs | 2 +- util/journaldb/src/overlaydb.rs | 6 +++++- util/journaldb/src/overlayrecentdb.rs | 10 ++++++++-- util/journaldb/src/refcounteddb.rs | 14 +++++++------- 5 files changed, 27 insertions(+), 17 deletions(-) diff --git a/util/journaldb/src/archivedb.rs b/util/journaldb/src/archivedb.rs index e00b37d3c4a..11677ef83b7 100644 --- a/util/journaldb/src/archivedb.rs +++ b/util/journaldb/src/archivedb.rs @@ -45,14 +45,14 @@ pub struct ArchiveDB { impl ArchiveDB { /// Create a new instance from a key-value db. - pub fn new(backing: Arc, col: Option) -> ArchiveDB { - let latest_era = backing.get(col, &LATEST_ERA_KEY).expect("Low-level database error.") - .map(|val| decode::(&val)); + pub fn new(backing: Arc, column: Option) -> ArchiveDB { + let latest_era = backing.get(column, &LATEST_ERA_KEY).expect("Low-level database error.") + .map(|val| decode::(&val).unwrap_or(0)); // REVIEW: is `0` an ok value for `latest_era` if the Rlp decoding fails? The alternative is to either panic or change the return value of the function to return `Result`. ArchiveDB { overlay: MemoryDB::new(), - backing: backing, - latest_era: latest_era, - column: col, + backing, + latest_era, + column, } } diff --git a/util/journaldb/src/earlymergedb.rs b/util/journaldb/src/earlymergedb.rs index bb8e49d41ed..5abeeb0c22b 100644 --- a/util/journaldb/src/earlymergedb.rs +++ b/util/journaldb/src/earlymergedb.rs @@ -263,7 +263,7 @@ impl EarlyMergeDB { let mut refs = HashMap::new(); let mut latest_era = None; if let Some(val) = db.get(col, &LATEST_ERA_KEY).expect("Low-level database error.") { - let mut era = decode::(&val); + let mut era = decode::(&val).unwrap_or(0); // REVIEW: The error is lost here. Log and return? Panic? Or is `0` an ok fallback value? latest_era = Some(era); loop { let mut db_key = DatabaseKey { diff --git a/util/journaldb/src/overlaydb.rs b/util/journaldb/src/overlaydb.rs index fa7ff04596e..e005449e763 100644 --- a/util/journaldb/src/overlaydb.rs +++ b/util/journaldb/src/overlaydb.rs @@ -135,9 +135,13 @@ impl OverlayDB { /// Get the refs and value of the given key. fn payload(&self, key: &H256) -> Option { - self.backing.get(self.column, key) + match self.backing.get(self.column, key) .expect("Low-level database error. Some issue with your hard disk?") .map(|d| decode(&d)) + { + Some(res) => res.ok(), // REVIEW: decoding errors are discarded here. Ok? + None => None + } } /// Put the refs and value of the given key, possibly deleting it from the db. diff --git a/util/journaldb/src/overlayrecentdb.rs b/util/journaldb/src/overlayrecentdb.rs index fdc178350e6..c7a2972d663 100644 --- a/util/journaldb/src/overlayrecentdb.rs +++ b/util/journaldb/src/overlayrecentdb.rs @@ -186,7 +186,7 @@ impl OverlayRecentDB { let mut earliest_era = None; let mut cumulative_size = 0; if let Some(val) = db.get(col, &LATEST_ERA_KEY).expect("Low-level database error.") { - let mut era = decode::(&val); + let mut era = decode::(&val).unwrap_or(0); // REVIEW: The error is lost here. Log and return? Panic? Or is `0` an ok fallback value? latest_era = Some(era); loop { let mut db_key = DatabaseKey { @@ -195,7 +195,13 @@ impl OverlayRecentDB { }; while let Some(rlp_data) = db.get(col, &encode(&db_key)).expect("Low-level database error.") { trace!("read_overlay: era={}, index={}", era, db_key.index); - let value = decode::(&rlp_data); + let value = match decode::(&rlp_data) { + Ok(v) => v, + Err(e) => { + trace!("read_overlay: Error decoding DatabaseValue err={}, era={}, index{}", e, era, db_key.index); + continue; + } + }; count += value.inserts.len(); let mut inserted_keys = Vec::new(); for (k, v) in value.inserts { diff --git a/util/journaldb/src/refcounteddb.rs b/util/journaldb/src/refcounteddb.rs index bf366faf753..b1c079feb36 100644 --- a/util/journaldb/src/refcounteddb.rs +++ b/util/journaldb/src/refcounteddb.rs @@ -62,17 +62,17 @@ pub struct RefCountedDB { impl RefCountedDB { /// Create a new instance given a `backing` database. - pub fn new(backing: Arc, col: Option) -> RefCountedDB { - let latest_era = backing.get(col, &LATEST_ERA_KEY).expect("Low-level database error.") - .map(|val| decode::(&val)); + pub fn new(backing: Arc, column: Option) -> RefCountedDB { + let latest_era = backing.get(column, &LATEST_ERA_KEY).expect("Low-level database error.") + .map(|val| decode::(&val).unwrap_or(0)); // REVIEW: is `0` an ok value for `latest_era` if the Rlp decoding fails? The alternative is to either panic or change the return value of the function to return `Result`. RefCountedDB { - forward: OverlayDB::new(backing.clone(), col), - backing: backing, + forward: OverlayDB::new(backing.clone(), column), + backing, inserts: vec![], removes: vec![], - latest_era: latest_era, - column: col, + latest_era, + column, } } } From f2f47a48adbb7e7d8e4bbaaf53c25a9447c39589 Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 1 May 2018 20:34:16 +0200 Subject: [PATCH 03/17] Fix ethcore to work with rlp::decode returning Result --- ethcore/src/blockchain/blockchain.rs | 5 ++++- ethcore/src/db.rs | 11 +++++++++-- ethcore/src/encoded.rs | 4 ++-- ethcore/src/engines/tendermint/mod.rs | 6 ++++-- ethcore/src/header.rs | 4 ++-- ethcore/src/snapshot/account.rs | 6 +++--- ethcore/src/snapshot/mod.rs | 6 +++--- ethcore/src/snapshot/tests/helpers.rs | 2 +- ethcore/src/snapshot/tests/state.rs | 2 +- ethcore/src/state/account.rs | 26 ++++++++++++++++++++++---- ethcore/src/state/mod.rs | 5 ++++- ethcore/src/trace/types/flat.rs | 2 +- 12 files changed, 56 insertions(+), 23 deletions(-) diff --git a/ethcore/src/blockchain/blockchain.rs b/ethcore/src/blockchain/blockchain.rs index 6ad8bf8111a..2e1b9848753 100644 --- a/ethcore/src/blockchain/blockchain.rs +++ b/ethcore/src/blockchain/blockchain.rs @@ -430,7 +430,10 @@ impl<'a> Iterator for EpochTransitionIter<'a> { return None } - let transitions: EpochTransitions = ::rlp::decode(&val[..]); + let transitions: EpochTransitions = match ::rlp::decode(&val[..]) { + Ok(decoded) => decoded, + Err(_) => return None + }; // if there are multiple candidates, at most one will be on the // canon chain. diff --git a/ethcore/src/db.rs b/ethcore/src/db.rs index d11adc7710d..603517d5bc2 100644 --- a/ethcore/src/db.rs +++ b/ethcore/src/db.rs @@ -218,11 +218,18 @@ impl Writable for DBTransaction { } impl Readable for KVDB { - fn read(&self, col: Option, key: &Key) -> Option where T: rlp::Decodable, R: Deref { + fn read(&self, col: Option, key: &Key) -> Option + where T: rlp::Decodable, R: Deref { let result = self.get(col, &key.key()); match result { - Ok(option) => option.map(|v| rlp::decode(&v)), + Ok(option) => option.map(|v| match rlp::decode(&v){ + Ok(decoded_rlp) => decoded_rlp, + //REVIEW: Should we panic here? + Err(decode_err) => { + panic!("decoding db value failed, key={:?}, err={}", &key.key() as &[u8], decode_err) + } + }), Err(err) => { panic!("db get failed, key: {:?}, err: {:?}", &key.key() as &[u8], err); } diff --git a/ethcore/src/encoded.rs b/ethcore/src/encoded.rs index 1f627666a90..fb0e05ce8cc 100644 --- a/ethcore/src/encoded.rs +++ b/ethcore/src/encoded.rs @@ -48,7 +48,7 @@ impl Header { pub fn new(encoded: Vec) -> Self { Header(encoded) } /// Upgrade this encoded view to a fully owned `Header` object. - pub fn decode(&self) -> FullHeader { ::rlp::decode(&self.0) } + pub fn decode(&self) -> FullHeader { ::rlp::decode(&self.0).unwrap_or(FullHeader::default()) } // REVIEW: is the default ok here? Useful? The real fix would be to make `decode` return a `Result` as well. /// Get a borrowed header view onto the data. #[inline] @@ -205,7 +205,7 @@ impl Block { pub fn header_view(&self) -> HeaderView { self.view().header_view() } /// Decode to a full block. - pub fn decode(&self) -> FullBlock { ::rlp::decode(&self.0) } + pub fn decode(&self) -> FullBlock { ::rlp::decode(&self.0).unwrap_or(FullBlock::default()) } /// Decode the header. pub fn decode_header(&self) -> FullHeader { self.view().rlp().val_at(0) } diff --git a/ethcore/src/engines/tendermint/mod.rs b/ethcore/src/engines/tendermint/mod.rs index 289beaad0cc..d6ad75d2e9a 100644 --- a/ethcore/src/engines/tendermint/mod.rs +++ b/ethcore/src/engines/tendermint/mod.rs @@ -142,8 +142,10 @@ impl super::EpochVerifier for EpochVerifier } fn check_finality_proof(&self, proof: &[u8]) -> Option> { - let header: Header = ::rlp::decode(proof); - self.verify_light(&header).ok().map(|_| vec![header.hash()]) + match ::rlp::decode(proof) { + Ok(header) => self.verify_light(&header).ok().map(|_| vec![header.hash()]), + Err(_) => None // REVIEW: log perhaps? Not sure what the policy is. + } } } diff --git a/ethcore/src/header.rs b/ethcore/src/header.rs index a31aa029b31..ba71eb30497 100644 --- a/ethcore/src/header.rs +++ b/ethcore/src/header.rs @@ -398,7 +398,7 @@ mod tests { let nonce = "88ab4e252a7e8c2a23".from_hex().unwrap(); let nonce_decoded = "ab4e252a7e8c2a23".from_hex().unwrap(); - let header: Header = rlp::decode(&header_rlp); + let header: Header = rlp::decode(&header_rlp).expect("error decoding header"); let seal_fields = header.seal.clone(); assert_eq!(seal_fields.len(), 2); assert_eq!(seal_fields[0], mix_hash); @@ -415,7 +415,7 @@ mod tests { // that's rlp of block header created with ethash engine. let header_rlp = "f901f9a0d405da4e66f1445d455195229624e133f5baafe72b5cf7b3c36c12c8146e98b7a01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347948888f1f195afa192cfee860698584c030f4c9db1a05fb2b4bfdef7b314451cb138a534d225c922fc0e5fbe25e451142732c3e25c25a088d2ec6b9860aae1a2c3b299f72b6a5d70d7f7ba4722c78f2c49ba96273c2158a007c6fdfa8eea7e86b81f5b0fc0f78f90cc19f4aa60d323151e0cac660199e9a1b90100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008302008003832fefba82524d84568e932a80a0a0349d8c3df71f1a48a9df7d03fd5f14aeee7d91332c009ecaff0a71ead405bd88ab4e252a7e8c2a23".from_hex().unwrap(); - let header: Header = rlp::decode(&header_rlp); + let header: Header = rlp::decode(&header_rlp).expect("error decoding header"); let encoded_header = rlp::encode(&header).into_vec(); assert_eq!(header_rlp, encoded_header); diff --git a/ethcore/src/snapshot/account.rs b/ethcore/src/snapshot/account.rs index 6c9e0f3d6e8..49f45136e9b 100644 --- a/ethcore/src/snapshot/account.rs +++ b/ethcore/src/snapshot/account.rs @@ -236,7 +236,7 @@ mod tests { }; let thin_rlp = ::rlp::encode(&account); - assert_eq!(::rlp::decode::(&thin_rlp), account); + assert_eq!(::rlp::decode::(&thin_rlp).unwrap(), account); let fat_rlps = to_fat_rlps(&keccak(&addr), &account, &AccountDB::new(db.as_hashdb(), &addr), &mut Default::default(), usize::max_value(), usize::max_value()).unwrap(); let fat_rlp = Rlp::new(&fat_rlps[0]).at(1).unwrap(); @@ -261,7 +261,7 @@ mod tests { }; let thin_rlp = ::rlp::encode(&account); - assert_eq!(::rlp::decode::(&thin_rlp), account); + assert_eq!(::rlp::decode::(&thin_rlp).unwrap(), account); let fat_rlp = to_fat_rlps(&keccak(&addr), &account, &AccountDB::new(db.as_hashdb(), &addr), &mut Default::default(), usize::max_value(), usize::max_value()).unwrap(); let fat_rlp = Rlp::new(&fat_rlp[0]).at(1).unwrap(); @@ -286,7 +286,7 @@ mod tests { }; let thin_rlp = ::rlp::encode(&account); - assert_eq!(::rlp::decode::(&thin_rlp), account); + assert_eq!(::rlp::decode::(&thin_rlp).unwrap(), account); let fat_rlps = to_fat_rlps(&keccak(addr), &account, &AccountDB::new(db.as_hashdb(), &addr), &mut Default::default(), 500, 1000).unwrap(); let mut root = KECCAK_NULL_RLP; diff --git a/ethcore/src/snapshot/mod.rs b/ethcore/src/snapshot/mod.rs index fbf0c5ca5ec..94236e9e95d 100644 --- a/ethcore/src/snapshot/mod.rs +++ b/ethcore/src/snapshot/mod.rs @@ -281,7 +281,7 @@ pub fn chunk_state<'a>(db: &HashDB, root: &H256, writer: &Mutex(rlp).storage_root); + known_storage_roots.insert(*hash, ::rlp::decode::(rlp)?.storage_root); } if let Some(&(ref hash, ref rlp)) = out_chunk.iter().next() { - known_storage_roots.insert(*hash, ::rlp::decode::(rlp).storage_root); + known_storage_roots.insert(*hash, ::rlp::decode::(rlp)?.storage_root); } Ok(status) } diff --git a/ethcore/src/snapshot/tests/helpers.rs b/ethcore/src/snapshot/tests/helpers.rs index 51f417149bf..067a3abab07 100644 --- a/ethcore/src/snapshot/tests/helpers.rs +++ b/ethcore/src/snapshot/tests/helpers.rs @@ -75,7 +75,7 @@ impl StateProducer { // sweep once to alter storage tries. for &mut (ref mut address_hash, ref mut account_data) in &mut accounts_to_modify { - let mut account: BasicAccount = ::rlp::decode(&*account_data); + let mut account: BasicAccount = ::rlp::decode(&*account_data).expect("error decoding basic account"); let acct_db = AccountDBMut::from_hash(db, *address_hash); fill_storage(acct_db, &mut account.storage_root, &mut self.storage_seed); *account_data = DBValue::from_vec(::rlp::encode(&account).into_vec()); diff --git a/ethcore/src/snapshot/tests/state.rs b/ethcore/src/snapshot/tests/state.rs index f17fa7dde5a..05926a7e662 100644 --- a/ethcore/src/snapshot/tests/state.rs +++ b/ethcore/src/snapshot/tests/state.rs @@ -114,7 +114,7 @@ fn get_code_from_prev_chunk() { // first one will have code inlined, // second will just have its hash. let thin_rlp = acc_stream.out(); - let acc: BasicAccount = ::rlp::decode(&thin_rlp); + let acc: BasicAccount = ::rlp::decode(&thin_rlp).expect("error decoding basic account"); let mut make_chunk = |acc, hash| { let mut db = MemoryDB::new(); diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index f9c8f258e60..d7802af8a49 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -145,8 +145,13 @@ impl Account { /// Create a new account from RLP. pub fn from_rlp(rlp: &[u8]) -> Account { - let basic: BasicAccount = ::rlp::decode(rlp); - basic.into() + match ::rlp::decode::(rlp) { + Ok(basic_account) => basic_account.into(), + Err(e) => { + error!("from_rlp, Rlp decoding error={}",e); + panic!("from_rlp, Rlp decoding error={}",e) + } + } } /// Create a new contract account. @@ -203,7 +208,14 @@ impl Account { } let db = SecTrieDB::new(db, &self.storage_root)?; - let item: U256 = db.get_with(key, ::rlp::decode)?.unwrap_or_else(U256::zero); + let unwrapping_decoder: fn(&[u8]) -> U256 = |bytes: &[u8]| { + match ::rlp::decode(bytes) { + Ok(u256) => u256, + Err(_) => U256::zero() + } + }; + + let item: U256 = db.get_with(key, unwrapping_decoder)?.unwrap_or_else(U256::zero); let value: H256 = item.into(); self.storage_cache.borrow_mut().insert(key.clone(), value.clone()); Ok(value) @@ -478,7 +490,13 @@ impl Account { let trie = TrieDB::new(db, &self.storage_root)?; let item: U256 = { - let query = (&mut recorder, ::rlp::decode); + let unwrapping_decoder: fn(&[u8]) -> U256 = |bytes: &[u8]| { + match ::rlp::decode(bytes) { + Ok(u256) => u256, + Err(_) => U256::zero() + } + }; + let query = (&mut recorder, unwrapping_decoder); trie.get_with(&storage_key, query)?.unwrap_or_else(U256::zero) }; diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 9fd3fd8525e..8ed63e2a349 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -1039,7 +1039,10 @@ impl State { let mut recorder = Recorder::new(); let trie = TrieDB::new(self.db.as_hashdb(), &self.root)?; let maybe_account: Option = { - let query = (&mut recorder, ::rlp::decode); + let panicky_decoder = |bytes: &[u8]| { + ::rlp::decode(bytes).expect(&format!("prove_account, could not query trie for account key={}", &account_key)) + }; + let query = (&mut recorder, panicky_decoder); trie.get_with(&account_key, query)? }; let account = maybe_account.unwrap_or_else(|| BasicAccount { diff --git a/ethcore/src/trace/types/flat.rs b/ethcore/src/trace/types/flat.rs index e2746ca7f7d..00cf517df80 100644 --- a/ethcore/src/trace/types/flat.rs +++ b/ethcore/src/trace/types/flat.rs @@ -244,7 +244,7 @@ mod tests { ]); let encoded = ::rlp::encode(&block_traces); - let decoded = ::rlp::decode(&encoded); + let decoded = ::rlp::decode(&encoded).expect("error decoding block traces"); assert_eq!(block_traces, decoded); } } From ae8483d42e4b17dcd53928ab7fdba240ab836324 Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 2 May 2018 10:07:50 +0200 Subject: [PATCH 04/17] Light client handles rlp::decode returning Result --- ethcore/light/src/client/header_chain.rs | 53 ++++++++++++++++++------ ethcore/light/src/net/request_credits.rs | 2 +- ethcore/light/src/types/request/mod.rs | 2 +- 3 files changed, 43 insertions(+), 14 deletions(-) diff --git a/ethcore/light/src/client/header_chain.rs b/ethcore/light/src/client/header_chain.rs index abcb04c3662..d796faaf6fa 100644 --- a/ethcore/light/src/client/header_chain.rs +++ b/ethcore/light/src/client/header_chain.rs @@ -228,7 +228,7 @@ impl HeaderChain { let decoded_header = spec.genesis_header(); let chain = if let Some(current) = db.get(col, CURRENT_KEY)? { - let curr : BestAndLatest = ::rlp::decode(¤t); + let curr : BestAndLatest = ::rlp::decode(¤t)?; let mut cur_number = curr.latest_num; let mut candidates = BTreeMap::new(); @@ -236,7 +236,7 @@ impl HeaderChain { // load all era entries, referenced headers within them, // and live epoch proofs. while let Some(entry) = db.get(col, era_key(cur_number).as_bytes())? { - let entry: Entry = ::rlp::decode(&entry); + let entry: Entry = ::rlp::decode(&entry)?; trace!(target: "chain", "loaded header chain entry for era {} with {} candidates", cur_number, entry.candidates.len()); @@ -524,7 +524,16 @@ impl HeaderChain { None } Ok(None) => panic!("stored candidates always have corresponding headers; qed"), - Ok(Some(header)) => Some((epoch_transition, ::rlp::decode(&header))), + Ok(Some(header)) => { + match ::rlp::decode(&header) { + Ok(decoded_header) => Some((epoch_transition, decoded_header)), + Err(e) => { + warn!(target: "chain", "Error decoding header: {}\n", e); + None + } + } + + }, }; } } @@ -591,7 +600,7 @@ impl HeaderChain { in an inconsistent state", h_num); ErrorKind::Database(msg.into()) })?; - ::rlp::decode(&bytes) + ::rlp::decode(&bytes)? }; let total_difficulty = entry.candidates.iter() @@ -604,9 +613,9 @@ impl HeaderChain { .total_difficulty; break Ok(Some(SpecHardcodedSync { - header: header, - total_difficulty: total_difficulty, - chts: chts, + header, + total_difficulty, + chts, })); }, None => { @@ -742,7 +751,17 @@ impl HeaderChain { /// so including it within a CHT would be redundant. pub fn cht_root(&self, n: usize) -> Option { match self.db.get(self.col, cht_key(n as u64).as_bytes()) { - Ok(val) => val.map(|x| ::rlp::decode(&x)), + Ok(db_fetch) => { + db_fetch.map_or(None, |bytes| { + match ::rlp::decode(&bytes) { + Ok(h256) => Some(h256), + Err(e) => { + warn!(target: "chain", "Error decoding data from database: {}", e); + None + } + } + }) + }, Err(e) => { warn!(target: "chain", "Error reading from database: {}", e); None @@ -793,7 +812,17 @@ impl HeaderChain { pub fn pending_transition(&self, hash: H256) -> Option { let key = pending_transition_key(hash); match self.db.get(self.col, &*key) { - Ok(val) => val.map(|x| ::rlp::decode(&x)), + Ok(db_fetch) => { + db_fetch.map_or(None, |bytes| { + match ::rlp::decode(&bytes) { + Ok(pending_transition) => Some(pending_transition), + Err(e) => { + warn!(target: "chain", "Error decoding data from database: {}", e); + None + } + } + }) + }, Err(e) => { warn!(target: "chain", "Error reading from database: {}", e); None @@ -1192,7 +1221,7 @@ mod tests { let cache = Arc::new(Mutex::new(Cache::new(Default::default(), Duration::from_secs(6 * 3600)))); - let chain = HeaderChain::new(db.clone(), None, &spec, cache, HardcodedSync::Allow).unwrap(); + let chain = HeaderChain::new(db.clone(), None, &spec, cache, HardcodedSync::Allow).expect("failed to instantiate a new HeaderChain"); let mut parent_hash = genesis_header.hash(); let mut rolling_timestamp = genesis_header.timestamp(); @@ -1211,14 +1240,14 @@ mod tests { parent_hash = header.hash(); let mut tx = db.transaction(); - let pending = chain.insert(&mut tx, header, None).unwrap(); + let pending = chain.insert(&mut tx, header, None).expect("failed inserting a transaction"); db.write(tx).unwrap(); chain.apply_pending(pending); rolling_timestamp += 10; } - let hardcoded_sync = chain.read_hardcoded_sync().unwrap().unwrap(); + let hardcoded_sync = chain.read_hardcoded_sync().expect("failed reading hardcoded sync").expect("failed unwrapping hardcoded sync"); assert_eq!(hardcoded_sync.chts.len(), 3); assert_eq!(hardcoded_sync.total_difficulty, total_difficulty); let decoded: Header = hardcoded_sync.header.decode(); diff --git a/ethcore/light/src/net/request_credits.rs b/ethcore/light/src/net/request_credits.rs index abe609dabbd..29570b613cf 100644 --- a/ethcore/light/src/net/request_credits.rs +++ b/ethcore/light/src/net/request_credits.rs @@ -407,7 +407,7 @@ mod tests { let costs = CostTable::default(); let serialized = ::rlp::encode(&costs); - let new_costs: CostTable = ::rlp::decode(&*serialized); + let new_costs: CostTable = ::rlp::decode(&*serialized).unwrap(); assert_eq!(costs, new_costs); } diff --git a/ethcore/light/src/types/request/mod.rs b/ethcore/light/src/types/request/mod.rs index 8d911d3f555..bda992df975 100644 --- a/ethcore/light/src/types/request/mod.rs +++ b/ethcore/light/src/types/request/mod.rs @@ -1642,7 +1642,7 @@ mod tests { { // check as single value. let bytes = ::rlp::encode(&val); - let new_val: T = ::rlp::decode(&bytes); + let new_val: T = ::rlp::decode(&bytes).unwrap(); assert_eq!(val, new_val); // check as list containing single value. From af4970afed99eb9f371342250d177b10afc32245 Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 2 May 2018 10:34:30 +0200 Subject: [PATCH 05/17] Fix tests in rlp_derive --- util/rlp_derive/tests/rlp.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/util/rlp_derive/tests/rlp.rs b/util/rlp_derive/tests/rlp.rs index c873805247d..ba51309146e 100644 --- a/util/rlp_derive/tests/rlp.rs +++ b/util/rlp_derive/tests/rlp.rs @@ -24,7 +24,7 @@ fn test_encode_foo() { let out = encode(&foo).into_vec(); assert_eq!(out, expected); - let decoded = decode(&expected); + let decoded = decode(&expected).expect("decode failure"); assert_eq!(foo, decoded); } @@ -38,7 +38,7 @@ fn test_encode_foo_wrapper() { let out = encode(&foo).into_vec(); assert_eq!(out, expected); - let decoded = decode(&expected); + let decoded = decode(&expected).expect("decode failure"); assert_eq!(foo, decoded); } From ffd7a65cb344068d8fbb0b8ceacd7d1933dad17b Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 2 May 2018 10:53:59 +0200 Subject: [PATCH 06/17] Fix tests --- ethcore/transaction/src/transaction.rs | 5 +++-- ethcore/types/src/receipt.rs | 4 ++-- ethcore/vm/src/call_type.rs | 2 +- rpc/src/v1/tests/mocked/eth.rs | 3 ++- whisper/src/message.rs | 4 ++-- 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/ethcore/transaction/src/transaction.rs b/ethcore/transaction/src/transaction.rs index 571dec3faeb..6152e61acb6 100644 --- a/ethcore/transaction/src/transaction.rs +++ b/ethcore/transaction/src/transaction.rs @@ -576,7 +576,8 @@ mod tests { #[test] fn sender_test() { - let t: UnverifiedTransaction = rlp::decode(&::rustc_hex::FromHex::from_hex("f85f800182520894095e7baea6a6c7c4c2dfeb977efac326af552d870a801ba048b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353a0efffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804").unwrap()); + let bytes = ::rustc_hex::FromHex::from_hex("f85f800182520894095e7baea6a6c7c4c2dfeb977efac326af552d870a801ba048b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353a0efffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804").unwrap(); + let t: UnverifiedTransaction = rlp::decode(&bytes).expect("decoding UnverifiedTransaction failed"); assert_eq!(t.data, b""); assert_eq!(t.gas, U256::from(0x5208u64)); assert_eq!(t.gas_price, U256::from(0x01u64)); @@ -645,7 +646,7 @@ mod tests { use rustc_hex::FromHex; let test_vector = |tx_data: &str, address: &'static str| { - let signed = rlp::decode(&FromHex::from_hex(tx_data).unwrap()); + let signed = rlp::decode(&FromHex::from_hex(tx_data).unwrap()).expect("decoding tx data failed"); let signed = SignedTransaction::new(signed).unwrap(); assert_eq!(signed.sender(), address.into()); println!("chainid: {:?}", signed.chain_id()); diff --git a/ethcore/types/src/receipt.rs b/ethcore/types/src/receipt.rs index c1defbc151f..8846d27c027 100644 --- a/ethcore/types/src/receipt.rs +++ b/ethcore/types/src/receipt.rs @@ -193,7 +193,7 @@ mod tests { ); let encoded = ::rlp::encode(&r); assert_eq!(&encoded[..], &expected[..]); - let decoded: Receipt = ::rlp::decode(&encoded); + let decoded: Receipt = ::rlp::decode(&encoded).expect("decoding receipt failed"); assert_eq!(decoded, r); } @@ -211,7 +211,7 @@ mod tests { ); let encoded = ::rlp::encode(&r); assert_eq!(&encoded[..], &expected[..]); - let decoded: Receipt = ::rlp::decode(&encoded); + let decoded: Receipt = ::rlp::decode(&encoded).expect("decoding receipt failed"); assert_eq!(decoded, r); } } diff --git a/ethcore/vm/src/call_type.rs b/ethcore/vm/src/call_type.rs index 83260825f3c..dc00b2b8392 100644 --- a/ethcore/vm/src/call_type.rs +++ b/ethcore/vm/src/call_type.rs @@ -64,7 +64,7 @@ mod tests { fn should_encode_and_decode_call_type() { let original = CallType::Call; let encoded = encode(&original); - let decoded = decode(&encoded); + let decoded = decode(&encoded).expect("failure decoding CallType"); assert_eq!(original, decoded); } } diff --git a/rpc/src/v1/tests/mocked/eth.rs b/rpc/src/v1/tests/mocked/eth.rs index badf5bff728..39a2e842db1 100644 --- a/rpc/src/v1/tests/mocked/eth.rs +++ b/rpc/src/v1/tests/mocked/eth.rs @@ -566,7 +566,8 @@ fn rpc_eth_pending_transaction_by_hash() { let tester = EthTester::default(); { - let tx = rlp::decode(&FromHex::from_hex("f85f800182520894095e7baea6a6c7c4c2dfeb977efac326af552d870a801ba048b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353a0efffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804").unwrap()); + let bytes = FromHex::from_hex("f85f800182520894095e7baea6a6c7c4c2dfeb977efac326af552d870a801ba048b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353a0efffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804").unwrap(); + let tx = rlp::decode(&bytes).expect("decoding failure"); let tx = SignedTransaction::new(tx).unwrap(); tester.miner.pending_transactions.lock().insert(H256::zero(), tx); } diff --git a/whisper/src/message.rs b/whisper/src/message.rs index fbf2faf3fdf..d0de9af4b5a 100644 --- a/whisper/src/message.rs +++ b/whisper/src/message.rs @@ -446,7 +446,7 @@ mod tests { }; let encoded = ::rlp::encode(&envelope); - let decoded = ::rlp::decode(&encoded); + let decoded = ::rlp::decode(&encoded).expect("failure decoding Envelope"); assert_eq!(envelope, decoded) } @@ -462,7 +462,7 @@ mod tests { }; let encoded = ::rlp::encode(&envelope); - let decoded = ::rlp::decode(&encoded); + let decoded = ::rlp::decode(&encoded).expect("failure decoding Envelope"); assert_eq!(envelope, decoded) } From 6eac3a2dd087f476565d73f2ff6e6b3e32566d28 Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 2 May 2018 11:18:30 +0200 Subject: [PATCH 07/17] Cleanup --- ethcore/src/state/account.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index d7802af8a49..30036c63fa7 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -148,6 +148,7 @@ impl Account { match ::rlp::decode::(rlp) { Ok(basic_account) => basic_account.into(), Err(e) => { + // REVIEW: Is panicking ok here? I'd like to change the return value here to `Result` but not sure if that's desirable or the right time. error!("from_rlp, Rlp decoding error={}",e); panic!("from_rlp, Rlp decoding error={}",e) } @@ -207,14 +208,7 @@ impl Account { return Ok(value); } let db = SecTrieDB::new(db, &self.storage_root)?; - - let unwrapping_decoder: fn(&[u8]) -> U256 = |bytes: &[u8]| { - match ::rlp::decode(bytes) { - Ok(u256) => u256, - Err(_) => U256::zero() - } - }; - + let unwrapping_decoder = |bytes:&[u8]| ::rlp::decode(&bytes).unwrap_or_else(|_| U256::zero()); let item: U256 = db.get_with(key, unwrapping_decoder)?.unwrap_or_else(U256::zero); let value: H256 = item.into(); self.storage_cache.borrow_mut().insert(key.clone(), value.clone()); From 23f45976b2bee811e0b0589de9e05b48989625ee Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 2 May 2018 12:08:40 +0200 Subject: [PATCH 08/17] cleanup --- ethcore/src/state/account.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index 30036c63fa7..973f94ccfd3 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -484,12 +484,7 @@ impl Account { let trie = TrieDB::new(db, &self.storage_root)?; let item: U256 = { - let unwrapping_decoder: fn(&[u8]) -> U256 = |bytes: &[u8]| { - match ::rlp::decode(bytes) { - Ok(u256) => u256, - Err(_) => U256::zero() - } - }; + let unwrapping_decoder = |bytes:&[u8]| ::rlp::decode(bytes).unwrap_or_else(|_| U256::zero() ); let query = (&mut recorder, unwrapping_decoder); trie.get_with(&storage_key, query)?.unwrap_or_else(U256::zero) }; From 40eed165a00d350dbc057487e0710c480eb70c7b Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 2 May 2018 12:20:26 +0200 Subject: [PATCH 09/17] Allow panic rather than breaking out of iterator --- ethcore/src/blockchain/blockchain.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/ethcore/src/blockchain/blockchain.rs b/ethcore/src/blockchain/blockchain.rs index 2e1b9848753..6a569fb4974 100644 --- a/ethcore/src/blockchain/blockchain.rs +++ b/ethcore/src/blockchain/blockchain.rs @@ -430,10 +430,7 @@ impl<'a> Iterator for EpochTransitionIter<'a> { return None } - let transitions: EpochTransitions = match ::rlp::decode(&val[..]) { - Ok(decoded) => decoded, - Err(_) => return None - }; + let transitions: EpochTransitions = ::rlp::decode(&val[..]).expect("decode error: the db is corrupted or the data structure has changed"); // if there are multiple candidates, at most one will be on the // canon chain. @@ -457,7 +454,7 @@ impl<'a> Iterator for EpochTransitionIter<'a> { impl BlockChain { /// Create new instance of blockchain from given Genesis. pub fn new(config: Config, genesis: &[u8], db: Arc) -> BlockChain { - // 400 is the avarage size of the key + // 400 is the average size of the key let cache_man = CacheManager::new(config.pref_cache_size, config.max_cache_size, 400); let mut bc = BlockChain { From c8b5c16f8568a2dabe3fd1aab19c7a35cbce81f2 Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 3 May 2018 14:16:50 +0200 Subject: [PATCH 10/17] Let decoding failures when reading from disk blow up --- ethcore/light/src/client/header_chain.rs | 34 +++--------------------- 1 file changed, 3 insertions(+), 31 deletions(-) diff --git a/ethcore/light/src/client/header_chain.rs b/ethcore/light/src/client/header_chain.rs index d796faaf6fa..af4bbcaeb69 100644 --- a/ethcore/light/src/client/header_chain.rs +++ b/ethcore/light/src/client/header_chain.rs @@ -524,15 +524,7 @@ impl HeaderChain { None } Ok(None) => panic!("stored candidates always have corresponding headers; qed"), - Ok(Some(header)) => { - match ::rlp::decode(&header) { - Ok(decoded_header) => Some((epoch_transition, decoded_header)), - Err(e) => { - warn!(target: "chain", "Error decoding header: {}\n", e); - None - } - } - + Ok(Some(header)) => ::rlp::decode(&header).expect("decoding value from db failed"), }, }; } @@ -751,17 +743,7 @@ impl HeaderChain { /// so including it within a CHT would be redundant. pub fn cht_root(&self, n: usize) -> Option { match self.db.get(self.col, cht_key(n as u64).as_bytes()) { - Ok(db_fetch) => { - db_fetch.map_or(None, |bytes| { - match ::rlp::decode(&bytes) { - Ok(h256) => Some(h256), - Err(e) => { - warn!(target: "chain", "Error decoding data from database: {}", e); - None - } - } - }) - }, + Ok(db_fetch) => db_fetch.map(|bytes| ::rlp::decode(bytes).expect("decoding value from db failed")), Err(e) => { warn!(target: "chain", "Error reading from database: {}", e); None @@ -812,17 +794,7 @@ impl HeaderChain { pub fn pending_transition(&self, hash: H256) -> Option { let key = pending_transition_key(hash); match self.db.get(self.col, &*key) { - Ok(db_fetch) => { - db_fetch.map_or(None, |bytes| { - match ::rlp::decode(&bytes) { - Ok(pending_transition) => Some(pending_transition), - Err(e) => { - warn!(target: "chain", "Error decoding data from database: {}", e); - None - } - } - }) - }, + Ok(db_fetch) => db_fetch.map(|bytes| ::rlp::decode(bytes).expect("decoding value from db failed")), Err(e) => { warn!(target: "chain", "Error reading from database: {}", e); None From ea4a6d2a7c42599430b9c9ad2860bef26f2d22f4 Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 3 May 2018 14:52:12 +0200 Subject: [PATCH 11/17] syntax --- ethcore/light/src/client/header_chain.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/ethcore/light/src/client/header_chain.rs b/ethcore/light/src/client/header_chain.rs index af4bbcaeb69..9d419386b5e 100644 --- a/ethcore/light/src/client/header_chain.rs +++ b/ethcore/light/src/client/header_chain.rs @@ -524,8 +524,10 @@ impl HeaderChain { None } Ok(None) => panic!("stored candidates always have corresponding headers; qed"), - Ok(Some(header)) => ::rlp::decode(&header).expect("decoding value from db failed"), - }, + Ok(Some(header)) => Some(( + epoch_transition, + ::rlp::decode(&header).expect("decoding value from db failed") + )), }; } } @@ -743,7 +745,7 @@ impl HeaderChain { /// so including it within a CHT would be redundant. pub fn cht_root(&self, n: usize) -> Option { match self.db.get(self.col, cht_key(n as u64).as_bytes()) { - Ok(db_fetch) => db_fetch.map(|bytes| ::rlp::decode(bytes).expect("decoding value from db failed")), + Ok(db_fetch) => db_fetch.map(|bytes| ::rlp::decode(&bytes).expect("decoding value from db failed")), Err(e) => { warn!(target: "chain", "Error reading from database: {}", e); None @@ -794,7 +796,7 @@ impl HeaderChain { pub fn pending_transition(&self, hash: H256) -> Option { let key = pending_transition_key(hash); match self.db.get(self.col, &*key) { - Ok(db_fetch) => db_fetch.map(|bytes| ::rlp::decode(bytes).expect("decoding value from db failed")), + Ok(db_fetch) => db_fetch.map(|bytes| ::rlp::decode(&bytes).expect("decoding value from db failed")), Err(e) => { warn!(target: "chain", "Error reading from database: {}", e); None From ea17bd746ef211957cc5f7bd1d2f2e5f204c5b3e Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 7 May 2018 08:03:19 +0200 Subject: [PATCH 12/17] Fix the trivial grumbles --- ethcore/light/src/client/header_chain.rs | 6 +++--- ethcore/src/db.rs | 19 +++++-------------- ethcore/src/state/account.rs | 8 ++++---- util/journaldb/src/archivedb.rs | 5 +++-- util/journaldb/src/earlymergedb.rs | 2 +- util/journaldb/src/overlaydb.rs | 8 ++------ util/journaldb/src/overlayrecentdb.rs | 2 +- util/journaldb/src/refcounteddb.rs | 5 +++-- 8 files changed, 22 insertions(+), 33 deletions(-) diff --git a/ethcore/light/src/client/header_chain.rs b/ethcore/light/src/client/header_chain.rs index 9d419386b5e..02a18a60dfe 100644 --- a/ethcore/light/src/client/header_chain.rs +++ b/ethcore/light/src/client/header_chain.rs @@ -228,7 +228,7 @@ impl HeaderChain { let decoded_header = spec.genesis_header(); let chain = if let Some(current) = db.get(col, CURRENT_KEY)? { - let curr : BestAndLatest = ::rlp::decode(¤t)?; + let curr : BestAndLatest = ::rlp::decode(¤t).expect("decoding db value failed"); let mut cur_number = curr.latest_num; let mut candidates = BTreeMap::new(); @@ -236,7 +236,7 @@ impl HeaderChain { // load all era entries, referenced headers within them, // and live epoch proofs. while let Some(entry) = db.get(col, era_key(cur_number).as_bytes())? { - let entry: Entry = ::rlp::decode(&entry)?; + let entry: Entry = ::rlp::decode(&entry).expect("decoding db value failed"); trace!(target: "chain", "loaded header chain entry for era {} with {} candidates", cur_number, entry.candidates.len()); @@ -594,7 +594,7 @@ impl HeaderChain { in an inconsistent state", h_num); ErrorKind::Database(msg.into()) })?; - ::rlp::decode(&bytes)? + ::rlp::decode(&bytes).expect("decoding db value failed") }; let total_difficulty = entry.candidates.iter() diff --git a/ethcore/src/db.rs b/ethcore/src/db.rs index 603517d5bc2..c31bda0a67a 100644 --- a/ethcore/src/db.rs +++ b/ethcore/src/db.rs @@ -220,20 +220,11 @@ impl Writable for DBTransaction { impl Readable for KVDB { fn read(&self, col: Option, key: &Key) -> Option where T: rlp::Decodable, R: Deref { - let result = self.get(col, &key.key()); - - match result { - Ok(option) => option.map(|v| match rlp::decode(&v){ - Ok(decoded_rlp) => decoded_rlp, - //REVIEW: Should we panic here? - Err(decode_err) => { - panic!("decoding db value failed, key={:?}, err={}", &key.key() as &[u8], decode_err) - } - }), - Err(err) => { - panic!("db get failed, key: {:?}, err: {:?}", &key.key() as &[u8], err); - } - } + self.get(col, &key.key()) + .expect(&format!("db get failed, key: {:?}", &key.key() as &[u8])) + .and_then(|v| + rlp::decode(&v).expect("decoding db value failed") + ) } fn exists(&self, col: Option, key: &Key) -> bool where R: Deref { diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index 973f94ccfd3..3c8d404241c 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -208,8 +208,8 @@ impl Account { return Ok(value); } let db = SecTrieDB::new(db, &self.storage_root)?; - let unwrapping_decoder = |bytes:&[u8]| ::rlp::decode(&bytes).unwrap_or_else(|_| U256::zero()); - let item: U256 = db.get_with(key, unwrapping_decoder)?.unwrap_or_else(U256::zero); + let panicky_decoder = |bytes:&[u8]| ::rlp::decode(&bytes).expect("decoding db value failed"); + let item: U256 = db.get_with(key, panicky_decoder)?.unwrap_or_else(U256::zero); let value: H256 = item.into(); self.storage_cache.borrow_mut().insert(key.clone(), value.clone()); Ok(value) @@ -484,8 +484,8 @@ impl Account { let trie = TrieDB::new(db, &self.storage_root)?; let item: U256 = { - let unwrapping_decoder = |bytes:&[u8]| ::rlp::decode(bytes).unwrap_or_else(|_| U256::zero() ); - let query = (&mut recorder, unwrapping_decoder); + let panicky_decoder = |bytes:&[u8]| ::rlp::decode(bytes).expect("decoding db value failed"); + let query = (&mut recorder, panicky_decoder); trie.get_with(&storage_key, query)?.unwrap_or_else(U256::zero) }; diff --git a/util/journaldb/src/archivedb.rs b/util/journaldb/src/archivedb.rs index 11677ef83b7..b58558a332a 100644 --- a/util/journaldb/src/archivedb.rs +++ b/util/journaldb/src/archivedb.rs @@ -46,8 +46,9 @@ pub struct ArchiveDB { impl ArchiveDB { /// Create a new instance from a key-value db. pub fn new(backing: Arc, column: Option) -> ArchiveDB { - let latest_era = backing.get(column, &LATEST_ERA_KEY).expect("Low-level database error.") - .map(|val| decode::(&val).unwrap_or(0)); // REVIEW: is `0` an ok value for `latest_era` if the Rlp decoding fails? The alternative is to either panic or change the return value of the function to return `Result`. + let latest_era = backing.get(column, &LATEST_ERA_KEY) + .expect("Low-level database error.") + .map(|val| decode::(&val).expect("decoding db value failed")); ArchiveDB { overlay: MemoryDB::new(), backing, diff --git a/util/journaldb/src/earlymergedb.rs b/util/journaldb/src/earlymergedb.rs index 5abeeb0c22b..e76cdcd313a 100644 --- a/util/journaldb/src/earlymergedb.rs +++ b/util/journaldb/src/earlymergedb.rs @@ -263,7 +263,7 @@ impl EarlyMergeDB { let mut refs = HashMap::new(); let mut latest_era = None; if let Some(val) = db.get(col, &LATEST_ERA_KEY).expect("Low-level database error.") { - let mut era = decode::(&val).unwrap_or(0); // REVIEW: The error is lost here. Log and return? Panic? Or is `0` an ok fallback value? + let mut era = decode::(&val).expect("decoding db value failed"); latest_era = Some(era); loop { let mut db_key = DatabaseKey { diff --git a/util/journaldb/src/overlaydb.rs b/util/journaldb/src/overlaydb.rs index e005449e763..54d0bb12d76 100644 --- a/util/journaldb/src/overlaydb.rs +++ b/util/journaldb/src/overlaydb.rs @@ -135,13 +135,9 @@ impl OverlayDB { /// Get the refs and value of the given key. fn payload(&self, key: &H256) -> Option { - match self.backing.get(self.column, key) + self.backing.get(self.column, key) .expect("Low-level database error. Some issue with your hard disk?") - .map(|d| decode(&d)) - { - Some(res) => res.ok(), // REVIEW: decoding errors are discarded here. Ok? - None => None - } + .map(|d| decode(&d).expect("decoding db value failed")) } /// Put the refs and value of the given key, possibly deleting it from the db. diff --git a/util/journaldb/src/overlayrecentdb.rs b/util/journaldb/src/overlayrecentdb.rs index c7a2972d663..1d438ec8984 100644 --- a/util/journaldb/src/overlayrecentdb.rs +++ b/util/journaldb/src/overlayrecentdb.rs @@ -186,7 +186,7 @@ impl OverlayRecentDB { let mut earliest_era = None; let mut cumulative_size = 0; if let Some(val) = db.get(col, &LATEST_ERA_KEY).expect("Low-level database error.") { - let mut era = decode::(&val).unwrap_or(0); // REVIEW: The error is lost here. Log and return? Panic? Or is `0` an ok fallback value? + let mut era = decode::(&val).expect("decoding db value failed"); latest_era = Some(era); loop { let mut db_key = DatabaseKey { diff --git a/util/journaldb/src/refcounteddb.rs b/util/journaldb/src/refcounteddb.rs index b1c079feb36..944d81d3733 100644 --- a/util/journaldb/src/refcounteddb.rs +++ b/util/journaldb/src/refcounteddb.rs @@ -63,8 +63,9 @@ pub struct RefCountedDB { impl RefCountedDB { /// Create a new instance given a `backing` database. pub fn new(backing: Arc, column: Option) -> RefCountedDB { - let latest_era = backing.get(column, &LATEST_ERA_KEY).expect("Low-level database error.") - .map(|val| decode::(&val).unwrap_or(0)); // REVIEW: is `0` an ok value for `latest_era` if the Rlp decoding fails? The alternative is to either panic or change the return value of the function to return `Result`. + let latest_era = backing.get(column, &LATEST_ERA_KEY) + .expect("Low-level database error.") + .map(|v| decode::(&v).expect("decoding db value failed")); RefCountedDB { forward: OverlayDB::new(backing.clone(), column), From 7b07b691f319db6b0f3046922576f447f7d05d62 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 7 May 2018 08:50:52 +0200 Subject: [PATCH 13/17] Fix failing tests --- ethcore/src/db.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ethcore/src/db.rs b/ethcore/src/db.rs index c31bda0a67a..a1c7d6b0f5e 100644 --- a/ethcore/src/db.rs +++ b/ethcore/src/db.rs @@ -222,9 +222,8 @@ impl Readable for KVDB { where T: rlp::Decodable, R: Deref { self.get(col, &key.key()) .expect(&format!("db get failed, key: {:?}", &key.key() as &[u8])) - .and_then(|v| - rlp::decode(&v).expect("decoding db value failed") - ) + .map(|v| rlp::decode(&v).expect("decode db value failed") ) + } fn exists(&self, col: Option, key: &Key) -> bool where R: Deref { From 0930b0a6eadedd0bba0feeeeb46d76f794175519 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 7 May 2018 09:39:45 +0200 Subject: [PATCH 14/17] Make Account::from_rlp return Result --- ethcore/src/state/account.rs | 23 +++++++++-------------- ethcore/src/state/mod.rs | 12 ++++++++---- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index 3c8d404241c..ff51738feea 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -21,6 +21,7 @@ use std::sync::Arc; use std::collections::{HashMap, BTreeMap}; use hash::{KECCAK_EMPTY, KECCAK_NULL_RLP, keccak}; use ethereum_types::{H256, U256, Address}; +use error::Error; use hashdb::HashDB; use kvdb::DBValue; use bytes::{Bytes, ToPretty}; @@ -144,16 +145,10 @@ impl Account { } /// Create a new account from RLP. - pub fn from_rlp(rlp: &[u8]) -> Account { - match ::rlp::decode::(rlp) { - Ok(basic_account) => basic_account.into(), - Err(e) => { - // REVIEW: Is panicking ok here? I'd like to change the return value here to `Result` but not sure if that's desirable or the right time. - error!("from_rlp, Rlp decoding error={}",e); - panic!("from_rlp, Rlp decoding error={}",e) - } - } - } + pub fn from_rlp(rlp: &[u8]) -> Result { + ::rlp::decode::(rlp) + .map(|ba| ba.into() ) + .map_err(|e| e.into() ) /// Create a new contract account. /// NOTE: make sure you use `init_code` on this before `commit`ing. @@ -535,7 +530,7 @@ mod tests { a.rlp() }; - let a = Account::from_rlp(&rlp); + let a = Account::from_rlp(&rlp).expect("decoding db value failed"); assert_eq!(*a.storage_root().unwrap(), "c57e1afb758b07f8d2c8f13a3b6e44fa5ff94ab266facc5a4fd3f062426e50b2".into()); assert_eq!(a.storage_at(&db.immutable(), &0x00u64.into()).unwrap(), 0x1234u64.into()); assert_eq!(a.storage_at(&db.immutable(), &0x01u64.into()).unwrap(), H256::default()); @@ -553,10 +548,10 @@ mod tests { a.rlp() }; - let mut a = Account::from_rlp(&rlp); + let mut a = Account::from_rlp(&rlp).expect("decoding db value failed"); assert!(a.cache_code(&db.immutable()).is_some()); - let mut a = Account::from_rlp(&rlp); + let mut a = Account::from_rlp(&rlp).expect("decoding db value failed"); assert_eq!(a.note_code(vec![0x55, 0x44, 0xffu8]), Ok(())); } @@ -616,7 +611,7 @@ mod tests { #[test] fn rlpio() { let a = Account::new(69u8.into(), 0u8.into(), HashMap::new(), Bytes::new()); - let b = Account::from_rlp(&a.rlp()); + let b = Account::from_rlp(&a.rlp()).unwrap(); assert_eq!(a.balance(), b.balance()); assert_eq!(a.nonce(), b.nonce()); assert_eq!(a.code_hash(), b.code_hash()); diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 8ed63e2a349..b9d63a4413b 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -605,7 +605,8 @@ impl State { // account is not found in the global cache, get from the DB and insert into local let db = self.factories.trie.readonly(self.db.as_hashdb(), &self.root).expect(SEC_TRIE_DB_UNWRAP_STR); - let maybe_acc = db.get_with(address, Account::from_rlp)?; + let from_rlp = |b: &[u8]| Account::from_rlp(b).expect("decoding db value failed"); + let maybe_acc = db.get_with(address, from_rlp)?; let r = maybe_acc.as_ref().map_or(Ok(H256::new()), |a| { let account_db = self.factories.accountdb.readonly(self.db.as_hashdb(), a.address_hash(address)); a.storage_at(account_db.as_hashdb(), key) @@ -958,7 +959,8 @@ impl State { // not found in the global cache, get from the DB and insert into local let db = self.factories.trie.readonly(self.db.as_hashdb(), &self.root)?; - let mut maybe_acc = db.get_with(a, Account::from_rlp)?; + let from_rlp = |b: &[u8]| Account::from_rlp(b).expect("decoding db value failed"); + let mut maybe_acc = db.get_with(a, from_rlp)?; if let Some(ref mut account) = maybe_acc.as_mut() { let accountdb = self.factories.accountdb.readonly(self.db.as_hashdb(), account.address_hash(a)); Self::update_account_cache(require, account, &self.db, accountdb.as_hashdb()); @@ -987,7 +989,8 @@ impl State { None => { let maybe_acc = if !self.db.is_known_null(a) { let db = self.factories.trie.readonly(self.db.as_hashdb(), &self.root)?; - AccountEntry::new_clean(db.get_with(a, Account::from_rlp)?) + let from_rlp = |b:&[u8]| { Account::from_rlp(b).expect("decoding db value failed") }; + AccountEntry::new_clean(db.get_with(a, from_rlp)?) } else { AccountEntry::new_clean(None) }; @@ -1064,7 +1067,8 @@ impl State { // TODO: probably could look into cache somehow but it's keyed by // address, not keccak(address). let trie = TrieDB::new(self.db.as_hashdb(), &self.root)?; - let acc = match trie.get_with(&account_key, Account::from_rlp)? { + let from_rlp = |b: &[u8]| Account::from_rlp(b).expect("decoding db value failed"); + let acc = match trie.get_with(&account_key, from_rlp)? { Some(acc) => acc, None => return Ok((Vec::new(), H256::new())), }; From 245ed85d1f2de2ad6c66875e1cc89d7e9dbd4ef5 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 7 May 2018 09:58:32 +0200 Subject: [PATCH 15/17] Syntx, sigh --- ethcore/src/state/account.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index ff51738feea..ff7d70bd3aa 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -147,8 +147,9 @@ impl Account { /// Create a new account from RLP. pub fn from_rlp(rlp: &[u8]) -> Result { ::rlp::decode::(rlp) - .map(|ba| ba.into() ) - .map_err(|e| e.into() ) + .map(|ba| ba.into()) + .map_err(|e| e.into()) + } /// Create a new contract account. /// NOTE: make sure you use `init_code` on this before `commit`ing. From 9cdad30f5587d87f46751546af5e2ce811c4d3c8 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 7 May 2018 10:09:34 +0200 Subject: [PATCH 16/17] Temp-fix for decoding failures --- ethcore/src/encoded.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/ethcore/src/encoded.rs b/ethcore/src/encoded.rs index fb0e05ce8cc..01df386cc2e 100644 --- a/ethcore/src/encoded.rs +++ b/ethcore/src/encoded.rs @@ -24,13 +24,12 @@ //! decoded object where parts like the hash can be saved. use block::Block as FullBlock; -use header::{BlockNumber, Header as FullHeader}; -use transaction::UnverifiedTransaction; - +use ethereum_types::{H256, Bloom, U256, Address}; use hash::keccak; +use header::{BlockNumber, Header as FullHeader}; use heapsize::HeapSizeOf; -use ethereum_types::{H256, Bloom, U256, Address}; use rlp::{Rlp, RlpStream}; +use transaction::UnverifiedTransaction; use views::{self, BlockView, HeaderView, BodyView}; /// Owning header view. @@ -48,7 +47,7 @@ impl Header { pub fn new(encoded: Vec) -> Self { Header(encoded) } /// Upgrade this encoded view to a fully owned `Header` object. - pub fn decode(&self) -> FullHeader { ::rlp::decode(&self.0).unwrap_or(FullHeader::default()) } // REVIEW: is the default ok here? Useful? The real fix would be to make `decode` return a `Result` as well. + pub fn decode(&self) -> FullHeader { ::rlp::decode(&self.0).expect("decoding failure") } /// Get a borrowed header view onto the data. #[inline] @@ -205,7 +204,7 @@ impl Block { pub fn header_view(&self) -> HeaderView { self.view().header_view() } /// Decode to a full block. - pub fn decode(&self) -> FullBlock { ::rlp::decode(&self.0).unwrap_or(FullBlock::default()) } + pub fn decode(&self) -> FullBlock { ::rlp::decode(&self.0).expect("decoding failure") } /// Decode the header. pub fn decode_header(&self) -> FullHeader { self.view().rlp().val_at(0) } From b7d7d5551e3333a76147e396146007ce06d5cb0a Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 8 May 2018 10:15:45 +0200 Subject: [PATCH 17/17] Do not continue reading from the DB when a value could not be read --- util/journaldb/src/overlayrecentdb.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/util/journaldb/src/overlayrecentdb.rs b/util/journaldb/src/overlayrecentdb.rs index 1d438ec8984..2c9ce5cb1dd 100644 --- a/util/journaldb/src/overlayrecentdb.rs +++ b/util/journaldb/src/overlayrecentdb.rs @@ -195,13 +195,7 @@ impl OverlayRecentDB { }; while let Some(rlp_data) = db.get(col, &encode(&db_key)).expect("Low-level database error.") { trace!("read_overlay: era={}, index={}", era, db_key.index); - let value = match decode::(&rlp_data) { - Ok(v) => v, - Err(e) => { - trace!("read_overlay: Error decoding DatabaseValue err={}, era={}, index{}", e, era, db_key.index); - continue; - } - }; + let value = decode::(&rlp_data).expect(&format!("read_overlay: Error decoding DatabaseValue era={}, index{}", era, db_key.index)); count += value.inserts.len(); let mut inserted_keys = Vec::new(); for (k, v) in value.inserts {