feat(client): l1 gas price #219

Mohiiit · 2024-08-07T07:25:05Z

Pull Request type

Feature

What is the current behavior?

Resolves: #205

What is the new behavior?

Added a worker within the eth crate in order to fetch the gas prices from the l1
Added tests regarding the same

Does this introduce a breaking change?

No

Other Information

In the current scope of the PR, we are not aiming to add oracle to fetch the STRK <-> ETH price for the strk conversion of the fees.

Current Progress

Creating L1 gas price worker
unit test for the worker
supplying gas price to the block production service

…with alloy rs

… simplicity

…ed to state_update.rs

apoorvsadana · 2024-08-08T17:10:44Z

crates/client/eth/src/l1_gas_price.rs

+    infinite_loop: bool,
+) -> anyhow::Result<()> {
+    let poll_time = eth_client.gas_price_poll_ms.unwrap_or(DEFAULT_GAS_PRICE_POLL_MS);
+    update_last_update_timestamp().await;


is this incorrect? i can see the timestamp is updated inside update_gas_price and it should only be there from what I understand

this is for the first time, time stamp is defined in the lazy static, and when the gas worker is called, it will update it for the first time and then for the next time it will get updated in the update_gas_price

this is removed because we are calling the update_gas_price while creating the sync service

apoorvsadana · 2024-08-08T17:13:35Z

crates/client/eth/src/l1_gas_price.rs

+            );
+        }
+
+        if !infinite_loop {


the purpose of the infinite_loop variable was to call this function once at the start of the node without looping to set the initial gas price, I think we should do that as well here

i think this function would look better if it was split into two:
the loop body is a function, which corresponds to when the current infinite_loop false when called directly, and a separate function for the infinite task which calls the first function

apoorvsadana · 2024-08-08T17:21:51Z

crates/client/eth/src/l1_gas_price.rs

+
+    let avg_blob_base_fee = blob_fee_history_one_hour.iter().sum::<u128>() / blob_fee_history_one_hour.len() as u128;
+
+    let eth_gas_price = fee_history.base_fee_per_blob_gas.last().context("Setting l1 last confirmed block number")?;


Suggested change

let eth_gas_price = fee_history.base_fee_per_blob_gas.last().context("Setting l1 last confirmed block number")?;

let eth_gas_price = fee_history.base_fee_gas.last().context("Getting eth gas price")?;

anyway we can ensure this in test cases? I think anvil allows you to set a custom gas price when you start it

yeah that's true, I think we can achieve this using mocks right?

apoorvsadana · 2024-08-08T17:23:47Z

crates/client/eth/src/l1_gas_price.rs

+    eth_client.l1_block_metrics.l1_block_number.set(latest_block_number as f64);
+    eth_client.l1_block_metrics.l1_gas_price_wei.set(eth_gas_price as f64);
+
+    // We're ignoring l1_gas_price_strk


can we import the strk code as well? i remember pragma did a PR. or do we want to do it in another issue

we would need an oracle for that right? I will check for the PR, it can be another PR with the scope of adding oracle

apoorvsadana · 2024-08-08T17:26:30Z

crates/client/eth/src/l1_gas_price.rs

+        let l1_data_provider: Arc<dyn L1DataProvider> = Arc::new(GasPriceProvider::new());
+
+        // Run the worker for a short time
+        let worker_handle = tokio::spawn(gas_price_worker(eth_client, l1_data_provider.clone(), false));


if infinite_loop is false, do we need to spawn tokio? we can just await here right?

we should also write a case for infinite loop true

how does the current code work without a delay?

yeah I think that should work, using the await only

will do, we have that when we are testing for the fail case but will create for this as well

there is a delay, it's in the eth_client

apoorvsadana · 2024-08-08T17:28:35Z

crates/client/eth/src/l1_gas_price.rs

+        let now = SystemTime::now().duration_since(UNIX_EPOCH).expect("Time went backwards").as_secs() as u128;
+
+        let last_update_timestamp = get_last_update_timestamp().await;
+        assert!(last_update_timestamp > now - 60, "Last update timestamp should be within the last minute");


what is the timestamp when the test starts?

it get's updated in the gas worker

apoorvsadana · 2024-08-08T17:31:06Z

crates/client/mempool/src/l1.rs

+#[async_trait::async_trait]
+impl L1DataProvider for GasPriceProvider {
+    async fn get_gas_prices(&self) -> GasPrices {
+        (*self.gas_prices.lock().await).clone()


let's mention that it's 4 u128 so it's ok to clone (unless we think it isn't?)

apoorvsadana · 2024-08-08T17:35:37Z

crates/node/src/cli/l1.rs

+
+    /// The L1 rpc endpoint url for state verification.
+    #[clap(long, value_parser = parse_url, value_name = "ETHEREUM RPC URL")]
+    pub l1_endpoint: Option<Url>,


you can enforce in clippy that sync_l1_disabled false requires l1_endpoint afaik

apoorvsadana · 2024-08-08T17:39:23Z

crates/node/src/service/l1.rs

+        let l1_endpoint = if !config.sync_l1_disabled {
+            if let Some(l1_rpc_url) = &config.l1_endpoint {
+                Some(l1_rpc_url.clone())
+            } else {
+                return Err(anyhow::anyhow!(
+                    "❗ No L1 endpoint provided. You must provide one in order to verify the synced state."
+                ));
+            }
+        } else {
+            None
+        };


Suggested change

let l1_endpoint = if !config.sync_l1_disabled {

if let Some(l1_rpc_url) = &config.l1_endpoint {

Some(l1_rpc_url.clone())

} else {

return Err(anyhow::anyhow!(

"❗ No L1 endpoint provided. You must provide one in order to verify the synced state."

));

}

} else {

None

};

let l1_endpoint = config.sync_l1_disabled

.not()

.then_some(config.l1_endpoint.clone()

.context("❗ No L1 endpoint provided. You must provide one in order to verify the synced state.")?

);

apoorvsadana · 2024-08-08T17:40:14Z

crates/node/src/service/l1.rs

+        };
+
+        let core_address = Address::from_slice(l1_core_address.as_bytes());
+        let eth_client = EthereumClient::new(l1_endpoint.unwrap(), core_address, metrics_handle)


Suggested change

let eth_client = EthereumClient::new(l1_endpoint.unwrap(), core_address, metrics_handle)

let eth_client = EthereumClient::new(l1_endpoint?, core_address, metrics_handle)

cchudant

I am not sure if this pull request:

fetches gas prices everytime a transaction is added to the mempool / a block is produced -- the L1DataProvider functions are async
or is an infinite task running in the background, watching for l1 gas prices and updating a mutex that is shared with the mempool through the L1DataProvider abstraction -- the L1DataProvider functions are sync

which of the two does this pr implement?

I don't like the first one as:

the mempool needs L1 prices for mempool validation
the block production task needs L1 prices to produce blocks
i don't like the idea of l1 gas prices request being initiated by a add_transaction rpc
i don't like having the block production be slowed down by l1, as it's potentially time critical

The second approach has a major drawback that i'm realizing now:

we shouldnt accept any mempool transaction before the first gas prices arrive.
we cannot produce any block before the first gas prices arrive otherwise we could have a huge problem

I think the easiest way to solve it is:

have L1DataProvider look like this

pub struct NewBlockL1Info {
  pub data_availability_mode: L1DataAvailabilityMode, // blob or calldata
  pub gas_prices: GasPrices,
}

trait L1DataProvider {
  /// This returns immediately when GasPrices are available,
  /// or will wait until the L1 watcher has started up and got the first prices.
  async fn get_gas_prices(&self) -> NewBlockL1Info;
}

// tokio watch channel
struct ProvideL1Data(tokio::sync::watch::Receiver<Option<NewBlockL1Info>>);
impl L1DataProvider for ProvideL1Data {
  async fn get_gas_prices(&self) -> NewBlockL1Info {
    self.0.wait_for(Option::is_some).await.expect("recv error").clone()
  }
}

async fn l1_gas_prices_task(set_gas_prices: tokio::sync::watch::Sender<Option<NewBlockL1Info>>) -> anyhow::Result<()> {
  loop {
    get prices
    if set_gas_prices.send(Some(gas_prices)).is_err() {
      // channel is closed, meaning no one is listening to gas prices
      return Ok(());
    }
    interval sleep
  }
}

cchudant · 2024-08-09T10:21:29Z

crates/client/eth/src/l1_gas_price.rs

+use std::time::{SystemTime, UNIX_EPOCH};
+
+lazy_static::lazy_static! {
+    static ref LAST_UPDATE_TIMESTAMP: Arc<Mutex<u128>> = Arc::new(Mutex::new(0));


can this be inside the service instead of global state?

also use a SystemTime directly instead of an untyped integer unix epoch?

cchudant · 2024-08-09T10:23:18Z

crates/node/src/service/l1.rs

+                anyhow::anyhow!("EthereumClient is required to start the l1 sync service but not provided.")
+            })?;
+            // running at-least once before the block production service
+            let _ = futures::executor::block_on(dc_eth::l1_gas_price::gas_price_worker(


what is that block_on? this looks suspicious

this is to fetch the gas price before returning the sync service, it make sure we have gas price

cchudant · 2024-08-09T10:24:25Z

crates/node/src/cli/l1.rs

+
+    /// Disable the gas price sync service. The sync service is responsible to fetch the fee history from the ethereum.
+    #[clap(long, alias = "no-gas-price-sync")]
+    pub gas_price_sync_disabled: bool,


i like it when everything is configurable thank you :)

cchudant · 2024-08-09T10:26:04Z

crates/node/src/service/sync.rs

-        };
-
-        let core_address = Address::from_slice(config.network.l1_core_address().as_bytes());
-        let eth_client = EthereumClient::new(l1_endpoint.unwrap(), core_address, metrics_handle)


