Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TipSet orders blocks by ticket, is immutable value type #2837

Merged
merged 3 commits into from Jun 3, 2019

Conversation

@anorth
Copy link
Contributor

commented May 28, 2019

Block ticket is the canonical traversal order when a tipset is processed. This changes the critical traversal in consensus/expected.go to match that (previously iterated in sorted CID order). This is most of the resolution to #2310. See also filecoin-project/specs#285

The representation is more memory efficient and can be iterated without allocating an intermediate slice. An empty/undefined tipset is now harder to construct and explicitly undefined; a few methods now always return nil error and I intend to remove the error return value from them in a follow-up.

SortedCidSet remains as the "key" type for a tipset. In a follow-up, I intend to also upgrade this to an immutable structure.

@anorth anorth force-pushed the anorth/tipset branch 2 times, most recently from f4e44a3 to 342307b May 29, 2019

@anorth anorth marked this pull request as ready for review May 29, 2019

@anorth anorth requested review from ZenGround0 and acruikshank May 29, 2019

@anorth

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Just FYI @whyrusleeping

@acruikshank
Copy link
Contributor

left a comment

Check-pointing. Looking good so far. Some mild suggestions.

types/tipset.go Outdated Show resolved Hide resolved
return uint64(0), ErrEmptyTipSet
}
return uint64(ts.ToSlice()[0].Height), nil
return uint64(ts.blocks[0].Height), nil

This comment has been minimized.

Copy link
@acruikshank

acruikshank May 29, 2019

Contributor

I like how making TipSet immutable simplifies the error handling here and elsewhere.

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 May 29, 2019

Contributor

Blocking: @anorth It seems like we're opening up a possibility for panicing the node here that immutability does not protect us again. I don't like the unstated precondition that the caller is not the undefined tipset.

This comment has been minimized.

Copy link
@anorth

anorth May 30, 2019

Author Contributor

It's stated at the struct level. I've asked in a Go forum for advice.

chain/default_syncer.go Outdated Show resolved Hide resolved
chain/default_syncer.go Show resolved Hide resolved
chain/traversal.go Show resolved Hide resolved
consensus/expected.go Show resolved Hide resolved
@acruikshank
Copy link
Contributor

left a comment

Code looks good. TipSet test coverage seems like an improvement overall, but its hard to tell because it's too concise. I'd like a bit more elaboration here.

Overall this is a great refactor. It highlights how often we end up reprocessing messages when we have more than one block. We should probably work to address that.

