Skip to content
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

Charge for new storage bytes #634

Merged
merged 17 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from 8 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- If the `witnessses_size > policies.witness_limit`, then transaction will be rejected.
- GTF opcode changed its hardcoded constants for fields. It should be updated according to the values from the specification on the Sway side.
- [#633](https://github.com/FuelLabs/fuel-vm/pull/633): Limit receipt count to `u16::MAX`.
- [#634](https://github.com/FuelLabs/fuel-vm/pull/634): Charge for storage per new byte written. Write opcodes now return the number of new storage slots created, instead of just a boolean on whether the value existed before.

### Fixed

Expand Down
3 changes: 3 additions & 0 deletions fuel-tx/src/transaction/consensus_parameters/gas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ pub struct GasCostsValues {
// Non-opcode costs
pub contract_root: DependentCost,
pub state_root: DependentCost,
pub new_storage_per_byte: Word,
Copy link
Member Author

Choose a reason for hiding this comment

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

Name still subject to bikeshedding

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is better to use DependentCost here because we need to charge for a new storage slot, plus, in the future(when we support dynamic storage values), charge per byte.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it makes any sense to future-proof this for the dynamic storage value support. The whole API is going to change anyway.

pub vm_initialization: Word,
}

Expand Down Expand Up @@ -477,6 +478,7 @@ impl GasCostsValues {
// Non-opcode costs
contract_root: DependentCost::free(),
state_root: DependentCost::free(),
new_storage_per_byte: 0,
vm_initialization: 0,
}
}
Expand Down Expand Up @@ -594,6 +596,7 @@ impl GasCostsValues {
// Non-opcode costs
contract_root: DependentCost::unit(),
state_root: DependentCost::unit(),
new_storage_per_byte: 1,
vm_initialization: 1,
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ pub fn default_gas_costs() -> GasCostsValues {
base: 412,
units_per_gas: 1,
},
new_storage_per_byte: 1,
vm_initialization: 2000,
}
}
177 changes: 160 additions & 17 deletions fuel-vm/src/interpreter/blockchain.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use core::mem::size_of;

