Feature: move partitions #8
Conversation
actors/miner/src/deadlines.rs
Outdated
| if to_deadline > from_deadline { | ||
| to_deadline - from_deadline | ||
| } else { | ||
| policy.wpost_period_deadlines - from_deadline + to_deadline |
There was a problem hiding this comment.
| if to_deadline > from_deadline { | |
| to_deadline - from_deadline | |
| } else { | |
| policy.wpost_period_deadlines - from_deadline + to_deadline | |
| if to_deadline >= from_deadline { | |
| to_deadline - from_deadline | |
| } else { | |
| policy.wpost_period_deadlines - from_deadline + to_deadline |
The distance should be 0~47.
And, this change makes the function deadline_available_for_move more reasonable. It will automatically return false if currDeadline=fromDeadline
actors/miner/src/lib.rs
Outdated
| if !deadline_available_for_move( | ||
| policy, | ||
| params.from_deadline, | ||
| params.to_deadline, | ||
| current_deadline.index, | ||
| ) { | ||
| return Err(actor_error!( | ||
| forbidden, | ||
| "cannot move to a deadline which is further from current deadline" | ||
| )); | ||
| } |
There was a problem hiding this comment.
Could we move this before the transaction handling?
I would think it is better if it can fail faster.
There was a problem hiding this comment.
Oops, seems not , because current_deadline depends on state, which is only available in the transaction.
actors/miner/src/lib.rs
Outdated
| if !deadline_available_for_compaction( | ||
| policy, | ||
| current_deadline.period_start, | ||
| params.to_deadline, | ||
| rt.curr_epoch(), | ||
| ) { | ||
| return Err(actor_error!( | ||
| forbidden, | ||
| "cannot move to deadline {} during its challenge window, \ | ||
| or the prior challenge window, | ||
| or before {} epochs have passed since its last challenge window ended", | ||
| params.to_deadline, | ||
| policy.wpost_dispute_window | ||
| )); | ||
| } |
There was a problem hiding this comment.
Is this still required if we had done the verification of PoSt?
There was a problem hiding this comment.
We only do the verification of from_deadline, but not to_deadline.
It's obviously forbidden to change deadline partitions during deadline window and challenge window, not so sure about whether it's possible to allow modifications to the partition during the dispute window though. But the original compact_partitions function doesn't allow it during dispute window, so I followed its decision.
actors/miner/src/lib.rs
Outdated
| if params.from_deadline == params.to_deadline { | ||
| return Err(actor_error!(illegal_argument, "from_deadline == to_deadline")); | ||
| } |
There was a problem hiding this comment.
This could be deleted if we fail it when !deadline_available_for_move earlier (before the transaction handling).
There was a problem hiding this comment.
same as reply for deadline_available_for_move
| @@ -128,6 +128,7 @@ pub enum Method { | |||
| ChangeBeneficiary = 30, | |||
| GetBeneficiary = 31, | |||
| ExtendSectorExpiration2 = 32, | |||
| MovePartitions = 33, | |||
There was a problem hiding this comment.
CompactPartitions 也没用exported，这里的规范是什么？
There was a problem hiding this comment.
这个知道，我指的是什么情况需要用exported方法？因为CompactPartitions也没用。貌似exported方法的好处是可以从evm调，暂时应该还没这个需求。
actors/miner/src/lib.rs
Outdated
| rt: &impl Runtime, | ||
| mut params: MovePartitionsParams, | ||
| ) -> Result<(), ActorError> { | ||
| let policy = rt.policy(); |
actors/miner/src/lib.rs
Outdated
| return Err(actor_error!( | ||
| illegal_argument, | ||
| "invalid deadline {}", | ||
| if params.from_deadline >= policy.wpost_period_deadlines { |
actors/miner/src/lib.rs
Outdated
| state.load_deadlines(store).map_err(|e| e.wrap("failed to load deadlines"))?; | ||
|
|
||
| // only try to do synchronous Window Post verification if from_deadline doesn't satisfy deadline_available_for_compaction | ||
| if !deadline_available_for_compaction( |
There was a problem hiding this comment.
建议把这些检查都封装到move里面，这里调一下move函数，函数有点太长了，deadline_available_for_compaction最后也抽到deadline_available_for_move里面，毕竟还是两个功能，现在可以套用，以后不一定可以套用。
There was a problem hiding this comment.
主要是verify_windowed_post篇幅比较长，另外2处verify_windowed_post也是这个风格，先跟风下-_-b
There was a problem hiding this comment.
这里两个check后续的处理不一样，所以没法合成一个判断。
There was a problem hiding this comment.
还是说指的是to_deadline的deadline_available_for_compaction判断？那个倒是可以跟deadline_available_for_move合并，不过错误消息要改下
actors/miner/src/lib.rs
Outdated
| ) | ||
| })?; | ||
|
|
||
| if partitions.len() == 0 { |
There was a problem hiding this comment.
挪到let mut from_deadline和let mut to_deadline之间？感觉没啥必要
There was a problem hiding this comment.
可以上面定义的变量拿到这里吗，定义和使用贴近一点。
这些变量下面也要用到。
There was a problem hiding this comment.
let partitions = &mut params.partitions;
if partitions.len() == 0 {
zhiqiangxu
force-pushed
the
feature/move_partition_verify
branch
from
23d838a to
181dc2e
Compare
zhiqiangxu
force-pushed
the
feature/move_partition_verify
branch
2 times, most recently
from
5f8a11b to
b6c9635
Compare
zhiqiangxu
force-pushed
the
feature/move_partition_verify
branch
from
b6c9635 to
7bc1156
Compare
2. remove `adjust_for_move`
…be zero when `faults` is empty
zhiqiangxu
force-pushed
the
feature/move_partition_verify
branch
2 times, most recently
from
e4eb9a5 to
1ec6468
Compare
zhiqiangxu
force-pushed
the
feature/move_partition_verify
branch
from
1ec6468 to
3c6a374
Compare
zhiqiangxu
force-pushed
the
feature/move_partition_verify
branch
from
76fb491 to
b2dc66c
Compare
2. add check for `max_partitions_per_deadline`
No description provided.