review followup

actors/builtin/miner/miner_actor.go

@@ -660,7 +660,7 @@ type PreCommitSectorBatchParams struct {
 
 // Pledges the miner to seal and commit some new sectors.
 // The caller specifies sector numbers, sealed sector data CIDs, seal randomness epoch, expiration, and the IDs
-// of any storage marge deals contained in the sector data. The storage deal proposals must be already submitted
+// of any storage deals contained in the sector data. The storage deal proposals must be already submitted
 // to the storage market actor.
 // A pre-commitment may specify an existing committed-capacity sector that the committed sector will replace
 // when proven.

actors/builtin/miner/miner_commitment_test.go

@@ -520,6 +520,7 @@ func TestPreCommitBatch(t *testing.T) {
 			sectors := make([]*miner0.SectorPreCommitInfo, batchSize)
 			conf := preCommitBatchConf{
 				sectorWeights: make([]market.SectorWeights, batchSize),
+				firstForMiner: true,
 			}
 			deposits := make([]big.Int, batchSize)
 			for i := 0; i < batchSize; i++ {
@@ -545,7 +546,7 @@ func TestPreCommitBatch(t *testing.T) {
 
 			if test.exit != exitcode.Ok {
 				rt.ExpectAbortContainsMessage(test.exit, test.error, func() {
-					actor.preCommitSectorBatch(rt, &miner.PreCommitSectorBatchParams{Sectors: sectors}, conf, true)
+					actor.preCommitSectorBatch(rt, &miner.PreCommitSectorBatchParams{Sectors: sectors}, conf)
 
 					// State untouched.
 					st := getState(rt)
@@ -555,7 +556,7 @@ func TestPreCommitBatch(t *testing.T) {
 				})
 				return
 			}
-			precommits := actor.preCommitSectorBatch(rt, &miner.PreCommitSectorBatchParams{Sectors: sectors}, conf, true)
+			precommits := actor.preCommitSectorBatch(rt, &miner.PreCommitSectorBatchParams{Sectors: sectors}, conf)
 
 			// Check precommits
 			st := getState(rt)
@@ -607,7 +608,7 @@ func TestPreCommitBatch(t *testing.T) {
 		}
 
 		rt.ExpectAbortContainsMessage(exitcode.ErrIllegalArgument, "sector expiration", func() {
-			actor.preCommitSectorBatch(rt, &miner.PreCommitSectorBatchParams{Sectors: sectors}, preCommitBatchConf{}, true)
+			actor.preCommitSectorBatch(rt, &miner.PreCommitSectorBatchParams{Sectors: sectors}, preCommitBatchConf{firstForMiner: true})
 		})
 	})
 
@@ -628,7 +629,7 @@ func TestPreCommitBatch(t *testing.T) {
 			actor.makePreCommit(100, precommitEpoch-1, sectorExpiration, nil),
 		}
 		rt.ExpectAbortContainsMessage(exitcode.ErrIllegalArgument, "duplicate sector number 100", func() {
-			actor.preCommitSectorBatch(rt, &miner.PreCommitSectorBatchParams{Sectors: sectors}, preCommitBatchConf{}, true)
+			actor.preCommitSectorBatch(rt, &miner.PreCommitSectorBatchParams{Sectors: sectors}, preCommitBatchConf{firstForMiner: true})
 		})
 	})
 }
@@ -786,9 +787,10 @@ func TestProveCommit(t *testing.T) {
 				{DealSpace: dealSpace, DealWeight: dealWeight, VerifiedDealWeight: verifiedDealWeight},
 				{DealSpace: dealSpace, DealWeight: dealWeight, VerifiedDealWeight: verifiedDealWeight},
 			},
+			firstForMiner: true,
 		}
 
-		precommits := actor.preCommitSectorBatch(rt, &miner.PreCommitSectorBatchParams{Sectors: sectors}, conf, true)
+		precommits := actor.preCommitSectorBatch(rt, &miner.PreCommitSectorBatchParams{Sectors: sectors}, conf)
 
 		rt.SetEpoch(proveCommitEpoch)
 		noDealPower := miner.QAPowerForWeight(actor.sectorSize, sectorExpiration-proveCommitEpoch, big.Zero(), big.Zero())

actors/builtin/miner/miner_state.go

