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

Remove deprecated precommit methods #1357

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Remove deprecated precommit methods #1357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
76 changes: 16 additions & 60 deletions actors/miner/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ pub enum Method {
ChangeWorkerAddress = 3,
ChangePeerID = 4,
SubmitWindowedPoSt = 5,
PreCommitSector = 6,
//PreCommitSector = 6, // Deprecated
ProveCommitSector = 7,
ExtendSectorExpiration = 8,
TerminateSectors = 9,
Expand All @@ -120,7 +120,7 @@ pub enum Method {
RepayDebt = 22,
ChangeOwnerAddress = 23,
DisputeWindowedPoSt = 24,
PreCommitSectorBatch = 25,
//PreCommitSectorBatch = 25, // Deprecated
ProveCommitAggregate = 26,
ProveReplicaUpdates = 27,
PreCommitSectorBatch2 = 28,
Expand Down Expand Up @@ -1696,54 +1696,6 @@ impl Actor {
Ok(())
}

/// Pledges to seal and commit a single sector.
/// See PreCommitSectorBatch for details.
fn pre_commit_sector(
rt: &impl Runtime,
params: PreCommitSectorParams,
) -> Result<(), ActorError> {
Self::pre_commit_sector_batch(rt, PreCommitSectorBatchParams { sectors: vec![params] })
}

/// Pledges the miner to seal and commit some new sectors.
/// The caller specifies sector numbers, sealed sector data CIDs, seal randomness epoch, expiration, and the IDs
/// of any storage deals contained in the sector data. The storage deal proposals must be already submitted
/// to the storage market actor.
/// A pre-commitment may specify an existing committed-capacity sector that the committed sector will replace
/// when proven.
/// This method calculates the sector's power, locks a pre-commit deposit for the sector, stores information about the
/// sector in state and waits for it to be proven or expire.
fn pre_commit_sector_batch(
rt: &impl Runtime,
params: PreCommitSectorBatchParams,
) -> Result<(), ActorError> {
let sectors = params
.sectors
.into_iter()
.map(|spci| {
if spci.replace_capacity {
Err(actor_error!(
forbidden,
"cc upgrade through precommit discontinued, use ProveReplicaUpdate"
))
} else {
Ok(SectorPreCommitInfoInner {
seal_proof: spci.seal_proof,
sector_number: spci.sector_number,
sealed_cid: spci.sealed_cid,
seal_rand_epoch: spci.seal_rand_epoch,
deal_ids: spci.deal_ids,
expiration: spci.expiration,
// This entry point computes the unsealed CID from deals via the market.
// A future one will accept it directly as a parameter.
unsealed_cid: None,
})
}
})
.collect::<Result<_, _>>()?;
Self::pre_commit_sector_batch_inner(rt, sectors)
}

/// Pledges the miner to seal and commit some new sectors.
/// The caller specifies sector numbers, sealed sector CIDs, unsealed sector CID, seal randomness epoch, expiration, and the IDs
/// of any storage deals contained in the sector data. The storage deal proposals must be already submitted
Expand Down Expand Up @@ -1844,10 +1796,20 @@ impl Actor {
));
}

