Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

seal_reentrant_count returns contract reentrant count #11539

Closed
wants to merge 32 commits into from
Closed
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
a8214fd
Add logic, test, broken benchmark
yarikbratashchuk May 18, 2022
143c158
Merge branch 'master' into seal_reentrant_count
yarikbratashchuk May 20, 2022
219eb84
Merge branch 'master' into seal_reentrant_count
yarikbratashchuk May 22, 2022
fccd751
account_entrance_count
yarikbratashchuk May 23, 2022
0a70239
Addressing comments
yarikbratashchuk May 29, 2022
6f3655a
Address @agryaznov's comments
yarikbratashchuk Jun 1, 2022
f29884b
Add test for account_entrance_count, fix ci
yarikbratashchuk Jun 2, 2022
72c8f83
Cargo fmt
yarikbratashchuk Jun 2, 2022
0f6f894
Fix tests
yarikbratashchuk Jun 2, 2022
949c438
Fix tests
yarikbratashchuk Jun 5, 2022
f34b6da
Remove delegated call from test, address comments
yarikbratashchuk Jun 16, 2022
a036585
Minor fixes and indentation in wat files
yarikbratashchuk Jun 21, 2022
3be772e
Update test for account_entrance_count
yarikbratashchuk Jun 21, 2022
5fdc100
Update reentrant_count_call test
yarikbratashchuk Jun 21, 2022
b656c88
Delegate call test
yarikbratashchuk Jun 23, 2022
17bb81f
Cargo +nightly fmt
yarikbratashchuk Jun 24, 2022
2f7f405
Address comments
yarikbratashchuk Jun 24, 2022
5f01797
Update reentrant_count_works test
yarikbratashchuk Jun 24, 2022
013e3bf
Merge branch 'master' into master
yarikbratashchuk Jun 24, 2022
3948142
Apply weights diff
yarikbratashchuk Jul 3, 2022
9c0c06f
Add fixture descriptions
yarikbratashchuk Jul 3, 2022
9c5bb47
Update comments as suggested
yarikbratashchuk Jul 3, 2022
35c0349
Update reentrant_count_call test to use seal_address
yarikbratashchuk Jul 3, 2022
3400632
Merge branch 'paritytech:master' into master
RustNinja Aug 2, 2022
ebde8e1
Merge branch 'paritytech:master' into master
Artemka374 Aug 10, 2022
321a828
change account_entrance_count_call fixture to use seal_caller
Artemka374 Sep 11, 2022
ee63f37
Merge remote-tracking branch 'paritytech/master'
Artemka374 Nov 2, 2022
3463795
fix compilation errors and apply some suggestions
Artemka374 Nov 4, 2022
1649d37
apply suggestions
Artemka374 Nov 5, 2022
8d94425
cargo fmt
Artemka374 Nov 7, 2022
0e48a2d
apply suggestions
Artemka374 Nov 10, 2022
7db2660
Merge remote-tracking branch 'paritytech/master'
Artemka374 Nov 10, 2022
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
38 changes: 38 additions & 0 deletions frame/contracts/fixtures/account_entrance_count_call.wat
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
(module
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved
(import "seal0" "seal_input" (func $seal_input (param i32 i32)))
(import "__unstable__" "seal_account_entrance_count" (func $seal_account_entrance_count (param i32) (result i32)))
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
(import "env" "memory" (memory 1 1))

;; [0, 32) buffer where input is copied

;; [32, 36) size of the input buffer
(data (i32.const 32) "\20")

(func $assert (param i32)
(block $ok
(br_if $ok
(get_local 0)
)
(unreachable)
)
)
(func (export "call")
(local $account_entrance_count i32)

;; Reading "callee" contract address (which is the address of the caller)
Copy link
Contributor

Choose a reason for hiding this comment

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

(which is the address of the caller)

This is not the case in general for this particular fixture, and solely depends on arguments it's been called with. In your account_entrance_count_works() test you indeed call it with the same address as the caller's, but I think it's better not to make the fixture so tightly coupled with the test. So I would suggest two ways to improve this:

  1. Use seal_caller and expect different seal_account_entrance_count results for the cases callee == caller and callee != caller; or
  2. Make the fixture accept both callee and caller addresses in its input, and still xpect different seal_account_entrance_count results for the cases callee == caller and callee != caller.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello, I'm not sure about it, but do callee and caller actually equal in test? Cause as I see the origin of call is ALICE and we are calling contract address, and if we are changing fixture the way you explained above, it says that caller != callee. So it is interesting to know am I getting this right=)

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant here that the way the contract fixture is being called, and hence the expected state of the call stack - this all should be set in the outer unit test, and no assumption on this regard should be hard-coded into the fixture. Just return the account_entrance_count with $seal_return and check it outside the contract.

(call $seal_input (i32.const 0) (i32.const 32))

;; assert account_entrance_count == 1
(call $assert
(i32.eq (call $seal_account_entrance_count (i32.const 0)) (i32.const 1))
)

;; assert account_entrance_count == 0 for another account
(call $assert
(i32.eq (call $seal_account_entrance_count (i32.const 32)) (i32.const 0))
)
)

(func (export "deploy"))

)
67 changes: 67 additions & 0 deletions frame/contracts/fixtures/reentrant_count_call.wat
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
(module
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved
(import "seal0" "seal_input" (func $seal_input (param i32 i32)))
(import "seal0" "seal_call" (func $seal_call (param i32 i32 i64 i32 i32 i32 i32 i32 i32) (result i32)))
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved
(import "__unstable__" "seal_reentrant_count" (func $seal_reentrant_count (result i32)))
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
(import "env" "memory" (memory 1 1))

;; [0, 32) buffer where code hash is copied
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved

;; [32, 36) buffer for the call stack high
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved

;; [36, 40) size of the input buffer
(data (i32.const 36) "\24")

(func $assert (param i32)
(block $ok
(br_if $ok
(get_local 0)
)
(unreachable)
)
)
(func (export "call")
(local $manual_reentrant_count i32)
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved
(local $seal_call_exit_code i32)

;; Reading input
(call $seal_input (i32.const 0) (i32.const 36))

;; reading manually passed reentrant count
(set_local $manual_reentrant_count (i32.load (i32.const 32)))
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved

;; reentrance count is calculated correctly
(call $assert
(i32.eq (call $seal_reentrant_count) (get_local $manual_reentrant_count))
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved
)

yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved
(i32.eq (call $seal_reentrant_count) (i32.const 5))
(if
(then) ;; recursion exit case
(else
;; incrementing manual reentrant count high
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved
(i32.store (i32.const 32) (i32.add (i32.load (i32.const 32)) (i32.const 1)))

;; Call to itself
(set_local $seal_call_exit_code
(call $seal_call
(i32.const 0) ;; Pointer to "callee" address.
(i32.const 32) ;; Length of "callee" address.
(i64.const 0) ;; How much gas to devote for the execution. 0 = all.
(i32.const 0) ;; Pointer to the buffer with value to transfer
(i32.const 0) ;; Length of the buffer with value to transfer.
(i32.const 0) ;; Pointer to input data buffer address
(i32.const 36) ;; Length of input data buffer
(i32.const 0xffffffff) ;; u32 max sentinel value: do not copy output
(i32.const 0) ;; Ptr to output buffer len
)
)

(call $assert
(i32.eq (get_local $seal_call_exit_code) (i32.const 0))
)
)
)
)

(func (export "deploy"))
)
69 changes: 69 additions & 0 deletions frame/contracts/fixtures/reentrant_count_delegated_call.wat
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
(module
(import "seal0" "seal_input" (func $seal_input (param i32 i32)))
(import "seal0" "seal_set_storage" (func $seal_set_storage (param i32 i32 i32)))
(import "seal0" "seal_delegate_call" (func $seal_delegate_call (param i32 i32 i32 i32 i32 i32) (result i32)))
(import "__unstable__" "seal_reentrant_count" (func $seal_reentrant_count (result i32)))
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
(import "env" "memory" (memory 1 1))

;; [0, 32) buffer where code hash is copied

;; [32, 36) buffer for the call stack high
agryaznov marked this conversation as resolved.
Show resolved Hide resolved

;; [36, 40) size of the input buffer
(data (i32.const 36) "\24")

(func $assert (param i32)
(block $ok
(br_if $ok
(get_local 0)
)
(unreachable)
)
)
(func (export "call")
(local $callstack_high i32)
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
(local $delegate_call_exit_code i32)

;; Reading input
(call $seal_input (i32.const 0) (i32.const 36))

;; reading passed callstack high
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
(set_local $callstack_high (i32.load (i32.const 32)))
agryaznov marked this conversation as resolved.
Show resolved Hide resolved

;; incrementing callstack high
(i32.store (i32.const 32) (i32.add (i32.load (i32.const 32)) (i32.const 1)))

;; reentrance count stays 0
(call $assert
(i32.eq (call $seal_reentrant_count) (i32.const 0))
)

(i32.eq (get_local $callstack_high) (i32.const 5))
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
(if
(then) ;; exit recursion case
(else
;; Call to itself
(set_local $delegate_call_exit_code
(call $seal_delegate_call
(i32.const 0) ;; Set no call flags
(i32.const 0) ;; Pointer to "callee" code_hash.
(i32.const 0) ;; Input is ingored
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
(i32.const 36) ;; Length of the input
(i32.const 4294967295) ;; u32 max sentinel value: do not copy output
(i32.const 0) ;; Length is ignored in this case
)
)

(call $assert
(i32.eq (get_local $delegate_call_exit_code) (i32.const 0))
)
)
)

(call $assert
(i32.le_s (get_local $callstack_high) (i32.const 5))
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
)
)

(func (export "deploy"))
)
53 changes: 53 additions & 0 deletions frame/contracts/src/benchmarking/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2086,6 +2086,59 @@ benchmarks! {
let origin = RawOrigin::Signed(instance.caller.clone());
}: call(origin, instance.addr, 0u32.into(), Weight::MAX, None, vec![])

