Add expected seal duration to custom deal logic #2384
Conversation
| @@ -46,6 +46,10 @@ type StorageMiner interface { | |||
| // SectorGetSealDelay gets the time that a newly-created sector | |||
| // waits for more deals before it starts sealing | |||
| SectorGetSealDelay(context.Context) (time.Duration, error) | |||
| // SectorSetExpectedSealDuration sets the expected time for a sector to seal | |||
| SectorSetExpectedSealDuration(context.Context, time.Duration) error | |||
There was a problem hiding this comment.
We should probably create a new Sealing namespace for some of those functions, Sector is a bit weird here
There was a problem hiding this comment.
Yeah, strong agree...you okay with that being a separate PR?
There was a problem hiding this comment.
Dovetailing with the comments on the deal market PR, I would do the following:
- make a struct to implement deal decision logic
- supply it with appropriate dependencies as needed (Chain Height API, get current seal duration)
- have one of its public methods be the dead decider
- remove all the extra unrelated dependencies to the StorageProvider constructor and just provide the deal decider struct instance
- make the markets PR simpler by just deleting the deal acceptance buffer rather than adding the extra chain height parameter.
node/modules/storageminer.go
Outdated
| net := smnet.NewFromLibp2pHost(h) | ||
| store, err := piecefilestore.NewLocalFileStore(piecefilestore.OsPath(r.Path())) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| opt := storageimpl.CustomDealDecisionLogic(func(ctx context.Context, deal storagemarket.MinerDeal) (bool, string, error) { | ||
| opt := storageimpl.CustomDealDecisionLogic(func(ctx context.Context, deal storagemarket.MinerDeal, ht abi.ChainEpoch) (bool, string, error) { |
There was a problem hiding this comment.
I haven't looked closely at this function but it looks like it's getting pretty long to live in a constructor. I feel like you might just want to create a struct (which could store the current SealDuration in it it) and have one of its methods be the deal decider?
Also, are you sure you don't want us to expose the config options on StorageProvider after construction, so they can be changed later?
|
my review is not blocking for this PR but I'd like to not add the height to deal decider unless it's absolutely neccesary. |
arajasek
force-pushed
the
asr/seal-duration
branch
from
c2bcf4a to
a5ef629
Compare
| func StorageProvider(minerAddress dtypes.MinerAddress, | ||
| ffiConfig *ffiwrapper.Config, | ||
| storedAsk *storedask.StoredAsk, | ||
| h host.Host, ds dtypes.MetadataDS, | ||
| ibs dtypes.StagingBlockstore, | ||
| r repo.LockedRepo, | ||
| pieceStore dtypes.ProviderPieceStore, | ||
| dataTransfer dtypes.ProviderDataTransfer, | ||
| spn storagemarket.StorageProviderNode, | ||
| onlineOk dtypes.ConsiderOnlineStorageDealsConfigFunc, | ||
| offlineOk dtypes.ConsiderOfflineStorageDealsConfigFunc, | ||
| blocklistFunc dtypes.StorageDealPieceCidBlocklistConfigFunc, | ||
| expectedSealTimeFunc dtypes.GetExpectedSealDurationFunc) (storagemarket.StorageProvider, error) { |
There was a problem hiding this comment.
Probably warrants a struct with fx.In (not blocking this PR)
And reject deals that start too early.
Fixes #2048