-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Rought PoST method #207
Rought PoST method #207
Conversation
License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@@ -247,11 +252,6 @@ func (sma StorageMinerActor) CommitSector(act *types.Actor, vmctx types.VMContex | |||
return nil, aerrors.New(3, "not enough collateral") | |||
} | |||
|
|||
// ensure that the miner cannot commit more sectors than can be proved with a single PoSt | |||
if self.SectorSetSize >= POST_SECTORS_COUNT { |
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.
Why remove this?
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 was removed from spec.
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.
So, i chatted with @dignifiedquire.
This was removed from the spec because technically, rational post can handle an unlimited number of sectors. We agreed though that for practical purposes, there should be some limit (otherwise there are dos vectors with 'infinite' sized arrays of sectors being passed around, and other such nonsense).
Let's keep this code in for now, and file some spec issues to fix it up
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.
my take: there should be none with current construction, however we shouldn't assume so for future construction and backward compatibility, so we should pick a number
@@ -325,6 +413,10 @@ func (sma StorageMinerActor) SubmitPoSt(act *types.Actor, vmctx types.VMContext, | |||
return nil, err | |||
} | |||
|
|||
self.ProvingSet = self.Sectors | |||
self.ProvingSetSize = self.SectorSetSize |
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.
Ah, we can drop the Size fields, the AMT tracks its own cardinality (the HAMT didnt)
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 Count
field in AMT checked in any way so Eve can't change it at a whim?
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.
Mostly LGTM, left some comments.
I'll write the AMT delete method and get back to you with that so we can use it here
} | ||
|
||
var sectorInfos []sectorbuilder.SectorInfo | ||
if err := pss.ForEach(func(id uint64, v *cbg.Deferred) error { |
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'd pull this out into a separate function. Maybe we can dedupe some code with #205, can also do that later too.
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.
Problem is: either api or actor code (or both) would then have to convert types. API wants to use api.SectorInfo
, chain wants to use sectorbuilder.SectorInfo
.
License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
, License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
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.
There are still some todos, but this moves us in the right direction
No description provided.