blks := ts.ToSlice()
if len(ts) == 1 {
b := blks[0]
if ts.IsSolo() {

This comment has been minimized.

Copy link
@acruikshank

acruikshank May 29, 2019

Contributor

Perhaps now that we have a tipset struct, some future work would be cache receipts in tip sets. That would keep us from having to run a full state transition to fetch a single receipt when there's more than one block. This method is pretty ugly.

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 May 29, 2019

Contributor

This is probably the ugliest code in the entire project and it should be made better with or without caching receipts. That said caching receipts is a great idea. We should probably think about this alongside caching aggregate state cids / aggregate state trees, and limiting the in-memory tipset cache.

types/tipset_test.go Outdated Show resolved Hide resolved
types/tipset_test.go Outdated Show resolved Hide resolved
types/tipset_test.go Outdated Show resolved Hide resolved
types/tipset_test.go Outdated Show resolved Hide resolved
@ZenGround0
Copy link
Contributor

left a comment

I would like to talk over the move from explicit errors to panics. Other than that this is a very welcome and aesthetic change.

}
startHeight := headTs.At(0).Height

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 May 29, 2019

Contributor

Slight preference for getting this value through the Height function of the head tipset.

This comment has been minimized.

Copy link
@anorth

anorth May 30, 2019

Author Contributor

Yeah but that returns an error that I know isn't, so nah.

@@ -16,21 +16,19 @@ type BlockProvider interface {
// GetParentTipSet returns the parent tipset of a tipset.
// The result is empty if the tipset has no parents (including if it is empty itself)
func GetParentTipSet(ctx context.Context, store BlockProvider, ts types.TipSet) (types.TipSet, error) {

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 May 29, 2019

Contributor

Note: just realizing that this should really go to the tipindex to save us a bunch of disk reads.

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 May 29, 2019

Contributor

BlockProvider should really be a TipSetProvider which calls the DefaultStore's GetTipSet which hits the tipIndex cache. cc @frrist not sure how this relates to your more principled architected caching, but this patch will stop reading all these iterator traversals from disk.

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 May 29, 2019

Contributor

FYI @anorth this is orthogonal to this PR, just pointing it out.

chain/traversal.go Show resolved Hide resolved
consensus/expected.go Show resolved Hide resolved
types/tipset.go Outdated Show resolved Hide resolved

// String returns a formatted string of the CIDs in the TipSet.
// "{ <cid1> <cid2> <cid3> }"
// Note: existing callers use this as a unique key for the tipset. We should change them

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 May 29, 2019

Contributor

Could link issue here too

return uint64(0), ErrEmptyTipSet
}
return uint64(ts.ToSlice()[0].Height), nil
return uint64(ts.blocks[0].Height), nil

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 May 29, 2019

Contributor

Blocking: @anorth It seems like we're opening up a possibility for panicing the node here that immutability does not protect us again. I don't like the unstated precondition that the caller is not the undefined tipset.

}

// ParentWeight returns the tipset's ParentWeight in fixed point form.
// ParentWeight returns the tipset's ParentWeight in fixed point form, and nil error.

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 May 29, 2019

Contributor

Blocking: I get the benefit of eventually removing errors from these methods. Having errorless accessors was something I tried to get past review when working on this type in the past. But I am worried that this is wrong now for the same reason @phritz claimed it was then: it is possible for Undefined tipsets to make their way to code paths that will call this and panic the node.

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 May 29, 2019

Contributor

Also if we do decide to go with this then there should be some comment explicitly saying that these errors are intended to go away soon. As it is this code is the worst of both worlds: we are forced to write error handling code but that error handling code never gets used because this code panics instead of passing along the error

This comment has been minimized.

Copy link
@anorth

anorth May 30, 2019

Author Contributor

If this lands, I'll fast follow with removing the errors. Pending broader advice on the pattern.

types/tipset.go Show resolved Hide resolved
sort.Slice(sorted, func(i, j int) bool {
cmp := bytes.Compare(sorted[i].Ticket, sorted[j].Ticket)
if cmp == 0 {
// Break ticket ties with the block CIDs, which are distinct.

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 May 29, 2019

Contributor

Lets make sure there is an issue somewhere (gfc or spec) tracking that this is recorded in the spec.

This comment has been minimized.

Copy link
@anorth

anorth May 30, 2019

Author Contributor

I'll do this when fixing the spec generally to define this. filecoin-project/specs#289

@anorth

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

I have pushed a commit with review changes, you may wish to look at just that one. Pending decision on suppressing the error from undefined tipset method calls.

@ZenGround0
Copy link
Contributor

left a comment

Removing block

I am still concerned that opening the node up to panics would come back to haunt us, but my opinion has been fully voiced and heard.

@anorth anorth referenced this pull request Jun 2, 2019
0 of 5 tasks complete
anorth added 2 commits May 26, 2019
Change TipSet to be an immutable list, ordered by block ticket.
Block ticket is the canonical traversal order when a tipset is processed. This change updates
the critical traversal in consensus/expected.go to match that (previously
iterated in sorted CID order). This is most of the resolution to #2310.

The representation is more memory efficient and can be iterated without allocating an
intermediate slice. An empty/undefined tipset is now harder to construct and explicitly
undefined; a few methods now always return nil error and I intend to remove the error return
value from them in a follow-up.

SortedCidSet remains as the "key" type for a tipset. In a follow-up, I intend to also
upgrade this to an immutable structure.
@anorth

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

After more thought, I've decided to leave the error return value for empty tipset methods there. It's less work than removing it and not obviously better either way.

@anorth anorth force-pushed the anorth/tipset branch from b076390 to 8bf7587 Jun 3, 2019

@anorth anorth merged commit 0201cdf into master Jun 3, 2019

5 checks passed

ci/circleci: build_linux-1 Your tests passed on CircleCI!
Details
ci/circleci: deps_linux Your tests passed on CircleCI!
Details
ci/circleci: functional_test_linux-1 Your tests passed on CircleCI!
Details
ci/circleci: integration_test_linux Your tests passed on CircleCI!
Details
ci/circleci: unit_test_linux Your tests passed on CircleCI!
Details

@anorth anorth deleted the anorth/tipset branch Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.