seal_reentrant_count {
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
let r in 0 .. API_BENCHMARK_BATCHES;
let code = WasmModule::<T>::from(ModuleDefinition {
memory: Some(ImportedMemory::max::<T>()),
imported_functions: vec![ImportedFunction {
module: "__unstable__",
name: "seal_reentrant_count",
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
params: vec![],
return_type: Some(ValueType::I32),
}],
call_body: Some(body::repeated(r * API_BENCHMARK_BATCH_SIZE, &[
Instruction::Call(0),
Instruction::Drop,
])),
.. Default::default()
});
let instance = Contract::<T>::new(code, vec![])?;
let origin = RawOrigin::Signed(instance.caller.clone());
}: call(origin, instance.addr, 0u32.into(), Weight::MAX, None, vec![])

seal_account_entrance_count {
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
let r in 0 .. API_BENCHMARK_BATCHES;
let dummy_code = WasmModule::<T>::dummy_with_bytes(0);
let accounts = (0..r * API_BENCHMARK_BATCH_SIZE)
.map(|i| Contract::with_index(i + 1, dummy_code.clone(), vec![]))
.collect::<Result<Vec<_>, _>>()?;
let account_id_len = accounts.get(0).map(|i| i.account_id.encode().len()).unwrap_or(0);
let account_id_bytes = accounts.iter().flat_map(|x| x.account_id.encode()).collect();
let code = WasmModule::<T>::from(ModuleDefinition {
memory: Some(ImportedMemory::max::<T>()),
imported_functions: vec![ImportedFunction {
module: "__unstable__",
name: "seal_account_entrance_count",
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
params: vec![ValueType::I32],
return_type: Some(ValueType::I32),
}],
data_segments: vec![
DataSegment {
offset: 0,
value: account_id_bytes,
},
],
call_body: Some(body::repeated_dyn(r * API_BENCHMARK_BATCH_SIZE, vec![
Counter(0, account_id_len as u32), // account_ptr
Regular(Instruction::Call(0)),
Regular(Instruction::Drop),
])),
.. Default::default()
});
let instance = Contract::<T>::new(code, vec![])?;
let origin = RawOrigin::Signed(instance.caller.clone());
}: call(origin, instance.addr, 0u32.into(), Weight::MAX, None, vec![])

