Implement FIP-0019 #1529

arajasek · 2021-11-10T22:39:38Z

Implements FIP-0019. There are outstanding work items such as Snapshotting PoSts, testing, some refactoring, etc, but this gets the job done.

ZenGround0

Still have more to go over and think more about but here are a few comments in the meantime.

actors/builtin/miner/miner_actor.go

ZenGround0 · 2021-11-10T23:17:59Z

actors/builtin/miner/miner_actor.go

+	var sectorsDataSpec []*market.SectorDataSpec
+	for _, update := range params.Updates {
+		// TODO: do we need an assertion that the same sector isn't upgraded twice?
+		// TODO: actual length


@Kubuxu @cryptonemo what is the expected length of these proofs? 192 bytes?

AFAIK 192*16=3072 currently, we can do aggregation in future.

@arajasek you can bound it by an even number like 4096

Going with <= 4096 for now, but @BlocksOnAChain can we get an AI to get final sign-offs and confirmation of this (after everything has been finalized)? We've had whoopsies here in the past.

actors/builtin/miner/miner_actor.go

support/mock/mockrt.go

actors/builtin/miner/miner_actor.go

actors/runtime/proof/verify.go

actors/builtin/miner/miner_actor.go

ZenGround0

Update partway through this review:

Let's use rtt.WARN for logging update-skip-on-error events

Introducing TODOs to specs-actors code is usually the wrong thing to do. To fixup your existing todos you should either 1) do the thing referenced 2) just delete the todo 3) file and issue in specs-actors repo about the todo if it is something we really should be doing

actors/builtin/market/market_actor.go

actors/builtin/miner/miner_actor.go

ZenGround0 · 2021-11-15T16:15:35Z

actors/builtin/miner/miner_actor.go

