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.

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

Support seal proof type switching #4873

Merged
merged 25 commits into from
Nov 17, 2020
Merged

Support seal proof type switching #4873

merged 25 commits into from
Nov 17, 2020

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Nov 16, 2020

No description provided.

Copy link
Contributor Author

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

(looks good to me)

@magik6k magik6k marked this pull request as ready for review November 16, 2020 20:43
@arajasek arajasek added this to the 🚢Lotus v1.2.0 milestone Nov 17, 2020
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Looks like it should work, and apparently it does.

@magik6k magik6k merged commit 693bbee into release/v1.2.0 Nov 17, 2020
@magik6k magik6k deleted the feat/sdr-upgrade branch November 17, 2020 09:57
@@ -40,13 +40,10 @@ const UpgradeKumquatHeight = 170000

// TODO: Height??
const UpgradeCalicoHeight = 999999
Copy link
Member

Choose a reason for hiding this comment

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

^^^^^^^ We need to change this.

@@ -40,13 +40,10 @@ const UpgradeKumquatHeight = 170000

// TODO: Height??
const UpgradeCalicoHeight = 999999
const UpgradePersianHeight = UpgradeCalicoHeight + (builtin2.EpochsInDay * 2)
Copy link
Member

Choose a reason for hiding this comment

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

And decide on this.

@@ -30,7 +30,7 @@ require (
github.com/filecoin-project/go-crypto v0.0.0-20191218222705-effae4ea9f03
github.com/filecoin-project/go-data-transfer v1.1.0
github.com/filecoin-project/go-fil-commcid v0.0.0-20200716160307-8f644712406f
github.com/filecoin-project/go-fil-markets v1.0.4
github.com/filecoin-project/go-fil-markets v1.0.5-0.20201113164554-c5eba40d5335
Copy link
Member

Choose a reason for hiding this comment

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

@@ -41,7 +41,7 @@ require (
github.com/filecoin-project/go-storedcounter v0.0.0-20200421200003-1c99c62e8a5b
github.com/filecoin-project/specs-actors v0.9.13
github.com/filecoin-project/specs-actors/v2 v2.3.0
github.com/filecoin-project/specs-storage v0.1.1-0.20200907031224-ed2e5cd13796
github.com/filecoin-project/specs-storage v0.1.1-0.20201105051918-5188d9774506
Copy link
Member

Choose a reason for hiding this comment

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

@@ -339,7 +340,7 @@ func Online() Option {
Override(new(stores.SectorIndex), From(new(*stores.Index))),
Override(new(dtypes.MinerID), modules.MinerID),
Override(new(dtypes.MinerAddress), modules.MinerAddress),
Override(new(*ffiwrapper.Config), modules.ProofsConfig),
Override(new(abi.RegisteredSealProof), modules.SealProofType),
Copy link
Member

Choose a reason for hiding this comment

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

We need to do this to fetch the params, but I'm not happy with this approach. We should, ideally, fetch the params on-demand where we actually need them. Alternatively, maybe just pre-fetch all params always?

@@ -171,7 +165,7 @@ func (a *API) ClientStartDeal(ctx context.Context, params *api.StartDealParams)
EndEpoch: calcDealExpiration(params.MinBlocksDuration, md, dealStart),
Price: params.EpochPrice,
Collateral: params.ProviderCollateral,
Rt: rt,
Rt: mi.SealProofType,
Copy link
Member

Choose a reason for hiding this comment

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

Note: we could just let the market figure out the correct seal proof type, I think.

@@ -133,7 +138,16 @@ func (m *StateModule) StateMinerInfo(ctx context.Context, actor address.Address,
return miner.MinerInfo{}, xerrors.Errorf("failed to load miner actor state: %w", err)
}

return mas.Info()
// TODO: You know, this is terrible.
Copy link
Member

Choose a reason for hiding this comment

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

Going forward, I believe the right way to do this may be to:

  1. Remove the proof type from the API MinerInfo type (leave it in the miner actor, just strip it from the API).
  2. Provide a new API for getting the proof type.

Well, right-ish enough. However, probably not the highest priority.

@top-org
Copy link

top-org commented Nov 24, 2020

Could anyone share to Filecoin Community about why Filecoin Team introduce new proof type at v1.2.0 ? It looks like that just wrap proof type from function into Sector.StorageRef structure ,it is better to have it but we not see any algorithm-related change at filecoin-ffi and Rust layer(Storage Proof 5.4.0) . so what is point/purpose to introduce those huge change ?

bibibong pushed a commit to EpiK-Protocol/go-epik that referenced this pull request Jan 7, 2021
…dr-upgrade

Support seal proof type switching
@dkkapur
Copy link
Contributor

dkkapur commented Jan 11, 2021

@top-org - A bug was identified in Filecoin’s Proving Subsystem which allowed miners to exploit parallelism in SDR. Specifically, the expander graph was truncated to always fall in the first 16GiB of a sector due to an integer truncation bug in the expander parents generation. The bug was reported and fixed by the rust-fil-proofs team, resulting in a new proof type being released (5.4.0). Old sectors affected cannot be added on chain since epoch 272400.

Thanks to @TyreeRoby for disclosing this issue to us and working with us to solve this in a timely manner.

@Kubuxu
Copy link
Contributor

Kubuxu commented Aug 30, 2022

For future reference, the underlying fix is here (as I decided to chase it down): filecoin-project/rust-fil-proofs@0d17d74#diff-f9e7b9c546dbd93a3981eb032af33d9f52f4e7d4403192d1f26f77c886d5fa31L158-L182

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants