Add READ_ONLY flag to contract call function #4418
Conversation
|
bot bench substrate-pallet --pallet=pallet_contracts |
|
@smiasojed https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6163849 was started for your command Comment |
smiasojed
added
the
T7-smart_contracts
|
@smiasojed Command |
smiasojed
force-pushed
the
sm/contracts-staticcall
branch
from
315819c
to
5278c7a
Compare
|
bot bench substrate-pallet --pallet=pallet_contracts |
|
@smiasojed |
|
bot cancel 5-a1be7227-1f82-4a82-9891-dfc753222de3 |
|
@smiasojed Command |
|
@athei I assumed that |
|
bot bench substrate-pallet --pallet=pallet_contracts |
|
@smiasojed https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6168456 was started for your command Comment |
…=dev --target_dir=substrate --pallet=pallet_contracts
|
@smiasojed Command |
There was a problem hiding this comment.
Some further ideas:
-
Maybe move the checks from
ExttoRuntime. I think it is easier to understand because the checks are per host function and that way we error out as early as possible. You could even implement this as attribute on the macro making it declarative. Having checks sprinkled aroundExtseems not the best approach. -
I think there is no difference in how
reentrancyandread_onlyis inherited. Can you unify them in one bit field? You may need to invertallow_reentrancyto make this happen.
Yes it needs to be exposed on the |
Co-authored-by: Andrew Jones <ascjones@gmail.com>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
|
The CI pipeline was cancelled due to failure one of the required jobs. |
There was a problem hiding this comment.
Another detail if we want to mimic EVM STATICCALL semantics in full: STATICCALLs to accounts without code (e.g. call with the static flag set and no value) should succeed.
Note that an account with no code will return success as true (1).
(To clarify, EVM does balance transfers via calls to EOA. So in general, calls to EOA don't fail)
|
This is a general difference we have to Ethereum that is independent of |
|
@smiasojed sorry for the new merge conflicts 😅 |
@pgherveou conflicts resolved |
| @@ -2125,7 +2134,15 @@ mod tests { | |||
| let value = Default::default(); | |||
| let recurse_ch = MockLoader::insert(Call, |ctx, _| { | |||
| // Try to call into yourself. | |||
| let r = ctx.ext.call(Weight::zero(), BalanceOf::<Test>::zero(), BOB, 0, vec![], true); | |||
| let r = ctx.ext.call( | |||
There was a problem hiding this comment.
all these diffs added every time we add a parameter are a bit annoying,
when we get a chance, we should add builder, like we did in test_utils/builder.rs,
|
bot bench substrate-pallet --pallet=pallet_contracts |
|
@smiasojed https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6328539 was started for your command Comment |
…=dev --target_dir=substrate --pallet=pallet_contracts
|
@smiasojed Command |
|
|
||
| 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); | ||
| } |
There was a problem hiding this comment.
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:
polkadot-sdk/substrate/frame/contracts/proc-macro/src/lib.rs
Lines 708 to 711 in 89604da
There was a problem hiding this comment.
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(|_| ())
};
There was a problem hiding this comment.
Ahh yes. Can you add a test that makes sure this weight is charged when erroring out early?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
You can call the tokens function on the gas meter to check which tokens were charged.
This PR implements the
READ_ONLYflag to be used as aCallflagin thecallfunction.The flag indicates that the callee is restricted from modifying the state during call execution.
It is equivalent to Ethereum's STATICCALL.