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

FIPXXXX: Introducing sealerID #993

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

FIPXXXX: Introducing sealerID #993

wants to merge 10 commits into from

Conversation

irenegia
Copy link
Contributor

This proposal introduced the sealerID. This is a new identifier created for parties that participate to the Filecoin network by creating replicas, which can then transferred to SPs.

This fully enables the Sealing-as-a-Service scenario already introduced in FIP0090 (NI-PoRep).

Discussion: #890

This proposal introduced the sealerID. This is a new identifier created for parties that participate to the Filecoin network by  creating replicas, which can then transferred to SPs.

This fully enables the Sealing-as-a-Service scenario already introduced in FIP0090 (NI-PoRep).

Discussion: #890
@irenegia irenegia changed the title Create fip-xxx-sealerid.md FIPXXXX: Introducing sealerID-sealerid.md Apr 18, 2024
@irenegia irenegia changed the title FIPXXXX: Introducing sealerID-sealerid.md FIPXXXX: Introducing sealerID Apr 18, 2024
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
2. The list of SealerIDs is disjoint from the MinerID list
3. Add a new map `SealerID -> [SectorNumber]` to the chain state (that is, we have a list of used sector numbers for each sealer);
1. ie, the actor needs `SectorNumber` facilities.
3. Modify the method `ProveCommitSectorsNI` to accept the sealerID to create `ReplicaID` .
Copy link
Member

Choose a reason for hiding this comment

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

If NI PoRep were accepted and shipped in an upgrade first, we will need to add a new method, ProveCommitSectorsNI2, for this change, given this is a breaking change.

Copy link

Choose a reason for hiding this comment

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

As this "fully enables" NI-Porep, does this make it worth considering folding this into that FIP?

Copy link
Contributor

Choose a reason for hiding this comment

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

However we organize the FIPS I think it would be ideal to land both at the same time

Copy link
Contributor Author

@irenegia irenegia Apr 29, 2024

Choose a reason for hiding this comment

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

@ZenGround0 we are planning to ship NI-PoRep in nv23, but I don't think sealearID can make for it, unless we prioritise it now (fine with me)... what's your opinion on this?

cc @rjan90

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I would prefer we ship them together so we can maintain the momentum from NI Porep and immediately unlock sealing as a service. But I don't know if filoz has the capacity to get both done on time.

FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
4. [Access control, needed to avoid front-run attacks]
5. Store in the Sealer Actor another address: this is an address to a proxy contract that can be used to implement ACL

When an SP onboards a sector (ie, call to method `ProveCommitSectorsNIParams` in the Miner Actor), then call to the Sealer Actor. If the ACL is enabled, then call the ACL contract.
Copy link
Member

Choose a reason for hiding this comment

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

Is the plan to ship an FRC for this hook/interface as well..?

@jennijuju
Copy link
Member

Editor: I think we will need a bit more spec design work to do before it can be merged😄 would recommend to turn this FIP into a draft PR until them

irenegia and others added 2 commits April 23, 2024 15:10
Co-authored-by: Jiaying Wang <42981373+jennijuju@users.noreply.github.com>
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
1. We add a new type of actor: the Sealer Actor (to be used for SaaS Providers);
1. This can be a “disposable” actor, no need for an owner key. If something bad happens, create a new id. This is okay because no tokens are locked for this actor;
2. Add the method to create and register `SealerID`
2. The list of SealerIDs is disjoint from the MinerID list
Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand it the SealerActor type should be a multiple instance type. Anyone can create a SealerActor and SealerID will simply be the actor ID. This ensures SealerIDs are disjoin from MinerIDs. If we wanted a singleton actor it would become quite difficult to ensure they are disjoint.

Copy link
Contributor Author

@irenegia irenegia Apr 29, 2024

Choose a reason for hiding this comment

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

Right, no singleton actor. We should go with the two separate actors. I'll update the spec with the details you suggested here 🙏 .

