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 all commits
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
127 changes: 73 additions & 54 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 @@ -3740,10 +3740,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 @@ -3753,49 +3749,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 @@ -3805,11 +3812,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 @@ -3820,43 +3828,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
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