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
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 180 additions & 0 deletions chain/actors/builtin/miner/diff_deadlines.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
package miner

import (
"errors"

"github.com/filecoin-project/go-bitfield"
"github.com/filecoin-project/go-state-types/exitcode"
)

type DeadlinesDiff map[uint64]*DeadlineDiff

func DiffDeadlines(pre, cur State) (*DeadlinesDiff, error) {
changed, err := pre.DeadlinesChanged(cur)
if err != nil {
return nil, err
}
if !changed {
return nil, nil
}

numDl, err := pre.NumDeadlines()
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.

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).

if err != nil {
return err
}

diff, err := DiffDeadline(preDl, curDl)
if err != nil {
return err
}

dlDiff[idx] = diff
return nil
}); err != nil {
return nil, err
}
return &dlDiff, nil
}

type DeadlineDiff map[uint64]*PartitionDiff

func DiffDeadline(pre, cur Deadline) (*DeadlineDiff, error) {
changed, err := pre.PartitionsChanged(cur)
if err != nil {
return nil, err
}
if !changed {
return nil, nil
}

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.

if err != nil {
if errors.Is(err, exitcode.ErrNotFound) {
// TODO correctness?
return nil // the partition was removed.
}
return err
}

// compare it with the previous partition
diff, err := DiffPartition(prePart, curPart)
if err != nil {
return err
}

partDiff[idx] = diff
return nil
}); err != nil {
return nil, err
}

// 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).

if err := cur.ForEachPartition(func(idx uint64, curPart Partition) error {
if _, found := partDiff[idx]; found {
return nil
}
faults, err := curPart.FaultySectors()
if err != nil {
return err
}
recovering, err := curPart.RecoveringSectors()
if err != nil {
return err
}
partDiff[idx] = &PartitionDiff{
Removed: bitfield.New(),
Recovered: bitfield.New(),
Faulted: faults,
Recovering: recovering,
}

return nil
}); err != nil {
return nil, err
}

return &partDiff, nil
}

type PartitionDiff struct {
Removed bitfield.BitField
Recovered bitfield.BitField
Faulted bitfield.BitField
Recovering bitfield.BitField
}

func DiffPartition(pre, cur Partition) (*PartitionDiff, error) {
prevLiveSectors, err := pre.LiveSectors()
if err != nil {
return nil, err
}
curLiveSectors, err := cur.LiveSectors()
if err != nil {
return nil, err
}

removed, err := bitfield.SubtractBitField(prevLiveSectors, curLiveSectors)
if err != nil {
return nil, err
}

prevRecoveries, err := pre.RecoveringSectors()
if err != nil {
return nil, err
}

curRecoveries, err := cur.RecoveringSectors()
if err != nil {
return nil, err
}

recovering, err := bitfield.SubtractBitField(curRecoveries, prevRecoveries)
if err != nil {
return nil, err
}

prevFaults, err := pre.FaultySectors()
if err != nil {
return nil, err
}

curFaults, err := cur.FaultySectors()
if err != nil {
return nil, err
}

faulted, err := bitfield.SubtractBitField(curFaults, prevFaults)
if err != nil {
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).

curActiveSectors, err := cur.ActiveSectors()
if err != nil {
return nil, err
}

// sectors that were previously fault and are now currently active are considered recovered.
recovered, err := bitfield.IntersectBitField(prevFaults, curActiveSectors)
if err != nil {
return nil, err
}

return &PartitionDiff{
Removed: removed,
Recovered: recovered,
Faulted: faulted,
Recovering: recovering,
}, nil
}