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

feat: runtime: Add read_only getter #1127

Merged
merged 1 commit into from
Jan 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 4 additions & 0 deletions runtime/src/runtime/fvm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,10 @@ where
fn base_fee(&self) -> TokenAmount {
fvm::network::base_fee()
}

fn read_only(&self) -> bool {
fvm::vm::read_only()
}
}

impl<B> Primitives for FvmRuntime<B>
Expand Down
4 changes: 4 additions & 0 deletions runtime/src/runtime/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,10 @@ pub trait Runtime: Primitives + Verifier + RuntimePolicy {

/// Returns the gas base fee (cost per unit) for the current epoch.
fn base_fee(&self) -> TokenAmount;

/// Returns true if the call is read_only.
/// All state updates, including actor creation and balance transfers, are rejected in read_only calls.
fn read_only(&self) -> bool;
}

/// Message information available to the actor about executing message.
Expand Down
5 changes: 5 additions & 0 deletions runtime/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1068,6 +1068,11 @@ impl<BS: Blockstore> Runtime for MockRuntime<BS> {
fn base_fee(&self) -> TokenAmount {
self.base_fee.clone()
}

fn read_only(&self) -> bool {
// Unsupported for unit tests
arajasek marked this conversation as resolved.
Show resolved Hide resolved
unimplemented!()
}
}

impl<BS> Primitives for MockRuntime<BS> {
Expand Down
63 changes: 58 additions & 5 deletions test_vm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,7 @@ impl<'bs> VM<'bs> {
msg,
allow_side_effects: true,
caller_validated: false,
read_only: false,
policy: &Policy::default(),
subinvocations: RefCell::new(vec![]),
};
Expand Down Expand Up @@ -575,6 +576,7 @@ pub struct InvocationCtx<'invocation, 'bs> {
msg: InternalMessage,
allow_side_effects: bool,
caller_validated: bool,
read_only: bool,
policy: &'invocation Policy,
subinvocations: RefCell<Vec<InvocationTrace>>,
}
Expand Down Expand Up @@ -605,6 +607,14 @@ impl<'invocation, 'bs> InvocationCtx<'invocation, 'bs> {
}
};

