-
Notifications
You must be signed in to change notification settings - Fork 743
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
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 Command |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some further ideas:
-
Maybe move the checks from
Ext
toRuntime
. 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 aroundExt
seems not the best approach. -
I think there is no difference in how
reentrancy
andread_only
is inherited. Can you unify them in one bit field? You may need to invertallow_reentrancy
to make this happen.
Yes it needs to be exposed on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another detail if we want to mimic EVM STATICCALL
semantics in full: STATICCALL
s 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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:
polkadot-sdk/substrate/frame/contracts/proc-macro/src/lib.rs
Lines 708 to 711 in 89604da
// Charge gas for host function execution. | |
__caller__.data_mut().charge_gas(crate::wasm::RuntimeCosts::HostFn) | |
.map_err(TrapReason::from) | |
.map_err(#into_host)?; |
There was a problem hiding this comment.
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(|_| ())
};
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to bump the ApiVersion
and then we are good to go.
This PR implements the `READ_ONLY` flag to be used as a `Callflag` in the `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). --------- Co-authored-by: command-bot <> Co-authored-by: Andrew Jones <ascjones@gmail.com> Co-authored-by: Alexander Theißen <alex.theissen@me.com>
This PR implements the `READ_ONLY` flag to be used as a `Callflag` in the `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). --------- Co-authored-by: command-bot <> Co-authored-by: Andrew Jones <ascjones@gmail.com> Co-authored-by: Alexander Theißen <alex.theissen@me.com>
This PR implements the
READ_ONLY
flag to be used as aCallflag
in thecall
function.The flag indicates that the callee is restricted from modifying the state during call execution.
It is equivalent to Ethereum's STATICCALL.