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

new api to calculate arbitrary forks revert and applys #1084

Merged
merged 1 commit into from
Jan 15, 2020
Merged

new api to calculate arbitrary forks revert and applys #1084

merged 1 commit into from
Jan 15, 2020

Conversation

jsign
Copy link
Contributor

@jsign jsign commented Jan 14, 2020

This PR adds a new API endpoint and implementation to get a similar output of ChainNotify but for arbitrary from and to.

The motivation for this is the following:

  • A client is following the chain to build up some index or view about it.
  • The client stops their service.
  • In the meantime, the chain can fork from the last known tipset for the client.
  • When the client resumes, it can't use ChainNotify directly since it inspects changes from the current head of the lotus node.
  • The client should check if the current head is in the same chain from his last known tipset, because if that isn't the case, it will apply new blocks using (what could be described) the wrong state.

The whole point of this API is to allow a client to provide his last known tipset, and some other new one (most probably, ChainHead), and do whatever is necessary to move on.

This API it's a bit more generic than this use case and can compare any two tipsets. Preferred doing it this way, than assuming will always compare to tipset of the heaviest chain.

TBH, I'm not quite sure that names or package where I made the changes are the correct ones, so there's a high chance that might be better to rename and/or move. If that's the case, I'm happy to fix it.


I thought about modifying ChainNotify to receive a from value, but realized that this way may be more orthogonal and allows resolving other cases other from current head.

I'd imagine being common a combination like the following:
Call ChainNotify, get the first Current type value, and call ChainGetPath to resolve using his from with the current head as to. The last call will give enough information to fix a possible unknown fork, and then it would continue listening to ChainNotify results as usual.


I chose some arbitrary maximum chain height for the fork to put a cap on the worst-case scenario. Maybe there's other more reasonable value.

@Kubuxu
Copy link
Contributor

Kubuxu commented Jan 14, 2020

You can use build.ForkLengthThreshold as the limit.

@@ -730,6 +734,55 @@ func (cs *ChainStore) readMsgMetaCids(mmc cid.Cid) ([]cid.Cid, []cid.Cid, error)
return blscids, secpkcids, nil
}

func (cs *ChainStore) GetPath(ctx context.Context, from types.TipSetKey, to types.TipSetKey) ([]*HeadChange, error) {
fts, err := cs.LoadTipSet(from)
Copy link
Member

Choose a reason for hiding this comment

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

We should probably just reuse code from the ReorgOps method, it does a good hunk of this already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely, good that you mentioned it. It doesn't cover some cases I checked explicitly, but it's doing the important work (an error in those cases). Most of those things shouldn't happen in any reasonable scenario so it should be OK. I'll call that and then just do the remaining transformations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think may worth to add the build.ForkLengthThreshold cap to ReorgOps?

Copy link
Member

Choose a reason for hiding this comment

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

Probably worth adding that in, yeah. Maybe pass it as a parameter?

Signed-off-by: jsign <jsign.uy@gmail.com>
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Probably good to add the limit to ReorgOps, but i'm happy to merge without that (since this is a user initiated action in any case). Let me know

@jsign
Copy link
Contributor Author

jsign commented Jan 15, 2020

Actually, I think it might be better not doing the check. Since chains are named left and right, it might not be that clear what is really the semantics of fork height. If right is actually the current head, that makes sense (left height from base tipset is the fork height).. but this is a quite generic method.

@whyrusleeping
Copy link
Member

SGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants