Skip to content

Commit

Permalink
Tests and fixes for ProveReplicaUpdates2 (#1411)
Browse files Browse the repository at this point in the history 
* Tests and fixes for ProveReplicaUpdates2

* Helper function
  • Loading branch information
@anorth
anorth committed Dec 14, 2023
1 parent 6ef0eb2 commit c9fdbaa
Show file tree
Hide file tree
Showing 5 changed files with 616 additions and 151 deletions.
127 changes: 73 additions & 54 deletions actors/miner/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1043,7 +1043,7 @@ impl Actor {

// Verify proofs before activating anything.
let mut proven_manifests: Vec<(&SectorUpdateManifest, &SectorOnChainInfo)> = vec![];
let mut proven_batch_gen = BatchReturnGen::new(validation_batch.size());
let mut proven_batch_gen = BatchReturnGen::new(validation_batch.success_count as usize);
if !params.sector_proofs.is_empty() {
// Batched proofs, one per sector
if params.sector_updates.len() != params.sector_proofs.len() {
Expand Down Expand Up @@ -1147,7 +1147,7 @@ impl Actor {
let (power_delta, pledge_delta) = update_replica_states(
rt,
&state_updates_by_dline,
proven_manifests.len(),
successful_manifests.len(),
&mut sectors,
info.sector_size,
)?;
Expand Down Expand Up @@ -1741,7 +1741,7 @@ impl Actor {
&SectorActivationManifest,
&SectorPreCommitOnChainInfo,
)> = vec![];
let mut proven_batch_gen = BatchReturnGen::new(validation_batch.size());
let mut proven_batch_gen = BatchReturnGen::new(validation_batch.success_count as usize);
if !params.sector_proofs.is_empty() {
// Verify batched proofs.
// Filter proof inputs to those for valid pre-commits.
Expand Down Expand Up @@ -1794,7 +1794,7 @@ impl Actor {
proven_activation_inputs = eligible_activation_inputs_iter
.map(|(activation, precommit)| (*activation, precommit))
.collect();
proven_batch_gen.add_successes(validation_batch.size());
proven_batch_gen.add_successes(proven_activation_inputs.len());
}
let proven_batch = proven_batch_gen.gen();
if proven_batch.success_count == 0 {
Expand Down Expand Up @@ -3715,10 +3715,6 @@ fn validate_replica_updates<'a, BS>(
where
BS: Blockstore,
{
let mut batch = BatchReturnGen::new(updates.len());
let mut sector_numbers = BTreeSet::<SectorNumber>::new();
let mut update_sector_infos: Vec<UpdateAndSectorInfo> = Vec::with_capacity(updates.len());

if updates.len() > policy.prove_replica_updates_max_size {
return Err(actor_error!(
illegal_argument,
Expand All @@ -3728,49 +3724,60 @@ where
));
}

for (i, (update, sector_info)) in updates.iter().zip(sector_infos).enumerate() {
// Build update and sector info for all updates, even if they fail validation.
let mut fail_validation = false;
update_sector_infos.push(UpdateAndSectorInfo { update, sector_info });

let mut sector_numbers = BTreeSet::<SectorNumber>::new();
let mut validate_one = |update: &ReplicaUpdateInner,
sector_info: &SectorOnChainInfo|
-> Result<(), ActorError> {
if !sector_numbers.insert(update.sector_number) {
info!("skipping duplicate sector {}", update.sector_number,);
fail_validation = true;
return Err(actor_error!(
illegal_argument,
"skipping duplicate sector {}",
update.sector_number
));
}

if update.replica_proof.len() > 4096 {
info!(
return Err(actor_error!(
illegal_argument,
"update proof is too large ({}), skipping sector {}",
update.replica_proof.len(),
update.sector_number,
);
fail_validation = true;
update.sector_number
));
}

if require_deals && update.deals.is_empty() {
info!("must have deals to update, skipping sector {}", update.sector_number,);
fail_validation = true;
return Err(actor_error!(
illegal_argument,
"must have deals to update, skipping sector {}",
update.sector_number
));
}

if update.deals.len() as u64 > sector_deals_max(policy, sector_size) {
info!("more deals than policy allows, skipping sector {}", update.sector_number,);
fail_validation = true;
return Err(actor_error!(
illegal_argument,
"more deals than policy allows, skipping sector {}",
update.sector_number
));
}

if update.deadline >= policy.wpost_period_deadlines {
info!(
return Err(actor_error!(
illegal_argument,
"deadline {} not in range 0..{}, skipping sector {}",
update.deadline, policy.wpost_period_deadlines, update.sector_number
);
fail_validation = true;
update.deadline,
policy.wpost_period_deadlines,
update.sector_number
));
}

if !is_sealed_sector(&update.new_sealed_cid) {
info!(
return Err(actor_error!(
illegal_argument,
"new sealed CID had wrong prefix {}, skipping sector {}",
update.new_sealed_cid, update.sector_number
);
fail_validation = true;
update.new_sealed_cid,
update.sector_number
));
}

// Disallow upgrading sectors in immutable deadlines.
Expand All @@ -3780,11 +3787,12 @@ where
update.deadline,
curr_epoch,
) {
info!(
return Err(actor_error!(
illegal_argument,
"cannot upgrade sectors in immutable deadline {}, skipping sector {}",
update.deadline, update.sector_number
);
fail_validation = true;
update.deadline,
update.sector_number
));
}

// This inefficiently loads deadline/partition info for each update.
Expand All @@ -3795,43 +3803,54 @@ where
update.sector_number,
true,
)? {
info!("sector isn't active, skipping sector {}", update.sector_number);
fail_validation = true;
return Err(actor_error!(
illegal_argument,
"sector isn't active, skipping sector {}",
update.sector_number
));
}

if (&sector_info.deal_weight + &sector_info.verified_deal_weight) != DealWeight::zero() {
info!(
return Err(actor_error!(
illegal_argument,
"cannot update sector with non-zero data, skipping sector {}",
update.sector_number
);
fail_validation = true;
));
}

let expected_proof_type = sector_info
.seal_proof
.registered_update_proof()
.context_code(ExitCode::USR_ILLEGAL_STATE, "couldn't load update proof type")?;
if update.update_proof_type != expected_proof_type {
info!(
return Err(actor_error!(
illegal_argument,
"expected proof type {}, was {}",
i64::from(expected_proof_type),
i64::from(update.update_proof_type)
);
fail_validation = true;
));
}
Ok(())
};

if fail_validation {
if all_or_nothing {
return Err(actor_error!(
illegal_argument,
"invalid update {} while requiring activation success: {:?}",
i,
update
));
let mut batch = BatchReturnGen::new(updates.len());
let mut update_sector_infos: Vec<UpdateAndSectorInfo> = Vec::with_capacity(updates.len());
for (i, (update, sector_info)) in updates.iter().zip(sector_infos).enumerate() {
// Build update and sector info for all updates, even if they fail validation.
update_sector_infos.push(UpdateAndSectorInfo { update, sector_info });

match validate_one(update, sector_info) {
Ok(_) => {
batch.add_success();
}
Err(e) => {
let e = e.wrap(format!("invalid update {} while requiring activation success", i));
info!("{}", e.msg());
if all_or_nothing {
return Err(e);
}
batch.add_fail(ExitCode::USR_ILLEGAL_ARGUMENT);
}
batch.add_fail(ExitCode::USR_ILLEGAL_ARGUMENT);
} else {
batch.add_success();
}
}
Ok((batch.gen(), update_sector_infos))
Expand Down
21 changes: 7 additions & 14 deletions actors/miner/tests/prove_commit2_test.rs
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use fvm_shared::sector::{SectorNumber, StoragePower};
use fvm_shared::sector::SectorNumber;
use fvm_shared::{bigint::Zero, clock::ChainEpoch, econ::TokenAmount, ActorID};

use fil_actor_miner::{ProveCommitSectors2Return, SectorPreCommitInfo};
use fil_actors_runtime::{BatchReturn, DealWeight, EPOCHS_IN_DAY};
use fil_actors_runtime::{BatchReturn, EPOCHS_IN_DAY};
use util::*;

mod util;
Expand Down Expand Up @@ -33,7 +33,7 @@ fn prove_commit2_basic() {
let snos: Vec<SectorNumber> =
precommits.iter().map(|pci: &SectorPreCommitInfo| pci.sector_number).collect();

// Update them in batch, each with a single piece.
// Prove them in batch, each with a single piece.
let piece_size = h.sector_size as u64;
let sector_activations = vec![
make_activation_manifest(snos[0], &[(piece_size, 0, 0, 0)]), // No alloc or deal
Expand All @@ -49,19 +49,12 @@ fn prove_commit2_basic() {
result
);

let duration = sector_expiry - *rt.epoch.borrow();
let expected_weight = DealWeight::from(piece_size) * duration;
let raw_power = StoragePower::from(h.sector_size as u64);
let verified_power = &raw_power * 10;
let raw_pledge = h.initial_pledge_for_power(&rt, &raw_power);
let verified_pledge = h.initial_pledge_for_power(&rt, &verified_power);

// Sector 0: Even though there's no "deal", the data weight is set.
verify_weights(&rt, &h, snos[0], &expected_weight, &DealWeight::zero(), &raw_pledge);
verify_weights(&rt, &h, snos[0], piece_size, 0);
// Sector 1: With an allocation, the verified weight is set instead.
verify_weights(&rt, &h, snos[1], &DealWeight::zero(), &expected_weight, &verified_pledge);
verify_weights(&rt, &h, snos[1], 0, piece_size);
// Sector 2: Deal weight is set.
verify_weights(&rt, &h, snos[2], &expected_weight, &DealWeight::zero(), &raw_pledge);
verify_weights(&rt, &h, snos[2], piece_size, 0);
// Sector 3: Deal doesn't make a difference to verified weight only set.
verify_weights(&rt, &h, snos[3], &DealWeight::zero(), &expected_weight, &verified_pledge);
verify_weights(&rt, &h, snos[3], 0, piece_size);
}
8 changes: 5 additions & 3 deletions actors/miner/tests/prove_replica_failures_test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ fn reject_aggregate_proof() {
#[test]
fn reject_all_proofs_fail() {
let (h, rt, sector_updates) = setup(2, 0, 0, 0);
let cfg = ProveReplicaUpdatesConfig { proofs_valid: vec![false, false], ..Default::default() };
let cfg = ProveReplicaUpdatesConfig { proof_failure: vec![0, 1], ..Default::default() };
expect_abort_contains_message(
ExitCode::USR_ILLEGAL_ARGUMENT,
"no valid updates",
Expand All @@ -131,7 +131,7 @@ fn reject_invalid_update() {
#[test]
fn reject_required_proof_failure() {
let (h, rt, sector_updates) = setup(2, 0, 0, 0);
let cfg = ProveReplicaUpdatesConfig { proofs_valid: vec![true, false], ..Default::default() };
let cfg = ProveReplicaUpdatesConfig { proof_failure: vec![0], ..Default::default() };
expect_abort_contains_message(
ExitCode::USR_ILLEGAL_ARGUMENT,
"requiring activation success",
Expand Down Expand Up @@ -202,7 +202,9 @@ fn setup(
let piece_size = h.sector_size as u64;
let sector_updates = sectors
.iter()
.map(|s| make_update_manifest(&st, store, s, &[(piece_size, client, alloc, deal)]))
.map(|s| {
make_update_manifest(&st, store, s.sector_number, &[(piece_size, client, alloc, deal)])
})
.collect();
(h, rt, sector_updates)
}
Loading

0 comments on commit c9fdbaa

Please sign in to comment.