when making #221 i found out that you broke sync_l1_disabled on main in the last pr due to this unwrap - i fixed it there for now but is it fixed in this pr?

I checked that, and yeah it's resolved in this PR

cchudant · 2024-08-09T10:32:40Z

crates/client/eth/src/sync.rs

+    gas_price_sync_disabled: bool,
+    gas_price_poll_ms: u64,
+) -> anyhow::Result<()> {
+    let state_update_fut = async { state_update_worker(backend, eth_client, chain_id).await };


this func can be simplified to

tokio::try_join!( state_update_worker(backend,eth_client, chain_id), async { if !gas_price_sync_disabled { gas_price_worker(eth_client, l1_data_provider, true, gas_price_poll_ms).await?; } Ok(()) }, )?;

cchudant · 2024-08-09T10:36:48Z

crates/client/mempool/src/l1.rs

 pub trait L1DataProvider: Send + Sync {
    /// Get L1 data gas prices. This needs an oracle for STRK prices.
-    fn get_gas_prices(&self) -> GasPrices;
+    async fn get_gas_prices(&self) -> GasPrices;
+    async fn set_gas_prices(&self, new_prices: GasPrices);


can you remove those setters from the trait? this is an internal impl detail that other crate shouldn't have access

cchudant · 2024-08-09T10:37:18Z

crates/client/mempool/src/lib.rs

@@ -52,7 +52,7 @@ impl Mempool {
        Mempool { backend, l1_data_provider, inner: Default::default() }
    }

-    pub fn accept_account_tx(
+    pub async fn accept_account_tx(


mmh i don't like that being async i need to think about this

cchudant · 2024-08-09T10:41:16Z

crates/client/eth/src/l1_gas_price.rs

+            .as_millis();
+
+        if current_timestamp - last_update_timestamp > 10 * gas_price_poll_ms as u128 {
+            panic!(


why panic here?

in case we are not able to fetch the latest gas prices, it was present in the madara code as well. I believe it exist to make sure we don't have gas fees deviation. but we can have a flag for this as well

cchudant · 2024-08-09T10:45:14Z

crates/client/eth/src/l1_gas_price.rs

+            );
+        }
+
+        if !infinite_loop {


i think this function would look better if it was split into two:
the loop body is a function, which corresponds to when the current infinite_loop false when called directly, and a separate function for the infinite task which calls the first function

cchudant · 2024-08-09T11:13:59Z

crates/client/eth/src/l1_gas_price.rs

+            return Ok(());
+        }
+
+        sleep(Duration::from_millis(gas_price_poll_ms)).await;


use

let mut interval = tokio::time::interval(pending_block_poll_interval); interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); loop { // work interval.tick().await; }

instead of sleep for more accurate intervals and tick skipping behavior in case the node is overloaded

actually, infinite loops should look like this:

let mut interval = tokio::time::interval(pending_block_poll_interval); interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); while wait_or_graceful_shutdown(interval.tick()).await.is_some() {

to handle graceful shutdown of the node

Mohiiit · 2024-08-09T12:12:30Z

I am not sure if this pull request:

fetches gas prices everytime a transaction is added to the mempool / a block is produced -- the L1DataProvider functions are async

or is an infinite task running in the background, watching for l1 gas prices and updating a mutex that is shared with the mempool through the L1DataProvider abstraction -- the L1DataProvider functions are sync

which of the two does this pr implement?

it implements the second one, the functions are async because reading from mutex is async and hence all the functions inside the block production service which requires the gas fees needs to be async

The second approach has a major drawback that i'm realizing now:

we shouldnt accept any mempool transaction before the first gas prices arrive.

that's why we are getting the gas price in the L1SyncService::new

e provider

cchudant

looks good to me :)

mohiiit added 30 commits August 7, 2024 10:35

setup for eth done

b9622f9

eth client implemented

26ccabd

removing wallet from the implementation

ddc67d8

updated the locatoin of eth crate

70dba5a

linting

6400d92

refactored l1, moving it to eth crate and l1 service

20ef6e0

alloy migration of eth client done

0074f1c

using Contract direclty instead of using core contract address

2aa8e25

block metrics moved to common place, l1.rs removed, refactored l1.rs …

3845a6d

…with alloy rs

removed unused imports

efe1139

adding testing in eth crate

b04f1cb

test for event subscription added using a custom contract

e6fa74e

format and changelog updated

c081f61

added comment regarding the config rs in the eth crate

994af5b

contract updated and source of abi added

5b61a60

change log updated, lazy static removed from tests and once added for…

302199a

… simplicity

refactor: config.rs removed from the eth crate and LogStateUpdate mov…

ef5661e

…ed to state_update.rs

test added for event subscription

fd139cd

simple storage abi removed, not required

159e259

linting and formatting

ad77848

toml file formatted

5a81f8f

block metrics moved to core folder of it's lib

295ed02

block metrics divided between L1 and L2

2f2f926

feature: l1 gas price functions added

1fb2c02

tests: test added for gas price

f951114

test: gas worker failing test added

7ab7b38

tests: test done for l1 gas price

c1204e1

feat: rebase done and l1 gas price added

5e21c15

feat: L1 service added

594c62a

feat: l1 sync service added

5d4bf03

mohiiit added 6 commits August 8, 2024 16:32

refactor: L1GasPrices replaced with GasPrices

f1a22b3

feat: gas price provider created

f1fefa7

refactor: Using GasPriceProvider struct instead of direct mutex

6f8062c

refactor: data provider added for block production service

fb7c44d

feat: changelog updated

a71c7de

lint: formatting and linting

72722bb

Mohiiit marked this pull request as ready for review August 8, 2024 12:57

apoorvsadana reviewed Aug 8, 2024

View reviewed changes

mohiiit added 3 commits August 9, 2024 12:07

refactor: gas price polling moved to cli params, comment resolution

8dece6b

refactor: issue resolved

968a137

test: test added for gas worker to check infinite loop is working

c2a5331

Mohiiit requested a review from apoorvsadana August 9, 2024 09:37

cchudant requested changes Aug 9, 2024

View reviewed changes

cchudant mentioned this pull request Aug 9, 2024

Node readiness overhaul #223

Open

1 task

mohiiit added 3 commits August 9, 2024 18:33

refactor: std mutex used instead of tokio mutex

52c539c

refactor: setters removed from l1 data provider

63b7532

refactor: issue resolved

4e2ec02

Mohiiit requested a review from cchudant August 10, 2024 06:19

mohiiit and others added 7 commits August 14, 2024 14:45

tests: test specific anvil instance added

3c90d1e

Merge branch 'main' into feature/l1-gas-price

db7d28f

lint: linting and formatting

e9dfff1

refactor: last update timestamp for the gas price moved to gas pric

8e4ef19

e provider

Merge branch 'main' into feature/l1-gas-price

3fdc315

refactor: code updated as per the new chain config

56f921b

Merge branch 'main' into feature/l1-gas-price

ae75fbf

cchudant approved these changes Aug 19, 2024

View reviewed changes

apoorvsadana approved these changes Aug 19, 2024

View reviewed changes

Merge branch 'main' into feature/l1-gas-price

16d46d1

antiyro merged commit 28fe200 into madara-alliance:main Aug 21, 2024
7 checks passed


		let avg_blob_base_fee = blob_fee_history_one_hour.iter().sum::<u128>() / blob_fee_history_one_hour.len() as u128;

		let eth_gas_price = fee_history.base_fee_per_blob_gas.last().context("Setting l1 last confirmed block number")?;

	let eth_client = EthereumClient::new(l1_endpoint.unwrap(), core_address, metrics_handle)
	let eth_client = EthereumClient::new(l1_endpoint?, core_address, metrics_handle)

feat(client): l1 gas price #219

feat(client): l1 gas price #219

Conversation

Mohiiit commented Aug 7, 2024 • edited Loading

Pull Request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other Information

Current Progress

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchudant left a comment • 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

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

Mohiiit commented Aug 9, 2024

cchudant left a comment

Choose a reason for hiding this comment

Mohiiit commented Aug 7, 2024 •

edited

Loading

cchudant left a comment •

edited

Loading