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

Add READ_ONLY flag to contract call function #4418

Merged
merged 33 commits into from
Jun 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
6847f88
Add static_call flag to contracts call function
smiasojed May 7, 2024
b19bc77
Refactoring
smiasojed May 8, 2024
161d4ac
Unit test added
smiasojed May 8, 2024
a96a4aa
Cleanup
smiasojed May 8, 2024
ae1151e
Fmt
smiasojed May 8, 2024
41d753e
Add prdoc
smiasojed May 8, 2024
5278c7a
Fix prdoc
smiasojed May 8, 2024
15fca3c
Update description
smiasojed May 9, 2024
5f9d957
Merge remote-tracking branch 'origin/master' into sm/contracts-static…
smiasojed May 9, 2024
7169214
Update prdoc
smiasojed May 9, 2024
cfd4198
Add read-only protection to call_runtime
smiasojed May 9, 2024
0a708a0
".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…
May 9, 2024
3369de6
Add test
smiasojed May 10, 2024
6ebfafc
Fix fixture description
smiasojed May 10, 2024
91d145e
Address comments
smiasojed May 13, 2024
2eeed62
Fmt
smiasojed May 13, 2024
fc15cd6
Revert deposit_event implementation
smiasojed May 13, 2024
47f9d76
Cleanup
smiasojed May 14, 2024
76e50fc
Update chain extension description
smiasojed May 14, 2024
b6c378b
Improved description
smiasojed May 14, 2024
70c654c
Update prdoc
smiasojed May 14, 2024
f10a0db
Update substrate/frame/contracts/proc-macro/src/lib.rs
smiasojed May 15, 2024
ac65bd7
Update substrate/frame/contracts/uapi/src/flags.rs
smiasojed May 15, 2024
b4ad72b
Update substrate/frame/contracts/src/lib.rs
smiasojed May 15, 2024
fb8b2dc
Update substrate/frame/contracts/src/exec.rs
smiasojed May 15, 2024
03a77bb
Update substrate/frame/contracts/src/wasm/runtime.rs
smiasojed May 15, 2024
08b4ca7
Address comments
smiasojed May 15, 2024
b3571d7
Move read-only condition to runtime
smiasojed May 16, 2024
c7fa16e
Merge remote-tracking branch 'origin/master' into sm/contracts-static…
smiasojed May 28, 2024
48e907b
Merge branch 'master' of https://github.com/paritytech/polkadot-sdk i…
May 28, 2024
2cb8084
".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…
May 28, 2024
d5f8d15
Bump API verision for contracts pallet
smiasojed Jun 3, 2024
21aaa70
Merge remote-tracking branch 'origin/master' into sm/contracts-static…
smiasojed Jun 3, 2024
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
19 changes: 19 additions & 0 deletions prdoc/pr_4418.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: "[pallet_contracts] Add `READ_ONLY` flag to contract `call` function"

doc:
- audience: Runtime User
description: |
This PR implements the `READ_ONLY` flag to be used as a `Callflag` in the contract `call` function.
The flag indicates that the callee is restricted from modifying the state during call execution.
It is equivalent to Ethereum's [STATICCALL](https://eips.ethereum.org/EIPS/eip-214).

crates:
- name: pallet-contracts
bump: minor
- name: pallet-contracts-uapi
bump: minor
- name: pallet-contracts-proc-macro
bump: minor
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// This file is part of Substrate.

// Copyright (C) Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//! This fixture calls the account_id with the flags and value.
#![no_std]
#![no_main]

use common::input;
use uapi::{HostFn, HostFnImpl as api};

#[no_mangle]
#[polkavm_derive::polkavm_export]
pub extern "C" fn deploy() {}

#[no_mangle]
#[polkavm_derive::polkavm_export]
pub extern "C" fn call() {
input!(
256,
callee_addr: [u8; 32],
flags: u32,
value: u64,
forwarded_input: [u8],
);

api::call_v2(
uapi::CallFlags::from_bits(flags).unwrap(),
callee_addr,
0u64, // How much ref_time to devote for the execution. 0 = all.
0u64, // How much proof_size to devote for the execution. 0 = all.
None, // No deposit limit.
&value.to_le_bytes(), // Value transferred to the contract.
forwarded_input,
None,
)
.unwrap();
}
50 changes: 50 additions & 0 deletions substrate/frame/contracts/fixtures/contracts/read_only_call.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// This file is part of Substrate.

// Copyright (C) Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// This fixture tests if read-only call works as expected.
#![no_std]
#![no_main]

use common::input;
use uapi::{HostFn, HostFnImpl as api};

#[no_mangle]
#[polkavm_derive::polkavm_export]
pub extern "C" fn deploy() {}

