Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PreCommitPolicy: Don't try to align expirations on proving period boundaries #7018

Merged
merged 1 commit into from
Aug 11, 2021
Merged

PreCommitPolicy: Don't try to align expirations on proving period boundaries #7018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion chain/actors/builtin/miner/actor.go.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func init() {

var Methods = builtin{{.latestVersion}}.MethodsMiner

// Unchanged between v0, v2, v3, and v4 actors
// Unchanged between v0, v2, v3, v4, and v5 actors
var WPoStProvingPeriod = miner0.WPoStProvingPeriod
var WPoStPeriodDeadlines = miner0.WPoStPeriodDeadlines
var WPoStChallengeWindow = miner0.WPoStChallengeWindow
Expand Down
2 changes: 1 addition & 1 deletion chain/actors/builtin/miner/miner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func init() {

var Methods = builtin5.MethodsMiner

// Unchanged between v0, v2, v3, and v4 actors
// Unchanged between v0, v2, v3, v4, and v5 actors
var WPoStProvingPeriod = miner0.WPoStProvingPeriod
var WPoStPeriodDeadlines = miner0.WPoStPeriodDeadlines
var WPoStChallengeWindow = miner0.WPoStChallengeWindow
Expand Down
4 changes: 4 additions & 0 deletions chain/actors/policy/policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,10 @@ func GetMaxSectorExpirationExtension() abi.ChainEpoch {
return miner5.MaxSectorExpirationExtension
}

func GetMinSectorExpiration() abi.ChainEpoch {
return miner5.MinSectorExpiration
}

func GetMaxPoStPartitions(nv network.Version, p abi.RegisteredPoStProof) (int, error) {
sectorsPerPart, err := builtin5.PoStProofWindowPoStPartitionSectors(p)
if err != nil {
Expand Down
4 changes: 4 additions & 0 deletions chain/actors/policy/policy.go.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,10 @@ func GetMaxSectorExpirationExtension() abi.ChainEpoch {
return miner{{.latestVersion}}.MaxSectorExpirationExtension
}

func GetMinSectorExpiration() abi.ChainEpoch {
return miner{{.latestVersion}}.MinSectorExpiration
}

func GetMaxPoStPartitions(nv network.Version, p abi.RegisteredPoStProof) (int, error) {
sectorsPerPart, err := builtin{{.latestVersion}}.PoStProofWindowPoStPartitionSectors(p)
if err != nil {
Expand Down
15 changes: 9 additions & 6 deletions extern/storage-sealing/precommit_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,17 @@ type BasicPreCommitPolicy struct {
api Chain
getSealingConfig GetSealingConfigFunc

provingBoundary abi.ChainEpoch
provingBuffer abi.ChainEpoch
provingBuffer abi.ChainEpoch
}

// NewBasicPreCommitPolicy produces a BasicPreCommitPolicy.
//
// The provided duration is used as the default sector expiry when the sector
// contains no deals. The proving boundary is used to adjust/align the sector's expiration.
func NewBasicPreCommitPolicy(api Chain, cfgGetter GetSealingConfigFunc, provingBoundary abi.ChainEpoch, provingBuffer abi.ChainEpoch) BasicPreCommitPolicy {
func NewBasicPreCommitPolicy(api Chain, cfgGetter GetSealingConfigFunc, provingBuffer abi.ChainEpoch) BasicPreCommitPolicy {
return BasicPreCommitPolicy{
api: api,
getSealingConfig: cfgGetter,
provingBoundary: provingBoundary,
provingBuffer: provingBuffer,
}
}
Expand Down Expand Up @@ -93,7 +91,12 @@ func (p *BasicPreCommitPolicy) Expiration(ctx context.Context, ps ...Piece) (abi
end = &tmp
}

*end += miner.WPoStProvingPeriod - (*end % miner.WPoStProvingPeriod) + p.provingBoundary - 1
// Ensure there is at least one day for the PC message to land without falling below min sector lifetime
// TODO: The "one day" should probably be a config, though it doesn't matter too much
minExp := epoch + policy.GetMinSectorExpiration() + miner.WPoStProvingPeriod
if *end < minExp {
end = &minExp
}

return *end, nil
}
Expand All @@ -119,5 +122,5 @@ func (p *BasicPreCommitPolicy) getCCSectorLifetime() (abi.ChainEpoch, error) {
return maxExpiration, nil
}

return (ccLifetimeEpochs - p.provingBuffer), nil
return ccLifetimeEpochs - p.provingBuffer, nil
}
36 changes: 14 additions & 22 deletions extern/storage-sealing/precommit_policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
api "github.com/filecoin-project/lotus/api"
"github.com/filecoin-project/lotus/build"
"github.com/filecoin-project/lotus/chain/actors/builtin"
"github.com/filecoin-project/lotus/chain/actors/builtin/miner"
"github.com/filecoin-project/lotus/chain/actors/policy"
sealing "github.com/filecoin-project/lotus/extern/storage-sealing"
"github.com/filecoin-project/lotus/extern/storage-sealing/sealiface"
Expand Down Expand Up @@ -60,17 +59,14 @@ func fakePieceCid(t *testing.T) cid.Cid {
func TestBasicPolicyEmptySector(t *testing.T) {
cfg := fakeConfigGetter(nil)
h := abi.ChainEpoch(55)
pBoundary := abi.ChainEpoch(0)
pBuffer := abi.ChainEpoch(2)
pcp := sealing.NewBasicPreCommitPolicy(&fakeChain{h: h}, cfg, pBoundary, pBuffer)
pcp := sealing.NewBasicPreCommitPolicy(&fakeChain{h: h}, cfg, pBuffer)
exp, err := pcp.Expiration(context.Background())

require.NoError(t, err)

// as set when there are no deal pieces
expected := h + policy.GetMaxSectorExpirationExtension() - (pBuffer * 2)
// as set just before returning within Expiration()
expected += miner.WPoStProvingPeriod - (expected % miner.WPoStProvingPeriod) + pBoundary - 1
expected := h + policy.GetMaxSectorExpirationExtension() - pBuffer
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why these were subtracting out pBuffer * 2, since that isn't what the cc lifetime method seems to do.

Copy link
Contributor

@placer14 placer14 Aug 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did ask for extra scrutiny, but my implementation is definitely wrong here, hidden by the chosen constants. Thanks for catching this. Only rationale I can come with here was to match this but it's external to the Policy entirely. Not sure why I landed with that.

assert.Equal(t, int(expected), int(exp))
}

Expand All @@ -80,27 +76,23 @@ func TestCustomCCSectorConfig(t *testing.T) {
cfgStub := fakeConfigStub{CCSectorLifetime: customLifetime}
cfg := fakeConfigGetter(&cfgStub)
h := abi.ChainEpoch(55)
pBoundary := abi.ChainEpoch(0)
pBuffer := abi.ChainEpoch(2)
pcp := sealing.NewBasicPreCommitPolicy(&fakeChain{h: h}, cfg, pBoundary, pBuffer)
pcp := sealing.NewBasicPreCommitPolicy(&fakeChain{h: h}, cfg, pBuffer)
exp, err := pcp.Expiration(context.Background())

require.NoError(t, err)

// as set when there are no deal pieces
expected := h + customLifetimeEpochs - (pBuffer * 2)
// as set just before returning within Expiration()
expected += miner.WPoStProvingPeriod - (expected % miner.WPoStProvingPeriod) + pBoundary - 1
expected := h + customLifetimeEpochs - pBuffer
assert.Equal(t, int(expected), int(exp))
}

func TestBasicPolicyMostConstrictiveSchedule(t *testing.T) {
cfg := fakeConfigGetter(nil)
pPeriod := abi.ChainEpoch(11)
policy := sealing.NewBasicPreCommitPolicy(&fakeChain{
h: abi.ChainEpoch(55),
}, cfg, pPeriod, 2)
longestDealEpochEnd := abi.ChainEpoch(100)
}, cfg, 2)
longestDealEpochEnd := abi.ChainEpoch(547300)
pieces := []sealing.Piece{
{
Piece: abi.PieceInfo{
Expand All @@ -111,7 +103,7 @@ func TestBasicPolicyMostConstrictiveSchedule(t *testing.T) {
DealID: abi.DealID(42),
DealSchedule: api.DealSchedule{
StartEpoch: abi.ChainEpoch(70),
EndEpoch: abi.ChainEpoch(75),
EndEpoch: abi.ChainEpoch(547275),
},
},
},
Expand All @@ -133,15 +125,14 @@ func TestBasicPolicyMostConstrictiveSchedule(t *testing.T) {
exp, err := policy.Expiration(context.Background(), pieces...)
require.NoError(t, err)

expected := longestDealEpochEnd + miner.WPoStProvingPeriod - (longestDealEpochEnd % miner.WPoStProvingPeriod) + pPeriod - 1
assert.Equal(t, int(expected), int(exp))
assert.Equal(t, int(longestDealEpochEnd), int(exp))
}

func TestBasicPolicyIgnoresExistingScheduleIfExpired(t *testing.T) {
cfg := fakeConfigGetter(nil)
policy := sealing.NewBasicPreCommitPolicy(&fakeChain{
h: abi.ChainEpoch(55),
}, cfg, 0, 0)
}, cfg, 0)

pieces := []sealing.Piece{
{
Expand All @@ -162,14 +153,15 @@ func TestBasicPolicyIgnoresExistingScheduleIfExpired(t *testing.T) {
exp, err := policy.Expiration(context.Background(), pieces...)
require.NoError(t, err)

assert.Equal(t, 1558079, int(exp))
// Treated as a CC sector, so expiration becomes currEpoch + maxLifetime = 55 + 1555200
assert.Equal(t, 1555255, int(exp))
}

func TestMissingDealIsIgnored(t *testing.T) {
cfg := fakeConfigGetter(nil)
policy := sealing.NewBasicPreCommitPolicy(&fakeChain{
h: abi.ChainEpoch(55),
}, cfg, 11, 0)
}, cfg, 0)

pieces := []sealing.Piece{
{
Expand All @@ -181,7 +173,7 @@ func TestMissingDealIsIgnored(t *testing.T) {
DealID: abi.DealID(44),
DealSchedule: api.DealSchedule{
StartEpoch: abi.ChainEpoch(1),
EndEpoch: abi.ChainEpoch(10),
EndEpoch: abi.ChainEpoch(547300),
},
},
},
Expand All @@ -197,5 +189,5 @@ func TestMissingDealIsIgnored(t *testing.T) {
exp, err := policy.Expiration(context.Background(), pieces...)
require.NoError(t, err)

assert.Equal(t, 1558090, int(exp))
assert.Equal(t, 547300, int(exp))
}
8 changes: 3 additions & 5 deletions storage/miner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,12 +179,10 @@ func (m *Miner) Run(ctx context.Context) error {
adaptedAPI = NewSealingAPIAdapter(m.api)

// Instantiate a precommit policy.
cfg = sealing.GetSealingConfigFunc(m.getSealConfig)
provingBoundary = md.PeriodStart % md.WPoStProvingPeriod
provingBuffer = md.WPoStProvingPeriod * 2
cfg = sealing.GetSealingConfigFunc(m.getSealConfig)
provingBuffer = md.WPoStProvingPeriod * 2

// TODO: Maybe we update this policy after actor upgrades?
pcp = sealing.NewBasicPreCommitPolicy(adaptedAPI, cfg, provingBoundary, provingBuffer)
pcp = sealing.NewBasicPreCommitPolicy(adaptedAPI, cfg, provingBuffer)

// address selector.
as = func(ctx context.Context, mi miner.MinerInfo, use api.AddrUse, goodFunds, minFunds abi.TokenAmount) (address.Address, abi.TokenAmount, error) {
Expand Down