@@ -311,12 +311,6 @@ const (
 // Marks a set of sector numbers as having been allocated.
 // If policy is `DenyCollisions`, fails if the set intersects with the sector numbers already allocated.
 func (st *State) AllocateSectorNumbers(store adt.Store, sectorNos bitfield.BitField, policy CollisionPolicy) error {
-	if lastSectorNo, err := sectorNos.Last(); err != nil {
-		return xc.ErrIllegalArgument.Wrapf("invalid sector bitfield: %w", err)
-	} else if lastSectorNo > abi.MaxSectorNumber {
-		return xc.ErrIllegalArgument.Wrapf("sector number %d exceeds max %d", lastSectorNo, abi.MaxSectorNumber)
-	}
-
 	var priorAllocation bitfield.BitField
 	if err := store.Get(store.Context(), st.AllocatedSectors, &priorAllocation); err != nil {
 		return xc.ErrIllegalState.Wrapf("failed to load allocated sectors bitfield: %w", err)

actors/builtin/miner/miner_state_test.go

@@ -798,7 +798,6 @@ func TestSectorNumberAllocation(t *testing.T) {
 
 		assert.NoError(t, allocate(harness, 0))
 		assert.NoError(t, allocate(harness, abi.MaxSectorNumber))
-		assert.Error(t, allocate(harness, abi.MaxSectorNumber+1))
 		expect(harness, bf(0, abi.MaxSectorNumber))
 	})
 
@@ -807,7 +806,7 @@ func TestSectorNumberAllocation(t *testing.T) {
 
 		assert.NoError(t, mask(harness, bf(0)))
 		assert.NoError(t, mask(harness, bf(abi.MaxSectorNumber)))
-		assert.Error(t, mask(harness, bf(abi.MaxSectorNumber+1)))
+		expect(harness, bf(0, abi.MaxSectorNumber))
 	})
 
 	t.Run("compaction with mask", func(t *testing.T) {

actors/builtin/miner/miner_test.go

@@ -4228,6 +4228,20 @@ func TestCompactSectorNumbers(t *testing.T) {
 		actor.checkState(rt)
 	})
 
+	t.Run("sector number range limits", func(t *testing.T) {
+		rt := builder.Build(t)
+		actor.constructAndVerify(rt)
+
+		// Limits ok
+		actor.compactSectorNumbers(rt, bf(0, abi.MaxSectorNumber))
+
+		// Out of range fails
+		rt.ExpectAbort(exitcode.ErrIllegalArgument, func() {
+			actor.compactSectorNumbers(rt, bf(abi.MaxSectorNumber+1))
+		})
+		actor.checkState(rt)
+	})
+
 	t.Run("compacting no sector numbers aborts", func(t *testing.T) {
 		rt := builder.Build(t)
 		actor.constructAndVerify(rt)
@@ -4654,10 +4668,14 @@ func (h *actorHarness) preCommitSector(rt *mock.Runtime, params *miner.PreCommit
 }
 
 type preCommitBatchConf struct {
+	// Weights to be returned from the market actor for sectors 0..len(sectorWeights).
+	// Any remaining sectors are taken to have zero deal weight.
 	sectorWeights []market.SectorWeights
+	// Set if this is the first commitment by this miner, hence should expect scheduling end-of-deadline cron.
+	firstForMiner bool
 }
 
-func (h *actorHarness) preCommitSectorBatch(rt *mock.Runtime, params *miner.PreCommitSectorBatchParams, conf preCommitBatchConf, first bool) []*miner.SectorPreCommitOnChainInfo {
+func (h *actorHarness) preCommitSectorBatch(rt *mock.Runtime, params *miner.PreCommitSectorBatchParams, conf preCommitBatchConf) []*miner.SectorPreCommitOnChainInfo {
 	rt.SetCaller(h.worker, builtin.AccountActorCodeID)
 	rt.ExpectValidateCallerAddr(append(h.controlAddrs, h.owner, h.worker)...)
 	{
@@ -4702,7 +4720,7 @@ func (h *actorHarness) preCommitSectorBatch(rt *mock.Runtime, params *miner.PreC
 		rt.ExpectSend(builtin.BurntFundsActorAddr, builtin.MethodSend, nil, st.FeeDebt, nil, exitcode.Ok)
 	}
 
-	if first {
+	if conf.firstForMiner {
 		dlInfo := miner.NewDeadlineInfoFromOffsetAndEpoch(st.ProvingPeriodStart, rt.Epoch())
 		cronParams := makeDeadlineCronEventParams(h.t, dlInfo.Last())
 		rt.ExpectSend(builtin.StoragePowerActorAddr, builtin.MethodsPower.EnrollCronEvent, cronParams, big.Zero(), nil, exitcode.Ok)