+	var sectorsDataSpec []*market.SectorDataSpec
+	for _, update := range params.Updates {
+		// TODO: do we need an assertion that the same sector isn't upgraded twice?
+		// TODO: actual length


@arajasek you can bound it by an even number like 4096

actors/builtin/miner/miner_actor.go

ZenGround0

I think this is the last iteration of major review, this is looking close to ready.

One general style nit is that you can remove newlines between logically connected areas of the code, i.e. a sequence of checks or a call and its error handling.

actors/builtin/miner/miner_actor.go

codecov-commenter · 2021-11-16T21:37:08Z

Codecov Report

Merging #1529 (af88422) into master (ce79c44) will decrease coverage by 1.4%.
The diff coverage is 0.5%.

@@           Coverage Diff            @@
##           master   #1529     +/-   ##
========================================
- Coverage    71.1%   69.6%   -1.5%     
========================================
  Files          72      72             
  Lines        8520    8701    +181     
========================================
+ Hits         6059    6060      +1     
- Misses       1601    1781    +180     
  Partials      860     860

actors/builtin/miner/miner_actor.go

test-vectors/determinism-check

actors/builtin/miner/miner_actor.go

actors/runtime/proof/verify.go

actors/builtin/miner/miner_actor.go

ZenGround0

This LGTM

actors/builtin/miner/miner_actor.go

Kubuxu · 2021-11-23T18:27:27Z

actors/builtin/miner/miner_actor.go

+
+				newSectorInfo.SealedCID = updateWithDetails.update.NewSealedSectorCID
+				if newSectorInfo.SectorKeyCID == nil {
+					newSectorInfo.SectorKeyCID = &updateWithDetails.sectorInfo.SealedCID


updateWithDetails.sectorInfo is aliased with newSectorInfo which makes this code do the wrong thing

Kubuxu · 2021-11-23T18:27:40Z

actors/builtin/miner/miner_actor.go

+
+				builtin.RequireNoErr(rt, err, exitcode.ErrIllegalArgument, "failed to verify replica proof for sector %d", updateWithDetails.sectorInfo.SectorNumber)
+
+				newSectorInfo := updateWithDetails.sectorInfo


solution is to make this a copy

actors/builtin/miner/miner_actor.go

jennijuju · 2021-11-24T04:23:39Z

actors/builtin/miner/miner_actor.go

+			rt.Abortf(exitcode.ErrIllegalArgument, "new sealed CID had wrong prefix")
+		}
+
+		err = stReadOnly.CheckSectorHealthExcludeUnproven(store, update.Deadline, update.Partition, update.SectorID)


soft failure please!

and why not just call CheckSectorProven?

if can be replaced by CheckSectorProven, we can have deadline and partition removed from PRU param too?

Yes on soft failure!

CheckSectorProven doesn't seem to be doing all that much -- it seems to just check if the sector no is in the Sectors at all. I'm not sure whether that's enough. Maybe that + calling the existing CheckSectorHealth method might be enough?

actors/builtin/miner/miner_actor.go

jennijuju · 2021-11-24T04:37:45Z

actors/builtin/miner/miner_actor.go

+func (a Actor) ProveReplicaUpdates(rt Runtime, params *ProveReplicaUpdatesParams) bitfield.BitField {
+	// Validate inputs
+
+	builtin.RequireParam(rt, len(params.Updates) <= ProveReplicaUpdatesMaxSize, "too many updates (%d > %d)", len(params.Updates), ProveReplicaUpdatesMaxSize)


I believe during the audit it was mentioned that ProveReplicaUpdatesMaxSize should be lower ?

Will defer to whatever's in the audit followups doc.

jennijuju · 2021-11-24T04:52:53Z

actors/builtin/miner/miner_actor.go

+
+		sectorNumbers.Set(uint64(update.SectorID))
+
+		if len(update.ReplicaProof) > 4096 {


put this as a static variable? and comment on how we got that value (as a part of more documentation in code practise)? and it should be 3072 according. to the internal audit?

Will follow up from the audit notes

jennijuju · 2021-11-24T05:16:35Z

actors/builtin/miner/miner_state.go

@@ -692,6 +693,50 @@ func (st *State) CheckSectorHealth(store adt.Store, dlIdx, pIdx uint64, sector a
 	return nil
 }

+// Returns an error if the target sector cannot be found and/or is faulty/terminated/unproven.
+func (st *State) CheckSectorHealthExcludeUnproven(store adt.Store, dlIdx, pIdx uint64, sector abi.SectorNumber) error {


what's the difference between this and CheckSectorProven ?

it also checks that the sector isn't faulty

jennijuju · 2021-11-24T05:23:27Z

actors/builtin/miner/miner_actor.go

+		err = stReadOnly.CheckSectorHealthExcludeUnproven(store, update.Deadline, update.Partition, update.SectorID)
+		builtin.RequireNoErr(rt, err, exitcode.ErrIllegalArgument, "sector is not healthy")
+
+		sectorInfo, err := sectors.MustGet(update.SectorID)


should this be checked and done before CheckSectorHealthExcludeUnproven? if the sectors cannot be found, it implies the sector won't be proven, may save us one state read? idk

i think you're right that it's less expensive than the health check, but i don't think it's worth changing

jennijuju · 2021-11-24T05:51:16Z

actors/builtin/miner/miner_actor.go

+			continue
+		}
+
+		if len(sectorInfo.DealIDs) != 0 {


would it be better to check if SectorKeyCID is null or not? (in preparation of snap on snap - IIUC, both CC sector and existing deal sectors will have null for SectorKeyCID - and they shouldn't be able to snapped.

Later on, when you can snap on snap deal sector, the DealID array wont be 0 length, but then you can get a non-null sectorkeycid -> can be updated again!

Yeah, that will become the change, but right now it's just "are there currently any deals"

We could also assert that the keycid is null -- if that isn't true but there are no deals, that's programmer error. Still good to catch

jennijuju · 2021-11-24T06:02:21Z

actors/builtin/miner/miner_actor.go

+			&builtin.Discard{},
+		)
+
+		if code != exitcode.Ok {


well.. annoying me again - but like...should we consider active the valid deals instead of skipping the whole sector? (similar to what was discussed for prove commit back then for chocolate )

Would be nice, but the Market Actor doesn't tell you which deals were successfully activated and which ones were failures, so it's impossible to selectively skip.

jennijuju · 2021-11-24T06:10:45Z

actors/builtin/miner/miner_actor.go

+
+	// Errors past this point cause the ProveReplicaUpdates call to fail (no more skipping sectors)
+
+	dealWeights := requestDealWeights(rt, sectorsDeals)


okie so this is more of a q for my understanding

so seems like we can get the deal weights from activate deal earlier if we want, so we should save the value back then to save some gas cost here? cuz seems like VerifyDealsForActivation will cause some gas usage for loading the state again ?

Hmm, that's a great question. It seems like VerifyDealsForActivation and ValidateDealsForActivation do different things? But they both calculate weights, but in different ways? And the call to ValidateDealsForActivation in ActivateDeals explicitly ignores the resultant weight calculations?

@ZenGround0 pls help

jennijuju · 2021-11-24T06:15:00Z

actors/builtin/miner/policy.go

@@ -167,6 +167,10 @@ var MaxProveCommitDuration = map[abi.RegisteredSealProof]abi.ChainEpoch{
 // 32 sectors per epoch would support a single miner onboarding 1EiB of 32GiB sectors in 1 year.
 const PreCommitSectorBatchMaxSize = 256

+// The maximum number of sector replica updates in a single batch.
+// Same as PreCommitSectorBatchMaxSize for consistency
+const ProveReplicaUpdatesMaxSize = PreCommitSectorBatchMaxSize


I believe this should be re-evaluate and lowered according to our call today?

Snapdeals wip

e2abd33

arajasek requested a review from a team as a code owner November 10, 2021 22:39

arajasek mentioned this pull request Nov 10, 2021

Integrate v7 actors filecoin-project/lotus#7617

Merged

ZenGround0 suggested changes Nov 11, 2021

View reviewed changes

ZenGround0 reviewed Nov 11, 2021

View reviewed changes

actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved

Address review wave 1

95a9aae

This was referenced Nov 11, 2021

ProveCommitReplicaUpdate implementation for SnapDeals #1511

Closed

Actors integration filecoin-project/lotus#7517

Closed

SnapDeals SectorKey #1530

Closed

BlocksOnAChain assigned arajasek Nov 12, 2021

BlocksOnAChain added this to the SnapDeals implementation milestone Nov 12, 2021

arajasek commented Nov 12, 2021

View reviewed changes

actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved

Group updates by deadline

7dd6ebe

arajasek force-pushed the asr/snapdeals branch from e490acd to 7dd6ebe Compare November 12, 2021 20:47

ZenGround0 reviewed Nov 15, 2021

View reviewed changes

Some improvements

d3addfe

ZenGround0 reviewed Nov 16, 2021

View reviewed changes

ZenGround0 suggested changes Nov 16, 2021

View reviewed changes

actors/builtin/miner/miner_actor.go Show resolved Hide resolved

actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved

actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved

actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved

arajasek force-pushed the asr/snapdeals branch from 48be608 to af88422 Compare November 16, 2021 21:39

arajasek commented Nov 16, 2021

View reviewed changes

actors/builtin/miner/miner_actor.go Show resolved Hide resolved

arajasek force-pushed the asr/snapdeals branch from af88422 to 74d8d15 Compare November 16, 2021 22:05

arajasek commented Nov 16, 2021

View reviewed changes

test-vectors/determinism-check Show resolved Hide resolved

More improvements

f703465

arajasek force-pushed the asr/snapdeals branch from 74d8d15 to f703465 Compare November 16, 2021 22:28

Add proof type to ReplicaUpdateInfo

301b685

arajasek commented Nov 16, 2021

View reviewed changes

actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved

ZenGround0 reviewed Nov 17, 2021

View reviewed changes

actors/runtime/proof/verify.go Outdated Show resolved Hide resolved

ZenGround0 reviewed Nov 17, 2021

View reviewed changes

actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved

ZenGround0 approved these changes Nov 17, 2021

View reviewed changes

arajasek force-pushed the asr/snapdeals branch from d108703 to b53b171 Compare November 17, 2021 17:03

Last wave of review

fd07a4c

arajasek force-pushed the asr/snapdeals branch from b53b171 to fd07a4c Compare November 17, 2021 17:09

ZenGround0 reviewed Nov 17, 2021

View reviewed changes

actors/builtin/miner/miner_actor.go Show resolved Hide resolved

ZenGround0 approved these changes Nov 17, 2021

View reviewed changes

arajasek merged commit ea5b782 into master Nov 17, 2021

Kubuxu reviewed Nov 23, 2021

View reviewed changes

jennijuju reviewed Nov 24, 2021

View reviewed changes

arajasek deleted the asr/snapdeals branch December 18, 2021 05:47

noot mentioned this pull request Jan 6, 2022

NV15 tracking issue ChainSafe/forest#1352

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement FIP-0019 #1529

Implement FIP-0019 #1529

arajasek commented Nov 10, 2021 •

edited

Loading

ZenGround0 left a comment

ZenGround0 Nov 10, 2021

Kubuxu Nov 14, 2021

ZenGround0 Nov 15, 2021

arajasek Nov 15, 2021

ZenGround0 left a comment

ZenGround0 Nov 15, 2021

ZenGround0 left a comment

codecov-commenter commented Nov 16, 2021 •

edited

Loading

ZenGround0 left a comment

Kubuxu Nov 23, 2021

Kubuxu Nov 23, 2021

jennijuju Nov 24, 2021

jennijuju Nov 24, 2021

jennijuju Nov 24, 2021

arajasek Nov 24, 2021

jennijuju Nov 24, 2021

arajasek Nov 24, 2021

jennijuju Nov 24, 2021

arajasek Nov 24, 2021

jennijuju Nov 24, 2021

arajasek Nov 24, 2021

jennijuju Nov 24, 2021

arajasek Nov 24, 2021

jennijuju Nov 24, 2021

arajasek Nov 24, 2021

jennijuju Nov 24, 2021

arajasek Nov 24, 2021

jennijuju Nov 24, 2021

arajasek Nov 24, 2021

jennijuju Nov 24, 2021


		builtin.RequireNoErr(rt, err, exitcode.ErrIllegalArgument, "failed to verify replica proof for sector %d", updateWithDetails.sectorInfo.SectorNumber)

		newSectorInfo := updateWithDetails.sectorInfo


		sectorNumbers.Set(uint64(update.SectorID))

		if len(update.ReplicaProof) > 4096 {


		// Errors past this point cause the ProveReplicaUpdates call to fail (no more skipping sectors)

		dealWeights := requestDealWeights(rt, sectorsDeals)

Implement FIP-0019 #1529

Implement FIP-0019 #1529

Conversation

arajasek commented Nov 10, 2021 • edited Loading

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZenGround0 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 16, 2021 • edited Loading

Codecov Report

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arajasek commented Nov 10, 2021 •

edited

Loading

codecov-commenter commented Nov 16, 2021 •

edited

Loading