feat(fee): add gas cost for calldata bytes

[avi-starkware]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8fe05a4) 69.04% compared to head (7533155) 69.12%.
Report is 1 commits behind head on main-v0.13.1.

Additional details and impacted files

@@               Coverage Diff                @@
##           main-v0.13.1    #1426      +/-   ##
================================================
+ Coverage         69.04%   69.12%   +0.08%     
================================================
  Files                58       58              
  Lines              7949     7967      +18     
  Branches           7949     7967      +18     
================================================
+ Hits               5488     5507      +19     
+ Misses             2005     2004       -1     
  Partials            456      456              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[noaov1]

Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: 4 of 7 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, and @Yoni-Starkware)

---

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r2 (raw file):

// L2 Calldata.
pub const GAS_PER_CALLDATA_BYTE: usize = 2;
pub const GAS_PER_CALLDATA_WORD: usize = GAS_PER_CALLDATA_BYTE * WORD_WIDTH;

When is it used?

Code quote:

pub const GAS_PER_CALLDATA_WORD: usize = GAS_PER_CALLDATA_BYTE * WORD_WIDTH;

---

crates/blockifier/src/fee/gas_usage.rs line 77 at r2 (raw file):

}

// Return the gas cost for transaction calldata.

Can you please elaborate which cost is it?

Code quote:

// Return the gas cost for transaction calldata.

---

crates/blockifier/src/fee/gas_usage_test.rs line 116 at r2 (raw file):

        std::iter::empty(),
        StateChangesCount::default(),
        0,

As l1_handler_payload_size = 4;
Same below.

Suggestion:

        4,

[avi-starkware]

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

When is it used?

I accidentally used the cost per byte instead of the cost per word. I fixed it.

Now it is used.

[avi-starkware]

Reviewable status: 2 of 7 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/fee/gas_usage.rs line 77 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can you please elaborate which cost is it?

Done.

---

crates/blockifier/src/fee/gas_usage_test.rs line 116 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

As l1_handler_payload_size = 4;
Same below.

I don't want to consider the l1_handler_payload as calldata for now. I will change it in another PR depending on what ariel wants to do with it.

[elintul]

Reviewed 1 of 5 files at r1, 2 of 5 files at r3, all commit messages.
Reviewable status: 4 of 7 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/fee/gas_usage.rs line 82 at r4 (raw file):

    let calldata_gas_cost = calldata_length as f64 * eth_gas_constants::GAS_PER_CALLDATA_WORD;
    GasVector { l1_gas: calldata_gas_cost.ceil() as u128, blob_gas: 0 }
}

@dorimedini-starkware, can you suggest an alternatives to the floats?

[Yoni-Starkware]

Reviewable status: 4 of 7 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, and @noaov1)

---

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r2 (raw file):

Previously, avi-starkware wrote…

I accidentally used the cost per byte instead of the cost per word. I fixed it.

Now it is used.

Can you avoid using as ?

[Yoni-Starkware]

Reviewable status: 4 of 7 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, and @noaov1)

---

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Can you avoid using as ?

Also in other places

[noaov1]

Reviewed 2 of 2 files at r2, 3 of 5 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware and @avi-starkware)

---

crates/blockifier/src/fee/gas_usage.rs line 77 at r2 (raw file):

Previously, avi-starkware wrote…

Done.

By word do you mean a felt? What gas cost? Storing it in the storage?

---

crates/blockifier/src/fee/gas_usage_test.rs line 116 at r2 (raw file):

Previously, avi-starkware wrote…

I don't want to consider the l1_handler_payload as calldata for now. I will change it in another PR depending on what ariel wants to do with it.

You already are :)
(you use the calldata_length field, which, for an l1 handler equals the payload size).
After you decide, please make it consistent.

---

crates/blockifier/src/transaction/account_transactions_test.rs line 832 at r2 (raw file):

    let TestInitData { mut state, account_address, contract_address, mut nonce_manager } =
        create_test_init_data(&block_context.chain_info, CairoVersion::Cairo0);
    let actual_gas_used = 6148;

