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

Feature: move partition #1326

Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
924722e
init
zhiqiangxu Jun 25, 2023
da95dbb
avoid recompute current_deadline
zhiqiangxu Jun 25, 2023
e502709
treat empty bitfield as all
zhiqiangxu Jun 26, 2023
eaf5370
rm useless quote
zhiqiangxu Jun 26, 2023
2fa9693
add verify
zhiqiangxu Jun 27, 2023
8725420
combine option1 & option2
zhiqiangxu Jun 28, 2023
d534373
fix
zhiqiangxu Jun 28, 2023
36caece
fix
zhiqiangxu Jun 28, 2023
badb691
nit
zhiqiangxu Jun 28, 2023
fd39aac
mod error
zhiqiangxu Jun 28, 2023
daab781
nit
zhiqiangxu Jun 28, 2023
0271369
fmt
zhiqiangxu Jun 28, 2023
c046cc6
fix ci
zhiqiangxu Jun 28, 2023
3eac91e
fix bug
zhiqiangxu Jul 3, 2023
181dc2e
add test
zhiqiangxu Jul 4, 2023
8eb2243
add more test
zhiqiangxu Jul 5, 2023
394ea44
Merge remote-tracking branch 'origin/master' into feature/move_partit…
zhiqiangxu Jul 11, 2023
d1fdb0f
partial fix for review
zhiqiangxu Jul 18, 2023
0804565
Merge remote-tracking branch 'origin/master' into feature/move_partit…
zhiqiangxu Jul 19, 2023
0353a28
adjust test
zhiqiangxu Jul 19, 2023
bc9e6a1
use .context_code
zhiqiangxu Jul 19, 2023
c1d88a2
fix for test
zhiqiangxu Jul 19, 2023
37d7d03
disallow empty partitions
zhiqiangxu Jul 19, 2023
89bcb74
refactor deadline_available_for_move
zhiqiangxu Jul 19, 2023
79ab15d
fix for clippy
zhiqiangxu Jul 19, 2023
39d6447
minor opt
zhiqiangxu Jul 19, 2023
0bd0611
only verify_windowed_post once
zhiqiangxu Jul 20, 2023
c01b54c
mod error msg
zhiqiangxu Jul 24, 2023
ccdb45f
1. verify_window_post batch by batch
zhiqiangxu Jul 25, 2023
b775fa8
fix ci
zhiqiangxu Jul 25, 2023
a5290cc
mod check for epoch
zhiqiangxu Jul 25, 2023
bf13558
partial review fix
zhiqiangxu Aug 2, 2023
9195aa1
Merge remote-tracking branch 'origin/master' into feature/move_partit…
zhiqiangxu Aug 2, 2023
70fd5fe
adjust test
zhiqiangxu Aug 2, 2023
511e170
refactor with Partition::adjust_for_move
zhiqiangxu Aug 2, 2023
d16077c
share the language with FIP
zhiqiangxu Aug 30, 2023
9ed53b8
deadline_available_for_move => ensure_deadline_available_for_move
zhiqiangxu Aug 30, 2023
15d5ac1
add some doc comment
zhiqiangxu Aug 30, 2023
5744223
more renaming
zhiqiangxu Aug 30, 2023
e8f0a22
more renaming
zhiqiangxu Aug 30, 2023
194b7cd
Merge remote-tracking branch 'origin/master' into feature/move_partit…
zhiqiangxu Aug 30, 2023
5e400d2
rename + merge master
zhiqiangxu Aug 30, 2023
c1159c5
mod wording
zhiqiangxu Aug 30, 2023
cb5e3c4
fix test
zhiqiangxu Aug 30, 2023
faa4873
renaming in test
zhiqiangxu Aug 31, 2023
7bc1156
apply alex's idea of not re-quantizing at all.
zhiqiangxu Sep 19, 2023
f7b7bd7
1. forbid moving when there're early terminations
zhiqiangxu Sep 22, 2023
3b25fdc
rm anyhow::Ok
zhiqiangxu Sep 22, 2023
f3ddb23
Merge branch 'master' into feature/move_partition_verify
zhiqiangxu Sep 23, 2023
9c8517a
minor optimization by observing that partition `faulty_power` should …
zhiqiangxu Sep 23, 2023
3c6a374
adjust find_sectors_by_expiration for not re-quantizing
zhiqiangxu Sep 25, 2023
271e3c0
add test
zhiqiangxu Sep 26, 2023
d2202ab
fix for review
zhiqiangxu Sep 26, 2023
d276241
add a comment about not re-quantizing when moving expirations_epochs
zhiqiangxu Sep 26, 2023
b2dc66c
minor optimization
zhiqiangxu Sep 27, 2023
fcc4cc0
avoid scanning the same range twice
zhiqiangxu Sep 28, 2023
7619065
1. review fix
zhiqiangxu Sep 28, 2023
6e91261
fix comment
zhiqiangxu Sep 28, 2023
9964257
use with_context_code
zhiqiangxu Sep 29, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions actors/miner/src/deadline_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use super::{
BitFieldQueue, ExpirationSet, Partition, PartitionSectorMap, PoStPartition, PowerPair,
SectorOnChainInfo, Sectors, TerminationResult,
};

