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

Replace Map with Map2 in miner actor #1523

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Replace Map with Map2 in miner actor #1523

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
35 changes: 27 additions & 8 deletions actors/miner/src/deadline_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ use std::collections::BTreeSet;
use anyhow::anyhow;
use cid::multihash::Code;
use cid::Cid;
use fil_actors_runtime::runtime::Policy;
use fil_actors_runtime::{actor_error, ActorDowncast, ActorError, Array, AsActorError};
use fvm_ipld_bitfield::BitField;
use fvm_ipld_blockstore::Blockstore;
use fvm_ipld_encoding::tuple::*;
Expand All @@ -19,11 +17,15 @@ use fvm_shared::error::ExitCode;
use fvm_shared::sector::{PoStProof, SectorSize};
use num_traits::{Signed, Zero};

use fil_actors_runtime::runtime::Policy;
use fil_actors_runtime::{actor_error, ActorDowncast, ActorError, Array, AsActorError};

use crate::SECTORS_AMT_BITWIDTH;

use super::{
BitFieldQueue, ExpirationSet, Partition, PartitionSectorMap, PoStPartition, PowerPair,
QuantSpec, SectorOnChainInfo, Sectors, TerminationResult,
};
use crate::SECTORS_AMT_BITWIDTH;

// Bitwidth of AMTs determined empirically from mutation patterns and projections of mainnet data.
// Usually a small array
Expand Down Expand Up @@ -184,24 +186,41 @@ pub struct DisputeInfo {
}

impl Deadline {
pub fn new<BS: Blockstore>(store: &BS) -> anyhow::Result<Self> {
pub fn new<BS: Blockstore>(store: &BS) -> Result<Self, ActorError> {
let empty_partitions_array =
Array::<(), BS>::new_with_bit_width(store, DEADLINE_PARTITIONS_AMT_BITWIDTH)
.flush()
.map_err(|e| e.downcast_wrap("Failed to create empty states array"))?;
.map_err(|e| {
e.downcast_default(
ExitCode::USR_ILLEGAL_STATE,
"Failed to create empty states array",
)
})?;
let empty_deadline_expiration_array =
Array::<(), BS>::new_with_bit_width(store, DEADLINE_EXPIRATIONS_AMT_BITWIDTH)
.flush()
.map_err(|e| e.downcast_wrap("Failed to create empty states array"))?;
.map_err(|e| {
e.downcast_default(
ExitCode::USR_ILLEGAL_STATE,
"Failed to create empty states array",
)
})?;
let empty_post_submissions_array = Array::<(), BS>::new_with_bit_width(
store,
DEADLINE_OPTIMISTIC_POST_SUBMISSIONS_AMT_BITWIDTH,
)
.flush()
.map_err(|e| e.downcast_wrap("Failed to create empty states array"))?;
.map_err(|e| {
e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "Failed to create empty states array")
})?;
let empty_sectors_array = Array::<(), BS>::new_with_bit_width(store, SECTORS_AMT_BITWIDTH)
.flush()
.map_err(|e| e.downcast_wrap("Failed to construct empty sectors snapshot array"))?;
.map_err(|e| {
e.downcast_default(
ExitCode::USR_ILLEGAL_STATE,
"Failed to construct empty sectors snapshot array",
)
})?;
Ok(Self {
partitions: empty_partitions_array,
expirations_epochs: empty_deadline_expiration_array,
Expand Down
24 changes: 5 additions & 19 deletions actors/miner/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ use fil_actors_runtime::{
BURNT_FUNDS_ACTOR_ADDR, INIT_ACTOR_ADDR, REWARD_ACTOR_ADDR, STORAGE_MARKET_ACTOR_ADDR,
STORAGE_POWER_ACTOR_ADDR, SYSTEM_ACTOR_ADDR, VERIFIED_REGISTRY_ACTOR_ADDR,
};

use crate::ext::market::NO_ALLOCATION_ID;
pub use monies::*;
pub use partition_state::*;
pub use policy::*;
Expand All @@ -67,6 +65,7 @@ pub use termination::*;
pub use types::*;
pub use vesting_state::*;

use crate::ext::market::NO_ALLOCATION_ID;
use crate::notifications::{notify_data_consumers, ActivationNotifications};

// The following errors are particular cases of illegal state.
Expand Down Expand Up @@ -239,12 +238,8 @@ impl Actor {
e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to construct illegal state")
})?;

let st =
State::new(policy, rt.store(), info_cid, period_start, deadline_idx).map_err(|e| {
e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to construct state")
})?;
let st = State::new(policy, rt.store(), info_cid, period_start, deadline_idx)?;
rt.create(&st)?;

Ok(())
}

Expand Down Expand Up @@ -1555,7 +1550,7 @@ impl Actor {
None => 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 @@ -1977,12 +1972,7 @@ impl Actor {
// Validate pre-commit.
let precommit = st
.get_precommitted_sector(store, params.sector_number)
.map_err(|e| {
e.downcast_default(
ExitCode::USR_ILLEGAL_STATE,
format!("failed to load pre-committed sector {}", params.sector_number),
)
})?
.with_context(|| format!("loading pre-commit {}", params.sector_number))?
.ok_or_else(|| {
actor_error!(not_found, "no pre-commited sector {}", params.sector_number)
})?;
Expand Down Expand Up @@ -5316,11 +5306,7 @@ fn activate_new_sector_infos(
state.put_sectors(store, new_sectors.clone()).map_err(|e| {
e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to put new sectors")
})?;

state.delete_precommitted_sectors(store, &new_sector_numbers).map_err(|e| {
e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to delete precommited sectors")
})?;

state.delete_precommitted_sectors(store, &new_sector_numbers)?;
state
.assign_sectors_to_deadlines(
policy,
Expand Down
80 changes: 34 additions & 46 deletions actors/miner/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,12 @@ use std::ops::Neg;
use anyhow::{anyhow, Error};
use cid::multihash::Code;
use cid::Cid;
use fil_actors_runtime::runtime::policy_constants::MAX_SECTOR_NUMBER;
use fil_actors_runtime::runtime::Policy;
use fil_actors_runtime::{
actor_error, make_empty_map, make_map_with_root_and_bitwidth, u64_key, ActorDowncast,
ActorError, Array, AsActorError,
};
use fvm_ipld_amt::Error as AmtError;
use fvm_ipld_bitfield::BitField;
use fvm_ipld_blockstore::Blockstore;
use fvm_ipld_encoding::tuple::*;
use fvm_ipld_encoding::{strict_bytes, BytesDe, CborStore};
use fvm_ipld_hamt::Error as HamtError;
use fvm_shared::address::Address;

use fvm_shared::clock::{ChainEpoch, EPOCH_UNDEFINED};
use fvm_shared::econ::TokenAmount;
use fvm_shared::error::ExitCode;
Expand All @@ -30,6 +22,13 @@ use fvm_shared::{ActorID, HAMT_BIT_WIDTH};
use itertools::Itertools;
use num_traits::Zero;

use fil_actors_runtime::runtime::policy_constants::MAX_SECTOR_NUMBER;
use fil_actors_runtime::runtime::Policy;
use fil_actors_runtime::{
actor_error, ActorContext, ActorDowncast, ActorError, Array, AsActorError, Config, Map2,
DEFAULT_HAMT_CONFIG,
};

use super::beneficiary::*;
use super::deadlines::new_deadline_info;
use super::policy::*;
Expand All @@ -40,6 +39,9 @@ use super::{
PowerPair, QuantSpec, Sectors, TerminationResult, VestingFunds,
};

pub type PreCommitMap<BS> = Map2<BS, SectorNumber, SectorPreCommitOnChainInfo>;
pub const PRECOMMIT_CONFIG: Config = Config { bit_width: HAMT_BIT_WIDTH, ..DEFAULT_HAMT_CONFIG };

const PRECOMMIT_EXPIRY_AMT_BITWIDTH: u32 = 6;
pub const SECTORS_AMT_BITWIDTH: u32 = 5;

Expand Down Expand Up @@ -126,14 +128,10 @@ impl State {
info_cid: Cid,
period_start: ChainEpoch,
deadline_idx: u64,
) -> anyhow::Result<Self> {
) -> Result<Self, ActorError> {
let empty_precommit_map =
make_empty_map::<_, ()>(store, HAMT_BIT_WIDTH).flush().map_err(|e| {
e.downcast_default(
ExitCode::USR_ILLEGAL_STATE,
"failed to construct empty precommit map",
)
})?;
PreCommitMap::empty(store, PRECOMMIT_CONFIG, "precommits").flush()?;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PreCommitMap::empty(store, PRECOMMIT_CONFIG, "precommits").flush()?;
PreCommitMap::empty(store, PRECOMMIT_CONFIG, "pre_committed_sectors").flush()?;

Maybe stick to names that match the state names so there's no ambiguity? There's a lot of these to replace though. s/"precommits"/"pre_committed_sectors"/g

Copy link
Member Author

@anorth anorth Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is not a bad suggestion, but is not the existing pattern (all by me) either. The labels here are for easy reading independent of renames that might happen in the code. The code names are not necessarily good, and some such collections are not even named (when nested in others).

E.g. in the market

SectorDealsMap::load(store, sectors_root, SECTOR_DEALS_CONFIG, "sector deals")


let empty_precommits_cleanup_array =
Array::<BitField, BS>::new_with_bit_width(store, PRECOMMIT_EXPIRY_AMT_BITWIDTH)
.flush()
Expand Down Expand Up @@ -292,14 +290,12 @@ impl State {
precommits: Vec<SectorPreCommitOnChainInfo>,
) -> anyhow::Result<()> {
let mut precommitted =
make_map_with_root_and_bitwidth(&self.pre_committed_sectors, store, HAMT_BIT_WIDTH)?;
PreCommitMap::load(store, &self.pre_committed_sectors, PRECOMMIT_CONFIG, "precommits")?;
for precommit in precommits.into_iter() {
let sector_no = precommit.info.sector_number;
let modified = precommitted
.set_if_absent(u64_key(precommit.info.sector_number), precommit)
.map_err(|e| {
e.downcast_wrap(format!("failed to store precommitment for {:?}", sector_no,))
})?;
.set_if_absent(&sector_no, precommit)
.with_context(|| format!("storing precommit for {}", sector_no))?;
if !modified {
return Err(anyhow!("sector {} already pre-commited", sector_no));
}
Expand All @@ -313,10 +309,10 @@ impl State {
&self,
store: &BS,
sector_num: SectorNumber,
) -> Result<Option<SectorPreCommitOnChainInfo>, HamtError> {
) -> Result<Option<SectorPreCommitOnChainInfo>, ActorError> {
let precommitted =
make_map_with_root_and_bitwidth(&self.pre_committed_sectors, store, HAMT_BIT_WIDTH)?;
Ok(precommitted.get(&u64_key(sector_num))?.cloned())
PreCommitMap::load(store, &self.pre_committed_sectors, PRECOMMIT_CONFIG, "precommits")?;
Ok(precommitted.get(&sector_num)?.cloned())
}

/// Gets and returns the requested pre-committed sectors, skipping missing sectors.
Expand All @@ -325,17 +321,15 @@ impl State {
store: &BS,
sector_numbers: &[SectorNumber],
) -> anyhow::Result<Vec<SectorPreCommitOnChainInfo>> {
let precommitted = make_map_with_root_and_bitwidth::<_, SectorPreCommitOnChainInfo>(
&self.pre_committed_sectors,
store,
HAMT_BIT_WIDTH,
)?;
let precommitted =
PreCommitMap::load(store, &self.pre_committed_sectors, PRECOMMIT_CONFIG, "precommits")?;
let mut result = Vec::with_capacity(sector_numbers.len());

for &sector_number in sector_numbers {
let info = match precommitted.get(&u64_key(sector_number)).map_err(|e| {
e.downcast_wrap(format!("failed to load precommitment for {}", sector_number))
})? {
let info = match precommitted
.get(&sector_number)
.with_context(|| format!("loading precommit {}", sector_number))?
{
Some(info) => info.clone(),
None => continue,
};
Expand All @@ -350,17 +344,13 @@ impl State {
&mut self,
store: &BS,
sector_nums: &[SectorNumber],
) -> Result<(), HamtError> {
let mut precommitted = make_map_with_root_and_bitwidth::<_, SectorPreCommitOnChainInfo>(
&self.pre_committed_sectors,
store,
HAMT_BIT_WIDTH,
)?;

) -> Result<(), ActorError> {
let mut precommitted =
PreCommitMap::load(store, &self.pre_committed_sectors, PRECOMMIT_CONFIG, "precommits")?;
for &sector_num in sector_nums {
let prev_entry = precommitted.delete(&u64_key(sector_num))?;
let prev_entry = precommitted.delete(&sector_num)?;
if prev_entry.is_none() {
return Err(format!("sector {} doesn't exist", sector_num).into());
return Err(actor_error!(illegal_state, "sector {} not pre-committed", sector_num));
}
}

Expand Down Expand Up @@ -1052,13 +1042,12 @@ impl State {

let mut precommits_to_delete = Vec::new();
let precommitted =
make_map_with_root_and_bitwidth(&self.pre_committed_sectors, store, HAMT_BIT_WIDTH)?;
PreCommitMap::load(store, &self.pre_committed_sectors, PRECOMMIT_CONFIG, "precommits")?;

for i in sectors.iter() {
let sector_number = i as SectorNumber;

let sector: SectorPreCommitOnChainInfo =
match precommitted.get(&u64_key(sector_number))?.cloned() {
match precommitted.get(&sector_number)?.cloned() {
Some(sector) => sector,
// already committed/deleted
None => continue,
Expand Down Expand Up @@ -1179,15 +1168,14 @@ impl State {
) -> Result<Vec<SectorPreCommitOnChainInfo>, ActorError> {
let mut precommits = Vec::new();
let precommitted =
make_map_with_root_and_bitwidth(&self.pre_committed_sectors, store, HAMT_BIT_WIDTH)
.context_code(ExitCode::USR_ILLEGAL_STATE, "failed to load precommitted sectors")?;
PreCommitMap::load(store, &self.pre_committed_sectors, PRECOMMIT_CONFIG, "precommits")?;
for sector_no in sector_nos.into_iter() {
let sector_no = *sector_no.borrow();
if sector_no > MAX_SECTOR_NUMBER {
return Err(actor_error!(illegal_argument; "sector number greater than maximum"));
}
let info: &SectorPreCommitOnChainInfo = precommitted
.get(&u64_key(sector_no))
.get(&sector_no)
.exit_code(ExitCode::USR_ILLEGAL_STATE)?
.ok_or_else(|| actor_error!(not_found, "sector {} not found", sector_no))?;
precommits.push(info.clone());
Expand Down
26 changes: 6 additions & 20 deletions actors/miner/src/testing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
use crate::{
power_for_sectors, BitFieldQueue, Deadline, ExpirationQueue, MinerInfo, Partition, PowerPair,
QuantSpec, SectorOnChainInfo, SectorOnChainInfoFlags, SectorPreCommitOnChainInfo, Sectors,
State, NO_QUANTIZATION,
PreCommitMap, QuantSpec, SectorOnChainInfo, SectorOnChainInfoFlags, Sectors, State,
NO_QUANTIZATION, PRECOMMIT_CONFIG,
};
use fil_actors_runtime::runtime::Policy;
use fil_actors_runtime::{
parse_uint_key, DealWeight, Map, MessageAccumulator, DEFAULT_HAMT_CONFIG,
};
use fil_actors_runtime::{DealWeight, MessageAccumulator};
use fvm_ipld_bitfield::BitField;
use fvm_ipld_blockstore::Blockstore;
use fvm_ipld_encoding::CborStore;
Expand Down Expand Up @@ -349,23 +347,11 @@ fn check_precommits<BS: Blockstore>(

let mut precommit_total = TokenAmount::zero();

let precommited_sectors = Map::<_, SectorPreCommitOnChainInfo>::load_with_config(
&state.pre_committed_sectors,
store,
DEFAULT_HAMT_CONFIG,
);

let precommited_sectors =
PreCommitMap::load(store, &state.pre_committed_sectors, PRECOMMIT_CONFIG, "precommits");
match precommited_sectors {
Ok(precommited_sectors) => {
let ret = precommited_sectors.for_each(|key, precommit| {
let sector_number = match parse_uint_key(key) {
Ok(sector_number) => sector_number,
Err(e) => {
acc.add(format!("error parsing pre-commit key as uint: {e}"));
return Ok(());
}
};

let ret = precommited_sectors.for_each(|sector_number, precommit| {
acc.require(
allocated_sectors.contains(&sector_number),
format!("pre-commited sector number has not been allocated {sector_number}"),
Expand Down
3 changes: 1 addition & 2 deletions actors/miner/tests/state_harness.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use fil_actors_runtime::{runtime::Policy, ActorError};
use fvm_ipld_bitfield::BitField;
use fvm_ipld_encoding::BytesDe;
use fvm_ipld_encoding::CborStore;
use fvm_ipld_hamt::Error as HamtError;
use fvm_shared::econ::TokenAmount;
use fvm_shared::sector::{SectorNumber, SectorSize};
use fvm_shared::{clock::ChainEpoch, sector::RegisteredPoStProof};
Expand Down Expand Up @@ -68,7 +67,7 @@ impl StateHarness {
pub fn delete_precommitted_sectors(
&mut self,
sector_nums: &[SectorNumber],
) -> Result<(), HamtError> {
) -> Result<(), ActorError> {
self.st.delete_precommitted_sectors(&self.store, sector_nums)
}

Expand Down
Loading
Loading