Remove gas price #3141

gterzian · 2022-10-10T09:08:34Z

document all added functions
try in sandbox /simulation/labnet
unit tests on the added/changed features
- make tests compile
- make tests pass
add logs allowing easy debugging in case the changes caused problems
if the API has changed, update the API specification

gterzian · 2022-10-17T07:20:41Z

@massalabs/core-team @damip Still have to fix a few tests that are failing(see those currently ignored), sorry I still cannot access discord...

gterzian · 2022-10-17T08:38:51Z

massa-execution-worker/src/interface_impl.rs

@@ -546,7 +546,7 @@ impl Interface for InterfaceImpl {
        validity_start: (u64, u8),
        validity_end: (u64, u8),
        max_gas: u64,
-        gas_price: u64,
+        _gas_price: u64,


@adrien-zinger I think removing this would change the public api for the running byte code correct? Is there a specific way to deal with such breaking changes?

@aoudiamoncef can you synchronize with @gterzian on that ? (through github)

damip · 2022-10-18T09:03:30Z

massa-async-pool/src/lib.rs

@@ -16,33 +16,32 @@
 //!
 //! ```json
 //! {
-//!     "sender": "xxxx",  // address that sent the message and spent max_gas*gas_price+coins on emission
+//!     "sender": "xxxx",  // address that sent the message and spent max_gas+coins on emission


max_gas is not an amount of coins, but a quantity of gas.

=> address that sent the message and spent fee+coins on emission

damip · 2022-10-18T09:04:08Z

massa-async-pool/src/lib.rs

 //!     "slot": {"period": 123455, "thread": 11},  // slot at which the message was emitted
 //!     "emission_index": 212,  // index of the message emitted in this slot
 //!     "destination": "xxxx",  // target address
 //!     "handler": "handle_message",  // name of the handler function to call in the target SC
 //!     "validity_start": {"period": 123456, "thread": 12},  // the message can be handled starting from the validity_start slot (included)
 //!     "validity_end": {"period": 123457, "thread": 16},  // the message can be handled until the validity_end slot (excluded)
 //!     "max_gas": 12334,  // max gas available when the handler is called
-//!     "gas_price": "124.23",  // gas price for the handler call


this needs to be replaced by a fee: Amount property

damip · 2022-10-18T09:04:35Z

massa-async-pool/src/lib.rs

 //!     "coins": "1111.11",  // amount of coins to transfer to the destination address when calling its handler
 //!     "data": { ... any object ... }  // data payload of the message, passed as the sole parameter of the destination handler when called
 //! }
 //! ```
 //!
 //! ## How to send a message during bytecode execution
 //!
-//! * messages are sent using an ABI: `send_message(target_address, target_handler, validity_start, validity_end, max_gas, gas_price, coins, data: JSON string) -> Result<(), ABIReturnError>`. Note that data has a configuration defined `max_async_message_data_size`.
+//! * messages are sent using an ABI: `send_message(target_address, target_handler, validity_start, validity_end, max_gas, coins, data: JSON string) -> Result<(), ABIReturnError>`. Note that data has a configuration defined `max_async_message_data_size`.


gas_price needs to be replaced by a fee property

damip · 2022-10-18T09:04:54Z

massa-async-pool/src/lib.rs

 //! * when called, this ABI does this:
 //!   * it consumes `compute_gas_cost_of_message_storage(context.current_slot, validity_end_slot)` of gas in the current execution. This allows making the message emission more gas-consuming when it requires storing the message in queue for longer
-//!   * it consumes `max_gas * gas_price + coins` coins from the sender
+//!   * it consumes `max_gas + coins` coins from the sender


it consumes fee + coins

damip · 2022-10-18T09:06:02Z

massa-async-pool/src/lib.rs

 //!   * it generates an `AsyncMessage` and stores it in an asynchronous pool
 //!
-//! Note that `max_gas*gas_price` coins are burned when sending the message.
+//! Note that `max_gas` coins are burned when sending the message.


Gas and coins are fundamentally different resources.

fee coins are burned when sending the message

damip · 2022-10-18T09:09:51Z

massa-async-pool/src/lib.rs

 //!
 //! ## How is the `AsyncPool` handled
 //! ```md
-//! * In the AsyncPool, Messages are kept sorted by `priority = AsyncMessageId(msg.max_gas * msg.gas_price, rev(msg.slot), rev(msg.emission_index))`
+//! * In the AsyncPool, Messages are kept sorted by `priority = AsyncMessageId(msg.max_gas, rev(msg.slot), rev(msg.emission_index))`


The main sorting criterion should be message.fee.

Sort by decreasing:

AsyncMessageId = (std::cmp::Reverse<Amount>, Slot, u64);

damip · 2022-10-18T09:10:28Z

massa-async-pool/src/lib.rs

@@ -55,7 +54,6 @@
 //!     * credit target_address with M.coins
 //!     * run the target handler function with M.payload as parameter and the context:
 //!       * max_gas = M.max_gas
-//!       * gas_price = M.gas_price


fee = M.fee

damip · 2022-10-18T09:13:20Z

massa-async-pool/src/message.rs

+/// `(rev(max_gas), emission_slot, emission_index)`
+pub type AsyncMessageId = (std::cmp::Reverse<u64>, Slot, u64);


let's change it according to the comment above

damip · 2022-10-18T09:14:21Z

massa-async-pool/src/message.rs


 pub struct AsyncMessageIdSerializer {
-    amount_serializer: AmountSerializer,


there should be an AmountSerializer for the fee

damip · 2022-10-18T09:15:28Z

massa-async-pool/src/message.rs

    slot_serializer: SlotSerializer,
    u64_serializer: U64VarIntSerializer,
 }

 impl AsyncMessageIdSerializer {
    pub fn new() -> Self {
        Self {
-            amount_serializer: AmountSerializer::new(),
+            max_gas_serializer: U64VarIntSerializer::new(),
            slot_serializer: SlotSerializer::new(),
            u64_serializer: U64VarIntSerializer::new(),


let's rename this one emission_index_serializer

damip · 2022-10-18T09:21:34Z

massa-client/src/cmds.rs

-                    match gas_price
-                        .checked_mul_u64(max_gas)
-                        .and_then(|x| x.checked_add(fee))
+                    match Amount::from_str(&max_gas.to_string())


Gas is not an amount of coins. This check should be done on fee property only

damip · 2022-10-18T10:57:00Z

massa-client/src/cmds.rs

-                    match gas_price
-                        .checked_mul_u64(max_gas)
-                        .and_then(|x| x.checked_add(fee))
+                    match Amount::from_str(&max_gas.to_string())


here as well, just use coins.checked_add(fee)

damip · 2022-10-18T11:36:37Z

massa-execution-worker/src/execution.rs

@@ -231,7 +231,7 @@ impl ExecutionState {
        // get operation ID
        let operation_id = operation.id;

-        // compute fee from (op.max_gas * op.gas_price + op.fee)
+        // compute fee from (op.max_gas + op.fee)


total fee should just be op.fee and nothing else

damip · 2022-10-18T11:38:15Z

massa-execution-worker/src/interface_impl.rs

@@ -571,7 +571,6 @@ impl Interface for InterfaceImpl {
            validity_start: Slot::new(validity_start.0, validity_start.1),
            validity_end: Slot::new(validity_end.0, validity_end.1),
            max_gas,
-            gas_price: Amount::from_raw(gas_price),


this should be replaced by fee: Amount

massa-sc-runtime should also be updated

damip · 2022-10-18T11:39:20Z

massa-final-state/src/state_changes.rs

@@ -74,7 +74,6 @@ impl Serializer<StateChanges> for StateChangesSerializer {
    ///     destination: Address::from_str("A12htxRWiEm8jDJpJptr6cwEhWNcCSFWstN1MLSa96DDkVM9Y42G").unwrap(),
    ///     handler: String::from("test"),
    ///     max_gas: 10000000,
-    ///     gas_price: Amount::from_str("1").unwrap(),


damip · 2022-10-18T11:39:25Z

massa-final-state/src/state_changes.rs

@@ -168,7 +167,6 @@ impl Deserializer<StateChanges> for StateChangesDeserializer {
    ///     destination: Address::from_str("A12htxRWiEm8jDJpJptr6cwEhWNcCSFWstN1MLSa96DDkVM9Y42G").unwrap(),
    ///     handler: String::from("test"),
    ///     max_gas: 10000000,
-    ///     gas_price: Amount::from_str("1").unwrap(),


damip · 2022-10-18T11:43:03Z

massa-models/src/operation.rs

-        self.get_gas_price()
-            .saturating_mul_u64(self.get_gas_usage())
-    }
-
    /// Get the total fee paid by the creator
    pub fn get_total_fee(&self) -> Amount {


this function is now useless. use op.fee directly

damip · 2022-10-18T11:46:17Z

massa-models/src/operation.rs

-            } => gas_price
-                .saturating_mul_u64(*max_gas)
+            OperationType::ExecuteSC { max_gas, .. } => {
+                Amount::from_str(&max_gas.to_string()).unwrap_or(Amount::default())


here we should return just the coins property of executesc

also, going through strings is inefficient

It seems OperationType::ExecuteSC does not have coins, would returning zero be appropriate?

true, let's return Amount::zero() like in the case of RollSell

continuing over at #3173

so this PR is abandoned and you continue on 3173 ?

damip · 2022-10-18T11:46:33Z

massa-models/src/operation.rs

+            OperationType::ExecuteSC { max_gas, .. } => {
+                Amount::from_str(&max_gas.to_string()).unwrap_or(Amount::default())
+            }
+            OperationType::CallSC { max_gas, coins, .. } => Amount::from_str(&max_gas.to_string())


here we should return just the coins property of CallSC

also, going through strings is inefficient

damip

There are a few misunderstandings that can be cleared by adding some documentation maybe:

Only the fee parameter represents coins spent as fees, only the max_gas parameter represents the amount of gas (computing power) expected to be spent during the execution, and only the size of the operation represents the amount of block space used by the operation
Coins, gas and block space are three different dimensions (see https://en.wikipedia.org/wiki/Dimensional_analysis )
The Amount structure encodes Coins and nothing else (already documented for the Amount structure)
gas is encoded as u64
block space is encoded as u64
quotes from https://en.wikipedia.org/wiki/Dimensional_analysis#Dimensional_homogeneity :
- One may take ratios of quantities with different dimensions, and multiply or divide them. However, only quantities having the same dimension may be compared, equated, added, or subtracted.
  - this means that max_gas + fee is illegal but max_gas / fee is legal

I've added the checklist to perform in order to consider this PR ready.
Let's make sure the PR is ready and reviewed by the end of the week.

gterzian · 2022-10-19T09:41:03Z

@damip Thanks for the clarification! So the fee in all cases would come from that found on the operation, except for the send_message ABI where we'd have to add it as a call parameter?

damip · 2022-10-19T09:53:45Z

@damip Thanks for the clarification! So the fee in all cases would come from that found on the operation, except for the send_message ABI where we'd have to add it as a call parameter?

Yes exactly

damip · 2022-10-19T09:57:10Z

@gterzian I made a mistake in one of my comments above: the first criterion of the async pool sorting should not be msg.fee but rev(Ratio(msg.fee, max(msg.max_gas,1)))

gterzian · 2022-10-20T09:39:36Z

continuing over at #3173

damip · 2022-10-20T19:35:08Z

massa-models/src/operation.rs

+                Amount::from_str(&max_gas.to_string()).unwrap_or(Amount::default())
+            }
+            OperationType::CallSC { max_gas, coins, .. } => Amount::from_str(&max_gas.to_string())
+                .unwrap_or(Amount::default())


clippy warn

damip · 2022-10-20T19:35:14Z

massa-models/src/operation.rs

-            } => gas_price
-                .saturating_mul_u64(*max_gas)
+            OperationType::ExecuteSC { max_gas, .. } => {
+                Amount::from_str(&max_gas.to_string()).unwrap_or(Amount::default())


clippy warn

damip · 2022-10-20T19:36:54Z

massa-models/src/operation.rs

    /// Get the total fee paid by the creator
    pub fn get_total_fee(&self) -> Amount {
-        self.get_gas_coins().saturating_add(self.content.fee)
+        self.content.fee.saturating_add(
+            Amount::from_str(&self.get_gas_usage().to_string()).unwrap_or(Amount::default()),


there is a design problem here as well (see other remarks)

gterzian · 2022-10-31T07:02:52Z

Closing in favor of #3173

gterzian changed the title ~~Remove gasp price~~ Remove gas price Oct 10, 2022

gterzian added 11 commits October 17, 2022 16:19

start removing gasp price

b3943e8

remove gas price from operation type callsc

607a3a4

remove gas price from operation type execute sc

c0603c8

remove gas price from read only execution requests

d86e63b

remove gas price from async message

892ed94

remove gas price from readonly call request

fcc91d7

remove gas price from more places

078db24

make tests compile

73a0318

fix warnings

58607ee

fmt

cf9a1bd

ignore failing tests, fix one doc test

1542258

gterzian force-pushed the remove_gas_price branch from 81988b1 to 1542258 Compare October 17, 2022 08:19

gterzian commented Oct 17, 2022

View reviewed changes

gterzian added 4 commits October 17, 2022 17:23

fix test_nested_call_gas_usage

6c35e62

fix async pool changes doc

418fb49

remove from_raw from operation

d65f59c

remove from_raw from client command processing

bdd45e1

aoudiamoncef added api Issues related to the API breaking labels Oct 17, 2022

gterzian added 3 commits October 18, 2022 15:28

remove gas price from client commands

1803250

un-ignore datastore manipulation test

7c37fbd

fix test_bootstrap_server

8b4e141

damip reviewed Oct 18, 2022

View reviewed changes

damip requested changes Oct 18, 2022

View reviewed changes

damip assigned gterzian Oct 18, 2022

damip reviewed Oct 20, 2022

View reviewed changes

gterzian closed this Oct 31, 2022

AurelienFT deleted the remove_gas_price branch May 29, 2023 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove gas price #3141

Remove gas price #3141

gterzian commented Oct 10, 2022 •

edited by damip

Loading

gterzian commented Oct 17, 2022

gterzian Oct 17, 2022

damip Oct 17, 2022

damip Oct 18, 2022

damip Oct 18, 2022

damip Oct 18, 2022

damip Oct 18, 2022

damip Oct 18, 2022

damip Oct 18, 2022 •

edited

Loading

damip Oct 18, 2022

damip Oct 18, 2022 •

edited

Loading

damip Oct 18, 2022 •

edited

Loading

damip Oct 18, 2022

damip Oct 18, 2022 •

edited

Loading

damip Oct 18, 2022

damip Oct 18, 2022 •

edited

Loading

damip Oct 18, 2022

damip Oct 18, 2022

damip Oct 18, 2022

damip Oct 18, 2022

damip Oct 18, 2022

gterzian Oct 20, 2022

damip Oct 20, 2022

damip Oct 20, 2022

damip Oct 18, 2022

damip left a comment •

edited

Loading

gterzian commented Oct 19, 2022 •

edited

Loading

damip commented Oct 19, 2022

damip commented Oct 19, 2022 •

edited

Loading

gterzian commented Oct 20, 2022

damip Oct 20, 2022

damip Oct 20, 2022

damip Oct 20, 2022

gterzian commented Oct 31, 2022

		/// `(rev(max_gas), emission_slot, emission_index)`
		pub type AsyncMessageId = (std::cmp::Reverse<u64>, Slot, u64);


		pub struct AsyncMessageIdSerializer {
		amount_serializer: AmountSerializer,

Remove gas price #3141

Remove gas price #3141

Conversation

gterzian commented Oct 10, 2022 • edited by damip Loading

gterzian commented Oct 17, 2022

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

Choose a reason for hiding this comment

damip Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damip Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

damip Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damip Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damip Oct 18, 2022 • edited Loading

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

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

damip left a comment • edited Loading

Choose a reason for hiding this comment

gterzian commented Oct 19, 2022 • edited Loading

damip commented Oct 19, 2022

damip commented Oct 19, 2022 • edited Loading

gterzian commented Oct 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gterzian commented Oct 31, 2022

gterzian commented Oct 10, 2022 •

edited by damip

Loading

damip Oct 18, 2022 •

edited

Loading

damip Oct 18, 2022 •

edited

Loading

damip Oct 18, 2022 •

edited

Loading

damip Oct 18, 2022 •

edited

Loading

damip Oct 18, 2022 •

edited

Loading

damip left a comment •

edited

Loading

gterzian commented Oct 19, 2022 •

edited

Loading

damip commented Oct 19, 2022 •

edited

Loading