// We make the assumption that pushing a constant and dropping a value takes roughly
// the same amount of time. We follow that `t.load` and `drop` both have the weight
// of this benchmark / 2. We need to make this assumption because there is no way
Expand Down
20 changes: 20 additions & 0 deletions frame/contracts/src/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,15 @@ pub trait Ext: sealing::Sealed {

/// Sets new code hash for existing contract.
fn set_code_hash(&mut self, hash: CodeHash<Self::T>) -> Result<(), DispatchError>;

/// Returns the number of times the currently executing contract exists on the call stack in
/// addition to the calling instance. A value of 0 means no reentrancy.
fn reentrant_count(&self) -> u32;

/// Returns the number of times the specified contract exists on the call stack. Delegated calls
/// are not calculated as separate entrance.
/// A value of 0 means it does not exist on the call stack.
fn account_entrance_count(&self, account_id: &AccountIdOf<Self::T>) -> u32;
}

/// Describes the different functions that can be exported by an [`Executable`].
Expand Down Expand Up @@ -1305,6 +1314,17 @@ where
});
Ok(())
}

fn reentrant_count(&self) -> u32 {
let id: &AccountIdOf<Self::T> = &self.top_frame().account_id;
self.account_entrance_count(id).saturating_sub(1)
}

fn account_entrance_count(&self, account_id: &AccountIdOf<Self::T>) -> u32 {
self.frames()
.filter(|f| f.delegate_caller.is_none() && &f.account_id == account_id)
.count() as u32
}
}

