-
Notifications
You must be signed in to change notification settings - Fork 170
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
First draft of FIP-0076: Direct data onboarding #804
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably planning to cover this in incomplete Design Rationale but make sure there is some discussion of the internal changes to sector termination and its role in motivating sector number => deal ids index in market actor
c797198
to
5448ea5
Compare
@anorth - should the |
5448ea5
to
093e566
Compare
FIPS/fip-direct-onboarding.md
Outdated
Removes the legacy `PreCommitSector` (method 6) and `PreCommitSectorBatch` (method 25) | ||
in favour of the existing `PreCommitSectorBatch2` (method 28) which requires | ||
the sector unsealed CID to be specified while pre-committing. | ||
Removes the legacy and unused `ProveReplicaUpdates2` (method 29) (giving that name to the new method 34). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Peer: Ideally I would prefer a new method number to be assigned, and deprecate the old message/drop the code an upgrade after. Imagine a user sends legacy PRU2 message right before the upgrade, the message will fail upon block inclusion if it happens right after the network upgrade. It is a failure we could avoid for users with minimum cost imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See design rationale. It's just not going to happen - it has never been used.
FIPS/fip-direct-onboarding.md
Outdated
|
||
## Simple Summary | ||
|
||
Adds new `ProveCommitSectors2` (method 33) and `ProveReplicaUpdates2` (method 34) methods to the miner actor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These names are gonna get confusing. Can we consider naming them something more specific, instead of just having ProveCommitSectors
and ProveCommitSectors2
(and so on)?
We can also rename the older methods too if we so wanted to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you suggest some names? I've been unable to find something appropriately concise yet more descriptive of the difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I didn't have any good ideas either :(
FIPS/fip-direct-onboarding.md
Outdated
- Support data onboarding, including Fil+ verified data, with no smart-contract intermediary; | ||
- Provide a new scheme for data activation which can later support | ||
user-programmed smart contracts to function as data storage applications (including as markets); | ||
- Continue supporting existing onboarding methods to give participants time to migrate their workflows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Continue supporting existing onboarding methods to give participants time to migrate their workflows. | |
- Continue supporting existing onboarding methods to keep existing f05 storage markets flow and functionality remain supported (for clients who would like to have onchain payments) and give participants time to migrate their workflows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parties can have on-chain payments without using the old methods. The old methods remain only for transition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parties can have on-chain payments without using the old methods
I understand - however, I think it is too soon to assume all network users want to migrate to the new flow. if we can have a quick mention on if one would like keep their exact existing flow - how should they do so might be useful. i.e: PSD -> PreCommitBatch2 -> ProveCommitSectors/ProveCommitAggregation; or PSD -> PreCommitBatch2 -> ProveCommitSectors2 with RequireNotificationSuccess=true && RequireActivationSuccess==true
|
||
This is a change in sequencing and semantics from the existing onboarding methods flow, | ||
where deal activation happens first and failure leads to an individual sector failing, | ||
and verified claims happen second and must all succeed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please specify if a sector contains both f05 deal pieces and non f05 pieces, when notification failed for f05 pieces yet the sector is successfully activated - what would the deal pub activated: ChainEpoch,
value be?
and how would sector_start_epoch
& last_updated_epoch
be updated accordingly? (if it is specified in the later session, please kindly send a premark link, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the notification fails the deal is not activated. So there is no activated epoch, sector_start_epoch, or last_updated_epoch.
0271adb
to
8aee21c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty close to landable, just a couple notes.
8aee21c
to
008028a
Compare
|
||
**The activation of each sector is independent**. | ||
A failed sector activation will not cause a top-level method to abort, unless all activations fail. | ||
An SP can specify `RequireActivationSuccess=true` to instead require every sector activation to succeed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
editorial: is there any reason why we are still using go spec actor convention even its already deprecated and we are only implementing in rust? it would took less time to look up code reference if we just using require_notification_success
here (and same for the rest of the fip
008028a
to
987b3f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have nothing to point out at this stage. The proposal is well supported and very well described, addressing all key concerns raised in discussion and review. Looks ready to merge. Excellent work.
|
||
#### ProveCommitSector | ||
|
||
The existing `ProveCommitSector` (method 7) and `ProveCommitAggregate` (method 26) remain, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so I understand- the only group of people who would still be using these methods and submitting sectors via the builtin market actor are those who are (theoretically) working with clients on-chain? Am I misunderstanding this workflow?
Please note this is just a curiosity; it's unrelated to my editorial review of this draft.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, everyone will be using these methods because it will take time for operations to migrate to the new flow. They remain as part of a two-stage migration to avoid requiring all operations to update in synchrony.
In the long run, no-one should use them and I think we should remove them. Parties can still use the built-in storage market actor with the new methods too.
### Migration | ||
|
||
The built-in market actor's `ProviderSectors` mapping is initialised from the existing deal state | ||
and miner actor state per-sector deal IDs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this jargon that I'm unfamiliar with? Either the hyphen should be removed, or it should read as "state-per-sector".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you're supposed to read it as the per-sector deal IDs in the miner actor state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial review in. A few (very small) nits. Otherwise, great proposal. Sorry for the delay on a detailed review.
4831750
to
7f53d4e
Compare
Adds new
ProveCommitSectors2
(method 33) andProveReplicaUpdates2
(method 34) methods to the miner actorwhich support committing to data and claiming verified allocations without requiring a built-in market deal.
Adds a sector→deal mapping in the built-in market actor and deprecates the storage of deal IDs in the miner actor.
Removes the legacy
PreCommitSector
(method 6) andPreCommitSectorBatch
(method 25)in favour of the existing
PreCommitSectorBatch2
(method 28) which requiresthe sector unsealed CID to be specified while pre-committing.
Removes the legacy and unused
ProveReplicaUpdates2
(method 29) (giving that name to the new method 34).Existing onboarding flows that use the built-in market actor remain fully supported, but optional.
Discussion: #730