use crate::SECTORS_AMT_BITWIDTH;

// Bitwidth of AMTs determined empirically from mutation patterns and projections of mainnet data.
Expand Down Expand Up @@ -99,6 +100,94 @@ impl Deadlines {
self.due[deadline_idx as usize] = store.put_cbor(deadline, Code::Blake2b256)?;
Ok(())
}

pub fn move_partitions<BS: Blockstore>(
Stebalien marked this conversation as resolved.
Show resolved Hide resolved
store: &BS,
from_deadline: &mut Deadline,
to_deadline: &mut Deadline,
to_quant: QuantSpec,
partitions: &BitField,
) -> anyhow::Result<()> {
let mut from_partitions = from_deadline.partitions_amt(store)?;
let mut to_partitions = to_deadline.partitions_amt(store)?;

Choose a reason for hiding this comment

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

I find these names a bit confusing, especially when they're elaborated below.

I think these correspond to OrigDeadline and DestDeadline in the FIP below - consider sharing the language?
https://github.com/filecoin-project/FIPs/blob/7b38d298f6f9d3f6bdf307a0074155c5acc8b425/FIPS/fip-0070.md#method-signature-and-parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


// even though we're moving partitions intact, we still need to update from/to `Deadline` accordingly.

let first_to_partition_idx = to_partitions.count();
for (i, from_partition_idx) in partitions.iter().enumerate() {
let mut moving_partition = from_partitions
.get(from_partition_idx)?
.ok_or_else(|| actor_error!(not_found, "no partition {}", from_partition_idx))?
.clone();
if !moving_partition.faults.is_empty() || !moving_partition.unproven.is_empty() {
return Err(actor_error!(forbidden, "partition with faults or unproven sectors are not allowed to move, partition_idx {}", from_partition_idx))?;
}

let to_partition_idx = first_to_partition_idx + i as u64;

moving_partition.adjust_for_move(store, &to_quant)?;

let all_sectors = moving_partition.sectors.len();
let live_sectors = moving_partition.live_sectors().len();
Copy link
Member

Choose a reason for hiding this comment

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

These two counts must be the same, because no sectors are faulty. So skip the bitfield difference caluclation of live_sectors and just use a single sector_count variable equal to sectors.len() (and leave a comment explaining this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

let early_terminations = from_deadline.early_terminations.get(from_partition_idx);

// start updating from/to `Deadline` here

from_deadline.total_sectors -= all_sectors;
from_deadline.live_sectors -= live_sectors;
from_deadline.faulty_power -= &moving_partition.faulty_power;
Copy link
Member

Choose a reason for hiding this comment

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

I think most of the code outside the block that is updating Partition could be factored out into methods on Deadline. We can then test these much more easily than by orchestrating actual moves.

// Removes a partition from the deadline, updating its metadata.
// An empty partition is left in its place so that partition indices don't change.
// The partition's expiration queue entries are (not?) removed.
fn remove_partition(idx: u32) -> Result<Partition, ActorError>;

// Adds a partition to the deadline at the next index, updating deadline metadata.
// The partition's expiration queue keys are added to the deadline's expiration queue.
fn add_partition(p: Partition) -> Result<(), ActorError>;

Copy link
Contributor Author

@zhiqiangxu zhiqiangxu Aug 2, 2023

Choose a reason for hiding this comment

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

But we don't want to update Deadline.expirations_epochs for each Partition, instead we want to do it in batch for all partitions being moved. It's semantically weird if this function updates all other fields except expirations_epochs.


to_deadline.total_sectors += all_sectors;
to_deadline.live_sectors += live_sectors;
to_deadline.faulty_power += &moving_partition.faulty_power;

// update early_terminations BitField of `Deadline`
if early_terminations {
from_deadline.early_terminations.unset(from_partition_idx);
to_deadline.early_terminations.set(to_partition_idx);
}

from_partitions.set(from_partition_idx, Partition::new(store)?)?;
to_partitions.set(to_partition_idx, moving_partition)?;
}

// update expirations_epochs Cid of Deadline.
{
let mut epochs_to_remove = Vec::<u64>::new();
let mut from_expirations_epochs: Array<BitField, _> =
Array::load(&from_deadline.expirations_epochs, store)?;
let mut to_expirations_epochs: Array<BitField, _> =
Array::load(&to_deadline.expirations_epochs, store)?;
from_expirations_epochs.for_each_mut(|from_epoch, from_bitfield| {
Copy link
Member

Choose a reason for hiding this comment

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

Iterating the deadline's whole expiration queue is fairly expensive. I don't think we need to.

  • For the from deadline, it's ok to just leave the entries there. The empty partition will no-op when the entries expire.
  • For the to deadline, you can instead iterate the new partition's expiration_epochs, each of which must be set in the destination deadline.
    This will iterate only the moved partition's queue, not any whole deadline's queue. It will be easier to fit this inside Deadline::add/remove_partition since you only need to access one deadline at a time. Again easier testing.

Note that this approach is only ok on the precondition that there are no faulty sectors, which would appear twice in the source deadline and must be propagated at both epochs. Document this.

Copy link
Contributor Author

@zhiqiangxu zhiqiangxu Aug 2, 2023

Choose a reason for hiding this comment

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

But it's essentially as expensive as expiration_epochs.cut which is called in remove_partitions, do we really want to save some gas by leaving some garbage data?

let to_epoch = to_quant.quantize_up(from_epoch as ChainEpoch);
let mut to_bitfield =
to_expirations_epochs.get(to_epoch as u64)?.cloned().unwrap_or_default();
for (i, partition_id) in partitions.iter().enumerate() {
if from_bitfield.get(partition_id) {
from_bitfield.unset(partition_id);
to_bitfield.set(first_to_partition_idx + i as u64);
}
}
to_expirations_epochs.set(to_epoch as u64, to_bitfield)?;

if from_bitfield.is_empty() {
epochs_to_remove.push(from_epoch);
}

Ok(())
})?;
if !epochs_to_remove.is_empty() {
from_expirations_epochs.batch_delete(epochs_to_remove, true)?;
}
from_deadline.expirations_epochs = from_expirations_epochs.flush()?;
to_deadline.expirations_epochs = to_expirations_epochs.flush()?;
}

from_deadline.partitions = from_partitions.flush()?;
to_deadline.partitions = to_partitions.flush()?;

Ok(())
}
}

