Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
FIPXXXX: Introducing sealerID #993
Changes from 3 commits
2b95255
15861ed
cc6dfb3
1b32df0
a9b7fcd
92df9ac
7df5745
d028a3c
712ff61
21277ca
5c813ce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
Right, no singleton actor. We should go with the two separate actors. I'll update the spec with the details you suggested here 🙏 .
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.
Concretely this sector number list would be a bitfield as is the case in 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.
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.
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.
As this "fully enables" NI-Porep, does this make it worth considering folding this into that FIP?
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.
However we organize the FIPS I think it would be ideal to land both at the same time
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.
@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
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.
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.
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.
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.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.
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.
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.
Or alternatively we could enforce SealerID is explicitly set as minerID in the empty case.
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 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}
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 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.
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 the plan to ship an FRC for this hook/interface as well..?
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.
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
Reasons AGAINST
@anorth @Kubuxu @Stebalien any thoughts?
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.
Moved design question to #890 (comment) so it’s more discoverable and trackable
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.
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)
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.
The sketch of this spec looks fine, but it's missing a lot of detail at the moment.
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'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.
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.
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.
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.
Correct, they do not need to match.
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.
@lucaniz and I checked for this, we did not find any
Nothing that come to mind to me now :)