Skip to content

Commit

Permalink
contracts: Run start function (#1367)
Browse files Browse the repository at this point in the history 
Fixes #116 

Start function wasn't allowed in a contract. Now it is allowed and is
being run.

It was disallowed because it is not used by Rust and supporting it made
the code more complex. However, not running the start function violates
the wasm standard. This makes life harder for some languages (see linked
ticket).
  • Loading branch information
@athei
athei authored Sep 11, 2023
1 parent 44dbb73 commit c879d1d
Show file tree
Hide file tree
Showing 6 changed files with 198 additions and 172 deletions.
10 changes: 10 additions & 0 deletions substrate/frame/contracts/fixtures/run_out_of_gas_start_fn.wat
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
(module
(import "env" "memory" (memory 1 1))
(start $start)
(func $start
(loop $inf (br $inf)) ;; just run out of gas
(unreachable)
)
(func (export "call"))
(func (export "deploy"))
)
8 changes: 7 additions & 1 deletion substrate/frame/contracts/src/benchmarking/sandbox.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,13 @@ impl<T: Config> From<&WasmModule<T>> for Sandbox {
.add_fuel(u64::MAX)
.expect("We've set up engine to fuel consuming mode; qed");

let entry_point = instance.get_export(&store, "call").unwrap().into_func().unwrap();
let entry_point = instance
.start(&mut store)
.unwrap()
.get_export(&store, "call")
.unwrap()
.into_func()
.unwrap();
Self { entry_point, store }
}
}
Expand Down
111 changes: 45 additions & 66 deletions substrate/frame/contracts/src/exec.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use crate::{
storage::{self, meter::Diff, WriteOutcome},
BalanceOf, CodeHash, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf,
DebugBufferVec, Determinism, Error, Event, Nonce, Origin, Pallet as Contracts, Schedule,
WasmBlob, LOG_TARGET,
LOG_TARGET,
};
use frame_support::{
crypto::ecdsa::ECDSAExt,
Expand Down Expand Up @@ -318,6 +318,22 @@ pub trait Ext: sealing::Sealed {
/// Returns a nonce that is incremented for every instantiated contract.
fn nonce(&mut self) -> u64;

/// Increment the reference count of a of a stored code by one.
///
/// # Errors
///
/// [`Error::CodeNotFound`] is returned if no stored code found having the specified
/// `code_hash`.
fn increment_refcount(code_hash: CodeHash<Self::T>) -> Result<(), DispatchError>;

/// Decrement the reference count of a stored code by one.
///
/// # Note
///
/// A contract whose reference count dropped to zero isn't automatically removed. A
/// `remove_code` transaction must be submitted by the original uploader to do so.
fn decrement_refcount(code_hash: CodeHash<Self::T>);

/// Adds a delegate dependency to [`ContractInfo`]'s `delegate_dependencies` field.
///
/// This ensures that the delegated contract is not removed while it is still in use. It
Expand Down Expand Up @@ -381,22 +397,6 @@ pub trait Executable<T: Config>: Sized {
gas_meter: &mut GasMeter<T>,
) -> Result<Self, DispatchError>;

/// Increment the reference count of a of a stored code by one.
///
/// # Errors
///
/// [`Error::CodeNotFound`] is returned if no stored code found having the specified
/// `code_hash`.
fn increment_refcount(code_hash: CodeHash<T>) -> Result<(), DispatchError>;

/// Decrement the reference count of a stored code by one.
///
/// # Note
///
/// A contract whose reference count dropped to zero isn't automatically removed. A
/// `remove_code` transaction must be submitted by the original uploader to do so.
fn decrement_refcount(code_hash: CodeHash<T>);

/// Execute the specified exported function and return the result.
///
/// When the specified function is `Constructor` the executable is stored and its
Expand Down Expand Up @@ -1285,10 +1285,10 @@ where

info.queue_trie_for_deletion();
ContractInfoOf::<T>::remove(&frame.account_id);
E::decrement_refcount(info.code_hash);
Self::decrement_refcount(info.code_hash);

for (code_hash, deposit) in info.delegate_dependencies() {
E::decrement_refcount(*code_hash);
Self::decrement_refcount(*code_hash);
frame
.nested_storage
.charge_deposit(frame.account_id.clone(), StorageDeposit::Refund(*deposit));
Expand Down Expand Up @@ -1491,8 +1491,8 @@ where

frame.nested_storage.charge_deposit(frame.account_id.clone(), deposit);

E::increment_refcount(hash)?;
E::decrement_refcount(prev_hash);
Self::increment_refcount(hash)?;
Self::decrement_refcount(prev_hash);
Contracts::<Self::T>::deposit_event(
vec![T::Hashing::hash_of(&frame.account_id), hash, prev_hash],
Event::ContractCodeUpdated {
Expand Down Expand Up @@ -1525,6 +1525,25 @@ where
}
}

fn increment_refcount(code_hash: CodeHash<Self::T>) -> Result<(), DispatchError> {
<CodeInfoOf<Self::T>>::mutate(code_hash, |existing| -> Result<(), DispatchError> {
if let Some(info) = existing {
*info.refcount_mut() = info.refcount().saturating_add(1);
Ok(())
} else {
Err(Error::<T>::CodeNotFound.into())
}
})
}

fn decrement_refcount(code_hash: CodeHash<T>) {
<CodeInfoOf<T>>::mutate(code_hash, |existing| {
if let Some(info) = existing {
*info.refcount_mut() = info.refcount().saturating_sub(1);
}
});
}

fn add_delegate_dependency(
&mut self,
code_hash: CodeHash<Self::T>,
Expand All @@ -1537,7 +1556,7 @@ where
let deposit = T::CodeHashLockupDepositPercent::get().mul_ceil(code_info.deposit());

info.add_delegate_dependency(code_hash, deposit)?;
<WasmBlob<T>>::increment_refcount(code_hash)?;
Self::increment_refcount(code_hash)?;
frame
.nested_storage
.charge_deposit(frame.account_id.clone(), StorageDeposit::Charge(deposit));
Expand All @@ -1552,8 +1571,7 @@ where
let info = frame.contract_info.get(&frame.account_id);

let deposit = info.remove_delegate_dependency(code_hash)?;
<WasmBlob<T>>::decrement_refcount(*code_hash);

Self::decrement_refcount(*code_hash);
frame
.nested_storage
.charge_deposit(frame.account_id.clone(), StorageDeposit::Refund(deposit));
Expand Down Expand Up @@ -1600,11 +1618,7 @@ mod tests {
use pallet_contracts_primitives::ReturnFlags;
use pretty_assertions::assert_eq;
use sp_runtime::{traits::Hash, DispatchError};
use std::{
cell::RefCell,
collections::hash_map::{Entry, HashMap},
rc::Rc,
};
use std::{cell::RefCell, collections::hash_map::HashMap, rc::Rc};

type System = frame_system::Pallet<Test>;

Expand Down Expand Up @@ -1635,7 +1649,6 @@ mod tests {
func_type: ExportedFunction,
code_hash: CodeHash<Test>,
code_info: CodeInfo<Test>,
refcount: u64,
}

#[derive(Default, Clone)]
Expand Down Expand Up @@ -1664,37 +1677,11 @@ mod tests {
func_type,
code_hash: hash,
code_info: CodeInfo::<Test>::new(ALICE),
refcount: 1,
},
);
hash
})
}

fn increment_refcount(code_hash: CodeHash<Test>) -> Result<(), DispatchError> {
Loader::mutate(|loader| {
match loader.map.entry(code_hash) {
Entry::Vacant(_) => Err(<Error<Test>>::CodeNotFound)?,
Entry::Occupied(mut entry) => entry.get_mut().refcount += 1,
}
Ok(())
})
}

fn decrement_refcount(code_hash: CodeHash<Test>) {
use std::collections::hash_map::Entry::Occupied;
Loader::mutate(|loader| {
let mut entry = match loader.map.entry(code_hash) {
Occupied(e) => e,
_ => panic!("code_hash does not exist"),
};
let refcount = &mut entry.get_mut().refcount;
*refcount -= 1;
if *refcount == 0 {
entry.remove();
}
});
}
}

impl Executable<Test> for MockExecutable {
Expand All @@ -1707,22 +1694,14 @@ mod tests {
})
}

fn increment_refcount(code_hash: CodeHash<Test>) -> Result<(), DispatchError> {
MockLoader::increment_refcount(code_hash)
}

fn decrement_refcount(code_hash: CodeHash<Test>) {
MockLoader::decrement_refcount(code_hash);
}

fn execute<E: Ext<T = Test>>(
self,
ext: &mut E,
function: &ExportedFunction,
input_data: Vec<u8>,
) -> ExecResult {
if let &Constructor = function {
Self::increment_refcount(self.code_hash).unwrap();
E::increment_refcount(self.code_hash).unwrap();
}
// # Safety
//
Expand All @@ -1733,7 +1712,7 @@ mod tests {
// The transmute is necessary because `execute` has to be generic over all
// `E: Ext`. However, `MockExecutable` can't be generic over `E` as it would
// constitute a cycle.
let ext = unsafe { std::mem::transmute(ext) };
let ext = unsafe { mem::transmute(ext) };
if function == &self.func_type {
(self.func)(MockCtx { ext, input_data }, &self)
} else {
Expand Down
8 changes: 5 additions & 3 deletions substrate/frame/contracts/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,9 @@ pub mod weights;
#[cfg(test)]
mod tests;
use crate::{
exec::{AccountIdOf, ErrorOrigin, ExecError, Executable, Key, MomentOf, Stack as ExecStack},
exec::{
AccountIdOf, ErrorOrigin, ExecError, Executable, Ext, Key, MomentOf, Stack as ExecStack,
},
gas::GasMeter,
storage::{meter::Meter as StorageMeter, ContractInfo, DeletionQueueManager},
wasm::{CodeInfo, WasmBlob},
Expand Down Expand Up @@ -658,8 +660,8 @@ pub mod pallet {
} else {
return Err(<Error<T>>::ContractNotFound.into())
};
<WasmBlob<T>>::increment_refcount(code_hash)?;
<WasmBlob<T>>::decrement_refcount(contract.code_hash);
<ExecStack<T, WasmBlob<T>>>::increment_refcount(code_hash)?;
<ExecStack<T, WasmBlob<T>>>::decrement_refcount(contract.code_hash);
Self::deposit_event(
vec![T::Hashing::hash_of(&dest), code_hash, contract.code_hash],
Event::ContractCodeUpdated {
Expand Down
21 changes: 21 additions & 0 deletions substrate/frame/contracts/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -862,6 +862,27 @@ fn deposit_event_max_value_limit() {
});
}

// Fail out of fuel (ref_time weight) inside the start function.
#[test]
fn run_out_of_fuel_start_fun() {
let (wasm, _code_hash) = compile_module::<Test>("run_out_of_gas_start_fn").unwrap();
ExtBuilder::default().existential_deposit(50).build().execute_with(|| {
let _ = <Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
assert_err_ignore_postinfo!(
Contracts::instantiate_with_code(
RuntimeOrigin::signed(ALICE),
0,
Weight::from_parts(1_000_000_000_000, u64::MAX),
None,
wasm,
vec![],
vec![],
),
Error::<Test>::OutOfGas,
);
});
}

// Fail out of fuel (ref_time weight) in the engine.
#[test]
fn run_out_of_fuel_engine() {
Expand Down
Loading

0 comments on commit c879d1d

Please sign in to comment.