use super::{
contract::{
balance,
Expand All @@ -6,6 +8,7 @@ use super::{
},
gas::{
dependent_gas_charge,
gas_charge,
ProfileGas,
},
internal::{
Expand Down Expand Up @@ -171,8 +174,18 @@ where

pub(crate) fn mint(&mut self, a: Word, b: Word) -> IoResult<(), S::DataError> {
let tx_offset = self.tx_offset();
let (SystemRegisters { fp, pc, is, .. }, _) =
split_registers(&mut self.registers);
let new_storage_gas_per_byte = self.gas_costs().new_storage_per_byte;
let (
SystemRegisters {
cgas,
ggas,
fp,
pc,
is,
..
},
_,
) = split_registers(&mut self.registers);
MintCtx {
storage: &mut self.storage,
context: &self.context,
Expand All @@ -182,6 +195,10 @@ where
tx_offset,
memory: &mut self.memory,
},
profiler: &mut self.profiler,
new_storage_gas_per_byte,
cgas,
ggas,
fp: fp.as_ref(),
pc,
is: is.as_ref(),
Expand Down Expand Up @@ -318,8 +335,18 @@ where
rb: RegisterId,
c: Word,
) -> IoResult<(), S::DataError> {
let (SystemRegisters { fp, pc, .. }, mut w) =
split_registers(&mut self.registers);
let new_storage_gas_per_byte = self.gas_costs().new_storage_per_byte;
Dentosal marked this conversation as resolved.
Show resolved Hide resolved
let (
SystemRegisters {
cgas,
ggas,
is,
fp,
pc,
..
},
mut w,
) = split_registers(&mut self.registers);
let (result, got_result) = w
.get_mut_two(WriteRegKey::try_from(ra)?, WriteRegKey::try_from(rb)?)
.ok_or(RuntimeError::Recoverable(
Expand All @@ -336,6 +363,12 @@ where
storage,
memory,
context,
profiler: &mut self.profiler,
new_storage_gas_per_byte,
current_contract: self.frames.last().map(|frame| frame.to()).copied(),
cgas,
ggas,
is: is.as_ref(),
fp: fp.as_ref(),
pc,
},
Expand Down Expand Up @@ -373,8 +406,18 @@ where
rb: RegisterId,
c: Word,
) -> IoResult<(), S::DataError> {
let (SystemRegisters { fp, pc, .. }, mut w) =
split_registers(&mut self.registers);
let new_storage_gas_per_byte = self.gas_costs().new_storage_per_byte;
let (
SystemRegisters {
cgas,
ggas,
is,
fp,
pc,
..
},
mut w,
) = split_registers(&mut self.registers);
let exists = &mut w[WriteRegKey::try_from(rb)?];
let Self {
ref mut storage,
Expand All @@ -387,6 +430,12 @@ where
storage,
memory,
context,
profiler: &mut self.profiler,
new_storage_gas_per_byte,
current_contract: self.frames.last().map(|frame| frame.to()).copied(),
cgas,
ggas,
is: is.as_ref(),
fp: fp.as_ref(),
pc,
},
Expand All @@ -403,8 +452,14 @@ where
c: Word,
d: Word,
) -> IoResult<(), S::DataError> {
let new_storage_per_byte = self.gas_costs().new_storage_per_byte;
let contract_id = self.internal_contract().copied();
let (SystemRegisters { pc, .. }, mut w) = split_registers(&mut self.registers);
let (
SystemRegisters {
is, cgas, ggas, pc, ..
},
mut w,
) = split_registers(&mut self.registers);
let result = &mut w[WriteRegKey::try_from(rb)?];

let input = StateWriteQWord::new(a, c, d)?;
Expand All @@ -414,7 +469,20 @@ where
..
} = self;

state_write_qword(&contract_id?, storage, memory.as_mut(), pc, result, input)
state_write_qword(
&contract_id?,
storage,
memory.as_mut(),
&mut self.profiler,
new_storage_per_byte,
self.frames.last().map(|frame| frame.to()).copied(),
cgas,
ggas,
is.as_ref(),
pc,
result,
input,
)
}

pub(crate) fn timestamp(
Expand Down Expand Up @@ -616,7 +684,8 @@ where
.checked_sub(a)
.ok_or(PanicReason::NotEnoughBalance)?;

self.storage
let _ = self
.storage
.merkle_contract_asset_id_balance_insert(contract_id, &asset_id, balance)
.map_err(RuntimeError::Storage)?;

Expand All @@ -631,7 +700,11 @@ where
struct MintCtx<'vm, S> {
storage: &'vm mut S,
context: &'vm Context,
profiler: &'vm mut Profiler,
new_storage_gas_per_byte: Word,
append: AppendReceipt<'vm>,
cgas: RegMut<'vm, CGAS>,
ggas: RegMut<'vm, GGAS>,
fp: Reg<'vm, FP>,
pc: RegMut<'vm, PC>,
is: Reg<'vm, IS>,
Expand All @@ -654,10 +727,28 @@ where
let balance = balance(self.storage, contract_id, &asset_id)?;
let balance = balance.checked_add(a).ok_or(PanicReason::BalanceOverflow)?;

self.storage
let old_value = self
.storage
.merkle_contract_asset_id_balance_insert(contract_id, &asset_id, balance)
.map_err(RuntimeError::Storage)?;

if old_value.is_none() {
// New data was written, charge gas for it
let profiler = ProfileGas {
pc: self.pc.as_ref(),
is: self.is,
current_contract: Some(*contract_id),
profiler: self.profiler,
};
gas_charge(
self.cgas,
self.ggas,
profiler,
((AssetId::LEN + size_of::<u64>()) as u64)
Dentosal marked this conversation as resolved.
Show resolved Hide resolved
* self.new_storage_gas_per_byte,
)?;
}

let receipt = Receipt::mint(*sub_id, *contract_id, a, *self.pc, *self.is);

append_receipt(self.append, receipt)?;
Expand Down Expand Up @@ -846,6 +937,12 @@ pub(crate) struct StateWordCtx<'vm, S> {
pub storage: &'vm mut S,
pub memory: &'vm [u8; MEM_SIZE],
pub context: &'vm Context,
pub profiler: &'vm mut Profiler,
pub new_storage_gas_per_byte: Word,
pub current_contract: Option<ContractId>,
pub cgas: RegMut<'vm, CGAS>,
pub ggas: RegMut<'vm, GGAS>,
pub is: Reg<'vm, IS>,
Copy link
Member Author

Choose a reason for hiding this comment

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

These are not needed for state read opcodes, so maybe it makes sense to split this struct into two? On the other hand, we're handing over over half of the VM anyway, so just passing the VM directly might make sense.

pub fp: Reg<'vm, FP>,
pub pc: RegMut<'vm, PC>,
}
Expand All @@ -857,6 +954,7 @@ pub(crate) fn state_read_word<S: InterpreterStorage>(
context,
fp,
pc,
..
}: StateWordCtx<S>,
result: &mut Word,
got_result: &mut Word,
Expand Down Expand Up @@ -890,11 +988,17 @@ pub(crate) fn state_write_word<S: InterpreterStorage>(
storage,
memory,
context,
profiler,
new_storage_gas_per_byte,
current_contract,
cgas,
ggas,
is,
fp,
pc,
}: StateWordCtx<S>,
a: Word,
exists: &mut Word,
created_new: &mut Word,
c: Word,
) -> IoResult<(), S::DataError> {
let key = CheckedMemConstLen::<{ Bytes32::LEN }>::new(a)?;
Expand All @@ -913,7 +1017,23 @@ pub(crate) fn state_write_word<S: InterpreterStorage>(
.merkle_contract_state_insert(contract, key, &value)
.map_err(RuntimeError::Storage)?;

*exists = result.is_some() as Word;
*created_new = result.is_none() as Word;

if result.is_none() {
// New data was written, charge gas for it
let profiler = ProfileGas {
pc: pc.as_ref(),
is,
current_contract,
profiler,
};
gas_charge(
cgas,
ggas,
profiler,
(Bytes32::LEN as u64) * new_storage_gas_per_byte,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to charge static cost and per byte=)

Copy link
Member Author

Choose a reason for hiding this comment

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

What static cost? The opcode itself already costs for instertion, this is only for the new slot which has a fixed cost.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is related to the comment about DepenentCost=) base cost can include inserting the new entry into the storage(it will represent the burden of the SMT), and deps_per_unit will be gas per byte that you will multiply with 32 or 64, but in the later versions with dynamic storage by the size of the value.

Dentosal marked this conversation as resolved.
Show resolved Hide resolved
)?;
}

Ok(inc_pc(pc)?)
}
Expand Down Expand Up @@ -1123,10 +1243,17 @@ impl StateWriteQWord {
}
}

