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

Fix RLP encoding for types recursively calling RlpStream::append #4362

Merged
merged 4 commits into from
Feb 6, 2017

Conversation

maciejhirsz
Copy link
Contributor

Fixes #4356.

@maciejhirsz maciejhirsz added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jan 30, 2017
@@ -194,7 +194,7 @@ impl Encodable for ConsensusMessage {

pub fn message_info_rlp(vote_step: &VoteStep, block_hash: Option<BlockHash>) -> Bytes {
// TODO: figure out whats wrong with nested list encoding
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this resolve this TODO?

Copy link
Contributor Author

@maciejhirsz maciejhirsz Jan 31, 2017

Choose a reason for hiding this comment

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

@arkpar yap, will remove it

@gavofyork
Copy link
Contributor

the changes made seem a little subtle - is there any reason these mistakes won't be repeated in the future?

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. Z1-question 🙋‍♀️ Issue is a question. Closer should answer. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 4, 2017
@maciejhirsz maciejhirsz merged commit 127baed into master Feb 6, 2017
@maciejhirsz maciejhirsz deleted the mh-tendermintrlpfix branch February 6, 2017 10:13
@maciejhirsz
Copy link
Contributor Author

@gavofyork there is more insight into it in #4356. In short - there are use cases when calling append inside rlp_append are valid - namely when the type implementing Encodable creates a list with subitems.

The long term solution is to refactor how RlpStream works. I'd suggest that we only ever implement Encodable for non-list types, and do list by generic array / tuple / Iterator impls, to completely avoid the problem of having to count how many items have been appended inside RlpStream.

@rphmeier
Copy link
Contributor

rphmeier commented Feb 6, 2017

do list by generic array / tuple / Iterator impls

This will be easier said than done as items in RLP lists often have different types. For instance, how would you write a block header as a Vec<T> or Iterator<Item=T>?

@maciejhirsz
Copy link
Contributor Author

Can use macros to generate generic impl for all tuple sizes up to some limit (20? 32?), such as:

impl<A, B, C, D...> Encodable for (A, B, C, D...) where
    A: Encodable,
    B: Encodable,
    C: Encodable,
    D: Encodable,
    ...
{ ... }

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. Z1-question 🙋‍♀️ Issue is a question. Closer should answer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants