-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rlp decode returns Result #8527
Changes from 6 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,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) { | ||
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 value was fetched from the database. It should never be invalid, therefore we should use |
||
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<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_or(None, |bytes| { | ||
match ::rlp::decode(&bytes) { | ||
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 |
||
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<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_or(None, |bytes| { | ||
match ::rlp::decode(&bytes) { | ||
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. and here |
||
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(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
}; | ||
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. Shouldn't break out of the iterator with invalid RLP. Since we wrote the |
||
|
||
// if there are multiple candidates, at most one will be on the | ||
// canon chain. | ||
|
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 |
||
} | ||
} | ||
} | ||
|
||
|
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 :)