fn deposit_event<T: Config>(topics: Vec<T::Hash>, event: Event<T>) {
Expand Down
8 changes: 8 additions & 0 deletions frame/contracts/src/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,12 @@ pub struct HostFnWeights<T: Config> {
/// Weight of calling `seal_ecdsa_to_eth_address`.
pub ecdsa_to_eth_address: Weight,

/// Weight of calling `seal_reentrant_count`.
pub reentrant_count: Weight,

/// Weight of calling `seal_account_entrance_count`.
pub account_entrance_count: Weight,

/// The type parameter is used in the default implementation.
#[codec(skip)]
pub _phantom: PhantomData<T>,
Expand Down Expand Up @@ -651,6 +657,8 @@ impl<T: Config> Default for HostFnWeights<T> {
hash_blake2_128_per_byte: cost_byte_batched!(seal_hash_blake2_128_per_kb),
ecdsa_recover: cost_batched!(seal_ecdsa_recover),
ecdsa_to_eth_address: cost_batched!(seal_ecdsa_to_eth_address),
reentrant_count: cost_batched!(seal_reentrant_count),
account_entrance_count: cost_batched!(seal_account_entrance_count),
_phantom: PhantomData,
}
}
Expand Down
89 changes: 89 additions & 0 deletions frame/contracts/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3272,3 +3272,92 @@ fn set_code_hash() {
);
});
}

#[test]
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(feature = "unstable-interface")]
fn reentrant_count_works_with_call() {
let (wasm, code_hash) = compile_module::<Test>("reentrant_count_call").unwrap();
let contract_addr = Contracts::contract_address(&ALICE, &code_hash, &[]);

ExtBuilder::default().existential_deposit(100).build().execute_with(|| {
let _ = Balances::deposit_creating(&ALICE, 1_000_000);

assert_ok!(Contracts::instantiate_with_code(
Origin::signed(ALICE),
300_000,
GAS_LIMIT,
None,
wasm,
vec![],
vec![],
));

// adding reentrant count to the input
let input = (contract_addr.clone(), 0).encode();
yarikbratashchuk marked this conversation as resolved.
Show resolved Hide resolved

Contracts::bare_call(ALICE, contract_addr, 0, GAS_LIMIT, None, input, true)
.result
.unwrap();
});
}

#[test]
#[cfg(feature = "unstable-interface")]
fn reentrant_count_works_with_delegated_call() {
let (wasm, code_hash) = compile_module::<Test>("reentrant_count_delegated_call").unwrap();
let contract_addr = Contracts::contract_address(&ALICE, &code_hash, &[]);

ExtBuilder::default().existential_deposit(100).build().execute_with(|| {
let _ = Balances::deposit_creating(&ALICE, 1_000_000);

assert_ok!(Contracts::instantiate_with_code(
Origin::signed(ALICE),
300_000,
GAS_LIMIT,
None,
wasm,
vec![],
vec![],
));

// adding a callstack height to the input
let input = (code_hash, 1).encode();

Contracts::bare_call(ALICE, contract_addr.clone(), 0, GAS_LIMIT, None, input, true)
.result
.unwrap();
});
}

#[test]
#[cfg(feature = "unstable-interface")]
fn account_entrance_count_works() {
let (wasm, code_hash) = compile_module::<Test>("account_entrance_count_call").unwrap();
let contract_addr = Contracts::contract_address(&ALICE, &code_hash, &[]);

ExtBuilder::default().existential_deposit(100).build().execute_with(|| {
let _ = Balances::deposit_creating(&ALICE, 1_000_000);

assert_ok!(Contracts::instantiate_with_code(
Origin::signed(ALICE),
300_000,
GAS_LIMIT,
None,
wasm,
vec![],
vec![],
));

Contracts::bare_call(
agryaznov marked this conversation as resolved.
Show resolved Hide resolved
ALICE,
contract_addr.clone(),
0,
GAS_LIMIT,
None,
contract_addr.encode(),
true,
)
.result
.unwrap();
});
}
Loading