fn state_write_qword<S: InterpreterStorage>(
#[allow(clippy::too_many_arguments)]
fn state_write_qword<'vm, S: InterpreterStorage>(
contract_id: &ContractId,
storage: &mut S,
memory: &[u8; MEM_SIZE],
profiler: &'vm mut Profiler,
new_storage_gas_per_byte: Word,
current_contract: Option<ContractId>,
cgas: RegMut<'vm, CGAS>,
ggas: RegMut<'vm, GGAS>,
is: Reg<'vm, IS>,
pc: RegMut<PC>,
result_register: &mut Word,
input: StateWriteQWord,
Expand All @@ -1139,11 +1266,27 @@ fn state_write_qword<S: InterpreterStorage>(
.flat_map(|chunk| Some(Bytes32::from(<[u8; 32]>::try_from(chunk).ok()?)))
.collect();

let any_none = storage
let unset_count = storage
.merkle_contract_state_insert_range(contract_id, destination_key, &values)
.map_err(RuntimeError::Storage)?
.is_some();
*result_register = any_none as Word;
.map_err(RuntimeError::Storage)?;
*result_register = unset_count as Word;

if unset_count > 0 {
// New data was written, charge gas for it
let profiler = ProfileGas {
pc: pc.as_ref(),
is,
current_contract,
profiler,
};
gas_charge(
cgas,
ggas,
profiler,
// Overflow safety: unset_count * 32 can be at most VM_MAX_RAM
(unset_count as u64) * (Bytes32::LEN as u64) * new_storage_gas_per_byte,
Dentosal marked this conversation as resolved.
Show resolved Hide resolved
)?;
}

inc_pc(pc)?;

Expand Down
Loading
Loading