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.

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

Test VM should use Rc<Blockstore> for consistency with the MockRuntime #1454

Merged
merged 4 commits into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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: 18 additions & 17 deletions test_vm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ use fvm_shared::{MethodNum, METHOD_SEND};
use serde::ser;
use std::cell::{RefCell, RefMut};
use std::collections::{BTreeMap, HashMap};
use std::rc::Rc;
use vm_api::trace::InvocationTrace;
use vm_api::{new_actor, ActorState, MessageResult, VMError, VM};

Expand All @@ -50,9 +51,9 @@ mod messaging;
pub use messaging::*;

/// An in-memory rust-execution VM for testing builtin-actors that yields sensible stack traces and debug info
pub struct TestVM<'bs> {
pub struct TestVM {
pub primitives: FakePrimitives,
pub store: &'bs MemoryBlockstore,
pub store: Rc<MemoryBlockstore>,
pub state_root: RefCell<Cid>,
circulating_supply: RefCell<TokenAmount>,
actors_dirty: RefCell<bool>,
Expand All @@ -62,11 +63,11 @@ pub struct TestVM<'bs> {
invocations: RefCell<Vec<InvocationTrace>>,
}

impl<'bs> TestVM<'bs> {
pub fn new(store: &'bs MemoryBlockstore) -> TestVM<'bs> {
impl TestVM {
pub fn new(store: Rc<MemoryBlockstore>) -> TestVM {
let mut actors =
Hamt::<&'bs MemoryBlockstore, ActorState, BytesKey, Sha256>::new_with_config(
store,
Hamt::<Rc<MemoryBlockstore>, ActorState, BytesKey, Sha256>::new_with_config(
Rc::clone(&store),
DEFAULT_HAMT_CONFIG,
);
TestVM {
Expand All @@ -82,15 +83,15 @@ impl<'bs> TestVM<'bs> {
}
}

pub fn new_with_singletons(store: &'bs MemoryBlockstore) -> TestVM<'bs> {
pub fn new_with_singletons(store: Rc<MemoryBlockstore>) -> TestVM {
Copy link
Member

Choose a reason for hiding this comment

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

I'd consider defining this as store: impl Into<Rc<MemoryBlockstoire>>, allowing the user to pass either a MemoryBlockstore or an Rc<MemoryBlockstore>>. I.e.

Suggested change
pub fn new_with_singletons(store: Rc<MemoryBlockstore>) -> TestVM {
pub fn new_with_singletons(store: impl Into<Rc<MemoryBlockstore>>) -> TestVM {
let store = store.into();

Same with TestVM::new.

But let's wait for @anorth to comment on this before continuing.

Copy link
Member

Choose a reason for hiding this comment

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

This would let us call TestVM::new_with_singletons(store) without having to explicitly wrap in an Rc.

Copy link
Member

Choose a reason for hiding this comment

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

Yes please (for new too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done for both. Great idea !

Copy link
Contributor Author

@aarshkshah1992 aarshkshah1992 Oct 20, 2023

Choose a reason for hiding this comment

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

@Stebalien @anorth Where is the implementation for

impl Into<Rc<MemoryBlockstore>> for MemoryBlockstore {
}

defined though in the code ? I also don't see a

impl From<MemoryBlockstore> for Rc<MemoryBlockstore>

anywhere. Does the Rust stdlib have come default impl here to convert a MemoryBlockstore to an Rc ?

Copy link
Member

Choose a reason for hiding this comment

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

First, there's also a blanket implementation of Into<U> for T where U: From<T>. That is, you always implement From<T> for U and get an automatic implementation of Into<U> for T. Never implement Into directly. I recommend reading the Into documentation and the From and Into section of the book.

Additionally, there's a blanket implementation of From<T> for Rc<T>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Stebalien ! Yeah the blanket implementation of From<T> for Rc<T> is what I missing on.

let reward_total = TokenAmount::from_whole(1_100_000_000i64);
let faucet_total = TokenAmount::from_whole(1_000_000_000i64);

let v = TestVM::<'_>::new(store);
let v = TestVM::new(Rc::clone(&store));
v.set_circulating_supply(&reward_total + &faucet_total);

// system
let sys_st = SystemState::new(store).unwrap();
let sys_st = SystemState::new(&store).unwrap();
let sys_head = v.put_store(&sys_st);
let sys_value = faucet_total.clone(); // delegate faucet funds to system so we can construct faucet by sending to bls addr
v.set_actor(
Expand All @@ -99,7 +100,7 @@ impl<'bs> TestVM<'bs> {
);

// init
let init_st = InitState::new(store, "integration-test".to_string()).unwrap();
let init_st = InitState::new(&store, "integration-test".to_string()).unwrap();
let init_head = v.put_store(&init_st);
v.set_actor(
&INIT_ACTOR_ADDR,
Expand Down Expand Up @@ -239,9 +240,9 @@ impl<'bs> TestVM<'bs> {
pub fn checkpoint(&self) -> Cid {
// persist cache on top of latest checkpoint and clear
let mut actors =
Hamt::<&'bs MemoryBlockstore, ActorState, BytesKey, Sha256>::load_with_config(
Hamt::<Rc<MemoryBlockstore>, ActorState, BytesKey, Sha256>::load_with_config(
&self.state_root.borrow(),
self.store,
Rc::clone(&self.store),
DEFAULT_HAMT_CONFIG,
)
.unwrap();
Expand Down Expand Up @@ -275,9 +276,9 @@ impl<'bs> TestVM<'bs> {
}
}

impl<'bs> VM for TestVM<'bs> {
impl VM for TestVM {
fn blockstore(&self) -> &dyn Blockstore {
self.store
self.store.as_ref()
}

fn epoch(&self) -> ChainEpoch {
Expand Down Expand Up @@ -364,7 +365,7 @@ impl<'bs> VM for TestVM<'bs> {
}
fn resolve_id_address(&self, address: &Address) -> Option<Address> {
let st: InitState = get_state(self, &INIT_ACTOR_ADDR).unwrap();
st.resolve_address(self.store, address).unwrap()
st.resolve_address(&self.store, address).unwrap()
}

fn set_epoch(&self, epoch: ChainEpoch) {
Expand All @@ -386,9 +387,9 @@ impl<'bs> VM for TestVM<'bs> {
return Some(act.clone());
}
// go to persisted map
let actors = Hamt::<&'bs MemoryBlockstore, ActorState, BytesKey, Sha256>::load_with_config(
let actors = Hamt::<Rc<MemoryBlockstore>, ActorState, BytesKey, Sha256>::load_with_config(
&self.state_root.borrow(),
self.store,
Rc::clone(&self.store),
DEFAULT_HAMT_CONFIG,
)
.unwrap();
Expand Down
23 changes: 12 additions & 11 deletions test_vm/src/messaging.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ use vm_api::{new_actor, ActorState, VM};

use fil_actors_runtime::test_blockstores::MemoryBlockstore;
use std::ops::Add;
use std::rc::Rc;

use crate::{TestVM, TEST_VM_INVALID_POST, TEST_VM_RAND_ARRAY};

Expand All @@ -82,7 +83,7 @@ pub struct InternalMessage {
pub params: Option<IpldBlock>,
}

impl MessageInfo for InvocationCtx<'_, '_> {
impl MessageInfo for InvocationCtx<'_> {
fn nonce(&self) -> u64 {
self.top.originator_call_seq
}
Expand All @@ -103,8 +104,8 @@ impl MessageInfo for InvocationCtx<'_, '_> {
}
}

pub struct InvocationCtx<'invocation, 'bs> {
pub v: &'invocation TestVM<'bs>,
pub struct InvocationCtx<'invocation> {
pub v: &'invocation TestVM,
pub top: TopCtx,
pub msg: InternalMessage,
pub allow_side_effects: RefCell<bool>,
Expand All @@ -114,7 +115,7 @@ pub struct InvocationCtx<'invocation, 'bs> {
pub subinvocations: RefCell<Vec<InvocationTrace>>,
}

impl<'invocation, 'bs> InvocationCtx<'invocation, 'bs> {
impl<'invocation> InvocationCtx<'invocation> {
fn resolve_target(
&'invocation self,
target: &Address,
Expand Down Expand Up @@ -152,7 +153,7 @@ impl<'invocation, 'bs> InvocationCtx<'invocation, 'bs> {
}

let mut st: InitState = get_state(self.v, &INIT_ACTOR_ADDR).unwrap();
let (target_id, existing) = st.map_addresses_to_id(self.v.store, target, None).unwrap();
let (target_id, existing) = st.map_addresses_to_id(&self.v.store, target, None).unwrap();
assert!(!existing, "should never have existing actor when no f4 address is specified");
let target_id_addr = Address::new_id(target_id);
let mut init_actor = self.v.actor(&INIT_ACTOR_ADDR).unwrap();
Expand Down Expand Up @@ -298,8 +299,8 @@ impl<'invocation, 'bs> InvocationCtx<'invocation, 'bs> {
}
}

impl<'invocation, 'bs> Runtime for InvocationCtx<'invocation, 'bs> {
type Blockstore = &'bs MemoryBlockstore;
impl<'invocation> Runtime for InvocationCtx<'invocation> {
type Blockstore = Rc<MemoryBlockstore>;

fn create_actor(
&self,
Expand Down Expand Up @@ -341,7 +342,7 @@ impl<'invocation, 'bs> Runtime for InvocationCtx<'invocation, 'bs> {
Ok(())
}

fn store(&self) -> &&'bs MemoryBlockstore {
fn store(&self) -> &Rc<MemoryBlockstore> {
&self.v.store
}

Expand Down Expand Up @@ -645,7 +646,7 @@ impl<'invocation, 'bs> Runtime for InvocationCtx<'invocation, 'bs> {
}
}

impl Primitives for InvocationCtx<'_, '_> {
impl Primitives for InvocationCtx<'_> {
fn verify_signature(
&self,
signature: &Signature,
Expand Down Expand Up @@ -684,7 +685,7 @@ impl Primitives for InvocationCtx<'_, '_> {
}
}

impl Verifier for InvocationCtx<'_, '_> {
impl Verifier for InvocationCtx<'_> {
fn verify_post(&self, verify_info: &WindowPoStVerifyInfo) -> Result<(), anyhow::Error> {
for proof in &verify_info.proofs {
if proof.proof_bytes.eq(&TEST_VM_INVALID_POST.as_bytes().to_vec()) {
Expand Down Expand Up @@ -720,7 +721,7 @@ impl Verifier for InvocationCtx<'_, '_> {
}
}

impl RuntimePolicy for InvocationCtx<'_, '_> {
impl RuntimePolicy for InvocationCtx<'_> {
fn policy(&self) -> &Policy {
self.policy
}
Expand Down
5 changes: 3 additions & 2 deletions test_vm/tests/suite/authenticate_message_test.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use fil_actors_integration_tests::tests::account_authenticate_message_test;
use fil_actors_runtime::test_blockstores::MemoryBlockstore;
use std::rc::Rc;
use test_vm::TestVM;

#[test]
fn account_authenticate_message() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let store = Rc::new(MemoryBlockstore::new());
let v = TestVM::new_with_singletons(store);
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
let store = Rc::new(MemoryBlockstore::new());
let v = TestVM::new_with_singletons(store);
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(store);

Given the changes to the new_with_singletons signature, we shouldn't need to manually wrap in an Rc. Apply below as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here and elsewhere.

account_authenticate_message_test(&v);
}
3 changes: 2 additions & 1 deletion test_vm/tests/suite/batch_onboarding.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use fil_actors_integration_tests::tests::batch_onboarding_test;
use fil_actors_runtime::test_blockstores::MemoryBlockstore;
use std::rc::Rc;
use test_case::test_case;
use test_vm::TestVM;

Expand All @@ -8,7 +9,7 @@ use test_vm::TestVM;
#[test_case(true; "v2")]
fn batch_onboarding(v2: bool) {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));

batch_onboarding_test(&v, v2);
}
3 changes: 2 additions & 1 deletion test_vm/tests/suite/batch_onboarding_deals_test.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use fil_actors_integration_tests::tests::batch_onboarding_deals_test;
use fil_actors_runtime::test_blockstores::MemoryBlockstore;
use std::rc::Rc;
use test_vm::TestVM;

#[test]
fn batch_onboarding_deals() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
batch_onboarding_deals_test(&v);
}
7 changes: 4 additions & 3 deletions test_vm/tests/suite/change_beneficiary_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,26 @@ use fil_actors_integration_tests::tests::{
change_beneficiary_success_test,
};
use fil_actors_runtime::test_blockstores::MemoryBlockstore;
use std::rc::Rc;
use test_vm::TestVM;

#[test]
fn change_beneficiary_success() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
change_beneficiary_success_test(&v);
}

#[test]
fn change_beneficiary_back_owner_success() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
change_beneficiary_back_owner_success_test(&v);
}

#[test]
fn change_beneficiary_fail() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
change_beneficiary_fail_test(&v);
}
7 changes: 4 additions & 3 deletions test_vm/tests/suite/change_owner_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,26 @@ use fil_actors_integration_tests::tests::{
change_owner_fail_test, change_owner_success_test, keep_beneficiary_when_owner_changed_test,
};
use fil_actors_runtime::test_blockstores::MemoryBlockstore;
use std::rc::Rc;
use test_vm::TestVM;

#[test]
fn change_owner_success() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
change_owner_success_test(&v);
}

#[test]
fn keep_beneficiary_when_owner_changed() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
keep_beneficiary_when_owner_changed_test(&v);
}

#[test]
fn change_owner_fail() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
change_owner_fail_test(&v);
}
17 changes: 9 additions & 8 deletions test_vm/tests/suite/commit_post_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,60 +5,61 @@ use fil_actors_integration_tests::tests::{
submit_post_succeeds_test,
};
use fil_actors_runtime::test_blockstores::MemoryBlockstore;
use std::rc::Rc;
use test_vm::TestVM;

#[test]
fn submit_post_succeeds() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
submit_post_succeeds_test(&v);
}

#[test]
fn skip_sector() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
skip_sector_test(&v);
}

#[test]
fn missed_first_post_deadline() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
missed_first_post_deadline_test(&v);
}

#[test]
fn overdue_precommit() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
overdue_precommit_test(&v);
}

#[test]
fn aggregate_bad_sector_number() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
aggregate_bad_sector_number_test(&v);
}

#[test]
fn aggregate_size_limits() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
aggregate_size_limits_test(&v);
}

#[test]
fn aggregate_bad_sender() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
aggregate_bad_sender_test(&v);
}

#[test]
fn aggregate_one_precommit_expires() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
aggregate_one_precommit_expires_test(&v);
}
5 changes: 3 additions & 2 deletions test_vm/tests/suite/datacap_tests.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
use fil_actors_integration_tests::tests::{call_name_symbol_test, datacap_transfer_test};
use fil_actors_runtime::test_blockstores::MemoryBlockstore;
use std::rc::Rc;
use test_vm::TestVM;

/* Mint a token for client and transfer it to a receiver, exercising error cases */
#[test]
fn datacap_transfer() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
datacap_transfer_test(&v);
}

/* Call name & symbol */
#[test]
fn call_name_symbol() {
let store = MemoryBlockstore::new();
let v = TestVM::new_with_singletons(&store);
let v = TestVM::new_with_singletons(Rc::new(store));
call_name_symbol_test(&v);
}
Loading
Loading