-
Notifications
You must be signed in to change notification settings - Fork 157
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
fix: update sync_headers_in_reverse
logic to match lotus
#4291
Conversation
994a715
to
2affcee
Compare
2affcee
to
23366b9
Compare
src/blocks/tipset.rs
Outdated
// FIXME: The height check might go beyond what is meant by | ||
// "parent", but many parts of the code rely on the tipset's | ||
// height for their processing logic at the moment to obviate it. | ||
&& self.epoch() > other.epoch() |
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.
q: what you want to say here is that there is just a sanity check, correct? Just an extra measure to make sure the relationship is somewhat correct?
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.
This is completely copied from lotus. See the link above : )
https://github.com/filecoin-project/lotus/blob/01ec22974942fb7328a1e665704c6cfd75d93372/chain/types/tipset.go#L258
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 we have any code that depends on this check? I think we reject blocks that do not have sensible epochs at an earlier stage. I think we can just remove this extra check.
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 we have any code that depends on this check? I think we reject blocks that do not have sensible epochs at an earlier stage. I think we can just remove this extra check.
@lemmih No other code depends on this check. Removed.
src/chain_sync/tipset_syncer.rs
Outdated
let oldest_tipset = parent_tipsets.last().clone(); | ||
// Determine if the local chain was a fork. | ||
// If it was, then sync the fork tipset range by iteratively walking back | ||
let oldest_tipset = parent_tipsets.last(); |
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.
nit: perhaps oldest_parent
is a better name, similar to https://github.com/ChainSafe/forest/pull/4291/files#diff-c2e102581e8811dd894188425099a003280f0c7e38b9c5a7ca75f2a0dd0bdaddR824
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.
Good point, actually parent
is quite confusing here, renamed parent_tipsets
to pending_tipsets(_for_sync)
, and oldest_tipset
to oldest_pending_tipset
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.
Logic appears to be sound (though still hard to follow).
src/blocks/tipset.rs
Outdated
// FIXME: The height check might go beyond what is meant by | ||
// "parent", but many parts of the code rely on the tipset's | ||
// height for their processing logic at the moment to obviate it. | ||
&& self.epoch() > other.epoch() |
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 we have any code that depends on this check? I think we reject blocks that do not have sensible epochs at an earlier stage. I think we can just remove this extra check.
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.
AIUI this is transpilation of some go code.
If so, WIBNI you checked in a comment to the reference code, so readers can understand why it is the way it is.
// Note: the extra `&& self.epoch() > other.epoch()` check in lotus is dropped | ||
// See <https://github.com/filecoin-project/lotus/blob/01ec22974942fb7328a1e665704c6cfd75d93372/chain/types/tipset.go#L258> |
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.
excellent comment
parent_tipsets.extend(fork_tipsets); | ||
break; | ||
} | ||
info!("Fork detected, searching for a common ancestor between the local chain and the network chain"); |
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.
nit: this log message is basically the same as the comment above, you can probably delete the comment
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.
Fixed.
@aatifsyed link added! |
Summary of changes
Changes introduced in this pull request:
sync_headers_in_reverse
logic to match lotusLog(importing a snapshot whose head is not a fork, it no longer re-validates the head):
lotus code(
func (syncer *Syncer) collectHeaders
): https://github.com/filecoin-project/lotus/blob/master/chain/sync.go#L684Some highlights:
Reference issue to close (if applicable)
Closes #4113
Other information and links
Change checklist