/// Deadline holds the state for all sectors due at a specific deadline.
Expand Down
67 changes: 67 additions & 0 deletions actors/miner/src/deadlines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,73 @@ pub fn deadline_available_for_compaction(
)
}

// the distance between from_deadline and to_deadline clockwise in deadline unit.

Choose a reason for hiding this comment

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

style: why have you chosen raw comments rather than doc-comments for all the functions?

Suggested change
// the distance between from_deadline and to_deadline clockwise in deadline unit.
/// the distance between from_deadline and to_deadline clockwise in deadline unit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I was following the original style of compact_partitions:)

fn deadline_distance(policy: &Policy, from_deadline: u64, to_deadline: u64) -> u64 {
if to_deadline >= from_deadline {
to_deadline - from_deadline
} else {
policy.wpost_period_deadlines - from_deadline + to_deadline
}
}

// only allow moving to a nearer deadline from current one
pub fn deadline_available_for_move(
policy: &Policy,
from_deadline: u64,
to_deadline: u64,
current_deadline: &DeadlineInfo,
) -> Result<(), String> {
if !deadline_is_mutable(
policy,
current_deadline.period_start,
from_deadline,
current_deadline.current_epoch,
) {
return Err(format!(
"cannot move from a deadline {}, immutable at epoch {}",
from_deadline, current_deadline.current_epoch
));
}

if !deadline_is_mutable(
policy,
current_deadline.period_start,
to_deadline,
current_deadline.current_epoch,
) {
return Err(format!(
"cannot move to a deadline {}, immutable at epoch {}",
to_deadline, current_deadline.current_epoch
));
}

if deadline_distance(policy, current_deadline.index, to_deadline)
>= deadline_distance(policy, current_deadline.index, from_deadline)
{
return Err(format!(
"can only move to a deadline which is nearer from current deadline {}, to_deadline {} is not nearer than from_deadline {}",
current_deadline.index, to_deadline, from_deadline
));
}

Ok(())
}

// returns the nearest deadline info with index `target_deadline` that has already occured from the point of view of the current deadline(including the current deadline).
pub fn nearest_occured_deadline_info(
policy: &Policy,
current_deadline: &DeadlineInfo,
target_deadline: u64,
) -> DeadlineInfo {
// Find the proving period start for the deadline in question.
let mut pp_start = current_deadline.period_start;
if current_deadline.index < target_deadline {
pp_start -= policy.wpost_proving_period
}

new_deadline_info(policy, pp_start, target_deadline, current_deadline.current_epoch)
}

Copy link
Member

Choose a reason for hiding this comment

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

These look correct, but please add tests to demonstrate all the cases.

// Determine current period start and deadline index directly from current epoch and
// the offset implied by the proving period. This works correctly even for the state
// of a miner actor without an active deadline cron
Expand Down
Loading