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

Handle rlp decoding Result in patricia trie #8166

Merged
merged 3 commits into from
Mar 22, 2018
Merged

Conversation

ascjones
Copy link
Contributor

Part of #8033, replacing existing usage of Rlp with UntrustedRlp.

Unwraps the result of decoding a node's rlp, assuming valid rlp since it is a local read from the db.

Next step might be to introduce another case to TrieError for a DecoderError to avoid the unwraps.

@ascjones ascjones added the A0-pleasereview 🤓 Pull request needs code review. label Mar 20, 2018
@parity-cla-bot
Copy link

It looks like @ascjones signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added the M4-core ⛓ Core client code / Rust. label Mar 20, 2018
@5chdn 5chdn added this to the 1.11 milestone Mar 20, 2018
Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

nice! I like the fact that printing an invalid trie finally does not panic :) I remember that we had a separate issue for that, but I can't find it right now

// something went wrong.
_ => panic!("Rlp is not valid.")
_ => Err(DecoderError::Custom("Rlp is not valid."))
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe use bail! instead docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

self.fmt_all(Node::decoded(&node), f, deepness)?;
match Node::decoded(&node) {
Ok(n) => self.fmt_all(n, f, deepness)?,
Err(err) => writeln!(f, "ERROR decoding node extension Rlp: {}", err)?,
Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated:
instead of writeln, we should use debug_struct which takes into an account an alternate flag of the formatter which allows pretty-printing

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 21, 2018
@@ -63,7 +63,7 @@ impl<'a> Node<'a> {
// an empty branch index.
Prototype::Data(0) => Ok(Node::Empty),
// something went wrong.
_ => Err(DecoderError::Custom("Rlp is not valid."))
_ => bail!("Rlp is not valid.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, sorry. I thought that this Error is already an error from error_chain. Wouldn't ask for this change if I knew it was not.

@debris
Copy link
Collaborator

debris commented Mar 22, 2018

I reverted this last commit. Sorry, my suggestion was wrong. I'll merge the pr once it's green

@debris debris merged commit d293f94 into master Mar 22, 2018
@debris debris deleted the patricia_trie_rlp branch March 22, 2018 02:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants