From 649f0a5984c1f51c9940dbe2a9bfa064608f78fd Mon Sep 17 00:00:00 2001 From: Alex North <445306+anorth@users.noreply.github.com> Date: Fri, 1 Mar 2024 09:09:55 +1300 Subject: [PATCH 1/3] Update SetMultimap to use Map2 internally and match conventions. --- actors/market/src/lib.rs | 6 +- actors/market/src/state.rs | 136 +++++++----------- actors/market/src/testing.rs | 48 +++---- actors/market/tests/harness.rs | 53 ++++--- actors/market/tests/market_actor_test.rs | 37 ++--- .../tests/random_cron_epoch_during_publish.rs | 30 ++-- actors/multisig/tests/util.rs | 13 +- actors/verifreg/tests/harness/mod.rs | 7 +- integration_tests/src/tests/multisig_test.rs | 12 +- runtime/src/lib.rs | 13 -- runtime/src/util/map.rs | 5 + runtime/src/util/mod.rs | 1 + runtime/src/util/set.rs | 69 ++++----- runtime/src/util/set_multimap.rs | 128 +++++++++++------ runtime/tests/set_multimap_test.rs | 42 +++--- runtime/tests/set_test.rs | 40 +++--- 16 files changed, 316 insertions(+), 324 deletions(-) diff --git a/actors/market/src/lib.rs b/actors/market/src/lib.rs index 75b0dd4e9..c1cc47960 100644 --- a/actors/market/src/lib.rs +++ b/actors/market/src/lib.rs @@ -33,7 +33,7 @@ use fil_actors_runtime::runtime::builtins::Type; use fil_actors_runtime::runtime::{ActorCode, Policy, Runtime}; use fil_actors_runtime::{ actor_dispatch, actor_error, deserialize_block, ActorContext, ActorDowncast, ActorError, - AsActorError, Set, BURNT_FUNDS_ACTOR_ADDR, CRON_ACTOR_ADDR, DATACAP_TOKEN_ACTOR_ADDR, + AsActorError, BURNT_FUNDS_ACTOR_ADDR, CRON_ACTOR_ADDR, DATACAP_TOKEN_ACTOR_ADDR, REWARD_ACTOR_ADDR, STORAGE_POWER_ACTOR_ADDR, SYSTEM_ACTOR_ADDR, VERIFIED_REGISTRY_ACTOR_ADDR, }; use fil_actors_runtime::{extract_send_result, BatchReturnGen, FIRST_ACTOR_SPECIFIC_EXIT_CODE}; @@ -1425,7 +1425,7 @@ fn preactivate_deal( deal_id: DealID, proposals: &DealArray, states: &DealMetaArray, - pending_proposals: &Set, + pending_proposals: &PendingProposalsSet<&BS>, provider: &Address, sector_commitment: ChainEpoch, curr_epoch: ChainEpoch, @@ -1456,7 +1456,7 @@ fn preactivate_deal( // The pending deals set exists to prevent duplicate proposals. // It should be impossible to have a proposal, no deal state, and not be in pending deals. let deal_cid = deal_cid(rt, &proposal)?; - if !has_pending_deal(pending_proposals, &deal_cid)? { + if !pending_proposals.has(&deal_cid)? { return Ok(Err(actor_error!(illegal_state, "deal {} is not in pending set", deal_cid))); } diff --git a/actors/market/src/state.rs b/actors/market/src/state.rs index dd0c9988d..ab8f93721 100644 --- a/actors/market/src/state.rs +++ b/actors/market/src/state.rs @@ -18,7 +18,7 @@ use num_traits::Zero; use fil_actors_runtime::{ actor_error, ActorContext, ActorError, Array, AsActorError, Config, Map2, Set, SetMultimap, - DEFAULT_HAMT_CONFIG, + SetMultimapConfig, DEFAULT_HAMT_CONFIG, }; use crate::balance_table::BalanceTable; @@ -49,6 +49,7 @@ pub struct State { /// PendingProposals tracks dealProposals that have not yet reached their deal start date. /// We track them here to ensure that miners can't publish the same deal proposal twice + /// Set pub pending_proposals: Cid, /// Total amount held in escrow, indexed by actor address (including both locked and unlocked amounts). @@ -87,6 +88,13 @@ pub struct State { pub provider_sectors: Cid, } +pub type PendingProposalsSet = Set; +pub const PENDING_PROPOSALS_CONFIG: Config = DEFAULT_HAMT_CONFIG; + +pub type DealOpsByEpoch = SetMultimap; +pub const DEAL_OPS_BY_EPOCH_CONFIG: SetMultimapConfig = + SetMultimapConfig { outer: DEFAULT_HAMT_CONFIG, inner: DEFAULT_HAMT_CONFIG }; + pub type PendingDealAllocationsMap = Map2; pub const PENDING_ALLOCATIONS_CONFIG: Config = Config { bit_width: HAMT_BIT_WIDTH, ..DEFAULT_HAMT_CONFIG }; @@ -112,16 +120,12 @@ impl State { .flush() .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to create empty states array")?; - let empty_pending_proposals_map = Set::new(store).root().context_code( - ExitCode::USR_ILLEGAL_STATE, - "failed to create empty pending proposals map state", - )?; - + let empty_pending_proposals = + PendingProposalsSet::empty(store, PENDING_PROPOSALS_CONFIG, "pending proposals") + .flush()?; let empty_balance_table = BalanceTable::new(store, "balance table").root()?; - - let empty_deal_ops_hamt = SetMultimap::new(store) - .root() - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to create empty multiset")?; + let empty_deal_ops = + DealOpsByEpoch::empty(store, DEAL_OPS_BY_EPOCH_CONFIG, "deal ops").flush()?; let empty_pending_deal_allocation_map = PendingDealAllocationsMap::empty( store, @@ -136,11 +140,11 @@ impl State { Ok(Self { proposals: empty_proposals_array, states: empty_states_array, - pending_proposals: empty_pending_proposals_map, + pending_proposals: empty_pending_proposals, escrow_table: empty_balance_table, locked_table: empty_balance_table, next_id: 0, - deal_ops_by_epoch: empty_deal_ops_hamt, + deal_ops_by_epoch: empty_deal_ops, last_cron: EPOCH_UNDEFINED, total_client_locked_collateral: TokenAmount::default(), @@ -381,6 +385,16 @@ impl State { Ok(maybe_alloc_id) } + pub fn load_deal_ops( + &self, + store: BS, + ) -> Result, ActorError> + where + BS: Blockstore + Clone, + { + DealOpsByEpoch::load(store, &self.deal_ops_by_epoch, DEAL_OPS_BY_EPOCH_CONFIG, "deal ops") + } + pub fn put_deals_by_epoch( &mut self, store: &BS, @@ -389,20 +403,13 @@ impl State { where BS: Blockstore, { - let mut deals_by_epoch = SetMultimap::from_root(store, &self.deal_ops_by_epoch) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to load deals by epoch")?; - + let mut deals_by_epoch = self.load_deal_ops(store)?; new_deals_by_epoch.iter().try_for_each(|(epoch, id)| -> Result<(), ActorError> { - deals_by_epoch - .put(*epoch, *id) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to set deal")?; + deals_by_epoch.put(epoch, *id)?; Ok(()) })?; - self.deal_ops_by_epoch = deals_by_epoch - .root() - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to flush deals by epoch")?; - + self.deal_ops_by_epoch = deals_by_epoch.flush()?; Ok(()) } @@ -414,22 +421,13 @@ impl State { where BS: Blockstore, { - let mut deals_by_epoch = SetMultimap::from_root(store, &self.deal_ops_by_epoch) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to load deals by epoch")?; - + let mut deals_by_epoch = self.load_deal_ops(store)?; new_deals_by_epoch.iter().try_for_each(|(epoch, deals)| -> Result<(), ActorError> { - deals_by_epoch - .put_many(*epoch, deals) - .with_context_code(ExitCode::USR_ILLEGAL_STATE, || { - format!("failed to reinsert deal IDs for epoch {}", epoch) - })?; + deals_by_epoch.put_many(epoch, deals)?; Ok(()) })?; - self.deal_ops_by_epoch = deals_by_epoch - .root() - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to flush deals by epoch")?; - + self.deal_ops_by_epoch = deals_by_epoch.flush()?; Ok(()) } @@ -442,16 +440,11 @@ impl State { BS: Blockstore, { let mut deal_ids = Vec::new(); - - let deals_by_epoch = SetMultimap::from_root(store, &self.deal_ops_by_epoch) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to load deals by epoch")?; - - deals_by_epoch - .for_each(key, |deal_id| { - deal_ids.push(deal_id); - Ok(()) - }) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to set deal state")?; + let deals_by_epoch = self.load_deal_ops(store)?; + deals_by_epoch.for_each_in(&key, |deal_id| { + deal_ids.push(deal_id); + Ok(()) + })?; Ok(deal_ids) } @@ -464,22 +457,13 @@ impl State { where BS: Blockstore, { - let mut deals_by_epoch = SetMultimap::from_root(store, &self.deal_ops_by_epoch) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to load deals by epoch")?; - + let mut deals_by_epoch = self.load_deal_ops(store)?; epochs_to_remove.iter().try_for_each(|epoch| -> Result<(), ActorError> { - deals_by_epoch - .remove_all(*epoch) - .with_context_code(ExitCode::USR_ILLEGAL_STATE, || { - format!("failed to delete deal ops for epoch {}", epoch) - })?; + deals_by_epoch.remove_all(epoch)?; Ok(()) })?; - self.deal_ops_by_epoch = deals_by_epoch - .root() - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to flush deals by epoch")?; - + self.deal_ops_by_epoch = deals_by_epoch.flush()?; Ok(()) } @@ -517,21 +501,26 @@ impl State { Ok(ex) } - pub fn load_pending_deals<'bs, BS>(&self, store: &'bs BS) -> Result, ActorError> + pub fn load_pending_deals(&self, store: BS) -> Result, ActorError> where BS: Blockstore, { - Set::from_root(store, &self.pending_proposals) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to get pending deals") + PendingProposalsSet::load( + store, + &self.pending_proposals, + PENDING_PROPOSALS_CONFIG, + "pending proposals", + ) } - fn save_pending_deals(&mut self, pending_deals: &mut Set) -> Result<(), ActorError> + fn save_pending_deals( + &mut self, + pending_deals: &mut PendingProposalsSet, + ) -> Result<(), ActorError> where BS: Blockstore, { - self.pending_proposals = pending_deals - .root() - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to flush pending deals")?; + self.pending_proposals = pending_deals.flush()?; Ok(()) } @@ -540,7 +529,7 @@ impl State { BS: Blockstore, { let pending_deals = self.load_pending_deals(store)?; - has_pending_deal(&pending_deals, key) + pending_deals.has(key) } pub fn put_pending_deals( @@ -553,9 +542,7 @@ impl State { { let mut pending_deals = self.load_pending_deals(store)?; new_pending_deals.iter().try_for_each(|key: &Cid| -> Result<(), ActorError> { - pending_deals - .put(key.to_bytes().into()) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to set deal")?; + pending_deals.put(key)?; Ok(()) })?; @@ -571,11 +558,7 @@ impl State { BS: Blockstore, { let mut pending_deals = self.load_pending_deals(store)?; - let removed = pending_deals - .delete(&pending_deal_key.to_bytes()) - .with_context_code(ExitCode::USR_ILLEGAL_STATE, || { - format!("failed to delete pending proposal {}", pending_deal_key) - })?; + let removed = pending_deals.delete(&pending_deal_key)?; self.save_pending_deals(&mut pending_deals)?; Ok(removed) @@ -1264,15 +1247,6 @@ where Ok(state.cloned()) } -pub fn has_pending_deal(pending: &Set, key: &Cid) -> Result -where - BS: Blockstore, -{ - pending - .has(&key.to_bytes()) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to lookup pending deal") -} - pub fn load_provider_sector_deals( store: BS, provider_sectors: &ProviderSectorsMap, diff --git a/actors/market/src/testing.rs b/actors/market/src/testing.rs index 3b74c9fdc..05b1d42de 100644 --- a/actors/market/src/testing.rs +++ b/actors/market/src/testing.rs @@ -8,6 +8,7 @@ use cid::multihash::{Code, MultihashDigest}; use cid::Cid; use fvm_ipld_blockstore::Blockstore; use fvm_ipld_encoding::DAG_CBOR; +use fvm_shared::error::ExitCode; use fvm_shared::sector::SectorNumber; use fvm_shared::{ address::{Address, Protocol}, @@ -22,13 +23,14 @@ use num_traits::Zero; use fil_actors_runtime::builtin::HAMT_BIT_WIDTH; use fil_actors_runtime::cbor::serialize; use fil_actors_runtime::{ - make_map_with_root_and_bitwidth, parse_uint_key, ActorError, MessageAccumulator, SetMultimap, + make_map_with_root_and_bitwidth, ActorError, AsActorError, MessageAccumulator, }; use crate::ext::verifreg::AllocationID; use crate::{ - balance_table::BalanceTable, DealArray, DealMetaArray, DealProposal, ProviderSectorsMap, - SectorDealsMap, State, PROPOSALS_AMT_BITWIDTH, PROVIDER_SECTORS_CONFIG, SECTOR_DEALS_CONFIG, + balance_table::BalanceTable, DealArray, DealMetaArray, DealOpsByEpoch, DealProposal, + PendingProposalsSet, ProviderSectorsMap, SectorDealsMap, State, DEAL_OPS_BY_EPOCH_CONFIG, + PENDING_PROPOSALS_CONFIG, PROVIDER_SECTORS_CONFIG, SECTOR_DEALS_CONFIG, }; #[derive(Clone)] @@ -292,15 +294,16 @@ pub fn check_state_invariants( // pending proposals let mut pending_proposal_count = 0; - match make_map_with_root_and_bitwidth::<_, ()>( - &state.pending_proposals, + match PendingProposalsSet::load( store, - PROPOSALS_AMT_BITWIDTH, + &state.pending_proposals, + PENDING_PROPOSALS_CONFIG, + "pending proposals", ) { Ok(pending_proposals) => { - let ret = pending_proposals.for_each(|key, _| { - let proposal_cid = Cid::try_from(key.0.to_owned())?; - + let ret = pending_proposals.for_each(|key| { + let proposal_cid = Cid::try_from(key.to_owned()) + .context_code(ExitCode::USR_ILLEGAL_STATE, "not a CID")?; acc.require( proposal_cids.contains(&proposal_cid), format!("pending proposal with cid {proposal_cid} not found within proposals"), @@ -364,23 +367,20 @@ pub fn check_state_invariants( // deals ops by epoch let (mut deal_op_epoch_count, mut deal_op_count) = (0, 0); - match SetMultimap::from_root(store, &state.deal_ops_by_epoch) { + match DealOpsByEpoch::load( + store, + &state.deal_ops_by_epoch, + DEAL_OPS_BY_EPOCH_CONFIG, + "deal ops", + ) { Ok(deal_ops) => { - // get into internals just to iterate through full data structure - let ret = deal_ops.0.for_each(|key, _| { - let epoch = parse_uint_key(key)? as i64; - + let ret = deal_ops.for_each(|epoch: ChainEpoch, _| { deal_op_epoch_count += 1; - - deal_ops - .for_each(epoch, |deal_id| { - expected_deal_ops.remove(&deal_id); - deal_op_count += 1; - Ok(()) - }) - .map_err(|e| { - anyhow::anyhow!("error iterating deal ops for epoch {}: {}", epoch, e) - }) + deal_ops.for_each_in(&epoch, |deal_id: DealID| { + expected_deal_ops.remove(&deal_id); + deal_op_count += 1; + Ok(()) + }) }); acc.require_no_error(ret, "error iterating all deal ops"); } diff --git a/actors/market/tests/harness.rs b/actors/market/tests/harness.rs index 963d414e5..dce418b0b 100644 --- a/actors/market/tests/harness.rs +++ b/actors/market/tests/harness.rs @@ -30,9 +30,9 @@ use fil_actor_market::ext::miner::{ use fil_actor_market::ext::verifreg::{AllocationID, AllocationRequest, AllocationsResponse}; use fil_actor_market::{ deal_cid, deal_get_payment_remaining, BatchActivateDealsParams, BatchActivateDealsResult, - PendingDealAllocationsMap, ProviderSectorsMap, SectorDealsMap, SettleDealPaymentsParams, - SettleDealPaymentsReturn, PENDING_ALLOCATIONS_CONFIG, PROVIDER_SECTORS_CONFIG, - SECTOR_DEALS_CONFIG, + DealOpsByEpoch, PendingDealAllocationsMap, PendingProposalsSet, ProviderSectorsMap, + SectorDealsMap, SettleDealPaymentsParams, SettleDealPaymentsReturn, PENDING_ALLOCATIONS_CONFIG, + PENDING_PROPOSALS_CONFIG, PROVIDER_SECTORS_CONFIG, SECTOR_DEALS_CONFIG, }; use fil_actor_market::{ ext, ext::miner::GetControlAddressesReturnParams, next_update_epoch, @@ -45,13 +45,12 @@ use fil_actor_market::{ use fil_actor_power::{CurrentTotalPowerReturn, Method as PowerMethod}; use fil_actor_reward::Method as RewardMethod; use fil_actors_runtime::cbor::serialize; -use fil_actors_runtime::parse_uint_key; use fil_actors_runtime::{ network::EPOCHS_IN_DAY, runtime::{builtins::Type, Policy, Runtime}, test_utils::*, - ActorError, BatchReturn, EventBuilder, Set, SetMultimap, BURNT_FUNDS_ACTOR_ADDR, - CRON_ACTOR_ADDR, DATACAP_TOKEN_ACTOR_ADDR, REWARD_ACTOR_ADDR, STORAGE_MARKET_ACTOR_ADDR, + ActorError, BatchReturn, EventBuilder, BURNT_FUNDS_ACTOR_ADDR, CRON_ACTOR_ADDR, + DATACAP_TOKEN_ACTOR_ADDR, REWARD_ACTOR_ADDR, STORAGE_MARKET_ACTOR_ADDR, STORAGE_POWER_ACTOR_ADDR, SYSTEM_ACTOR_ADDR, VERIFIED_REGISTRY_ACTOR_ADDR, }; @@ -133,21 +132,16 @@ pub fn assert_deal_ops_clean(rt: &MockRuntime) { }) .unwrap(); - let deal_ops = SetMultimap::from_root(rt.store(), &st.deal_ops_by_epoch).unwrap(); - deal_ops - .0 - .for_each(|key, _| { - let epoch = parse_uint_key(key).unwrap() as i64; - - deal_ops - .for_each(epoch, |ref deal_id| { - assert!(proposal_set.contains(deal_id), "deal op found for deal id {deal_id} with missing proposal at epoch {epoch}"); - Ok(()) - }) - .unwrap(); - Ok(()) - }) - .unwrap(); + let deal_ops = st.load_deal_ops(rt.store()).unwrap(); + deal_ops.for_each(|epoch, _| { + deal_ops + .for_each_in(&epoch, |deal_id| { + assert!(proposal_set.contains(&deal_id), "deal op found for deal id {deal_id} with missing proposal at epoch {epoch}"); + Ok(()) + }) + .unwrap(); + Ok(()) + }).unwrap(); } /// Checks internal invariants of market state asserting none of them are broken. @@ -1171,15 +1165,15 @@ pub fn expect_query_network_info(rt: &MockRuntime) { } pub fn assert_n_good_deals( - dobe: &SetMultimap, + dobe: &DealOpsByEpoch, updates_interval: ChainEpoch, epoch: ChainEpoch, n: isize, ) where - BS: fvm_ipld_blockstore::Blockstore, + BS: fvm_ipld_blockstore::Blockstore + Clone, { let mut count = 0; - dobe.for_each(epoch, |id| { + dobe.for_each_in(&epoch, |id| { assert_eq!(epoch % updates_interval, (id as i64) % updates_interval); count += 1; Ok(()) @@ -1203,7 +1197,6 @@ pub fn assert_deal_deleted( ) { use cid::multihash::Code; use cid::multihash::MultihashDigest; - use fvm_ipld_hamt::BytesKey; let st: State = rt.get_state(); @@ -1220,8 +1213,14 @@ pub fn assert_deal_deleted( let mh_code = Code::Blake2b256; let p_cid = Cid::new_v1(fvm_ipld_encoding::DAG_CBOR, mh_code.digest(&to_vec(p).unwrap())); // Check that the deal_id is not in st.pending_proposals. - let pending_deals = Set::from_root(rt.store(), &st.pending_proposals).unwrap(); - assert!(!pending_deals.has(&BytesKey(p_cid.to_bytes())).unwrap()); + let pending_deals = PendingProposalsSet::load( + rt.store(), + &st.pending_proposals, + PENDING_PROPOSALS_CONFIG, + "pending proposals", + ) + .unwrap(); + assert!(!pending_deals.has(&p_cid).unwrap()); // Check deal is no longer associated with sector let sector_deals = get_sector_deal_ids(rt, p.provider.id().unwrap(), &[sector_number]); diff --git a/actors/market/tests/market_actor_test.rs b/actors/market/tests/market_actor_test.rs index 2cc9e930e..9117f2c8e 100644 --- a/actors/market/tests/market_actor_test.rs +++ b/actors/market/tests/market_actor_test.rs @@ -17,7 +17,7 @@ use fvm_shared::error::ExitCode; use fvm_shared::piece::PaddedPieceSize; use fvm_shared::sector::{RegisteredSealProof, StoragePower}; use fvm_shared::sys::SendFlags; -use fvm_shared::{MethodNum, HAMT_BIT_WIDTH, METHOD_CONSTRUCTOR, METHOD_SEND}; +use fvm_shared::{MethodNum, METHOD_CONSTRUCTOR, METHOD_SEND}; use num_traits::{FromPrimitive, Zero}; use regex::Regex; @@ -27,18 +27,19 @@ use fil_actor_market::ext::verifreg::{AllocationRequest, AllocationsResponse}; use fil_actor_market::policy::detail::DEAL_MAX_LABEL_SIZE; use fil_actor_market::{ ext, Actor as MarketActor, BatchActivateDealsResult, ClientDealProposal, DealArray, - DealMetaArray, Label, MarketNotifyDealParams, Method, PendingDealAllocationsMap, - PublishStorageDealsParams, PublishStorageDealsReturn, SectorDeals, State, - WithdrawBalanceParams, EX_DEAL_EXPIRED, MARKET_NOTIFY_DEAL_METHOD, PENDING_ALLOCATIONS_CONFIG, - PROPOSALS_AMT_BITWIDTH, STATES_AMT_BITWIDTH, + DealMetaArray, DealOpsByEpoch, Label, MarketNotifyDealParams, Method, + PendingDealAllocationsMap, PendingProposalsSet, PublishStorageDealsParams, + PublishStorageDealsReturn, SectorDeals, State, WithdrawBalanceParams, DEAL_OPS_BY_EPOCH_CONFIG, + EX_DEAL_EXPIRED, MARKET_NOTIFY_DEAL_METHOD, PENDING_ALLOCATIONS_CONFIG, + PENDING_PROPOSALS_CONFIG, PROPOSALS_AMT_BITWIDTH, STATES_AMT_BITWIDTH, }; use fil_actors_runtime::cbor::{deserialize, serialize}; use fil_actors_runtime::network::EPOCHS_IN_DAY; use fil_actors_runtime::runtime::{Policy, Runtime}; use fil_actors_runtime::test_utils::*; use fil_actors_runtime::{ - make_empty_map, ActorError, BatchReturn, SetMultimap, BURNT_FUNDS_ACTOR_ADDR, - DATACAP_TOKEN_ACTOR_ADDR, SYSTEM_ACTOR_ADDR, VERIFIED_REGISTRY_ACTOR_ADDR, + ActorError, BatchReturn, SetMultimap, SetMultimapConfig, BURNT_FUNDS_ACTOR_ADDR, + DATACAP_TOKEN_ACTOR_ADDR, DEFAULT_HAMT_CONFIG, SYSTEM_ACTOR_ADDR, VERIFIED_REGISTRY_ACTOR_ADDR, }; use harness::*; @@ -48,8 +49,10 @@ mod harness; fn test_remove_all_error() { let market_actor = Address::new_id(100); let rt = MockRuntime { receiver: market_actor, ..Default::default() }; - - SetMultimap::new(&rt.store()).remove_all(42).expect("expected no error"); + let config = SetMultimapConfig { outer: DEFAULT_HAMT_CONFIG, inner: DEFAULT_HAMT_CONFIG }; + SetMultimap::<_, u64, u64>::empty(&rt.store(), config, "test") + .remove_all(&42u64) + .expect("expected no error"); } #[test] @@ -71,22 +74,24 @@ fn simple_construction() { let store = &rt.store; let empty_balance_table = BalanceTable::new(store, "empty").root().unwrap(); - let empty_map = make_empty_map::<_, ()>(store, HAMT_BIT_WIDTH).flush().unwrap(); + let empty_pending_proposals = + PendingProposalsSet::empty(store, PENDING_PROPOSALS_CONFIG, "empty").flush().unwrap(); let empty_proposals_array = Amt::<(), _>::new_with_bit_width(store, PROPOSALS_AMT_BITWIDTH).flush().unwrap(); let empty_states_array = Amt::<(), _>::new_with_bit_width(store, STATES_AMT_BITWIDTH).flush().unwrap(); - let empty_multimap = SetMultimap::new(store).root().unwrap(); + let empty_deal_ops = + DealOpsByEpoch::empty(store, DEAL_OPS_BY_EPOCH_CONFIG, "empty").flush().unwrap(); let state_data: State = rt.get_state(); assert_eq!(empty_proposals_array, state_data.proposals); assert_eq!(empty_states_array, state_data.states); - assert_eq!(empty_map, state_data.pending_proposals); + assert_eq!(empty_pending_proposals, state_data.pending_proposals); assert_eq!(empty_balance_table, state_data.escrow_table); assert_eq!(empty_balance_table, state_data.locked_table); assert_eq!(0, state_data.next_id); - assert_eq!(empty_multimap, state_data.deal_ops_by_epoch); + assert_eq!(empty_deal_ops, state_data.deal_ops_by_epoch); assert_eq!(state_data.last_cron, EPOCH_UNDEFINED); } @@ -579,7 +584,7 @@ fn deal_starts_on_day_boundary() { // Check that DOBE has exactly 3 deals scheduled every epoch in the day following the start time let st: State = rt.get_state(); let store = &rt.store; - let dobe = SetMultimap::from_root(store, &st.deal_ops_by_epoch).unwrap(); + let dobe = st.load_deal_ops(store).unwrap(); for e in interval..(2 * interval) { assert_n_good_deals(&dobe, interval, e, 3); } @@ -622,7 +627,7 @@ fn deal_starts_partway_through_day() { } let st: State = rt.get_state(); let store = &rt.store; - let dobe = SetMultimap::from_root(store, &st.deal_ops_by_epoch).unwrap(); + let dobe = st.load_deal_ops(store).unwrap(); for e in interval..(interval + start_epoch) { assert_n_good_deals(&dobe, interval, e, 1); } @@ -647,7 +652,7 @@ fn deal_starts_partway_through_day() { } let st: State = rt.get_state(); let store = &rt.store; - let dobe = SetMultimap::from_root(store, &st.deal_ops_by_epoch).unwrap(); + let dobe = st.load_deal_ops(store).unwrap(); for e in start_epoch..(start_epoch + 50) { assert_n_good_deals(&dobe, interval, e, 1); } diff --git a/actors/market/tests/random_cron_epoch_during_publish.rs b/actors/market/tests/random_cron_epoch_during_publish.rs index 67f392774..51e42971b 100644 --- a/actors/market/tests/random_cron_epoch_during_publish.rs +++ b/actors/market/tests/random_cron_epoch_during_publish.rs @@ -1,20 +1,21 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT //! TODO: remove tests for legacy behaviour: https://github.com/filecoin-project/builtin-actors/issues/1389 -use fil_actor_market::EX_DEAL_EXPIRED; -use fil_actor_market::{deal_cid, State}; -use fil_actors_runtime::network::EPOCHS_IN_DAY; -use fil_actors_runtime::runtime::Runtime; -use fil_actors_runtime::{parse_uint_key, u64_key, SetMultimap, BURNT_FUNDS_ACTOR_ADDR}; + use fvm_shared::clock::ChainEpoch; use fvm_shared::error::ExitCode; use fvm_shared::sector::SectorNumber; use fvm_shared::METHOD_SEND; -mod harness; - +use fil_actor_market::EX_DEAL_EXPIRED; +use fil_actor_market::{deal_cid, State}; +use fil_actors_runtime::network::EPOCHS_IN_DAY; +use fil_actors_runtime::runtime::Runtime; +use fil_actors_runtime::BURNT_FUNDS_ACTOR_ADDR; use harness::*; +mod harness; + const START_EPOCH: ChainEpoch = 50; const END_EPOCH: ChainEpoch = START_EPOCH + 200 * EPOCHS_IN_DAY; const SECTOR_EXPIRY: ChainEpoch = END_EPOCH + 1; @@ -59,21 +60,16 @@ fn cron_processing_happens_at_processing_epoch_not_start_epoch() { // check that deal was not rescheduled let state: State = rt.get_state(); - let deal_ops = SetMultimap::from_root(rt.store(), &state.deal_ops_by_epoch).unwrap(); - - // get into internals just to iterate through full data structure + let deal_ops = state.load_deal_ops(&rt.store).unwrap(); deal_ops - .0 - .for_each(|key, _| { - let epoch = parse_uint_key(key)? as i64; - let epoch_ops = deal_ops.get(epoch).unwrap().unwrap(); - assert!(!epoch_ops.has(&u64_key(deal_id))?); + .for_each(|epoch, _| { + let epoch_ops = deal_ops.get(&epoch).unwrap().unwrap(); + assert!(!epoch_ops.has(&deal_id).unwrap()); Ok(()) }) .unwrap(); - assert!(!state.has_pending_deal(rt.store(), &dcid).unwrap()); - + assert!(!state.has_pending_deal(&rt.store(), &dcid).unwrap()); check_state(&rt); } diff --git a/actors/multisig/tests/util.rs b/actors/multisig/tests/util.rs index 4a639e9e8..c5666e61f 100644 --- a/actors/multisig/tests/util.rs +++ b/actors/multisig/tests/util.rs @@ -1,17 +1,16 @@ use fil_actor_multisig::{ compute_proposal_hash, Actor, AddSignerParams, ApproveReturn, ConstructorParams, Method, - ProposeParams, ProposeReturn, RemoveSignerParams, State, SwapSignerParams, Transaction, TxnID, - TxnIDParams, + PendingTxnMap, ProposeParams, ProposeReturn, RemoveSignerParams, State, SwapSignerParams, + Transaction, TxnID, TxnIDParams, PENDING_TXN_CONFIG, }; use fil_actor_multisig::{ChangeNumApprovalsThresholdParams, LockBalanceParams}; use fil_actors_runtime::test_utils::*; +use fil_actors_runtime::ActorError; use fil_actors_runtime::INIT_ACTOR_ADDR; -use fil_actors_runtime::{make_map_with_root, ActorError}; use fvm_ipld_encoding::RawBytes; use fvm_shared::address::Address; use fvm_shared::clock::ChainEpoch; use fvm_shared::econ::TokenAmount; -use integer_encoding::VarInt; use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_shared::error::ExitCode; @@ -207,11 +206,11 @@ impl ActorHarness { mut expect_txns: Vec<(TxnID, Transaction)>, ) { let st: State = rt.get_state(); - let ptx = make_map_with_root::<_, Transaction>(&st.pending_txs, &rt.store).unwrap(); + let ptx = + PendingTxnMap::load(&rt.store, &st.pending_txs, PENDING_TXN_CONFIG, "pending").unwrap(); let mut actual_txns = Vec::new(); ptx.for_each(|k, txn: &Transaction| { - let id = i64::decode_var(k).unwrap().0; - actual_txns.push((TxnID(id), txn.clone())); + actual_txns.push((k, txn.clone())); Ok(()) }) .unwrap(); diff --git a/actors/verifreg/tests/harness/mod.rs b/actors/verifreg/tests/harness/mod.rs index 26aa97ba5..5df23f5ef 100644 --- a/actors/verifreg/tests/harness/mod.rs +++ b/actors/verifreg/tests/harness/mod.rs @@ -15,9 +15,10 @@ use fvm_shared::error::ExitCode; use fvm_shared::piece::PaddedPieceSize; use fvm_shared::sector::SectorNumber; use fvm_shared::sys::SendFlags; -use fvm_shared::{ActorID, MethodNum, HAMT_BIT_WIDTH}; +use fvm_shared::{ActorID, MethodNum}; use num_traits::{ToPrimitive, Zero}; +use fil_actor_verifreg::state::{DataCapMap, DATACAP_MAP_CONFIG}; use fil_actor_verifreg::testing::check_state_invariants; use fil_actor_verifreg::{ ext, Actor as VerifregActor, AddVerifiedClientParams, AddVerifierParams, Allocation, @@ -35,7 +36,7 @@ use fil_actors_runtime::runtime::policy_constants::{ use fil_actors_runtime::runtime::Runtime; use fil_actors_runtime::test_utils::*; use fil_actors_runtime::{ - make_empty_map, ActorError, AsActorError, BatchReturn, EventBuilder, DATACAP_TOKEN_ACTOR_ADDR, + ActorError, AsActorError, BatchReturn, EventBuilder, DATACAP_TOKEN_ACTOR_ADDR, STORAGE_MARKET_ACTOR_ADDR, SYSTEM_ACTOR_ADDR, VERIFIED_REGISTRY_ACTOR_ADDR, }; @@ -101,7 +102,7 @@ impl Harness { assert!(ret.is_none()); rt.verify(); - let empty_map = make_empty_map::<_, ()>(&rt.store, HAMT_BIT_WIDTH).flush().unwrap(); + let empty_map = DataCapMap::empty(&rt.store, DATACAP_MAP_CONFIG, "empty").flush().unwrap(); let state: State = rt.get_state(); assert_eq!(self.root, state.root_key); assert_eq!(empty_map, state.verifiers); diff --git a/integration_tests/src/tests/multisig_test.rs b/integration_tests/src/tests/multisig_test.rs index cf8d4e16d..d9ae09b23 100644 --- a/integration_tests/src/tests/multisig_test.rs +++ b/integration_tests/src/tests/multisig_test.rs @@ -1,20 +1,19 @@ use export_macro::vm_test; use fil_actor_init::ExecReturn; use fil_actor_multisig::{ - compute_proposal_hash, Method as MsigMethod, ProposeParams, RemoveSignerParams, - State as MsigState, SwapSignerParams, Transaction, TxnID, TxnIDParams, + compute_proposal_hash, Method as MsigMethod, PendingTxnMap, ProposeParams, RemoveSignerParams, + State as MsigState, SwapSignerParams, Transaction, TxnID, TxnIDParams, PENDING_TXN_CONFIG, }; use fil_actors_runtime::cbor::serialize; use fil_actors_runtime::runtime::Policy; use fil_actors_runtime::test_utils::*; -use fil_actors_runtime::{make_map_with_root, INIT_ACTOR_ADDR, SYSTEM_ACTOR_ADDR}; +use fil_actors_runtime::{INIT_ACTOR_ADDR, SYSTEM_ACTOR_ADDR}; use fvm_ipld_encoding::RawBytes; use fvm_shared::address::Address; use fvm_shared::bigint::Zero; use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; use fvm_shared::METHOD_SEND; -use integer_encoding::VarInt; use std::collections::HashSet; use std::iter::FromIterator; use vm_api::trace::ExpectInvocation; @@ -300,11 +299,10 @@ fn create_msig(v: &dyn VM, signers: &[Address], threshold: u64) -> Address { fn check_txs(v: &dyn VM, msig_addr: Address, mut expect_txns: Vec<(TxnID, Transaction)>) { let st: MsigState = get_state(v, &msig_addr).unwrap(); let store = DynBlockstore::wrap(v.blockstore()); - let ptx = make_map_with_root::<_, Transaction>(&st.pending_txs, &store).unwrap(); + let ptx = PendingTxnMap::load(&store, &st.pending_txs, PENDING_TXN_CONFIG, "pending").unwrap(); let mut actual_txns = Vec::new(); ptx.for_each(|k, txn: &Transaction| { - let id = i64::decode_var(k).unwrap().0; - actual_txns.push((TxnID(id), txn.clone())); + actual_txns.push((k, txn.clone())); Ok(()) }) .unwrap(); diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 77f3bfb05..f6f82c243 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -67,19 +67,6 @@ where Map::<_, V>::new_with_bit_width(store, bitwidth) } -/// Create a map with a root cid. -#[inline] -pub fn make_map_with_root<'bs, BS, V>( - root: &Cid, - store: &'bs BS, -) -> Result, HamtError> -where - BS: Blockstore, - V: DeserializeOwned + Serialize, -{ - Map::<_, V>::load_with_bit_width(root, store, HAMT_BIT_WIDTH) -} - /// Create a map with a root cid. #[inline] pub fn make_map_with_root_and_bitwidth<'bs, BS, V>( diff --git a/runtime/src/util/map.rs b/runtime/src/util/map.rs index dc43440ec..b3c3e3ee2 100644 --- a/runtime/src/util/map.rs +++ b/runtime/src/util/map.rs @@ -87,6 +87,11 @@ where }) } + /// Returns a reference to the underlying blockstore. + pub fn store(&self) -> &BS { + self.hamt.store() + } + /// Returns whether the map is empty. pub fn is_empty(&self) -> bool { self.hamt.is_empty() diff --git a/runtime/src/util/mod.rs b/runtime/src/util/mod.rs index 9202f6acf..2857acf92 100644 --- a/runtime/src/util/mod.rs +++ b/runtime/src/util/mod.rs @@ -10,6 +10,7 @@ pub use self::message_accumulator::MessageAccumulator; pub use self::multimap::*; pub use self::set::Set; pub use self::set_multimap::SetMultimap; +pub use self::set_multimap::SetMultimapConfig; mod batch_return; pub mod cbor; diff --git a/runtime/src/util/set.rs b/runtime/src/util/set.rs index 67f0faca3..63c9ee250 100644 --- a/runtime/src/util/set.rs +++ b/runtime/src/util/set.rs @@ -3,87 +3,74 @@ use cid::Cid; use fvm_ipld_blockstore::Blockstore; -use fvm_ipld_hamt::Error; -use fvm_shared::HAMT_BIT_WIDTH; -use crate::{make_empty_map, make_map_with_root, BytesKey, Map}; +use crate::{ActorError, Config, Map2, MapKey}; -/// Set is a Hamt with empty values for the purpose of acting as a hash set. -#[derive(Debug)] -pub struct Set<'a, BS>(Map<'a, BS, ()>); - -impl<'a, BS: Blockstore> PartialEq for Set<'a, BS> { - fn eq(&self, other: &Self) -> bool { - self.0 == other.0 - } -} +/// Set is a HAMT with empty values. +pub struct Set(Map2) +where + BS: Blockstore, + K: MapKey; -impl<'a, BS> Set<'a, BS> +impl Set where BS: Blockstore, + K: MapKey, { /// Initializes a new empty Set with the default bitwidth. - pub fn new(bs: &'a BS) -> Self { - Self(make_empty_map(bs, HAMT_BIT_WIDTH)) - } - - /// Initializes a new empty Set given a bitwidth. - pub fn new_set_with_bitwidth(bs: &'a BS, bitwidth: u32) -> Self { - Self(make_empty_map(bs, bitwidth)) + pub fn empty(bs: BS, config: Config, name: &'static str) -> Self { + Self(Map2::empty(bs, config, name)) } /// Initializes a Set from a root Cid. - pub fn from_root(bs: &'a BS, cid: &Cid) -> Result { - Ok(Self(make_map_with_root(cid, bs)?)) + pub fn load( + bs: BS, + root: &Cid, + config: Config, + name: &'static str, + ) -> Result { + Ok(Self(Map2::load(bs, root, config, name)?)) } /// Retrieve root from the Set. #[inline] - pub fn root(&mut self) -> Result { + pub fn flush(&mut self) -> Result { self.0.flush() } /// Adds key to the set. #[inline] - pub fn put(&mut self, key: BytesKey) -> Result<(), Error> { - // Set hamt node to array root - self.0.set(key, ())?; - Ok(()) + pub fn put(&mut self, key: &K) -> Result, ActorError> { + self.0.set(key, ()) } /// Checks if key exists in the set. #[inline] - pub fn has(&self, key: &[u8]) -> Result { + pub fn has(&self, key: &K) -> Result { self.0.contains_key(key) } /// Deletes key from set. #[inline] - pub fn delete(&mut self, key: &[u8]) -> Result, Error> { - match self.0.delete(key)? { - Some(_) => Ok(Some(())), - None => Ok(None), - } + pub fn delete(&mut self, key: &K) -> Result, ActorError> { + self.0.delete(key) } /// Iterates through all keys in the set. - pub fn for_each(&self, mut f: F) -> Result<(), Error> + pub fn for_each(&self, mut f: F) -> Result<(), ActorError> where - F: FnMut(&BytesKey) -> anyhow::Result<()>, + F: FnMut(K) -> Result<(), ActorError>, { - // Calls the for each function on the hamt with ignoring the value - self.0.for_each(|s, _: &()| f(s)) + self.0.for_each(|s, _| f(s)) } /// Collects all keys from the set into a vector. - pub fn collect_keys(&self) -> Result, Error> { + pub fn collect_keys(&self) -> Result, ActorError> { let mut ret_keys = Vec::new(); - self.for_each(|k| { - ret_keys.push(k.clone()); + ret_keys.push(k); Ok(()) })?; - Ok(ret_keys) } } diff --git a/runtime/src/util/set_multimap.rs b/runtime/src/util/set_multimap.rs index f2686e4d6..22e24a4b9 100644 --- a/runtime/src/util/set_multimap.rs +++ b/runtime/src/util/set_multimap.rs @@ -1,113 +1,155 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT -use std::borrow::Borrow; +use std::marker::PhantomData; use cid::Cid; use fvm_ipld_blockstore::Blockstore; -use fvm_ipld_hamt::Error; -use fvm_shared::clock::ChainEpoch; -use fvm_shared::deal::DealID; -use fvm_shared::HAMT_BIT_WIDTH; + +use crate::{ActorError, Config, Map2, MapKey}; use super::Set; -use crate::{make_empty_map, make_map_with_root, parse_uint_key, u64_key, Map}; + +pub struct SetMultimapConfig { + pub outer: Config, + pub inner: Config, +} /// SetMultimap is a hamt with values that are also a hamt but are of the set variant. /// This allows hash sets to be indexable by an address. -pub struct SetMultimap<'a, BS>(pub Map<'a, BS, Cid>); - -impl<'a, BS> SetMultimap<'a, BS> +pub struct SetMultimap where BS: Blockstore, + K: MapKey, + V: MapKey, +{ + outer: Map2, + inner_config: Config, + value_type: PhantomData, +} + +impl SetMultimap +where + BS: Blockstore + Clone, + K: MapKey, + V: MapKey, { /// Initializes a new empty SetMultimap. - pub fn new(bs: &'a BS) -> Self { - Self(make_empty_map(bs, HAMT_BIT_WIDTH)) + pub fn empty(bs: BS, config: SetMultimapConfig, name: &'static str) -> Self { + Self { + outer: Map2::empty(bs, config.outer, name), + inner_config: config.inner, + value_type: Default::default(), + } } /// Initializes a SetMultimap from a root Cid. - pub fn from_root(bs: &'a BS, cid: &Cid) -> Result { - Ok(Self(make_map_with_root(cid, bs)?)) + pub fn load( + bs: BS, + root: &Cid, + config: SetMultimapConfig, + name: &'static str, + ) -> Result { + Ok(Self { + outer: Map2::load(bs, root, config.outer, name)?, + inner_config: config.inner, + value_type: Default::default(), + }) } /// Retrieve root from the SetMultimap. #[inline] - pub fn root(&mut self) -> Result { - self.0.flush() + pub fn flush(&mut self) -> Result { + self.outer.flush() } /// Puts the DealID in the hash set of the key. - pub fn put(&mut self, key: ChainEpoch, value: DealID) -> Result<(), Error> { + pub fn put(&mut self, key: &K, value: V) -> Result<(), ActorError> { // Get construct amt from retrieved cid or create new - let mut set = self.get(key)?.unwrap_or_else(|| Set::new(self.0.store())); + let mut set = self.get(key)?.unwrap_or_else(|| { + Set::empty(self.outer.store().clone(), self.inner_config.clone(), "multimap inner") + }); - set.put(u64_key(value))?; + set.put(&value)?; // Save and calculate new root - let new_root = set.root()?; + let new_root = set.flush()?; // Set hamt node to set new root - self.0.set(u64_key(key as u64), new_root)?; + self.outer.set(key, new_root)?; Ok(()) } /// Puts slice of DealIDs in the hash set of the key. - pub fn put_many(&mut self, key: ChainEpoch, values: &[DealID]) -> Result<(), Error> { + pub fn put_many(&mut self, key: &K, values: &[V]) -> Result<(), ActorError> { // Get construct amt from retrieved cid or create new - let mut set = self.get(key)?.unwrap_or_else(|| Set::new(self.0.store())); + let mut set = self.get(key)?.unwrap_or_else(|| { + Set::empty(self.outer.store().clone(), self.inner_config.clone(), "multimap inner") + }); - for &v in values { - set.put(u64_key(v))?; + for v in values { + set.put(v)?; } // Save and calculate new root - let new_root = set.root()?; + let new_root = set.flush()?; // Set hamt node to set new root - self.0.set(u64_key(key as u64), new_root)?; + self.outer.set(key, new_root)?; Ok(()) } /// Gets the set at the given index of the `SetMultimap` #[inline] - pub fn get(&self, key: ChainEpoch) -> Result>, Error> { - match self.0.get(&u64_key(key as u64))? { - Some(cid) => Ok(Some(Set::from_root(*self.0.store(), cid)?)), + pub fn get(&self, key: &K) -> Result>, ActorError> { + match self.outer.get(key)? { + Some(cid) => Ok(Some(Set::load( + self.outer.store().clone(), + cid, + self.inner_config.clone(), + "multimap inner", + )?)), None => Ok(None), } } /// Removes a DealID from a key hash set. #[inline] - pub fn remove(&mut self, key: ChainEpoch, v: DealID) -> Result<(), Error> { + pub fn remove(&mut self, key: &K, v: V) -> Result<(), ActorError> { // Get construct amt from retrieved cid and return if no set exists let mut set = match self.get(key)? { Some(s) => s, None => return Ok(()), }; - set.delete(u64_key(v).borrow())?; + set.delete(&v)?; // Save and calculate new root - let new_root = set.root()?; - self.0.set(u64_key(key as u64), new_root)?; + let new_root = set.flush()?; + self.outer.set(key, new_root)?; Ok(()) } /// Removes set at index. #[inline] - pub fn remove_all(&mut self, key: ChainEpoch) -> Result<(), Error> { + pub fn remove_all(&mut self, key: &K) -> Result<(), ActorError> { // Remove entry from table - self.0.delete(&u64_key(key as u64))?; - + self.outer.delete(key)?; Ok(()) } - /// Iterates through keys and converts them to a DealID to call a function on each. - pub fn for_each(&self, key: ChainEpoch, mut f: F) -> Result<(), Error> + /// Iterates over all keys. + pub fn for_each(&self, mut f: F) -> Result<(), ActorError> + where + F: FnMut(K, &Cid) -> Result<(), ActorError>, + { + self.outer.for_each(|k, v| f(k, v)) + } + + /// Iterates values for a key. + pub fn for_each_in(&self, key: &K, f: F) -> Result<(), ActorError> where - F: FnMut(DealID) -> Result<(), Error>, + F: FnMut(V) -> Result<(), ActorError>, { // Get construct amt from retrieved cid and return if no set exists let set = match self.get(key)? { @@ -115,12 +157,6 @@ where None => return Ok(()), }; - set.for_each(|k| { - let v = parse_uint_key(k) - .map_err(|e| anyhow::anyhow!("Could not parse key: {:?}, ({})", &k.0, e))?; - - // Run function on all parsed keys - Ok(f(v)?) - }) + set.for_each(f) } } diff --git a/runtime/tests/set_multimap_test.rs b/runtime/tests/set_multimap_test.rs index 1ca4fe71e..309352af5 100644 --- a/runtime/tests/set_multimap_test.rs +++ b/runtime/tests/set_multimap_test.rs @@ -1,45 +1,49 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT -use fil_actors_runtime::test_blockstores::MemoryBlockstore; -use fil_actors_runtime::{u64_key, SetMultimap}; use fvm_shared::clock::ChainEpoch; +use fil_actors_runtime::test_blockstores::MemoryBlockstore; +use fil_actors_runtime::{SetMultimap, SetMultimapConfig, DEFAULT_HAMT_CONFIG}; + +pub const CONFIG: SetMultimapConfig = + SetMultimapConfig { outer: DEFAULT_HAMT_CONFIG, inner: DEFAULT_HAMT_CONFIG }; + #[test] fn put_remove() { let store = MemoryBlockstore::new(); - let mut smm = SetMultimap::new(&store); + let mut smm = SetMultimap::<_, ChainEpoch, u64>::empty(&store, CONFIG, "t"); let epoch: ChainEpoch = 100; - assert_eq!(smm.get(epoch).unwrap(), None); + assert!(smm.get(&epoch).unwrap().is_none()); - smm.put(epoch, 8).unwrap(); - smm.put(epoch, 2).unwrap(); - smm.remove(epoch, 2).unwrap(); + smm.put(&epoch, 8).unwrap(); + smm.put(&epoch, 2).unwrap(); + smm.remove(&epoch, 2).unwrap(); - let set = smm.get(epoch).unwrap().unwrap(); - assert!(set.has(&u64_key(8)).unwrap()); - assert!(!set.has(&u64_key(2)).unwrap()); + let set = smm.get(&epoch).unwrap().unwrap(); + assert!(set.has(&8).unwrap()); + assert!(!set.has(&2).unwrap()); - smm.remove_all(epoch).unwrap(); - assert_eq!(smm.get(epoch).unwrap(), None); + smm.remove_all(&epoch).unwrap(); + assert!(smm.get(&epoch).unwrap().is_none()); } #[test] fn for_each() { let store = MemoryBlockstore::new(); - let mut smm = SetMultimap::new(&store); + let mut smm = SetMultimap::<_, ChainEpoch, u64>::empty(&store, CONFIG, "t"); let epoch: ChainEpoch = 100; - assert_eq!(smm.get(epoch).unwrap(), None); + assert!(smm.get(&epoch).unwrap().is_none()); - smm.put(epoch, 8).unwrap(); - smm.put(epoch, 3).unwrap(); - smm.put(epoch, 2).unwrap(); - smm.put(epoch, 8).unwrap(); + smm.put(&epoch, 8).unwrap(); + smm.put(&epoch, 3).unwrap(); + smm.put(&epoch, 2).unwrap(); + smm.put(&epoch, 8).unwrap(); let mut vals: Vec = Vec::new(); - smm.for_each(epoch, |i| { + smm.for_each_in(&epoch, |i| { vals.push(i); Ok(()) }) diff --git a/runtime/tests/set_test.rs b/runtime/tests/set_test.rs index d12f10ea9..bbd3df5ef 100644 --- a/runtime/tests/set_test.rs +++ b/runtime/tests/set_test.rs @@ -1,32 +1,32 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT -use fil_actors_runtime::Set; +use fil_actors_runtime::{Set, DEFAULT_HAMT_CONFIG}; #[test] fn put() { let store = fil_actors_runtime::test_blockstores::MemoryBlockstore::new(); - let mut set = Set::new(&store); + let mut set = Set::empty(&store, DEFAULT_HAMT_CONFIG, "t"); - let key = "test".as_bytes(); - assert!(!set.has(key).unwrap()); + let key: Vec = "test".into(); + assert!(!set.has(&key).unwrap()); - set.put(key.into()).unwrap(); - assert!(set.has(key).unwrap()); + set.put(&key).unwrap(); + assert!(set.has(&key).unwrap()); } #[test] fn collect_keys() { let store = fil_actors_runtime::test_blockstores::MemoryBlockstore::new(); - let mut set = Set::new(&store); + let mut set = Set::<_, u64>::empty(&store, DEFAULT_HAMT_CONFIG, "t"); - set.put("0".into()).unwrap(); + set.put(&0u64).unwrap(); - assert_eq!(set.collect_keys().unwrap(), ["0".into()]); + assert_eq!(set.collect_keys().unwrap(), [0u64]); - set.put("1".into()).unwrap(); - set.put("2".into()).unwrap(); - set.put("3".into()).unwrap(); + set.put(&1u64).unwrap(); + set.put(&2u64).unwrap(); + set.put(&3u64).unwrap(); assert_eq!(set.collect_keys().unwrap().len(), 4); } @@ -34,16 +34,16 @@ fn collect_keys() { #[test] fn delete() { let store = fil_actors_runtime::test_blockstores::MemoryBlockstore::new(); - let mut set = Set::new(&store); + let mut set = Set::empty(&store, DEFAULT_HAMT_CONFIG, "t"); - let key = "0".as_bytes(); + let key = 0u64; - assert!(!set.has(key).unwrap()); - set.put(key.into()).unwrap(); - assert!(set.has(key).unwrap()); - set.delete(key).unwrap(); - assert!(!set.has(key).unwrap()); + assert!(!set.has(&key).unwrap()); + set.put(&key).unwrap(); + assert!(set.has(&key).unwrap()); + set.delete(&key).unwrap(); + assert!(!set.has(&key).unwrap()); // Test delete when doesn't exist doesn't error - set.delete(key).unwrap(); + set.delete(&key).unwrap(); } From 4e4bdd192e1ac2e8506de8b8f6e05cc5b0667687 Mon Sep 17 00:00:00 2001 From: Alex North <445306+anorth@users.noreply.github.com> Date: Fri, 1 Mar 2024 12:50:04 +1300 Subject: [PATCH 2/3] review --- runtime/src/util/set_multimap.rs | 42 +++++++++++--------------------- 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/runtime/src/util/set_multimap.rs b/runtime/src/util/set_multimap.rs index 22e24a4b9..12771c1bc 100644 --- a/runtime/src/util/set_multimap.rs +++ b/runtime/src/util/set_multimap.rs @@ -15,8 +15,7 @@ pub struct SetMultimapConfig { pub inner: Config, } -/// SetMultimap is a hamt with values that are also a hamt but are of the set variant. -/// This allows hash sets to be indexable by an address. +/// SetMultimap is a HAMT with values that are also a HAMT treated as a set of keys. pub struct SetMultimap where BS: Blockstore, @@ -63,43 +62,34 @@ where self.outer.flush() } - /// Puts the DealID in the hash set of the key. + /// Puts a value in the set associated with a key. pub fn put(&mut self, key: &K, value: V) -> Result<(), ActorError> { - // Get construct amt from retrieved cid or create new - let mut set = self.get(key)?.unwrap_or_else(|| { + // Load HAMT from retrieved cid or create a new empty one. + let mut inner = self.get(key)?.unwrap_or_else(|| { Set::empty(self.outer.store().clone(), self.inner_config.clone(), "multimap inner") }); - set.put(&value)?; - - // Save and calculate new root - let new_root = set.flush()?; - - // Set hamt node to set new root + inner.put(&value)?; + let new_root = inner.flush()?; self.outer.set(key, new_root)?; Ok(()) } - /// Puts slice of DealIDs in the hash set of the key. + /// Puts slice of values in the hash set associated with a key. pub fn put_many(&mut self, key: &K, values: &[V]) -> Result<(), ActorError> { - // Get construct amt from retrieved cid or create new - let mut set = self.get(key)?.unwrap_or_else(|| { + let mut inner = self.get(key)?.unwrap_or_else(|| { Set::empty(self.outer.store().clone(), self.inner_config.clone(), "multimap inner") }); for v in values { - set.put(v)?; + inner.put(v)?; } - - // Save and calculate new root - let new_root = set.flush()?; - - // Set hamt node to set new root + let new_root = inner.flush()?; self.outer.set(key, new_root)?; Ok(()) } - /// Gets the set at the given index of the `SetMultimap` + /// Gets the set of values for a key. #[inline] pub fn get(&self, key: &K) -> Result>, ActorError> { match self.outer.get(key)? { @@ -113,18 +103,15 @@ where } } - /// Removes a DealID from a key hash set. + /// Removes a value from the set associated with a key, if it was present. #[inline] - pub fn remove(&mut self, key: &K, v: V) -> Result<(), ActorError> { - // Get construct amt from retrieved cid and return if no set exists + pub fn remove(&mut self, key: &K, value: V) -> Result<(), ActorError> { let mut set = match self.get(key)? { Some(s) => s, None => return Ok(()), }; - set.delete(&v)?; - - // Save and calculate new root + set.delete(&value)?; let new_root = set.flush()?; self.outer.set(key, new_root)?; Ok(()) @@ -133,7 +120,6 @@ where /// Removes set at index. #[inline] pub fn remove_all(&mut self, key: &K) -> Result<(), ActorError> { - // Remove entry from table self.outer.delete(key)?; Ok(()) } From f55f46664029490ddf304a62f09b52012e6f758d Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 29 Feb 2024 17:59:18 -0700 Subject: [PATCH 3/3] feat: remove clone by borrowing --- actors/market/src/state.rs | 2 +- actors/market/tests/harness.rs | 2 +- runtime/src/util/set_multimap.rs | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/actors/market/src/state.rs b/actors/market/src/state.rs index ab8f93721..963b2b3ab 100644 --- a/actors/market/src/state.rs +++ b/actors/market/src/state.rs @@ -390,7 +390,7 @@ impl State { store: BS, ) -> Result, ActorError> where - BS: Blockstore + Clone, + BS: Blockstore, { DealOpsByEpoch::load(store, &self.deal_ops_by_epoch, DEAL_OPS_BY_EPOCH_CONFIG, "deal ops") } diff --git a/actors/market/tests/harness.rs b/actors/market/tests/harness.rs index dce418b0b..c40c35efe 100644 --- a/actors/market/tests/harness.rs +++ b/actors/market/tests/harness.rs @@ -1170,7 +1170,7 @@ pub fn assert_n_good_deals( epoch: ChainEpoch, n: isize, ) where - BS: fvm_ipld_blockstore::Blockstore + Clone, + BS: fvm_ipld_blockstore::Blockstore, { let mut count = 0; dobe.for_each_in(&epoch, |id| { diff --git a/runtime/src/util/set_multimap.rs b/runtime/src/util/set_multimap.rs index 12771c1bc..8ade55787 100644 --- a/runtime/src/util/set_multimap.rs +++ b/runtime/src/util/set_multimap.rs @@ -29,7 +29,7 @@ where impl SetMultimap where - BS: Blockstore + Clone, + BS: Blockstore, K: MapKey, V: MapKey, { @@ -66,7 +66,7 @@ where pub fn put(&mut self, key: &K, value: V) -> Result<(), ActorError> { // Load HAMT from retrieved cid or create a new empty one. let mut inner = self.get(key)?.unwrap_or_else(|| { - Set::empty(self.outer.store().clone(), self.inner_config.clone(), "multimap inner") + Set::empty(self.outer.store(), self.inner_config.clone(), "multimap inner") }); inner.put(&value)?; @@ -78,7 +78,7 @@ where /// Puts slice of values in the hash set associated with a key. pub fn put_many(&mut self, key: &K, values: &[V]) -> Result<(), ActorError> { let mut inner = self.get(key)?.unwrap_or_else(|| { - Set::empty(self.outer.store().clone(), self.inner_config.clone(), "multimap inner") + Set::empty(self.outer.store(), self.inner_config.clone(), "multimap inner") }); for v in values { @@ -91,10 +91,10 @@ where /// Gets the set of values for a key. #[inline] - pub fn get(&self, key: &K) -> Result>, ActorError> { + pub fn get(&self, key: &K) -> Result>, ActorError> { match self.outer.get(key)? { Some(cid) => Ok(Some(Set::load( - self.outer.store().clone(), + self.outer.store(), cid, self.inner_config.clone(), "multimap inner",