if let Some(ref commd) = precommit.unsealed_cid.as_ref().and_then(|c| c.0) {
if !is_unsealed_sector(commd) {
return Err(actor_error!(illegal_argument, "unsealed CID had wrong prefix"));
if let Some(compact_commd) = &precommit.unsealed_cid {
if let Some(commd) = compact_commd.0 {
if !is_unsealed_sector(&commd) {
return Err(actor_error!(
illegal_argument,
"unsealed CID had wrong prefix"
));
}
}
} else {
return Err(actor_error!(
illegal_argument,
"unspecified CompactCommD not allowed past nv21, need explicit None value for CC or CommD"
));
}

// Require sector lifetime meets minimum by assuming activation happens at last epoch permitted for seal proof.
Expand Down Expand Up @@ -1953,11 +1915,7 @@ impl Actor {
// 1. verify that precommit.unsealed_cid is correct
// 2. create a new on_chain_precommit

let commd = match precommit.unsealed_cid {
// if the CommD is unknown, use CommD computed by the market
None => CompactCommD::new(deal_data.commd),
Some(x) => x,
};
let commd = precommit.unsealed_cid.unwrap();
if commd.0 != deal_data.commd {
return Err(actor_error!(illegal_argument, "computed {:?} and passed {:?} CommDs not equal",
deal_data.commd, commd));
Expand Down Expand Up @@ -5094,7 +5052,6 @@ impl ActorCode for Actor {
ChangeWorkerAddress|ChangeWorkerAddressExported => change_worker_address,
ChangePeerID|ChangePeerIDExported => change_peer_id,
SubmitWindowedPoSt => submit_windowed_post,
PreCommitSector => pre_commit_sector,
ProveCommitSector => prove_commit_sector,
ExtendSectorExpiration => extend_sector_expiration,
TerminateSectors => terminate_sectors,
Expand All @@ -5113,7 +5070,6 @@ impl ActorCode for Actor {
RepayDebt|RepayDebtExported => repay_debt,
ChangeOwnerAddress|ChangeOwnerAddressExported => change_owner_address,
DisputeWindowedPoSt => dispute_windowed_post,
PreCommitSectorBatch => pre_commit_sector_batch,
ProveCommitAggregate => prove_commit_aggregate,
ProveReplicaUpdates => prove_replica_updates,
PreCommitSectorBatch2 => pre_commit_sector_batch2,
Expand Down
104 changes: 30 additions & 74 deletions actors/miner/tests/miner_actor_test_precommit_batch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ struct DealSpec {
}

fn assert_simple_batch(
v2: bool,
batch_size: usize,
balance_surplus: TokenAmount,
base_fee: TokenAmount,
Expand All @@ -44,10 +43,7 @@ fn assert_simple_batch(
) {
let period_offset = ChainEpoch::from(100);

let h = ActorHarness::new_with_options(HarnessOptions {
use_v2_pre_commit_and_replica_update: v2,
proving_period_offset: period_offset,
});
let h = ActorHarness::new_with_options(HarnessOptions { proving_period_offset: period_offset });
let rt = h.new_runtime();

let precommit_epoch = period_offset + 1;
Expand Down Expand Up @@ -158,47 +154,25 @@ mod miner_actor_precommit_batch {
};
use fil_actors_runtime::{STORAGE_MARKET_ACTOR_ADDR, STORAGE_POWER_ACTOR_ADDR};
use fvm_ipld_encoding::ipld_block::IpldBlock;
use test_case::test_case;

#[test_case(false; "v1")]
#[test_case(true; "v2")]
fn one_sector(v2: bool) {
assert_simple_batch(v2, 1, TokenAmount::zero(), TokenAmount::zero(), &[], ExitCode::OK, "");
#[test]
fn one_sector() {
assert_simple_batch(1, TokenAmount::zero(), TokenAmount::zero(), &[], ExitCode::OK, "");
}

#[test_case(false; "v1")]
#[test_case(true; "v2")]
fn thirty_two_sectors(v2: bool) {
assert_simple_batch(
v2,
32,
TokenAmount::zero(),
TokenAmount::zero(),
&[],
ExitCode::OK,
"",
);
#[test]
fn thirty_two_sectors() {
assert_simple_batch(32, TokenAmount::zero(), TokenAmount::zero(), &[], ExitCode::OK, "");
}

#[test_case(false; "v1")]
#[test_case(true; "v2")]
fn max_sectors(v2: bool) {
assert_simple_batch(
v2,
256,
TokenAmount::zero(),
TokenAmount::zero(),
&[],
ExitCode::OK,
"",
);
#[test]
fn max_sectors() {
assert_simple_batch(256, TokenAmount::zero(), TokenAmount::zero(), &[], ExitCode::OK, "");
}

#[test_case(false; "v1")]
#[test_case(true; "v2")]
fn one_deal(v2: bool) {
#[test]
fn one_deal() {
assert_simple_batch(
v2,
3,
TokenAmount::zero(),
TokenAmount::zero(),
Expand All @@ -207,12 +181,9 @@ mod miner_actor_precommit_batch {
"",
);
}

#[test_case(false; "v1")]
#[test_case(true; "v2")]
fn many_deals(v2: bool) {
#[test]
fn many_deals() {
assert_simple_batch(
v2,
3,
TokenAmount::zero(),
TokenAmount::zero(),
Expand All @@ -226,11 +197,9 @@ mod miner_actor_precommit_batch {
);
}

#[test_case(false; "v1")]
#[test_case(true; "v2")]
fn empty_batch(v2: bool) {
#[test]
fn empty_batch() {
assert_simple_batch(
v2,
0,
TokenAmount::zero(),
TokenAmount::zero(),
Expand All @@ -240,11 +209,9 @@ mod miner_actor_precommit_batch {
);
}

#[test_case(false; "v1")]
#[test_case(true; "v2")]
fn too_many_sectors(v2: bool) {
#[test]
fn too_many_sectors() {
assert_simple_batch(
v2,
Policy::default().pre_commit_sector_batch_max_size + 1,
TokenAmount::zero(),
TokenAmount::zero(),
Expand All @@ -253,12 +220,9 @@ mod miner_actor_precommit_batch {
"batch of 257 too large",
);
}

#[test_case(false; "v1")]
#[test_case(true; "v2")]
fn insufficient_balance(v2: bool) {
#[test]
fn insufficient_balance() {
assert_simple_batch(
v2,
10,
TokenAmount::from_atto(-1),
TokenAmount::zero(),
Expand All @@ -268,19 +232,16 @@ mod miner_actor_precommit_batch {
);
}

#[test_case(false; "v1")]
#[test_case(true; "v2")]
fn one_bad_apple_ruins_batch(v2: bool) {
#[test]
fn one_bad_apple_ruins_batch() {
// This test does not enumerate all the individual conditions that could cause a single precommit
// to be rejected. Those are covered in the PreCommitSector tests, and we know that that
// method is implemented in terms of a batch of one.

let period_offset = ChainEpoch::from(100);

let h = ActorHarness::new_with_options(HarnessOptions {
use_v2_pre_commit_and_replica_update: v2,
proving_period_offset: period_offset,
});
let h =
ActorHarness::new_with_options(HarnessOptions { proving_period_offset: period_offset });

let rt = h.new_runtime();

Expand Down Expand Up @@ -313,15 +274,12 @@ mod miner_actor_precommit_batch {
rt.reset();
}

#[test_case(false; "v1")]
#[test_case(true; "v2")]
fn duplicate_sector_rejects_batch(v2: bool) {
#[test]
fn duplicate_sector_rejects_batch() {
let period_offset = ChainEpoch::from(100);

let h = ActorHarness::new_with_options(HarnessOptions {
use_v2_pre_commit_and_replica_update: v2,
proving_period_offset: period_offset,
});
let h =
ActorHarness::new_with_options(HarnessOptions { proving_period_offset: period_offset });
let rt = h.new_runtime();

rt.set_balance(BIG_BALANCE.clone());
Expand Down Expand Up @@ -357,10 +315,8 @@ mod miner_actor_precommit_batch {
fn mismatch_of_commd() {
let period_offset = ChainEpoch::from(100);

let h = ActorHarness::new_with_options(HarnessOptions {
use_v2_pre_commit_and_replica_update: true,
proving_period_offset: period_offset,
});
let h =
ActorHarness::new_with_options(HarnessOptions { proving_period_offset: period_offset });
let rt = h.new_runtime();

rt.set_balance(BIG_BALANCE.clone());
Expand Down
Loading