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.

Sign up for GitHub

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

Tests and fixes for ProveReplicaUpdates2 #1411

Merged
merged 2 commits into from
Sep 15, 2023
Merged

Tests and fixes for ProveReplicaUpdates2 #1411

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffa657a
Tests and fixes for ProveReplicaUpdates2
anorth Sep 11, 2023
b71ebe9
Helper function
anorth Sep 13, 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
54 changes: 30 additions & 24 deletions actors/miner/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1068,7 +1068,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 @@ -1172,7 +1172,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 @@ -1766,7 +1766,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 @@ -1819,7 +1819,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 @@ -3763,7 +3763,7 @@ where
fail_validation = true;
}

if update.replica_proof.len() > 4096 {
if !fail_validation && update.replica_proof.len() > 4096 {
Copy link
Member

Choose a reason for hiding this comment

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

This should just use a helper function with an early return.

info!(
"update proof is too large ({}), skipping sector {}",
update.replica_proof.len(),
Expand All @@ -3772,25 +3772,25 @@ where
fail_validation = true;
}

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

if update.deals.len() as u64 > sector_deals_max(policy, sector_size) {
if !fail_validation && 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;
}

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

if !is_sealed_sector(&update.new_sealed_cid) {
if !fail_validation && !is_sealed_sector(&update.new_sealed_cid) {
info!(
"new sealed CID had wrong prefix {}, skipping sector {}",
update.new_sealed_cid, update.sector_number
Expand All @@ -3799,12 +3799,14 @@ where
}

// Disallow upgrading sectors in immutable deadlines.
if !deadline_is_mutable(
policy,
state.current_proving_period_start(policy, curr_epoch),
update.deadline,
curr_epoch,
) {
if !fail_validation
&& !deadline_is_mutable(
policy,
state.current_proving_period_start(policy, curr_epoch),
update.deadline,
curr_epoch,
)
{
info!(
"cannot upgrade sectors in immutable deadline {}, skipping sector {}",
update.deadline, update.sector_number
Expand All @@ -3813,18 +3815,22 @@ where
}

// This inefficiently loads deadline/partition info for each update.
if !state.check_sector_active(
&store,
update.deadline,
update.partition,
update.sector_number,
true,
)? {
if !fail_validation
&& !state.check_sector_active(
&store,
update.deadline,
update.partition,
update.sector_number,
true,
)?
{
info!("sector isn't active, skipping sector {}", update.sector_number);
fail_validation = true;
}

if (&sector_info.deal_weight + &sector_info.verified_deal_weight) != DealWeight::zero() {
if !fail_validation
&& (&sector_info.deal_weight + &sector_info.verified_deal_weight) != DealWeight::zero()
{
info!(
"cannot update sector with non-zero data, skipping sector {}",
update.sector_number
Expand All @@ -3836,7 +3842,7 @@ where
.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 {
if !fail_validation && update.update_proof_type != expected_proof_type {
info!(
"expected proof type {}, was {}",
i64::from(expected_proof_type),
Expand Down
21 changes: 7 additions & 14 deletions actors/miner/tests/prove_commit2_test.rs
View file
Open in desktop
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
Open in desktop
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