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

feat(miner): add miner deadline diffing logic. #4178

Merged
merged 1 commit into from
Oct 7, 2020

Conversation

frrist
Copy link
Member

@frrist frrist commented Oct 5, 2020

  • logic gathering all removed, faulted, recovered, and recovering sectorIDs.

@frrist frrist self-assigned this Oct 5, 2020
@frrist frrist requested a review from Stebalien October 5, 2020 23:54
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM!

}
dlDiff := make(DeadlinesDiff, numDl)
if err := pre.ForEachDeadline(func(idx uint64, preDl Deadline) error {
curDl, err := cur.LoadDeadline(idx)
Copy link
Member

Choose a reason for hiding this comment

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

This is, unfortunately, going to be a bit slow as it has to keep loading the deadline object.

It's probably better to just load all deadlines into two arrays, then compare them.

Copy link
Member

Choose a reason for hiding this comment

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

Really low priority (only if it becomes an issue).

partDiff := make(DeadlineDiff)
if err := pre.ForEachPartition(func(idx uint64, prePart Partition) error {
// try loading current partition at this index
curPart, err := cur.LoadPartition(idx)
Copy link
Member

Choose a reason for hiding this comment

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

Ditto my comment above about loading into arrays, also really low priority.


// all previous partitions have been walked.
// all partitions in cur and not in prev are new... can they be faulty already?
// TODO is this correct?
Copy link
Member

Choose a reason for hiding this comment

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

I believe you're right that there can't be any faults, but I'm not 100% sure. We should be allocating new partitions in ConfirmSectorProofsValid, which should happen during cron (after all fault messages have been processed).

return nil, err
}

// all current good sectors
Copy link
Member

Choose a reason for hiding this comment

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

This code is correct. However, note: "active" means actively contributing towards power. In actors v2, this won't include new but not-yet-posted sectors.

But that doesn't matter for this code recovered sectors will always be "posted" (i.e., we'll submit a post to recover them).

if err != nil {
return nil, err
}
dlDiff := make(DeadlinesDiff, numDl)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we care about cases where a new deadline has been added in cur that was not in pre, or vice-versa?
Is there a reason to make dlDiff an array of all deadlines (with nil entries for those that have not changed) rather than a map of index->diff? if we expect sparse deadline changes, that might be a more natural representation.

Copy link
Member Author

@frrist frrist Oct 6, 2020

Choose a reason for hiding this comment

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

do we care about cases where a new deadline has been added in cur that was not in pre, or vice-versa?

My understanding (is somewhat incomplete here, but I think) since Deadlines are a fixed size it will never be the case that there exists a deadline in cur and not pre, or vice-versa, meaning we will walk each deadline on calls to ForEachDeadline. I suppose there could be an edge case here if we update the miner actor with a larger deadlines array and this code is called at the upgrade boundary -- I will leave a comment ackonwledging this as I am unsure how to address it...

I could be the case (perhaps this is what was meant) that there exists an empty deadline (a deadline with no partitions) in cur or pre, but I think this addressed by walking both cur and pre in the DiffDeadline method.

Is there a reason to make dlDiff an array of all deadlines (with nil entries for those that have not changed) rather than a map of index->diff? if we expect sparse deadline changes, that might be a more natural representation.

I don't have a strong reason for it, and agree a map would be better choice -- will address.

Copy link
Contributor

Choose a reason for hiding this comment

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

- logic gathering all removed, faulted, recovered, and recovering
sectors for a miner.
@frrist frrist force-pushed the frrist/miner-diff-deadlines branch from 45cad54 to d4cbd4f Compare October 6, 2020 18:39
@frrist frrist marked this pull request as ready for review October 6, 2020 18:42
@whyrusleeping whyrusleeping merged commit 099b982 into master Oct 7, 2020
@whyrusleeping whyrusleeping deleted the frrist/miner-diff-deadlines branch October 7, 2020 17:00
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.

4 participants