1. This can be a “disposable” actor, no need for an owner key. If something bad happens, create a new id. This is okay because no tokens are locked for this actor;
2. Add the method to create and register `SealerID`
2. The list of SealerIDs is disjoint from the MinerID list
3. Add a new map `SealerID -> [SectorNumber]` to the chain state (that is, we have a list of used sector numbers for each sealer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Concretely this sector number list would be a bitfield as is the case in the miner actor.

3. Add a new map `SealerID -> [SectorNumber]` to the chain state (that is, we have a list of used sector numbers for each sealer);
1. ie, the actor needs `SectorNumber` facilities.
3. Modify the method `ProveCommitSectorsNI` to accept the sealerID to create `ReplicaID` .
4. Add the `SealerID` field to `SectorNIActivactionInfo`
Copy link
Contributor

Choose a reason for hiding this comment

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

Note we'll also need to add SealerSectorNumber to the activation info as this will generally need to be separately declared from the miner actor's sector number.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could use CBOR -1 or some well defined value <= current actor id to indicate EMPTY_SEALER_ID. SealerSectorNumber could either be ignored or enforced to be 0 if we have the EMPTY_SEALER_ID case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or alternatively we could enforce SealerID is explicitly set as minerID in the empty case.

Copy link

Choose a reason for hiding this comment

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

If these are disjoint sets, your recent post would make the proposal simpler to code and verify: struct{sealerID, sectorNumber} would be a unique identifier, replacing struct{minerID, sectorNumber}

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I agree with {sealerID, sectorNumber} for replica ID info. Annoyingly replica ID sector number and miner metadata sectorNumber will be different so we do have to handle that. Probably with {minerID, sectorNumber, minerSectorNumber} where last value can be -1.

5. Store in the Sealer Actor another address: this is an address to a proxy contract that can be used to implement ACL

When an SP onboards a sector (ie, call to method `ProveCommitSectorsNIParams` in the Miner Actor), then call to the Sealer Actor. If the ACL is enabled, then call the ACL contract.

Copy link
Contributor

Choose a reason for hiding this comment

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

An idea from @jennijuju: One design direction to consider is making the SealerActor an Fevm actor. We could do this by writing a user contract that enforces sector numbers are only claimed once. Then when calling sealer id from miner actor we check that we are 1) calling evm actor 2) the bytecode of the evm actor matches the expected contract.

Reasons for and against are pretty split so I don't know which to advocate for.

Reasons FOR

  • Take advantage of solidity development tooling
  • Less actor bundle overhead
  • No migration overhead
  • We can deploy and test out the contract before the upgrade

Reasons AGAINST

  • Performance might suffer
  • Probably a bit harder to develop for our team. Less copy past of rust code, more learning new stuff. Not in builtin actors repository. Miss out on builtin actors testing framework.
  • Finding/building a performant analogue to the bitfield data type in solidity might be too much

@anorth @Kubuxu @Stebalien any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Moved design question to #890 (comment) so it’s more discoverable and trackable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems to me that the conclusion here is to go with the original proposed design (sealer id in actor code and all other feature, as for example the ACL, in FEVM)
#890 (reply in thread)



## Backwards Compatibility

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been looking into the use of prover_id and sector_number by the window post circuit. I think that these values do not need to match the prover_id and sector_number used by sealing as they are used for challenge generation independent of the sealing process.

However if these values do need to be the same across sealing and post we will have to change miner actor state to keep around enough information to reconstruct the sealer id and sector number.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any other instances where we need the inputs to the ReplicaID (original sector number and prover id) after commiting a sector? Are there any reasons we might want these in the future? If so it is important to identify them so we can make sure we have enough information in the miner actor state.

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've been looking into the use of prover_id and sector_number by the window post circuit. I think that these values do not need to match the prover_id and sector_number used by sealing as they are used for challenge generation independent of the sealing process

Correct, they do not need to match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any other instances where we need the inputs to the ReplicaID (original sector number and prover id) after commiting a sector?

@lucaniz and I checked for this, we did not find any

Are there any reasons we might want these in the future?

Nothing that come to mind to me now :)

FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved
- The new actor needs `sectorNumber` facilities: There is a map `sealerID -> [sealerSectorNumber]` in the state (that is, we have a list of used sector numbers for each sealer); Concretely this sector number list would be a bitfield as is the case of the miner actor.

2. Modify the method `ProveCommitSectorsNI` to accept the sealerID to create `ReplicaID` .
- Add a new field to `SectorNIActivactionInfo` for passing the sealerID info:
Copy link
Member

@rvagg rvagg May 13, 2024

Choose a reason for hiding this comment

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

Suggested change
- Add a new field to `SectorNIActivactionInfo` for passing the sealerID info:
- Add a new field to `SectorNIActivationInfo` for passing the `SealerID`:

Copy link
Member

Choose a reason for hiding this comment

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

I see that FIP-0090 already has a SealerID in it, but it's not nullable / Option, is there some reconciliation that needs to happen between the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ZenGround0 could you help here please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not allow these to be nullable. FIP 0090 needs to assert some constraints (SealerID == ActorID) but once the sealer actor shows up we won't need any constraints, we'll just check whatever actor is at SealerID for unused SealingNumber. If we don't rely on an external SealingActor the caller can set SealerID to ActorID

FIPS/fip-xxx-sealerid.md Outdated Show resolved Hide resolved

2. Modify the method `ProveCommitSectorsNI` to accept the sealerID to create `ReplicaID` .
- Add a new field to `SectorNIActivactionInfo` for passing the sealerID info:
- if empty, use the the minerID when creating the `ReplicaID`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- if empty, use the the minerID when creating the `ReplicaID`
- if null, use the the minerID when creating the `ReplicaID`

alternatively 0?

- if empty, use the the minerID when creating the `ReplicaID`
- if a value is passed, use this for `ReplicaID` (in the place of `minerID`)
- Add a new field to `SectorNIActivactionInfo` for passing `sealerSectorNumber`:
- again, if we are in the case EMPTY_SEALER_ID, then `sealerSectorNumber` could either be ignored or enforced to be 0, otherwis eit is used for `ReplicaID`;
Copy link
Member

@rvagg rvagg May 13, 2024

Choose a reason for hiding this comment

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

introducing new terminology, maybe just keep it simple?

Suggested change
- again, if we are in the case EMPTY_SEALER_ID, then `sealerSectorNumber` could either be ignored or enforced to be 0, otherwis eit is used for `ReplicaID`;
- again, if `SealerID` is null, then `SealerSectorNumber` could either be ignored or enforced to be 0, otherwise it is used for `SectorNumber`;

Co-authored-by: Rod Vagg <rod@vagg.org>
- if a value is passed, use this for `ReplicaID` (in the place of `minerID`)
- Add a new field to `SectorNIActivactionInfo` for passing `sealerSectorNumber`:
- again, if we are in the case EMPTY_SEALER_ID, then `sealerSectorNumber` could either be ignored or enforced to be 0, otherwis eit is used for `ReplicaID`;
- note that the miner actor claims its own sector number in its internal state along side with the sealer's sector number. This means that the structure `{minerID, sectorNumber}` is replaced by `{minerID, sectorNumber, minerSectorNumber}` where last value can be 0.
Copy link
Member

Choose a reason for hiding this comment

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

I might be parsing this wrong, but I think the word "state" here might be misplaced, or suggesting something that it shouldn't? I think this might be referring to the internal way that the actor identifies a sector, via the SectorID compound structure: https://github.com/filecoin-project/ref-fvm/blob/31118cfad2e83dc3f6202f38ce7d96cd1c747aea/shared/src/sector/mod.rs#L54-L59

Suggested change
- note that the miner actor claims its own sector number in its internal state along side with the sealer's sector number. This means that the structure `{minerID, sectorNumber}` is replaced by `{minerID, sectorNumber, minerSectorNumber}` where last value can be 0.
- note that the miner actor identifies sectors internally using a `SectorID` structure with a `{minerID, sectorNumber}` pair. This is replaced by `{minerID, sectorNumber, minerSectorNumber}` where last value can be 0.

Does that sound right?

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.

None yet

6 participants