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.

Sign up for GitHub

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: many incorrect pointer equality comparisons #1018

Merged
merged 2 commits into from
Jul 5, 2022
Merged

fix: many incorrect pointer equality comparisons #1018

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a52270b
fix: many incorrect pointer equality comparisons
frrist Jul 5, 2022
dbfc140
fix: simplify previous actor state handling
frrist Jul 5, 2022
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
33 changes: 18 additions & 15 deletions tasks/actorstate/miner/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func NewMinerStateExtractionContext(ctx context.Context, a actorstate.ActorInfo,
}

prevTipset := a.Current
prevState := curState
var prevState miner.State
if a.Current.Height() != 1 {
prevTipset = a.Executed

Expand All @@ -32,11 +32,12 @@ func NewMinerStateExtractionContext(ctx context.Context, a actorstate.ActorInfo,
// actor was created in the current state.
if err == types.ErrActorNotFound {
return &MinerStateExtractionContext{
PrevState: prevState,
PrevTs: prevTipset,
CurrActor: &a.Actor,
CurrState: curState,
CurrTs: a.Current,
PrevState: prevState,
PrevTs: prevTipset,
CurrActor: &a.Actor,
CurrState: curState,
CurrTs: a.Current,
PreviousState: false,
}, nil
}
return nil, fmt.Errorf("loading previous miner %s at tipset %s epoch %d: %w", a.Address, a.Executed.Key(), a.Current.Height(), err)
Expand All @@ -49,23 +50,25 @@ func NewMinerStateExtractionContext(ctx context.Context, a actorstate.ActorInfo,
}

return &MinerStateExtractionContext{
PrevState: prevState,
PrevTs: prevTipset,
CurrActor: &a.Actor,
CurrState: curState,
CurrTs: a.Current,
PrevState: prevState,
PrevTs: prevTipset,
CurrActor: &a.Actor,
CurrState: curState,
CurrTs: a.Current,
PreviousState: true,
}, nil
}

type MinerStateExtractionContext struct {
PrevState miner.State
PrevTs *types.TipSet

CurrActor *types.Actor
CurrState miner.State
CurrTs *types.TipSet
CurrActor *types.Actor
CurrState miner.State
CurrTs *types.TipSet
PreviousState bool
Copy link
Contributor

@placer14 placer14 Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit: A slightly more expressive name:

Suggested change
PreviousState bool
HasPreviousState bool

}

func (m *MinerStateExtractionContext) HasPreviousState() bool {
return !(m.CurrTs.Height() == 1 || m.PrevState == m.CurrState)
return m.PreviousState
}
4 changes: 3 additions & 1 deletion tasks/actorstate/miner/deadline_info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ func (DeadlineInfoExtractor) Extract(ctx context.Context, a actorstate.ActorInfo
if err != nil {
return nil, err
}
if prevDeadlineInfo == currDeadlineInfo {
// TODO implement equality function
// dereference pointers to check equality
if *prevDeadlineInfo == *currDeadlineInfo {
Copy link
Contributor

@placer14 placer14 Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is brittle. We have to be sure that err == nil guarantees we're protected from a nil-deref panic here and forever into the future. How is that being guaranteed? I would prefer this check is explicit here instead of expecting it inside the called method. If we have a local equality function doing that, that could work just a well.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is brittle. We have to be sure that err == nil guarantees we're protected from a nil-deref panic here and forever into the future. How is that being guaranteed? I would prefer this check is explicit here instead of expecting it inside the caller. If we have a local equality function doing that, that could work just a well.

Suggested change
if *prevDeadlineInfo == *currDeadlineInfo {
if prevDeadlineInfo != nil &&
currDeadlineInfo != nil &&
*prevDeadlineInfo == *currDeadlineInfo {

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we know the miner has state we can safely assume it has non-nil deadline information. (the method that creates deadline info will never return null: https://github.com/filecoin-project/specs-actors/blob/v8.0.1/actors/builtin/miner/deadlines.go#L15

return nil, nil
}
}
Expand Down
2 changes: 1 addition & 1 deletion tasks/actorstate/miner/fee_debt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func (FeeDebtExtractor) Extract(ctx context.Context, a actorstate.ActorInfo, nod
if err != nil {
return nil, fmt.Errorf("loading previous miner fee debt: %w", err)
}
if prevDebt == currDebt {
if prevDebt.Equals(currDebt) {
return nil, nil
}
}
Expand Down
6 changes: 5 additions & 1 deletion tasks/actorstate/miner/locked_funds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,11 @@ func (LockedFundsExtractor) Extract(ctx context.Context, a actorstate.ActorInfo,
if err != nil {
return nil, fmt.Errorf("loading previous miner locked funds: %w", err)
}
if prevLocked == currLocked {

// if all values are equal there is no change.
if prevLocked.VestingFunds.Equals(currLocked.VestingFunds) &&
prevLocked.PreCommitDeposits.Equals(currLocked.PreCommitDeposits) &&
prevLocked.InitialPledgeRequirement.Equals(currLocked.InitialPledgeRequirement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Protect against nil-deref panic here?

Copy link
Member Author

Choose a reason for hiding this comment

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

the LockedFunds type isn't a pointer, nor are its fields so we can safely use them here.

return nil, nil
}
}
Expand Down
27 changes: 15 additions & 12 deletions tasks/actorstate/multisig/multisig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,12 @@ type MsigExtractionContext struct {
CurrState multisig.State
CurrTs *types.TipSet

Store adt.Store
Store adt.Store
PreviousState bool
Copy link
Contributor

@placer14 placer14 Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit:

Suggested change
PreviousState bool
HasPreviousState bool

}

func (m *MsigExtractionContext) HasPreviousState() bool {
return !(m.CurrTs.Height() == 1 || m.CurrState == m.PrevState)
return m.PreviousState
}

func NewMultiSigExtractionContext(ctx context.Context, a actorstate.ActorInfo, node actorstate.ActorStateAPI) (*MsigExtractionContext, error) {
Expand All @@ -140,11 +141,12 @@ func NewMultiSigExtractionContext(ctx context.Context, a actorstate.ActorInfo, n
// actor was created in the current state.
if err == types.ErrActorNotFound {
return &MsigExtractionContext{
PrevState: prevState,
CurrActor: &a.Actor,
CurrState: curState,
CurrTs: a.Current,
Store: node.Store(),
PrevState: prevState,
CurrActor: &a.Actor,
CurrState: curState,
CurrTs: a.Current,
Store: node.Store(),
PreviousState: false,
}, nil
}
return nil, fmt.Errorf("loading previous multisig %s at tipset %s epoch %d: %w", a.Address, a.Executed.Key(), a.Current.Height(), err)
Expand All @@ -157,10 +159,11 @@ func NewMultiSigExtractionContext(ctx context.Context, a actorstate.ActorInfo, n
}

return &MsigExtractionContext{
PrevState: prevState,
CurrActor: &a.Actor,
CurrState: curState,
CurrTs: a.Current,
Store: node.Store(),
PrevState: prevState,
CurrActor: &a.Actor,
CurrState: curState,
CurrTs: a.Current,
Store: node.Store(),
PreviousState: true,
}, nil
}
23 changes: 13 additions & 10 deletions tasks/actorstate/power/power.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,11 @@ func NewPowerStateExtractionContext(ctx context.Context, a actorstate.ActorInfo,
// actor was created in the current state.
if err == types.ErrActorNotFound {
return &PowerStateExtractionContext{
PrevState: prevState,
CurrState: curState,
CurrTs: a.Current,
Store: node.Store(),
PrevState: prevState,
CurrState: curState,
CurrTs: a.Current,
Store: node.Store(),
PreviousState: false,
}, nil
}
return nil, fmt.Errorf("loading previous power actor at tipset %s epoch %d: %w", a.Executed.Key(), a.Current.Height(), err)
Expand All @@ -48,10 +49,11 @@ func NewPowerStateExtractionContext(ctx context.Context, a actorstate.ActorInfo,
}
}
return &PowerStateExtractionContext{
PrevState: prevState,
CurrState: curState,
CurrTs: a.Current,
Store: node.Store(),
PrevState: prevState,
CurrState: curState,
CurrTs: a.Current,
Store: node.Store(),
PreviousState: true,
}, nil
}

Expand All @@ -60,11 +62,12 @@ type PowerStateExtractionContext struct {
CurrState power.State
CurrTs *types.TipSet

Store adt.Store
Store adt.Store
PreviousState bool
}

func (p *PowerStateExtractionContext) HasPreviousState() bool {
return !(p.CurrTs.Height() == 1 || p.PrevState == p.CurrState)
return p.PreviousState
}

func (StoragePowerExtractor) Extract(ctx context.Context, a actorstate.ActorInfo, node actorstate.ActorStateAPI) (model.Persistable, error) {
Expand Down
27 changes: 15 additions & 12 deletions tasks/actorstate/verifreg/verifreg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@ type VerifiedRegistryExtractionContext struct {
PrevState, CurrState verifreg.State
PrevTs, CurrTs *types.TipSet

Store adt.Store
Store adt.Store
PreviousState bool
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
PreviousState bool
HasPreviousState bool

}

func (v *VerifiedRegistryExtractionContext) HasPreviousState() bool {
return !(v.CurrTs.Height() == 1 || v.PrevState == v.CurrState)
return v.PreviousState
}

func NewVerifiedRegistryExtractorContext(ctx context.Context, a actorstate.ActorInfo, node actorstate.ActorStateAPI) (*VerifiedRegistryExtractionContext, error) {
Expand All @@ -41,11 +42,12 @@ func NewVerifiedRegistryExtractorContext(ctx context.Context, a actorstate.Actor
// actor was created in the current state.
if err == types.ErrActorNotFound {
return &VerifiedRegistryExtractionContext{
PrevState: prevState,
CurrState: curState,
PrevTs: a.Executed,
CurrTs: a.Current,
Store: node.Store(),
PrevState: prevState,
CurrState: curState,
PrevTs: a.Executed,
CurrTs: a.Current,
Store: node.Store(),
PreviousState: false,
}, nil
}
return nil, fmt.Errorf("loading previous verified registry actor at tipset %s epoch %d: %w", a.Executed.Key(), a.Current.Height(), err)
Expand All @@ -57,11 +59,12 @@ func NewVerifiedRegistryExtractorContext(ctx context.Context, a actorstate.Actor
}
}
return &VerifiedRegistryExtractionContext{
PrevState: prevState,
CurrState: curState,
PrevTs: a.Executed,
CurrTs: a.Current,
Store: node.Store(),
PrevState: prevState,
CurrState: curState,
PrevTs: a.Executed,
CurrTs: a.Current,
Store: node.Store(),
PreviousState: true,
}, nil
}

Expand Down