refactor MinerFilter func, update comments

lib/harmony/harmonydb/sql/20240401-storage-miner-filter.sql

@@ -0,0 +1,2 @@
+ALTER TABLE storage_path ADD COLUMN IF NOT EXISTS allow_miners varchar DEFAULT '';
+ALTER TABLE storage_path ADD COLUMN IF NOT EXISTS deny_miners varchar DEFAULT ''; -- comma separated list of miner addresses

storage/paths/db_index.go

@@ -24,6 +24,8 @@ import (
 	"github.com/filecoin-project/lotus/storage/sealer/storiface"
 )
 
+const NoMinerFilter = abi.ActorID(0)
+
 var errAlreadyLocked = errors.New("already locked")
 
 type DBIndex struct {
@@ -578,21 +580,13 @@ func (dbi *DBIndex) StorageFindSector(ctx context.Context, s abi.SectorID, ft st
 				continue
 			}
 			allowMiners := splitString(row.AllowMiners)
-			proceed, err := MinerFilter(allowMiners, false, s.Miner)
-			if err != nil {
-				return nil, err
-			}
-			if !proceed {
-				log.Debugf("not allocating on %s, miner %s not allowed", row.StorageId, s.Miner.String())
-				continue
-			}
 			denyMiners := splitString(row.DenyMiners)
-			proceed, err = MinerFilter(denyMiners, true, s.Miner)
+			proceed, msg, err := MinerFilter(allowMiners, denyMiners, s.Miner)
 			if err != nil {
 				return nil, err
 			}
 			if !proceed {
-				log.Debugf("not allocating on %s, miner %s denied", row.StorageId, s.Miner.String())
+				log.Debugf("not allocating on %s, miner %s %s", row.StorageId, s.Miner.String(), msg)
 				continue
 			}
 
@@ -734,7 +728,6 @@ func (dbi *DBIndex) StorageBestAlloc(ctx context.Context, allocate storiface.Sec
 		SkippedHeartbeatThresh.Seconds(),
 		pathType == storiface.PathSealing,
 		pathType == storiface.PathStorage,
-		miner.String(),
 	)
 	if err != nil {
 		return nil, xerrors.Errorf("Querying for best storage sectors fails with err %w: ", err)
@@ -745,23 +738,15 @@ func (dbi *DBIndex) StorageBestAlloc(ctx context.Context, allocate storiface.Sec
 	for _, row := range rows {
 		// Matching with 0 as a workaround to avoid having minerID
 		// present when calling TaskStorage.HasCapacity()
-		if !(miner == abi.ActorID(0)) {
+		if !(miner == NoMinerFilter) {
 			allowMiners := splitString(row.AllowMiners)
-			proceed, err := MinerFilter(allowMiners, false, miner)
-			if err != nil {
-				return nil, err
-			}
-			if !proceed {
-				log.Debugf("not allocating on %s, miner %s not allowed", row.StorageId, miner.String())
-				continue
-			}
 			denyMiners := splitString(row.DenyMiners)
-			proceed, err = MinerFilter(denyMiners, true, miner)
+			proceed, msg, err := MinerFilter(allowMiners, denyMiners, miner)
 			if err != nil {
 				return nil, err
 			}
 			if !proceed {
-				log.Debugf("not allocating on %s, miner %s denied", row.StorageId, miner.String())
+				log.Debugf("not allocating on %s, miner %s %s", row.StorageId, miner.String(), msg)
 				continue
 			}
 		}

storage/paths/index.go

@@ -498,23 +498,13 @@ func (i *MemIndex) StorageFindSector(ctx context.Context, s abi.SectorID, ft sto
 				continue
 			}
 
-			proceed, err := MinerFilter(st.info.AllowMiners, false, s.Miner)
+			proceed, msg, err := MinerFilter(st.info.AllowMiners, st.info.DenyMiners, s.Miner)
 			if err != nil {
 				return nil, err
 			}
 
 			if !proceed {
-				log.Debugf("not allocating on %s, miner %s not allowed", st.info.ID, s.Miner.String())
-				continue
-			}
-
-			proceed, err = MinerFilter(st.info.DenyMiners, true, s.Miner)
-			if err != nil {
-				return nil, err
-			}
-
-			if !proceed {
-				log.Debugf("not allocating on %s, miner %s denied", st.info.ID, s.Miner.String())
+				log.Debugf("not allocating on %s, miner %s %s", st.info.ID, s.Miner.String(), msg)
 				continue
 			}
 
@@ -673,23 +663,13 @@ func (i *MemIndex) StorageBestAlloc(ctx context.Context, allocate storiface.Sect
 			continue
 		}
 
-		proceed, err := MinerFilter(p.info.AllowMiners, false, miner)
+		proceed, msg, err := MinerFilter(p.info.AllowMiners, p.info.DenyMiners, miner)
 		if err != nil {
 			return nil, err
 		}
 
 		if !proceed {
-			log.Debugf("not allocating on %s, miner %s not allowed", p.info.ID, miner.String())
-			continue
-		}
-
-		proceed, err = MinerFilter(p.info.DenyMiners, true, miner)
-		if err != nil {
-			return nil, err
-		}
-
-		if !proceed {
-			log.Debugf("not allocating on %s, miner %s denied", p.info.ID, miner.String())
+			log.Debugf("not allocating on %s, miner %s %s", p.info.ID, miner.String(), msg)
 			continue
 		}
 
@@ -751,10 +731,9 @@ func (i *MemIndex) FindSector(id abi.SectorID, typ storiface.SectorFileType) ([]
 
 var _ SectorIndex = &MemIndex{}
 
-func MinerFilter(miners []string, deny bool, miner abi.ActorID) (bool, error) {
-	if len(miners) > 0 {
-		found := false
-		for _, m := range miners {
+func MinerFilter(allowMiners, denyMiners []string, miner abi.ActorID) (bool, string, error) {
+	checkMinerInList := func(minersList []string, miner abi.ActorID) (bool, error) {
+		for _, m := range minersList {
 			minerIDStr := m
 			maddr, err := address.NewFromString(minerIDStr)
 			if err != nil {
@@ -765,26 +744,24 @@ func MinerFilter(miners []string, deny bool, miner abi.ActorID) (bool, error) {
 				return false, xerrors.Errorf("converting miner address to ID: %w", err)
 			}
 			if abi.ActorID(mid) == miner {
-				found = true
-				break
+				return true, nil
 			}
 		}
-		// If not found in list
-		if !found {
-			// If this is allowed list
-			if !deny {
-				return false, nil
-			}
-			// If this is denied list
-			return true, nil
+		return false, nil
+	}
+
+	if len(allowMiners) > 0 {
+		found, err := checkMinerInList(allowMiners, miner)
+		if err != nil || !found {
+			return false, "not allowed", err
 		}
-		// If found in list and
-		// If this is allowed list
-		if !deny {
-			return true, nil
+	}
+
+	if len(denyMiners) > 0 {
+		found, err := checkMinerInList(denyMiners, miner)
+		if err != nil || found {
+			return false, "denied", err
 		}
-		// If this is denied list
-		return false, nil
 	}
-	return true, nil
+	return true, "", nil
 }

storage/paths/local.go

@@ -511,14 +511,7 @@ func (st *Local) AcquireSector(ctx context.Context, sid storiface.SectorRef, exi
 		if !fileType.Allowed(allowTypes, denyTypes) {
 			return false, nil
 		}
-		proceed, err := MinerFilter(allowMiners, false, miner)
-		if err != nil {
-			return false, err
-		}
-		if !proceed {
-			return false, nil
-		}
-		proceed, err = MinerFilter(denyMiners, true, miner)
+		proceed, _, err := MinerFilter(allowMiners, denyMiners, miner)
 		if err != nil {
 			return false, err
 		}

storage/sealer/storiface/filetype.go

@@ -8,28 +8,28 @@ import (
 	"github.com/filecoin-project/go-state-types/abi"
 )
 
-// FTUnsealed represents an unsealed sector file type.
-// FTSealed represents a sealed sector file type.
-// FTCache represents a cache sector file type.
-// FTUpdate represents an update sector file type.
-// FTUpdateCache represents an update cache sector file type.
-// FTPiece represents a Piece Park sector file type.
-// FileTypes represents the total number of file types.
+// FTUnsealed represents an unsealed sector file.
+// FTSealed represents a sealed sector file.
+// FTCache represents a cache sector file.
+// FTUpdate represents an update sector file.
+// FTUpdateCache represents an update cache sector file.
+// FTPiece represents a Piece Park sector file.
+// FileTypes represents the total number of file.
 //
-// The SectorFileType type is an integer type that represents different sector file types.
-// It has several methods to manipulate and query the file type.
-// The String method returns a string representation of the file type.
-// The Strings method returns a slice of string representations of all the file types that are set in the receiver object.
-// The AllSet method returns a slice of all the file types that are set in the receiver object.
+// The SectorFileType type is an integer type that represents different sector file.
+// It has several methods to manipulate and query the file.
+// The String method returns a string representation of the file.
+// The Strings method returns a slice of string representations of all the file that are set in the receiver object.
+// The AllSet method returns a slice of all the file that are set in the receiver object.
 // The Has method checks whether a specific file type is set in the receiver object.
 // The SealSpaceUse method calculates the space used by the receiver object in sealing a sector of a given size.
-// The SubAllowed method removes selected file types from the receiver object based on a list of allowed and denied file types.
-// The Unset method removes selected file types from the receiver object.
-// The AnyAllowed method checks whether any file types in the receiver object are allowed based on a list of allowed and denied file types.
-// The Allowed method checks whether all file types in the receiver object are allowed based on a list of allowed and denied file types.
+// The SubAllowed method removes selected file from the receiver object based on a list of allowed and denied file.
+// The Unset method removes selected file from the receiver object.
+// The AnyAllowed method checks whether any file in the receiver object are allowed based on a list of allowed and denied file.
+// The Allowed method checks whether all file in the receiver object are allowed based on a list of allowed and denied file.
 // The StoreSpaceUse method calculates the space used by the receiver object in storing a sector of a given size.
-// The All method returns an array that represents which file types are set in the receiver object.
-// The IsNone method checks whether the receiver object represents no file types.
+// The All method returns an array that represents which file are set in the receiver object.
+// The IsNone method checks whether the receiver object represents no file.
 const (
 	// "regular" sectors
 	FTUnsealed SectorFileType = 1 << iota
@@ -56,14 +56,14 @@ const (
 // - FTPiece: represents Piece Park sectors
 var PathTypes = []SectorFileType{FTUnsealed, FTSealed, FTCache, FTUpdate, FTUpdateCache, FTPiece}
 
-// FTNone represents a sector file type of none. This constant is used in the StorageLock method to specify that a sector should not have any file types locked.
+// FTNone represents a sector file type of none. This constant is used in the StorageLock method to specify that a sector should not have any file locked.
 // Example usage:
 // err := m.index.StorageLock(ctx, sector.ID, storiface.FTNone, storiface.FTSealed|storiface.FTUnsealed|storiface.FTCache)
 const (
 	FTNone SectorFileType = 0
 )
 
-// FTAll represents the combination of all available sector file types.
+// FTAll represents the combination of all available sector file.
 // It is a variable of type SectorFileType.
 // The value of FTAll is calculated by iterating over the PathTypes slice and using the |= operator to perform a bitwise OR operation on each path type.
 // The result is assigned to the variable out and returned.
@@ -78,7 +78,7 @@ var FTAll = func() (out SectorFileType) {
 // FSOverheadDen represents the constant value 10, which is used to calculate the overhead in various storage space utilization calculations.
 const FSOverheadDen = 10
 
-// FSOverheadSeal is a map that represents the overheads for different SectorFileType in sealed sectors.
+// FSOverheadSeal is a map that represents the overheads for different SectorFileType in sectors which are being sealed.
 var FSOverheadSeal = map[SectorFileType]int{ // 10x overheads
 	FTUnsealed:    FSOverheadDen,
 	FTSealed:      FSOverheadDen,
@@ -93,27 +93,9 @@ var FSOverheadSeal = map[SectorFileType]int{ // 10x overheads
 // FsOverheadFinalized is a map that represents the finalized overhead for different types of SectorFileType.
 // The keys in the map are the SectorFileType values, and the values are integers representing the overhead.
 // It is used to calculate the storage space usage for different types of sectors, as shown in the example below:
-//
-//	func (t SectorFileType) StoreSpaceUse(ssize abi.SectorSize) (uint64, error) {
-//		var need uint64
-//		for _, pathType := range PathTypes {
-//			if !t.Has(pathType) {
-//				continue
-//			}
-//
-//			oh, ok := FsOverheadFinalized[pathType]
-//			if !ok {
-//				return 0, xerrors.Errorf("no finalized overhead info for %s", pathType)
-//			}
-//
-//			need += uint64(oh) * uint64(ssize) / FSOverheadDen
-//		}
-//
-//		return need, nil
-//	}
-//
 // The overhead value is retrieved from FsOverheadFinalized by using the SectorFileType value as the key.
-// If the overhead value is not found in the map, an error is returned indicating that there is no finalized overhead information for the given sector type.
+// If the overhead value is not found in the map, an error is returned indicating that there is no finalized
+// overhead information for the given sector type.
 var FsOverheadFinalized = map[SectorFileType]int{
 	FTUnsealed:    FSOverheadDen,
 	FTSealed:      FSOverheadDen,