-
Notifications
You must be signed in to change notification settings - Fork 740
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
[pallet-revive] Add chain ID to config an runtime API #5807
Changes from 7 commits
708bd81
17e5dfa
d1f70d2
e016926
83b6426
8289a48
32b4b0a
ae1b232
2eb574b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
title: "[pallet-revive] last call return data API" | ||
|
||
doc: | ||
- audience: Runtime Dev | ||
description: | | ||
This PR adds the EVM chain ID to Config as well as a corresponding runtime API so contracts can query it. | ||
|
||
Related issue: https://github.com/paritytech/revive/issues/44 | ||
|
||
crates: | ||
- name: pallet-revive | ||
bump: major | ||
- name: pallet-revive-fixtures | ||
bump: patch | ||
- name: pallet-revive-uapi | ||
bump: minor |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
// 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. | ||
|
||
#![no_std] | ||
#![no_main] | ||
|
||
extern crate common; | ||
|
||
use uapi::{HostFn, HostFnImpl as api, ReturnFlags}; | ||
|
||
#[no_mangle] | ||
#[polkavm_derive::polkavm_export] | ||
pub extern "C" fn deploy() { | ||
call() | ||
} | ||
|
||
#[no_mangle] | ||
#[polkavm_derive::polkavm_export] | ||
pub extern "C" fn call() { | ||
let mut buf = [0; 32]; | ||
api::chain_id(&mut buf); | ||
api::return_value(ReturnFlags::empty(), &buf); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,7 +60,7 @@ use frame_support::{ | |
ensure, | ||
traits::{ | ||
fungible::{Inspect, Mutate, MutateHold}, | ||
ConstU32, Contains, EnsureOrigin, Get, Time, | ||
ConstU32, ConstU64, Contains, EnsureOrigin, Get, Time, | ||
}, | ||
weights::{Weight, WeightMeter}, | ||
BoundedVec, RuntimeDebugNoBound, | ||
|
@@ -293,6 +293,13 @@ pub mod pallet { | |
/// This value is usually higher than [`Self::RuntimeMemory`] to account for the fact | ||
/// that validators have to hold all storage items in PvF memory. | ||
type PVFMemory: Get<u32>; | ||
|
||
/// The [EIP-155](https://eips.ethereum.org/EIPS/eip-155) chain ID. | ||
/// | ||
/// This is a unique identifier assigned to each blockchain network, | ||
/// preventing replay attacks. | ||
#[pallet::constant] | ||
type ChainId: Get<u64>; | ||
} | ||
|
||
/// Container for different types that implement [`DefaultConfig`]` of this pallet. | ||
|
@@ -365,6 +372,7 @@ pub mod pallet { | |
type Xcm = (); | ||
type RuntimeMemory = ConstU32<{ 128 * 1024 * 1024 }>; | ||
type PVFMemory = ConstU32<{ 512 * 1024 * 1024 }>; | ||
type ChainId = ConstU64<{ 0 }>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mind setting it to 42 here, I need to reconfigure my Metamask extension otherwise 🙃 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean by extension? Shouldn't Metamask just use an RPC to query to chain id? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also this ChainId is not really used. It is just the default derive that can be used for tests. But it is overriden in tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you configure a local network with metamask, you need to specify the chain_id, you can update it later on, but from what I experienced there is a bug and it still use the old value, so you delete your custom network and recreate it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 42 is already taken and I thought this should either be zero or something that's not taken yet? (Something that's not taken yet could however get taken later on). WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good then, I just need to remember to change this once I rebase on top of this |
||
} | ||
} | ||
|
||
|
@@ -659,8 +667,8 @@ pub mod pallet { | |
|
||
// We can use storage to store items using the available block ref_time with the | ||
// `set_storage` host function. | ||
let max_storage_size: u32 = ((max_block_ref_time / | ||
(<RuntimeCosts as gas::Token<T>>::weight(&RuntimeCosts::SetStorage { | ||
let max_storage_size: u32 = ((max_block_ref_time | ||
/ (<RuntimeCosts as gas::Token<T>>::weight(&RuntimeCosts::SetStorage { | ||
new_bytes: max_payload_size, | ||
old_bytes: 0, | ||
}) | ||
|
@@ -682,8 +690,8 @@ pub mod pallet { | |
// We can use storage to store events using the available block ref_time with the | ||
// `deposit_event` host function. The overhead of stored events, which is around 100B, | ||
// is not taken into account to simplify calculations, as it does not change much. | ||
let max_events_size: u32 = ((max_block_ref_time / | ||
(<RuntimeCosts as gas::Token<T>>::weight(&RuntimeCosts::DepositEvent { | ||
let max_events_size: u32 = ((max_block_ref_time | ||
/ (<RuntimeCosts as gas::Token<T>>::weight(&RuntimeCosts::DepositEvent { | ||
num_topic: 0, | ||
len: max_payload_size, | ||
}) | ||
|
@@ -919,7 +927,7 @@ pub mod pallet { | |
let contract = if let Some(contract) = contract { | ||
contract | ||
} else { | ||
return Err(<Error<T>>::ContractNotFound.into()) | ||
return Err(<Error<T>>::ContractNotFound.into()); | ||
}; | ||
<ExecStack<T, WasmBlob<T>>>::increment_refcount(code_hash)?; | ||
<ExecStack<T, WasmBlob<T>>>::decrement_refcount(contract.code_hash); | ||
|
@@ -1041,8 +1049,9 @@ where | |
storage_deposit_limit.saturating_reduce(upload_deposit); | ||
(executable, upload_deposit) | ||
}, | ||
Code::Existing(code_hash) => | ||
(WasmBlob::from_storage(code_hash, &mut gas_meter)?, Default::default()), | ||
Code::Existing(code_hash) => { | ||
(WasmBlob::from_storage(code_hash, &mut gas_meter)?, Default::default()) | ||
}, | ||
}; | ||
let instantiate_origin = Origin::from_account_id(instantiate_account.clone()); | ||
let mut storage_meter = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -468,22 +468,28 @@ impl<T: Config> Token<T> for RuntimeCosts { | |
Terminate(locked_dependencies) => T::WeightInfo::seal_terminate(locked_dependencies), | ||
DepositEvent { num_topic, len } => T::WeightInfo::seal_deposit_event(num_topic, len), | ||
DebugMessage(len) => T::WeightInfo::seal_debug_message(len), | ||
SetStorage { new_bytes, old_bytes } => | ||
cost_storage!(write, seal_set_storage, new_bytes, old_bytes), | ||
SetStorage { new_bytes, old_bytes } => { | ||
cost_storage!(write, seal_set_storage, new_bytes, old_bytes) | ||
}, | ||
ClearStorage(len) => cost_storage!(write, seal_clear_storage, len), | ||
ContainsStorage(len) => cost_storage!(read, seal_contains_storage, len), | ||
GetStorage(len) => cost_storage!(read, seal_get_storage, len), | ||
TakeStorage(len) => cost_storage!(write, seal_take_storage, len), | ||
SetTransientStorage { new_bytes, old_bytes } => | ||
cost_storage!(write_transient, seal_set_transient_storage, new_bytes, old_bytes), | ||
ClearTransientStorage(len) => | ||
cost_storage!(write_transient, seal_clear_transient_storage, len), | ||
ContainsTransientStorage(len) => | ||
cost_storage!(read_transient, seal_contains_transient_storage, len), | ||
GetTransientStorage(len) => | ||
cost_storage!(read_transient, seal_get_transient_storage, len), | ||
TakeTransientStorage(len) => | ||
cost_storage!(write_transient, seal_take_transient_storage, len), | ||
SetTransientStorage { new_bytes, old_bytes } => { | ||
cost_storage!(write_transient, seal_set_transient_storage, new_bytes, old_bytes) | ||
}, | ||
ClearTransientStorage(len) => { | ||
cost_storage!(write_transient, seal_clear_transient_storage, len) | ||
}, | ||
ContainsTransientStorage(len) => { | ||
cost_storage!(read_transient, seal_contains_transient_storage, len) | ||
}, | ||
GetTransientStorage(len) => { | ||
cost_storage!(read_transient, seal_get_transient_storage, len) | ||
}, | ||
TakeTransientStorage(len) => { | ||
cost_storage!(write_transient, seal_take_transient_storage, len) | ||
}, | ||
Transfer => T::WeightInfo::seal_transfer(), | ||
CallBase => T::WeightInfo::seal_call(0, 0), | ||
DelegateCallBase => T::WeightInfo::seal_delegate_call(), | ||
|
@@ -571,7 +577,7 @@ impl<'a, E: Ext, M: PolkaVmInstance<E::T>> Runtime<'a, E, M> { | |
Ok(Step) => None, | ||
Ok(Ecalli(idx)) => { | ||
let Some(syscall_symbol) = module.imports().get(idx) else { | ||
return Some(Err(<Error<E::T>>::InvalidSyscall.into())) | ||
return Some(Err(<Error<E::T>>::InvalidSyscall.into())); | ||
}; | ||
match self.handle_ecall(instance, syscall_symbol.as_bytes(), api_version) { | ||
Ok(None) => None, | ||
|
@@ -679,7 +685,7 @@ impl<'a, E: Ext, M: ?Sized + Memory<E::T>> Runtime<'a, E, M> { | |
create_token: impl FnOnce(u32) -> Option<RuntimeCosts>, | ||
) -> Result<(), DispatchError> { | ||
if allow_skip && out_ptr == SENTINEL { | ||
return Ok(()) | ||
return Ok(()); | ||
} | ||
|
||
let len = memory.read_u32(out_len_ptr)?; | ||
|
@@ -703,7 +709,7 @@ impl<'a, E: Ext, M: ?Sized + Memory<E::T>> Runtime<'a, E, M> { | |
create_token: impl FnOnce(u32) -> Option<RuntimeCosts>, | ||
) -> Result<(), DispatchError> { | ||
if allow_skip && out_ptr == SENTINEL { | ||
return Ok(()) | ||
return Ok(()); | ||
} | ||
|
||
let buf_len = buf.len() as u32; | ||
|
@@ -820,7 +826,7 @@ impl<'a, E: Ext, M: ?Sized + Memory<E::T>> Runtime<'a, E, M> { | |
let max_size = self.ext.max_value_size(); | ||
let charged = self.charge_gas(costs(value_len, self.ext.max_value_size()))?; | ||
if value_len > max_size { | ||
return Err(Error::<E::T>::ValueTooLarge.into()) | ||
return Err(Error::<E::T>::ValueTooLarge.into()); | ||
} | ||
let key = self.decode_key(memory, key_ptr, key_len)?; | ||
let value = Some(memory.read(value_ptr, value_len)?); | ||
|
@@ -1022,7 +1028,7 @@ impl<'a, E: Ext, M: ?Sized + Memory<E::T>> Runtime<'a, E, M> { | |
}, | ||
CallType::DelegateCall { code_hash_ptr } => { | ||
if flags.intersects(CallFlags::ALLOW_REENTRY | CallFlags::READ_ONLY) { | ||
return Err(Error::<E::T>::InvalidCallFlags.into()) | ||
return Err(Error::<E::T>::InvalidCallFlags.into()); | ||
} | ||
|
||
let code_hash = memory.read_h256(code_hash_ptr)?; | ||
|
@@ -1037,7 +1043,7 @@ impl<'a, E: Ext, M: ?Sized + Memory<E::T>> Runtime<'a, E, M> { | |
return Err(TrapReason::Return(ReturnData { | ||
flags: return_value.flags.bits(), | ||
data: return_value.data, | ||
})) | ||
})); | ||
} | ||
} | ||
|
||
|
@@ -1536,6 +1542,19 @@ pub mod env { | |
)?) | ||
} | ||
|
||
/// Returns the chain ID. | ||
/// See [`pallet_revive_uapi::HostFn::chain_id`]. | ||
#[api_version(0)] | ||
fn chain_id(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { | ||
Ok(self.write_fixed_sandbox_output( | ||
Comment on lines
+1545
to
+1549
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't charge any gas except the syscall overhead. Needs a benchmark. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes good catch. Since this doesn't really do anything except writing a constant to contract memory and the syscall overhead is apparently already charged, would the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this should be fine. However, we need to remember to make our There are probably also many other "getters" which don't access storage that could be refactored this way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah right I see. Just added the gas token for now, thanks! |
||
memory, | ||
out_ptr, | ||
&as_bytes(U256::from(<E::T as Config>::ChainId::get())), | ||
false, | ||
|_| Some(RuntimeCosts::CopyToContract(32)), | ||
)?) | ||
} | ||
|
||
/// Stores the value transferred along with this call/instantiate into the supplied buffer. | ||
/// See [`pallet_revive_uapi::HostFn::value_transferred`]. | ||
#[api_version(0)] | ||
|
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.
Why not U256 if that is the native format of the ID?
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.
People seem to generally pick them within the bounds of 64bits. I guess they want to have easily readable for humans? Also there is no
ConstU256
yet. But it this be fine, I hope humanity never needs more than2**64
blockchains :PThere 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.
Ah I have the same patch in my branch I used u32, there..., both are fine I guess
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.
Ok let's stick with u64. As long as we don't need to convert back to u64 it's fine.
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.
On the RPC side I'm not sure but for the runtime I don't see where we would need to parse this ever.
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.
However even if so, I feel like this would need to be a fallible conversion anyways (you can parse any U256 but if you want it to match against the configured chain ID, if the U256 is > u64::MAX you know that the chain ID would not match anyways).