fix: many incorrect pointer equality comparisons #1018
Conversation
- the hardest of facepalms
Codecov Report
@@ Coverage Diff @@
## frrist/update-lotusv1.16.0-rc #1018 +/- ##
===============================================================
- Coverage 35.6% 35.6% -0.1%
===============================================================
Files 44 44
Lines 2877 2879 +2
===============================================================
Hits 1025 1025
- Misses 1748 1750 +2
Partials 104 104 |
tasks/actorstate/miner/context.go
Outdated
| CurrActor *types.Actor | ||
| CurrState miner.State | ||
| CurrTs *types.TipSet | ||
| PreviousState bool |
There was a problem hiding this comment.
nit: A slightly more expressive name:
| PreviousState bool | |
| HasPreviousState bool |
| if prevDeadlineInfo == currDeadlineInfo { | ||
| // TODO implement equality function | ||
| // dereference pointers to check equality | ||
| if *prevDeadlineInfo == *currDeadlineInfo { |
There was a problem hiding this comment.
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.
| if prevDeadlineInfo == currDeadlineInfo { | ||
| // TODO implement equality function | ||
| // dereference pointers to check equality | ||
| if *prevDeadlineInfo == *currDeadlineInfo { |
There was a problem hiding this comment.
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.
| if *prevDeadlineInfo == *currDeadlineInfo { | |
| if prevDeadlineInfo != nil && | |
| currDeadlineInfo != nil && | |
| *prevDeadlineInfo == *currDeadlineInfo { |
There was a problem hiding this comment.
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
| // 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) { |
There was a problem hiding this comment.
Protect against nil-deref panic here?
There was a problem hiding this comment.
the LockedFunds type isn't a pointer, nor are its fields so we can safely use them here.
| @@ -119,11 +119,12 @@ type MsigExtractionContext struct { | |||
| CurrState multisig.State | |||
| CurrTs *types.TipSet | |||
|
|
|||
| Store adt.Store | |||
| Store adt.Store | |||
| PreviousState bool | |||
There was a problem hiding this comment.
nit:
| PreviousState bool | |
| HasPreviousState bool |
| @@ -20,11 +20,12 @@ type VerifiedRegistryExtractionContext struct { | |||
| PrevState, CurrState verifreg.State | |||
| PrevTs, CurrTs *types.TipSet | |||
|
|
|||
| Store adt.Store | |||
| Store adt.Store | |||
| PreviousState bool | |||
There was a problem hiding this comment.
nit:
| PreviousState bool | |
| HasPreviousState bool |
* deps: update to lotus v0.16.0 * refactor: remove unused actor gen code * chore: update actor code generator for v8 * gen: init actor accessors * gen: market actor accessors * gen: multisig actor accessors * gen: power actor accessors * gen: reward actor accessors * gen verifreg actor accessors * gen: miner actor accessors * chore: update miner state extractors - required due to changes in miner actor gen accessors * chore: update market actor extractors - required for market actor gen accessors * gen: builtin accessors * refactor: remove unused map opts lookup - actor accessors now return map opt values * chore: update message test deps - use v8 actor structs * feat: add actor CID lookups - replace with filecoin-project/lotus#8941 when lotus v1.17.0 is cut * chore: update lens interface to match lotus api - required due to lotus update v1.16.0 * refactor: register new actor code in processor * fix: update parsed message extractor with new exitcode * fix: only init VM for network version prior to 13 - ensures we can process message on FVM upgrade boundary * Add is_string column to market_deal_proposal model (#1015) * chore: add is_string column to market_deal_proposal model * fix: many incorrect pointer equality comparisons (#1018) * fix: many incorrect pointer equality comparisons
This PR fixes a bug which was causing lily to export redundant data from models which had not actually changed state. The impact is limited to the same state being persisted over and over on multiple subsequent epochs.
The following models are impacted:
miner_*multisig_*power_actor_claimsverified_registry_*fixes #1017