Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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::debug!(target: crate::LOG_TARGET, "sload read invalid storage value length. Expected 32.");
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