Use PutIfAbsent to store new sector pre-commit

actors/builtin/miner/miner_actor.go

@@ -739,15 +739,7 @@ func (a Actor) PreCommitSector(rt Runtime, params *PreCommitSectorParams) *abi.E
 		err = st.AllocateSectorNumber(store, params.SectorNumber)
 		builtin.RequireNoErr(rt, err, exitcode.ErrIllegalState, "failed to allocate sector id %d", params.SectorNumber)
 
-		// The following two checks shouldn't be necessary, but it can't
-		// hurt to double-check (unless it's really just too
-		// expensive?).
-		_, preCommitFound, err := st.GetPrecommittedSector(store, params.SectorNumber)
-		builtin.RequireNoErr(rt, err, exitcode.ErrIllegalState, "failed to check pre-commit %v", params.SectorNumber)
-		if preCommitFound {
-			rt.Abortf(exitcode.ErrIllegalState, "sector %v already pre-committed", params.SectorNumber)
-		}
-
+		// This sector check is redundant given the allocated sectors bitfield, but remains for safety.
 		sectorFound, err := st.HasSectorNo(store, params.SectorNumber)
 		builtin.RequireNoErr(rt, err, exitcode.ErrIllegalState, "failed to check sector %v", params.SectorNumber)
 		if sectorFound {

actors/builtin/miner/miner_state.go

@@ -337,15 +337,17 @@ func (st *State) MaskSectorNumbers(store adt.Store, sectorNos bitfield.BitField)
 	return nil
 }
 
+// Stores a pre-committed sector info, failing if the sector number is already present.
 func (st *State) PutPrecommittedSector(store adt.Store, info *SectorPreCommitOnChainInfo) error {
 	precommitted, err := adt.AsMap(store, st.PreCommittedSectors, builtin.DefaultHamtBitwidth)
 	if err != nil {
 		return err
 	}
 
-	err = precommitted.Put(SectorKey(info.Info.SectorNumber), info)
-	if err != nil {
-		return errors.Wrapf(err, "failed to store precommitment for %v", info)
+	if modified, err := precommitted.PutIfAbsent(SectorKey(info.Info.SectorNumber), info); err != nil {
+		return errors.Wrapf(err, "failed to store pre-commitment for %v", info)
+	} else if !modified {
+		return xerrors.Errorf("sector %v already pre-committed", info.Info.SectorNumber)
 	}
 	st.PreCommittedSectors, err = precommitted.Root()
 	return err

actors/builtin/miner/miner_state_test.go

@@ -24,31 +24,35 @@ import (
 func TestPrecommittedSectorsStore(t *testing.T) {
 	t.Run("Put, get and delete", func(t *testing.T) {
 		harness := constructStateHarness(t, abi.ChainEpoch(0))
-		sectorNo := abi.SectorNumber(1)
-
-		pc1 := newSectorPreCommitOnChainInfo(sectorNo, tutils.MakeCID("1", &miner.SealedCIDPrefix), abi.NewTokenAmount(1), abi.ChainEpoch(1))
+		pc1 := newSectorPreCommitOnChainInfo(1, tutils.MakeCID("1", &miner.SealedCIDPrefix), abi.NewTokenAmount(1), abi.ChainEpoch(1))
 		harness.putPreCommit(pc1)
-		assert.Equal(t, pc1, harness.getPreCommit(sectorNo))
+		assert.Equal(t, pc1, harness.getPreCommit(1))
 
-		pc2 := newSectorPreCommitOnChainInfo(sectorNo, tutils.MakeCID("2", &miner.SealedCIDPrefix), abi.NewTokenAmount(1), abi.ChainEpoch(1))
+		pc2 := newSectorPreCommitOnChainInfo(2, tutils.MakeCID("2", &miner.SealedCIDPrefix), abi.NewTokenAmount(1), abi.ChainEpoch(1))
 		harness.putPreCommit(pc2)
-		assert.Equal(t, pc2, harness.getPreCommit(sectorNo))
+		assert.Equal(t, pc2, harness.getPreCommit(2))
 
-		harness.deletePreCommit(sectorNo)
-		assert.False(t, harness.hasPreCommit(sectorNo))
+		harness.deletePreCommit(1)
+		assert.False(t, harness.hasPreCommit(1))
 	})
 
 	t.Run("Delete nonexistent value returns an error", func(t *testing.T) {
 		harness := constructStateHarness(t, abi.ChainEpoch(0))
-		sectorNo := abi.SectorNumber(1)
-		err := harness.s.DeletePrecommittedSectors(harness.store, sectorNo)
+		err := harness.s.DeletePrecommittedSectors(harness.store, 1)
 		assert.Error(t, err)
 	})
 
 	t.Run("Get nonexistent value returns false", func(t *testing.T) {
 		harness := constructStateHarness(t, abi.ChainEpoch(0))
-		sectorNo := abi.SectorNumber(1)
-		assert.False(t, harness.hasPreCommit(sectorNo))
+		assert.False(t, harness.hasPreCommit(1))
+	})
+
+	t.Run("Duplicate put rejected", func(t *testing.T) {
+		harness := constructStateHarness(t, abi.ChainEpoch(0))
+		pc1 := newSectorPreCommitOnChainInfo(1, tutils.MakeCID("1", &miner.SealedCIDPrefix), abi.NewTokenAmount(1), abi.ChainEpoch(1))
+		harness.putPreCommit(pc1)
+		err := harness.s.PutPrecommittedSector(harness.store, pc1)
+		assert.Error(t, err)
 	})
 }
 

actors/util/adt/map.go

@@ -110,6 +110,15 @@ func (m *Map) Has(k abi.Keyer) (bool, error) {
 	}
 }
 
+// Sets key key `k` to value `v` iff the key is not already present.
+func (m *Map) PutIfAbsent(k abi.Keyer, v cbor.Marshaler) (bool, error) {
+	if modified, err := m.root.SetIfAbsent(m.store.Context(), k.Key(), v); err != nil {
+		return false, xerrors.Errorf("failed to set key %v value %v in node %v: %w", k.Key(), v, m.lastCid, err)
+	} else {
+		return modified, nil
+	}
+}
+
 // Removes the value at `k` from the hamt store, if it exists.
 // Returns whether the key was previously present.
 func (m *Map) TryDelete(k abi.Keyer) (bool, error) {