-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
You can use |
@@ -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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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>
There was a problem hiding this 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
Actually, I think it might be better not doing the check. Since chains are named |
SGTM |
This PR adds a new API endpoint and implementation to get a similar output of
ChainNotify
but for arbitraryfrom
andto
.The motivation for this is the following:
ChainNotify
directly since it inspects changes from the current head of the lotus node.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 afrom
value, but realized that this way may be more orthogonal and allows resolving other cases other from currenthead
.I'd imagine being common a combination like the following:
Call
ChainNotify
, get the firstCurrent
type value, and callChainGetPath
to resolve using hisfrom
with the current head asto
. The last call will give enough information to fix a possible unknown fork, and then it would continue listening toChainNotify
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.