Charge for new storage bytes #634

Dentosal · 2023-11-10T08:45:33Z

Closes #602
Spec PR: FuelLabs/fuel-specs#536

Breaking changes:

Gas is charged for new storage slots, StorageSlot::LEN * gas_costs.new_storage_per_byte * num_slots
Write opcodes now return the number of new storage slots instead of just a boolean on whether the value existed. The return value is currently ignored by the Sway stdlib, so it shouldn't be too bad

Dentosal · 2023-11-10T09:04:06Z

fuel-tx/src/transaction/consensus_parameters/gas.rs

@@ -321,6 +321,7 @@ pub struct GasCostsValues {
    // Non-opcode costs
    pub contract_root: DependentCost,
    pub state_root: DependentCost,
+    pub new_storage_per_byte: Word,


Name still subject to bikeshedding

Dentosal · 2023-11-10T09:07:16Z

fuel-vm/src/interpreter/blockchain.rs

+    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>,


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.

xgreenx

The issues says:

We need to find all opcodes that modify the storage first. For that, we can find all places where we use methods from the InterpreterStorage trait.

It is similar to the issue FuelLabs/fuel-core#1239

xgreenx · 2023-11-10T09:16:15Z

fuel-tx/src/transaction/consensus_parameters/gas.rs

@@ -321,6 +321,7 @@ pub struct GasCostsValues {
    // Non-opcode costs
    pub contract_root: DependentCost,
    pub state_root: DependentCost,
+    pub new_storage_per_byte: Word,


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.

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.

xgreenx · 2023-11-10T09:21:49Z

fuel-vm/src/interpreter/blockchain.rs

+            cgas,
+            ggas,
+            profiler,
+            (Bytes32::LEN as u64) * new_storage_gas_per_byte,


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

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

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.

Voxelot · 2023-11-10T10:17:55Z

Should we also charge for 32 bytes(asset id) + 8 bytes (amount) when inserting a new asset type into the balances tree?

xgreenx · 2023-11-10T10:44:29Z

Should we also charge for 32 bytes(asset id) + 8 bytes (amount) when inserting a new asset type into the balances tree?

Yep

We need to find all opcodes that modify the storage first. For that, we can find all places where we use methods from the InterpreterStorage trait.

fuel-vm/src/interpreter/blockchain.rs

Co-authored-by: Green Baneling <[email protected]>

xgreenx

Looks good=) Waiting for rest of opcodes

fuel-vm/src/interpreter/blockchain.rs

fuel-vm/src/interpreter/contract.rs

fuel-vm/src/interpreter/flow.rs

xgreenx · 2023-11-10T23:09:21Z

fuel-vm/src/interpreter/contract.rs

@@ -196,8 +230,12 @@ struct TransferCtx<'vm, S, Tx> {
    context: &'vm Context,
    balances: &'vm mut RuntimeBalances,
    receipts: &'vm mut ReceiptsCtx,
+    profiler: &'vm mut Profiler,


You forgot to update transfer_output

I though TRO doesn't create markle tree entries? Am I missing something here?

See this issue for context: FuelLabs/fuel-vm#602 FuelVM PR: FuelLabs/fuel-vm#634

Dentosal added 2 commits November 9, 2023 16:51

WIP

2d944a3

Fix tests

e76b1fa

Dentosal added breaking A breaking api change fuel-vm Related to the `fuel-vm` crate. fuel-storage Related to the `fuel-storage` crate. labels Nov 10, 2023

Dentosal self-assigned this Nov 10, 2023

Dentosal added 3 commits November 10, 2023 11:50

Merge branch 'master' into dento/charge-for-new-storage-bytes

ab1033e

Remove obsolete todo

fc05023

Changelog

28603d9

Dentosal mentioned this pull request Nov 10, 2023

Charge for new storage slots FuelLabs/fuel-specs#536

Merged

Dentosal commented Nov 10, 2023

View reviewed changes

Dentosal requested a review from a team November 10, 2023 09:07

Dentosal marked this pull request as ready for review November 10, 2023 09:08

xgreenx reviewed Nov 10, 2023

View reviewed changes

Dentosal marked this pull request as draft November 10, 2023 11:22

Dentosal added 3 commits November 10, 2023 15:37

Add gas costing to mint instruction

f54a751

Add test cases to prove gas is getting charged

6dbdaa8

fmt

a510192

Dentosal marked this pull request as ready for review November 10, 2023 13:41

Dentosal requested a review from xgreenx November 10, 2023 13:41

xgreenx reviewed Nov 10, 2023

View reviewed changes

fuel-vm/src/interpreter/blockchain.rs Outdated Show resolved Hide resolved

fuel-vm/src/interpreter/blockchain.rs Outdated Show resolved Hide resolved

fuel-vm/src/interpreter/blockchain.rs Outdated Show resolved Hide resolved

Dentosal and others added 5 commits November 10, 2023 13:58

Update fuel-vm/src/interpreter/blockchain.rs

bc13cec

Co-authored-by: Green Baneling <[email protected]>

Update fuel-vm/src/interpreter/blockchain.rs

906fcfd

Co-authored-by: Green Baneling <[email protected]>

Update fuel-vm/src/interpreter/blockchain.rs

0c0bd01

Co-authored-by: Green Baneling <[email protected]>

fmt

3088340

Cleanup

14f71c1

xgreenx reviewed Nov 10, 2023

View reviewed changes

Dentosal added 2 commits November 10, 2023 18:18

Add TR/TRO new stroage slot gas costing

52c8b29

Add gas costing to storage created by CALL forwarding coins

4743ad4

xgreenx reviewed Nov 10, 2023

View reviewed changes

Dentosal added 2 commits November 13, 2023 14:07

Separate state read and write ctxs

7863362

Fix balance entry state size

c593198

xgreenx approved these changes Nov 13, 2023

View reviewed changes

Dentosal added this pull request to the merge queue Nov 13, 2023

Merged via the queue into master with commit da5b9af Nov 13, 2023
37 checks passed

Dentosal deleted the dento/charge-for-new-storage-bytes branch November 13, 2023 14:09

xgreenx mentioned this pull request Nov 13, 2023

Release v0.42.0 #635

Merged

Dentosal added a commit to FuelLabs/fuel-specs that referenced this pull request Nov 17, 2023

Charge for new storage slots (#536)

6bdea62

See this issue for context: FuelLabs/fuel-vm#602 FuelVM PR: FuelLabs/fuel-vm#634

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Charge for new storage bytes #634

Charge for new storage bytes #634

Dentosal commented Nov 10, 2023 •

edited

Loading

Dentosal Nov 10, 2023

Dentosal Nov 10, 2023

xgreenx left a comment

xgreenx Nov 10, 2023

Dentosal Nov 10, 2023

xgreenx Nov 10, 2023

Dentosal Nov 10, 2023

xgreenx Nov 10, 2023

Voxelot commented Nov 10, 2023

xgreenx commented Nov 10, 2023 •

edited

Loading

xgreenx left a comment

xgreenx Nov 10, 2023

Dentosal Nov 13, 2023

Charge for new storage bytes #634

Charge for new storage bytes #634

Conversation

Dentosal commented Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot commented Nov 10, 2023

xgreenx commented Nov 10, 2023 • edited Loading

xgreenx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dentosal commented Nov 10, 2023 •

edited

Loading

xgreenx commented Nov 10, 2023 •

edited

Loading