6140 + 2 * 4 = 6148. Working!

Code quote:

let actual_gas_used = 6148;

[noaov1]

Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware and @avi-starkware)

[elintul]

Reviewed all commit messages.
Reviewable status: 1 of 7 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, and @noaov1)

---

crates/blockifier/src/fee/gas_usage_test.rs line 7 at r5 (raw file):

use starknet_api::transaction::L2ToL1Payload;

use super::get_calldata_gas_cost;

Suggestion:

crate...

[avi-starkware]

crates/blockifier/src/fee/gas_usage.rs line 82 at r4 (raw file):

Previously, elintul (Elin) wrote…

@dorimedini-starkware, can you suggest an alternatives to the floats?

@liorgold2 told me to use integers (i.e., milligas instead of gas units) and then convert from milligas to gas units via integer division by 1000.

[avi-starkware]

Reviewable status: 1 of 7 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @dorimedini-starkware, @elintul, @liorgold2, and @noaov1)

---

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Also in other places

Done.

---

crates/blockifier/src/fee/gas_usage.rs line 77 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

By word do you mean a felt? What gas cost? Storing it in the storage?

Done.

---

crates/blockifier/src/fee/gas_usage_test.rs line 116 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

You already are :)
(you use the calldata_length field, which, for an l1 handler equals the payload size).
After you decide, please make it consistent.

Okay, I added it everywhere in this test. Please check that it is correct. Especially the case with l2_to_l1 messages which I am the least sure about.

[avi-starkware]

crates/blockifier/src/fee/gas_usage_test.rs line 7 at r5 (raw file):

use starknet_api::transaction::L2ToL1Payload;

use super::get_calldata_gas_cost;

Done.

Sorry. Auto import...

[avi-starkware]

crates/blockifier/src/fee/gas_usage_test.rs line 116 at r2 (raw file):

Previously, avi-starkware wrote…

Okay, I added it everywhere in this test. Please check that it is correct. Especially the case with l2_to_l1 messages which I am the least sure about.

The l2_to_l1 messages are calculated separately. They are not part of the calldata as far as I can tell, so I am keeping the part of the test dealing with this case unchaged.

[dorimedini-starkware]

Reviewable status: 1 of 7 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @elintul, @liorgold2, and @noaov1)

---

crates/blockifier/src/fee/gas_usage.rs line 82 at r4 (raw file):

Previously, avi-starkware wrote…

@liorgold2 told me to use integers (i.e., milligas instead of gas units) and then convert from milligas to gas units via integer division by 1000.

SGTM

[avi-starkware]

Reviewable status: 1 of 7 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @elintul, @liorgold2, and @noaov1)

---

crates/blockifier/src/fee/gas_usage.rs line 82 at r4 (raw file):

Previously, dorimedini-starkware wrote…

SGTM

We discussed using rational numbers instead of integers, but they are not implemented yet.

I added a TODO to use rational numbers once they are implemented.

[giladchase]

Reviewable status: 1 of 7 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, @elintul, @liorgold2, and @noaov1)

---

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r8 (raw file):

// L2 Calldata.
pub const MILLIGAS_PER_CALLDATA_BYTE: usize = 160;
pub const MILLIGAS_PER_CALLDATA_WORD: usize = MILLIGAS_PER_CALLDATA_BYTE * WORD_WIDTH;

Are these meant to be overridden by nodes that use the blockifier? (i think Ariel mentioned this at some channel)

Code quote:

pub const MILLIGAS_PER_CALLDATA_BYTE: usize = 160;
pub const MILLIGAS_PER_CALLDATA_WORD: usize = MILLIGAS_PER_CALLDATA_BYTE * WORD_WIDTH;

[noaov1]

Reviewed 3 of 6 files at r5, 1 of 3 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, and @liorgold2)

---

crates/blockifier/src/fee/gas_usage.rs line 77 at r2 (raw file):

