-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rlp decode returns Result #8527
Changes from 11 commits
70db4f5
3b6f5c9
f2f47a4
ae8483d
af4970a
ffd7a65
6eac3a2
23f4597
40eed16
c8b5c16
ea4a6d2
ea17bd7
7b07b69
0930b0a
245ed85
9cdad30
b7d7d55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -228,15 +228,15 @@ 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(); | ||
|
||
// 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)?; | ||
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. it should be expect, cause we're reading from the db :) |
||
trace!(target: "chain", "loaded header chain entry for era {} with {} candidates", | ||
cur_number, entry.candidates.len()); | ||
|
||
|
@@ -524,7 +524,10 @@ 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)) => Some(( | ||
epoch_transition, | ||
::rlp::decode(&header).expect("decoding value from db failed") | ||
)), | ||
}; | ||
} | ||
} | ||
|
@@ -591,7 +594,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 +607,9 @@ impl HeaderChain { | |
.total_difficulty; | ||
|
||
break Ok(Some(SpecHardcodedSync { | ||
header: header, | ||
total_difficulty: total_difficulty, | ||
chts: chts, | ||
header, | ||
total_difficulty, | ||
chts, | ||
})); | ||
}, | ||
None => { | ||
|
@@ -742,7 +745,7 @@ impl HeaderChain { | |
/// so including it within a CHT would be redundant. | ||
pub fn cht_root(&self, n: usize) -> Option<H256> { | ||
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(|bytes| ::rlp::decode(&bytes).expect("decoding value from db failed")), | ||
Err(e) => { | ||
warn!(target: "chain", "Error reading from database: {}", e); | ||
None | ||
|
@@ -793,7 +796,7 @@ impl HeaderChain { | |
pub fn pending_transition(&self, hash: H256) -> Option<PendingEpochTransition> { | ||
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(|bytes| ::rlp::decode(&bytes).expect("decoding value from db failed")), | ||
Err(e) => { | ||
warn!(target: "chain", "Error reading from database: {}", e); | ||
None | ||
|
@@ -1192,7 +1195,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 +1214,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(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -218,11 +218,18 @@ impl Writable for DBTransaction { | |
} | ||
|
||
impl<KVDB: KeyValueDB + ?Sized> Readable for KVDB { | ||
fn read<T, R>(&self, col: Option<u32>, key: &Key<T, Target = R>) -> Option<T> where T: rlp::Decodable, R: Deref<Target = [u8]> { | ||
fn read<T, R>(&self, col: Option<u32>, key: &Key<T, Target = R>) -> Option<T> | ||
where T: rlp::Decodable, R: Deref<Target = [u8]> { | ||
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) | ||
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. @debris the situation here is similar to the above, db value read from disc, so panicking is ok. Q: do you prefer me to shorten this up with a 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 believe it could be shortened with: result.and_then(|option| match option {
None => Ok(None),
Some(v) => rlp::decode(&v).map(Some)
}).expect("decoding db value failed) |
||
} | ||
}), | ||
Err(err) => { | ||
panic!("db get failed, key: {:?}, err: {:?}", &key.key() as &[u8], err); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,7 @@ impl Header { | |
pub fn new(encoded: Vec<u8>) -> 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. | ||
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. It should definitely return 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. |
||
|
||
/// 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()) } | ||
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. Same here. Returning 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. |
||
|
||
/// Decode the header. | ||
pub fn decode_header(&self) -> FullHeader { self.view().rlp().val_at(0) } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,8 +142,10 @@ impl <F> super::EpochVerifier<EthereumMachine> for EpochVerifier<F> | |
} | ||
|
||
fn check_finality_proof(&self, proof: &[u8]) -> Option<Vec<H256>> { | ||
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. | ||
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. This is a good question. According to interface documentation: /// Returns `Some(hashes)` if the proof proves finality of these hashes.
/// Returns `None` if the proof doesn't prove anything.
it should be ok to return |
||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,8 +145,14 @@ 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::<BasicAccount>(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<Account, SomeError>` but not sure if that's desirable or the right time. | ||
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 believe it's not ok, cause we don't know where decoded slice of bytes comes from. It's better to change it to |
||
error!("from_rlp, Rlp decoding error={}",e); | ||
panic!("from_rlp, Rlp decoding error={}",e) | ||
} | ||
} | ||
} | ||
|
||
/// Create a new contract account. | ||
|
@@ -202,8 +208,8 @@ impl Account { | |
return Ok(value); | ||
} | ||
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 = |bytes:&[u8]| ::rlp::decode(&bytes).unwrap_or_else(|_| U256::zero()); | ||
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 believe we should have |
||
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 +484,8 @@ impl Account { | |
|
||
let trie = TrieDB::new(db, &self.storage_root)?; | ||
let item: U256 = { | ||
let query = (&mut recorder, ::rlp::decode); | ||
let unwrapping_decoder = |bytes:&[u8]| ::rlp::decode(bytes).unwrap_or_else(|_| U256::zero() ); | ||
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. same here :) |
||
let query = (&mut recorder, unwrapping_decoder); | ||
trie.get_with(&storage_key, query)?.unwrap_or_else(U256::zero) | ||
}; | ||
|
||
|
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.
it should be expect, cause we're reading from the db :)