// But only if we're not in read-only mode.
if self.read_only() {
return Err(ActorError::unchecked(
ExitCode::USR_READ_ONLY,
format!("cannot create actor {target} in read-only mode"),
));
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you point to the spec for read-only? Depending on what it says, I would expect either:

  • no checks like this, but something at the end of an invocation that restores the state root to its pre-invocation value, or
  • a lot more checks in create, transaction, new_actor_address, create_actor, delete_actor` (or something lower down that propagates through these) that actively prevent modifications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spec: https://github.com/filecoin-project/FIPs/blob/322604f79c2447a4906ed2fc959e43aaedeccb46/FIPS/fip-0054.md#changed-syscalls

You're right. I've added checks to create, create_actor, transaction, set_state_root and invoke.

I'm tweaking the behaviour of the test_vm's new_actor_address to be read-only, which is how it's actually implemented. The increment in moving to create_actor which is where it belongs.


let mut st = self.v.get_state::<InitState>(INIT_ACTOR_ADDR).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");
Expand All @@ -627,6 +637,7 @@ impl<'invocation, 'bs> InvocationCtx<'invocation, 'bs> {
msg: new_actor_msg,
allow_side_effects: true,
caller_validated: false,
read_only: false,
policy: self.policy,
subinvocations: RefCell::new(vec![]),
};
Expand Down Expand Up @@ -684,6 +695,12 @@ impl<'invocation, 'bs> InvocationCtx<'invocation, 'bs> {
"insufficient balance to transfer".to_string(),
));
}
if self.read_only() {
return Err(ActorError::unchecked(
ExitCode::USR_READ_ONLY,
"cannot transfer value in read-only mode".to_string(),
));
}
}

// Load, deduct, store from actor before loading to actor to handle self-send case
Expand Down Expand Up @@ -755,6 +772,15 @@ impl<'invocation, 'bs> Runtime for InvocationCtx<'invocation, 'bs> {
));
}
let a = actor(code_id, EMPTY_ARR_CID, 0, TokenAmount::zero(), predictable_address);

if self.read_only() {
return Err(ActorError::unchecked(
ExitCode::USR_READ_ONLY,
"cannot create actor in read-only mode".into(),
));
}

self.top.new_actor_addr_count.replace_with(|old| *old + 1);
self.v.set_actor(addr, a);
Ok(())
}
Expand Down Expand Up @@ -862,8 +888,13 @@ impl<'invocation, 'bs> Runtime for InvocationCtx<'invocation, 'bs> {
params: Option<IpldBlock>,
value: TokenAmount,
_gas_limit: Option<u64>,
_send_flags: SendFlags,
mut send_flags: SendFlags,
) -> Result<Response, ErrorNumber> {
// replicate FVM by silently propagating read only flag to subcalls
if self.read_only() {
send_flags.set(SendFlags::READ_ONLY, true)
}

if !self.allow_side_effects {
return Ok(Response { exit_code: ExitCode::SYS_ASSERTION_FAILED, return_data: None });
}
Expand All @@ -875,6 +906,7 @@ impl<'invocation, 'bs> Runtime for InvocationCtx<'invocation, 'bs> {
msg: new_actor_msg,
allow_side_effects: true,
caller_validated: false,
read_only: send_flags.read_only(),
policy: self.policy,
subinvocations: RefCell::new(vec![]),
};
Expand Down Expand Up @@ -910,6 +942,13 @@ impl<'invocation, 'bs> Runtime for InvocationCtx<'invocation, 'bs> {
}

fn create<T: Serialize>(&mut self, obj: &T) -> Result<(), ActorError> {
if self.read_only {
return Err(ActorError::unchecked(
ExitCode::USR_READ_ONLY,
"cannot create state in read-only".to_string(),
));
}

let maybe_act = self.v.get_actor(self.to());
match maybe_act {
None => Err(ActorError::unchecked(
Expand Down Expand Up @@ -942,11 +981,15 @@ impl<'invocation, 'bs> Runtime for InvocationCtx<'invocation, 'bs> {
ExitCode::SYS_ASSERTION_FAILED,
"actor does not exist".to_string(),
)),
Some(mut act) => {
Some(mut act) if !self.read_only() => {
act.head = *root;
self.v.set_actor(self.to(), act);
Ok(())
}
_ => Err(ActorError::unchecked(
ExitCode::USR_READ_ONLY,
"actor is read-only".to_string(),
)),
}
}

Expand All @@ -966,16 +1009,22 @@ impl<'invocation, 'bs> Runtime for InvocationCtx<'invocation, 'bs> {
let ret = result?;
let mut act = self.v.get_actor(self.to()).unwrap();
act.head = self.v.store.put_cbor(&st, Code::Blake2b256).unwrap();

if self.read_only {
return Err(ActorError::unchecked(
ExitCode::USR_READ_ONLY,
"actor is read-only".to_string(),
));
}

self.v.set_actor(self.to(), act);
Ok(ret)
}

fn new_actor_address(&mut self) -> Result<Address, ActorError> {
let mut b = self.top.originator_stable_addr.to_bytes();
b.extend_from_slice(&self.top.originator_call_seq.to_be_bytes());
b.extend_from_slice(
&self.top.new_actor_addr_count.replace_with(|old| *old + 1).to_be_bytes(),
);
b.extend_from_slice(&self.top.new_actor_addr_count.borrow().to_be_bytes());
Ok(Address::new_actor(&b))
}

Expand All @@ -1000,6 +1049,10 @@ impl<'invocation, 'bs> Runtime for InvocationCtx<'invocation, 'bs> {
fn base_fee(&self) -> TokenAmount {
TokenAmount::zero()
}

fn read_only(&self) -> bool {
self.read_only
}
}

impl Primitives for VM<'_> {
Expand Down