#[no_mangle]
#[polkavm_derive::polkavm_export]
pub extern "C" fn call() {
input!(
256,
callee_addr: [u8; 32],
callee_input: [u8],
);

// Call the callee
api::call_v2(
uapi::CallFlags::READ_ONLY,
callee_addr,
0u64, // How much ref_time to devote for the execution. 0 = all.
0u64, // How much proof_size to devote for the execution. 0 = all.
None, // No deposit limit.
&0u64.to_le_bytes(), // Value transferred to the contract.
callee_input,
None,
)
.unwrap();
}
19 changes: 18 additions & 1 deletion substrate/frame/contracts/proc-macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,14 +170,15 @@ impl HostFn {

// process attributes
let msg =
"Only #[version(<u8>)], #[unstable], #[prefixed_alias], #[cfg] and #[deprecated] attributes are allowed.";
"Only #[version(<u8>)], #[unstable], #[prefixed_alias], #[cfg], #[mutating] and #[deprecated] attributes are allowed.";
let span = item.span();
let mut attrs = item.attrs.clone();
attrs.retain(|a| !a.path().is_ident("doc"));
let mut maybe_version = None;
let mut is_stable = true;
let mut alias_to = None;
let mut not_deprecated = true;
let mut mutating = false;
let mut cfg = None;
while let Some(attr) = attrs.pop() {
let ident = attr.path().get_ident().ok_or(err(span, msg))?.to_string();
Expand Down Expand Up @@ -208,6 +209,12 @@ impl HostFn {
}
not_deprecated = false;
},
"mutating" => {
if mutating {
return Err(err(span, "#[mutating] can only be specified once"))
}
mutating = true;
},
"cfg" => {
if cfg.is_some() {
return Err(err(span, "#[cfg] can only be specified once"))
Expand All @@ -217,6 +224,16 @@ impl HostFn {
id => return Err(err(span, &format!("Unsupported attribute \"{id}\". {msg}"))),
}
}

if mutating {
let stmt = syn::parse_quote! {
if ctx.ext().is_read_only() {
return Err(Error::<E::T>::StateChangeDenied.into());
}
};
item.block.stmts.insert(0, stmt);
}
Comment on lines +227 to +235
Copy link
Member

@athei athei May 29, 2024

Choose a reason for hiding this comment

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

As stated before: We can't return that early here. This codes runs before we charge the gas for the host function call invocation itself. This has to be charged even if no work has been performed. You have to move down this statement after:

// Charge gas for host function execution.
__caller__.data_mut().charge_gas(crate::wasm::RuntimeCosts::HostFn)
.map_err(TrapReason::from)
.map_err(#into_host)?;

Copy link
Contributor Author

@smiasojed smiasojed May 29, 2024

Choose a reason for hiding this comment

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

I agree, but my understanding is that the gas charge charge_gas(crate::wasm::RuntimeCosts::HostFn) is added before function body (mutating check is added to the function body)
Expanded:

                                    __caller__
                                        .data_mut()
                                        .charge_gas(crate::wasm::RuntimeCosts::HostFn)
                                        .map_err(TrapReason::from)
                                        .map_err(|reason| { ::wasmi::core::Trap::from(reason) })?;
                                    let mut func = || -> Result<(), TrapReason> {
                                        let (memory, ctx) = __caller__
                                            .data()
                                            .memory()
                                            .expect("Memory must be set when setting up host data; qed")
                                            .data_and_store_mut(&mut __caller__);
                                        let result = {
                                            if ctx.ext().is_read_only() {
                                                return Err(Error::<E::T>::StateChangeDenied.into());
                                            }
                                            ctx.set_storage(
                                                    memory,
                                                    KeyType::Fix,
                                                    key_ptr,
                                                    value_ptr,
                                                    value_len,
                                                )
                                                .map(|_| ())
                                        };

Copy link
Member

Choose a reason for hiding this comment

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

Ahh yes. Can you add a test that makes sure this weight is charged when erroring out early?

Copy link
Contributor Author

@smiasojed smiasojed May 29, 2024

Choose a reason for hiding this comment

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

I tried to test it using fixture:

pub extern "C" fn call() {
	input!(n: u32, );

	match n {
		0 => unsafe {core::ptr::read_volatile(core::ptr::null())},
		_ => api::set_storage(&[1u8; 32], &[1u8; 1]),
	}
}

The fixture is called with n equal to 0 and 1. Both calls result in a trap, but the difference is more significant than just the host function call cost because (I assume) the WASMI execution time differs for both cases. The difference is the host function call cost plus 15%.
HostFn ref_time is 72994

Copy link
Member

Choose a reason for hiding this comment

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

You can call the tokens function on the gas meter to check which tokens were charged.

Copy link
Contributor Author

@smiasojed smiasojed May 31, 2024

Choose a reason for hiding this comment

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

As discussed in priv, we do not have access to gas_meter in our tests. We could add such functionality to our tests to trace gas metering more deeply during testing. This will be implemented as a separate PR.


let name = item.sig.ident.to_string();

if !(is_stable || not_deprecated) {
Expand Down
6 changes: 6 additions & 0 deletions substrate/frame/contracts/src/chain_extension.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,12 @@ pub trait ChainExtension<C: Config> {
/// In case of `Err` the contract execution is immediately suspended and the passed error
/// is returned to the caller. Otherwise the value of [`RetVal`] determines the exit
/// behaviour.
///
/// # Note
///
/// The [`Self::call`] can be invoked within a read-only context, where any state-changing calls
/// are disallowed. This information can be obtained using `env.ext().is_read_only()`. It is
/// crucial for the implementer to handle this scenario appropriately.
fn call<E: Ext<T = C>>(&mut self, env: Environment<E, InitState>) -> Result<RetVal>;

/// Determines whether chain extensions are enabled for this chain.
Expand Down
Loading
Loading