Jaden/tipsetconversions

[flodesi]

Summary of changes
Changes introduced in this pull request:

• Remove result when converting from FullTipset to Tipset
• Add From traits to convert from FullTipset to Tipset

Reference issue to close (if applicable)

Closes #398

[CLAassistant]

All committers have signed the CLA.

[austinabell]

Approving because my suggested change is functionally the same, but I think it makes it a bit more readable

[austinabell]

I can't add a suggestion, because a few lines are outside of the changes, but can simplify to:

    pub fn new(mut blocks: Vec<Block>) -> Result<Self, Error> {
        verify_blocks(blocks.iter().map(Block::header))?;

        // sort blocks on creation to allow for more seamless conversions between FullTipset
        // and Tipset
        blocks.sort_by_key(|block| {
            (
                block.header.ticket().vrfproof.clone(),
                block.header.cid().to_bytes(),
            )
        });
        Ok(Self { blocks })
    }

[timvermeulen]

Why aren't we only sorting by the ticket anyway? Aren't those already unique? (I saw that Tipset::new sorts by a tuple as well)

[flodesi]

Ah didn't catch that, will add that change and push commit.

[austinabell]

It is (or was) written into the spec to handle the secondary sorting. I'll try to find again, because quickly looking it is just referring to only sorting by ticket.

[timvermeulen]

If tickets are guaranteed to be unique then it will never hit the second tuple element — if not, then it's indeed needed for a canonical ordering

[dutterbutter]

The spec reads as follows:

The blocks in a tipset are canonically ordered by the lexicographic 
ordering of the bytes in each block’s ticket, breaking ties with the bytes
of the CID of the block itself.

My interpretation of that is that we order based on ticket size and if ticket sizes are equal we check the cid of the block itself.

[austinabell]

Thanks @dutterbutter for finding :) Yeah the comparison can still be improved by not cloning the ticket and only comparing cid bytes when other matches (having some sort function) if you want to take that on now @flodesi

[timvermeulen]

My wording was a bit vague, I was questioning whether block.header.cid().to_bytes() was needed there as well. But you raise a good point, Ticket's ordering is defined by the ordering of its vrfproof so leaving out .vrfproof there will have no impact on the ordering whatsoever.

We unfortunately can't get rid of the .clone() though because of a language limitation (StackOverflow)

[austinabell]

what I'm suggesting is not sorting by keys and just having a sort function, there should be no need to clone the ticket, only cids in the extremely rare case of a ticket match

[timvermeulen]

@flodesi If you call max_by instead of max_by_key, you get in two blocks which lets you compare the tickets without cloning them, and only compare the CIDs if necessary like Austin suggested.