Previously, avi-starkware wrote…

Done.

I'm confused :) You multiply the MILLIGAS_PER_CALLDATA_WORD by a WORD_WIDTH. Is it okay?

[avi-starkware]

crates/blockifier/src/fee/gas_usage.rs line 77 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

I'm confused :) You multiply the MILLIGAS_PER_CALLDATA_WORD by a WORD_WIDTH. Is it okay?

I multiply MILLIGAS_PER_CALLDATA_BYTE by WORD_WIDTH to get MILLIGAS_PER_CALLDATA_WORD, which is the price in milligas of a word. I assume each felt is the size of a word, so the milligas cost of a felt is MILLIGAS_PER_CALLDATA_WORD.

Note: this assumption is no longer correct, but I am working on another PR where I count the actual number of bytes each felt consists of. Within the scope of the current PR, a felt is always 32 bytes.

[avi-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @giladchase, @liorgold2, and @noaov1)

---

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r8 (raw file):

Previously, giladchase wrote…

Are these meant to be overridden by nodes that use the blockifier? (i think Ariel mentioned this at some channel)

I don't know...

If so, does it affect my task in some way?

[noaov1]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @liorgold2)

---

crates/blockifier/src/fee/gas_usage.rs line 77 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Oh, under your assumption it is correct. Thanks.

Add a TODO?

[avi-starkware]

crates/blockifier/src/fee/gas_usage.rs line 77 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Add a TODO?

Done.

[giladchase]

Reviewed 1 of 3 files at r6.
Reviewable status: 2 of 7 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, @elintul, @liorgold2, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r8 (raw file):

Previously, avi-starkware wrote…

I don't know...

If so, does it affect my task in some way?

It affects, ya.
Verify with Yoni just to be sure.

If that's the case, then instead of defining them here, you need to define the consts as new keys in versioned_constants.json, and similarly as fields in VersionedConstants struct.

To access them you need to do it through the context instance you have available.
Lookup usages of versioned_constants for example, or talk to me about it.

[giladchase]

Reviewable status: 2 of 7 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @avi-starkware, @elintul, @liorgold2, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r8 (raw file):

Previously, giladchase wrote…

It affects, ya.
Verify with Yoni just to be sure.

If that's the case, then instead of defining them here, you need to define the consts as new keys in versioned_constants.json, and similarly as fields in VersionedConstants struct.

To access them you need to do it through the context instance you have available.
Lookup usages of versioned_constants for example, or talk to me about it.

The idea is that constant files will now only be for constants that are generally fixed.

Constants that are likely to vary between versions are set in versioned_constants.json, to make it easier for blockifier users to play around with them.

[noaov1]

Reviewable status: 2 of 7 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @elintul, @giladchase, @liorgold2, and @Yoni-Starkware)

---

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r8 (raw file):

Previously, giladchase wrote…

The idea is that constant files will now only be for constants that are generally fixed.

Constants that are likely to vary between versions are set in versioned_constants.json, to make it easier for blockifier users to play around with them.

Are other constants in this file not configurable?

[avi-starkware]

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @elintul, @giladchase, @liorgold2, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r8 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Are other constants in this file not configurable?

I moved the constant to versioned_constants.json

[noaov1]

Reviewed 2 of 5 files at r9, 7 of 7 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

---

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r8 (raw file):

Previously, avi-starkware wrote…

I moved the constant to versioned_constants.json

You may need to change the file in python as well. @giladchase ?

[elintul]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

[avi-starkware]

crates/blockifier/src/fee/eth_gas_constants.rs line 9 at r8 (raw file):

Previously, noaov1 (Noa Oved) wrote…

You may need to change the file in python as well. @giladchase ?

@giladchase said the python file is not relevant yet.

dismiss

[noaov1]

Reviewed 8 of 8 files at r11, 3 of 3 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

[noaov1]

Python PR?

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

[avi-starkware]

https://reviewable.io/reviews/starkware-industries/starkware/33890#-