Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
8 changes: 8 additions & 0 deletions prdoc/pr_9823.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
title: bugfix revm set_storage gas cost
doc:
- audience: Runtime Dev
description: "Fixes bug in revm gasmetering where the initial charge was less than\
\ the adjusted charge.\r\n"
crates:
- name: pallet-revive
bump: patch
53 changes: 53 additions & 0 deletions substrate/frame/revive/src/tests/sol/host.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,59 @@ fn sload_works() {
}
}

#[test]
fn sload_error_reading_non_32_byte_value() {
let (code, _) = compile_module_with_type("Host", FixtureType::Solc).unwrap();

let index = U256::from(13);
let expected_value = U256::from(17);

ExtBuilder::default().build().execute_with(|| {
<Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);

let Contract { addr, .. } =
builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract();

{
// Test that reading storage value of 31 bytes results in contract trapped
let contract_info = test_utils::get_contract(&addr);
let key = Key::Fix(index.to_be_bytes());
contract_info
.write(&key, Some(expected_value.to_be_bytes::<32>()[..31].to_vec()), None, false)
.unwrap();

let result = builder::bare_call(addr)
.data(Host::HostCalls::sloadOp(Host::sloadOpCall { slot: index }).abi_encode())
.build();
assert!(result.result.is_err(), "test should error");
let err = result.result.unwrap_err();
let sp_runtime::DispatchError::Module(module_err) = &err else {
panic!("expected Module error (ContractTrapped), got {:?}", err)
};
assert_eq!(module_err.message, Some("ContractTrapped"));
Comment on lines +391 to +396

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you can do a
assert_return_code!(result, RuntimeReturnCode::CalleeTrapped);
see other example in src/tests/pvm.rs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This only works with build_and_unwrap_result but the result itself contains an error:

thread 'tests::sol::host::sload_error_reading_non_32_byte_value' panicked at substrate/frame/revive/src/test_utils/builder.rs:229:29:
called `Result::unwrap()` on an `Err` value: Module(ModuleError { index: 4, error: [11, 0, 0, 0], message: Some("ContractTrapped") })

}

{
// Test that reading storage value of 33 bytes results in contract trapped
let contract_info = test_utils::get_contract(&addr);
let key = Key::Fix(index.to_be_bytes());
let mut bytes = expected_value.to_be_bytes::<32>().to_vec();
bytes.push(0u8);
contract_info.write(&key, Some(bytes), None, false).unwrap();

let result = builder::bare_call(addr)
.data(Host::HostCalls::sloadOp(Host::sloadOpCall { slot: index }).abi_encode())
.build();
assert!(result.result.is_err(), "test should error");
let err = result.result.unwrap_err();
let sp_runtime::DispatchError::Module(module_err) = &err else {
panic!("expected Module error (ContractTrapped), got {:?}", err)
};
assert_eq!(module_err.message, Some("ContractTrapped"));
}
});
}

#[test]
fn sstore_works() {
for fixture_type in [FixtureType::Solc, FixtureType::Resolc] {
Expand Down
8 changes: 6 additions & 2 deletions substrate/frame/revive/src/vm/evm/instructions/host.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ pub fn sload<'ext, E: Ext>(context: Context<'_, 'ext, E>) {
*index = if let Some(storage_value) = value {
// sload always reads a word
let Ok::<[u8; 32], _>(bytes) = storage_value.try_into() else {
log::error!(target: crate::LOG_TARGET, "sload read invalid storage value length. Expected 32.");
Comment thread
0xRVE marked this conversation as resolved.
Outdated
context.interpreter.halt(InstructionResult::FatalExternalError);
return
};
Expand Down Expand Up @@ -169,9 +170,10 @@ fn store_helper<'ext, E: Ext>(
///
/// Stores a word to storage.
pub fn sstore<'ext, E: Ext>(context: Context<'_, 'ext, E>) {
let old_bytes = context.interpreter.extend.max_value_size();
Comment thread
pgherveou marked this conversation as resolved.
store_helper(
context,
RuntimeCosts::SetStorage { new_bytes: 32, old_bytes: 0 },
RuntimeCosts::SetStorage { new_bytes: 32, old_bytes },
|ext, key, value, take_old| ext.set_storage(key, value, take_old),
|new_bytes, old_bytes| RuntimeCosts::SetStorage { new_bytes, old_bytes },
);
Expand All @@ -180,9 +182,10 @@ pub fn sstore<'ext, E: Ext>(context: Context<'_, 'ext, E>) {
/// EIP-1153: Transient storage opcodes
/// Store value to transient storage
pub fn tstore<'ext, E: Ext>(context: Context<'_, 'ext, E>) {
let old_bytes = context.interpreter.extend.max_value_size();
Comment thread
pgherveou marked this conversation as resolved.
store_helper(
context,
RuntimeCosts::SetTransientStorage { new_bytes: 32, old_bytes: 0 },
RuntimeCosts::SetTransientStorage { new_bytes: 32, old_bytes },
|ext, key, value, take_old| ext.set_transient_storage(key, value, take_old),
|new_bytes, old_bytes| RuntimeCosts::SetTransientStorage { new_bytes, old_bytes },
);
Expand All @@ -199,6 +202,7 @@ pub fn tload<'ext, E: Ext>(context: Context<'_, 'ext, E>) {
*index = if let Some(storage_value) = bytes {
if storage_value.len() != 32 {
// tload always reads a word
log::error!(target: crate::LOG_TARGET, "tload read invalid storage value length. Expected 32.");
context.interpreter.halt(InstructionResult::FatalExternalError);
return;
}
Expand Down
Loading