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

feat(katana): l1 gas sampling #2652

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

augustin-v
Copy link

Description

Gas oracle on katana related to issue #2558

Designed a 60 capacity FIFO buffer with a worker that runs every 60 seconds following the starknet fee calculation.

Problems encountered:

  • Wasn't able to implement the actual logic of when to decide whether to use hardcoded values or values sampled by the oracle, as well as the worker initiation.

Copy link
Member

@kariy kariy left a comment

Choose a reason for hiding this comment

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

Wasn't able to implement the actual logic of when to decide whether to use hardcoded values or values sampled by the oracle, as well as the worker initiation.

So the idea is, by default it should always use the proper sampling mechanism. Which means the user has to specify the L1 provider url. Soon this will be a required argument because it is necessary for other stuff to function correctly as well. But right now, the default would always be hardcoded, unless an L1 provider url is supplied.

So we should add a new cli arg, --l1.provider, and use this to determine the sampling logic.

lets turn the L1GasOracle into an enum and implements the current_gas_prices() and current_data_gas_prices() for both variants.

pub enum L1GasOracle {
    Fixed(FixedL1GasOracle),
    Sampled(SampledL1GasOracle),
}

The component should be built inside of

/// Build the node components from the given [`Config`].
///
/// This returns a [`Node`] instance which can be launched with the all the necessary components
/// configured.
pub async fn build(mut config: Config) -> Result<Node> {
if config.metrics.is_some() {
// Metrics recorder must be initialized before calling any of the metrics macros, in order
// for it to be registered.

But the worker task should only be executed in

/// Start the node.
///
/// This method will start all the node process, running them until the node is stopped.
pub async fn launch(mut self) -> Result<LaunchedNode> {
let chain = self.backend.chain_spec.id;
info!(%chain, "Starting node.");
// TODO: maybe move this to the build stage

So what we shouldn't do is start the worker task automatically after creating the component and instead have a way to start the task manually on the call to .launch().

Let's create a concrete type for the worker task, that implements a Future. Which we can then spawn onto the task manager.

We should also add this new struct into the Node struct

pub struct Node {
pub pool: TxPool,
pub db: Option<DbEnv>,
pub task_manager: TaskManager,
pub backend: Arc<Backend<BlockifierFactory>>,
pub block_producer: BlockProducer<BlockifierFactory>,
pub rpc_config: RpcConfig,
pub metrics_config: Option<MetricsConfig>,
pub sequencing_config: SequencingConfig,
pub messaging_config: Option<MessagingConfig>,
forked_client: Option<ForkedClient>,
}

Comment on lines +37 to +63
pub async fn gas_price_worker(
&mut self,
shutdown_rx: &mut watch::Receiver<()>,
) -> anyhow::Result<()> {
let mut buffer = GasPriceBuffer::new();
let rpc_url = "https://eth.merkle.io".parse()?;
let provider = ProviderBuilder::new().on_http(rpc_url);
// every 60 seconds, Starknet samples the base price of gas and data gas on L1
let mut interval = tokio::time::interval(INTERVAL);
interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip);

loop {
tokio::select! {
_ = shutdown_rx.changed() => {
break;
}
// tick every 60 seconds
_ = interval.tick() => {
if let Err(e) = update_gas_price(self, provider.clone(), &mut buffer).await {
eprintln!("Error updating gas price: {}", e);
}
}
}
}

Ok(())
}
Copy link
Member

@kariy kariy Nov 7, 2024

Choose a reason for hiding this comment

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

We don't need to handle cancellation manually here, the task manager already handles that for us. You can simply just spawn the task on it.

Example:

let server = MetricsServer::new(exporter).with_process_metrics().with_reports(reports);
self.task_manager.task_spawner().build_task().spawn(server.start(cfg.addr));
info!(addr = %cfg.addr, "Metrics server started.");

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much for the detailed review @kariy. This is incredibly helpful I’ll start making the suggested changes right away and will let you know when the updates are ready for another look. Thanks again

@glihm glihm added katana This issue is related to Katana contributor labels Nov 7, 2024
@kariy kariy changed the title WIP: progress on issue #2558 gas oracle feature on katana feat(katana): l1 gas sampling Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor katana This issue is related to Katana
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants