Implemented PreCommitSectorBatch method (FIP-0008) #1407
Conversation
anorth
force-pushed
the
anorth/batchprecommit
branch
2 times, most recently
from
c6835d3
to
9292fec
Compare
…8 (discussion) The non-batched PreCommitSector method remains, but should be considered deprecated. The implementation delegates internally to the batched method. This consumes less gas than before this change (due to removal of redundant checks in the batched method) but is slightly less efficient at runtime than a strictly-optimized singleton method could be. I expect it to be removed in the future.
anorth
force-pushed
the
anorth/batchprecommit
branch
from
9292fec
to
a22763b
Compare
| @@ -647,60 +648,97 @@ func (a Actor) DisputeWindowedPoSt(rt Runtime, params *DisputeWindowedPoStParams | |||
| //} | |||
| type PreCommitSectorParams = miner0.SectorPreCommitInfo | |||
There was a problem hiding this comment.
nit: v5 (along with every other version so far) has been redefining its own SectorPreCommitInfo and generating cbor (un)marshalling code for it. We should pick one strategy either relying only on miner0 for serialization and removing v5.SectorPreCommitInfo from gen.go or using miner5.SectorPreCommitInfo here. I slightly prefer the second option.
There was a problem hiding this comment.
Actually, every version has aliased this PreCommitSectorParams onto miner0.SectorPreCommitInfo, e.g. in v4. This is necessary to preserve type-compatibility in Lotus without extra effort. But then yes redefining a version used in state.
I would redefine the params object inline rather than reference the v5 state type, to decouple them. But either would require abstraction and version-switching in Lotus, which we currently avoid. PreCommitSectorParams is the ugly special case here where a state object (which we usually redefine) was used as a parameter type.
I opened a discussion in Filecoin slack about how we might consider aliasing the state types too in the future.
| if params.SealRandEpoch < challengeEarliest { | ||
| rt.Abortf(exitcode.ErrIllegalArgument, "seal challenge epoch %v too old, must be after %v", params.SealRandEpoch, challengeEarliest) | ||
| } | ||
| if !CanPreCommitSealProof(precommit.SealProof, nv) { |
There was a problem hiding this comment.
suggestion: because the actors major version is incrementing this might be a good time to simplify CanPreCommtiSealProof to only allow v1_1 seal proofs and not take in network version information
There was a problem hiding this comment.
Agreed, I will follow up with that. #1414
Implements a new
PreCommitSectorBatchmethod, as specified by FIP-0008 (discussion)The non-batched
PreCommitSectormethod remains, but should be considered deprecated. The implementation delegates internally to the batched method. This consumes less gas than before this change (due to removal of redundant checks in the batched method) but is slightly less efficient at runtime than a strictly-optimized singleton method could be. I expect it to be removed in the future.