-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add new FuelGasPriceProvider
#1889
Conversation
We might want to take into account the average number of txs per block. Or we could get a similar result with an "I" value (PID). |
*self.profitablility_totals.totaled_block_height.borrow() | ||
} | ||
|
||
fn update_totals(&self, latest_block_height: BlockHeight) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to take into account the fact that DA recording has some lag to it. Possibly 12-24 blocks? How does that effect the gas_price
calculation? The current design doesn't take that into account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented a solution for this. The DA gas price is only updated when the da recorded height changes. Might need more testing.
crates/fuel-core/src/service/adapters/fuel_gas_price_provider.rs
Outdated
Show resolved
Hide resolved
…bug in min gas price calculation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this file, it is not used anymore=)
fn gas_price( | ||
&self, | ||
params: GasPriceParams, | ||
) -> Result<u64, Box<dyn std::error::Error>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to return anyhow::Error
instead of Box<dyn ...>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so. Since our "Producer Error" is anyhow::Error
.
There are other places where I might invert the error dep for the adapters as well.
I'm torn on this in Rust in general (and have been for years), because adapters necessarily have their own error type, so you have to use type erasure. And because of that, I figured we could just put the domain errors on the business side rather than expecting the adapter to return the right error type.
For example:
let previous_gas_prices = self
.gas_price_history
.gas_prices(latest_block_height)
.map_err(Error::UnableToGetGasPrices)?
.ok_or(Error::GasPricesNotFoundForBlockHeight(latest_block_height))?;
But that's noisy, and we are already trusting the adapter code to uphold the contract of the port, so maybe we just push into the adapters instead of using this ForiegnError
construct that I used.
let new_gas_prices = self.calculate_new_gas_price(latest_block)?; | ||
self.gas_price_history | ||
.store_gas_prices(latest_block, &new_gas_prices) | ||
.map_err(Error::UnableToStoreGasPrices)?; |
There was a problem hiding this comment.
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 calculate the new gas price in this getter. We need to have a separate function that generates the next gas price without storing it in the database. We need to store it in the database when we receive a new block from BlockImporter
.
Also, maybe we want to pass the size of transactions like calculate_next_gas_price(txs_size: usize) -> u64
. But about this in another comment=)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, you're suggesting that the GasPriceProvider
trait should have more methods? Like maybe a get_gas_price
, a new_gas_price
and a write_gas_price
?
We create a new gas price, build the block, and then write it once it's built?
I don't know if the producer trait GasPriceProvider
will ever need to get historical gas prices. But some of the other traits might.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember why I'm storing it here. Because we only have one gas_price
, but under the hood we need to record the da_gas_price
and the exec_gas_price
separately.
But you're right, if that block fails for some reason, then we have wrong values. I think that's fine because we are getting latest_block_height
independently from requested_block_height
.
That means that this should never be a getter though. So it assumes that the requested block is always the head of the chain and it can remove "bad branches".
fn calculate_new_gas_price( | ||
&self, | ||
latest_block_height: BlockHeight, | ||
) -> Result<GasPrices> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the next price should be calculated without touching the database. The service itself should contain all the information in memory required to calculate the next gas price. During the service's creation, we will fetch everything that we need, and the service updates the inner state(and database as well) after each imported block. It should simplify the code and remove the return of errors from calculate_next_gas_price
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. If we are able to move the logic for updating DA information in the non-live scenarios into the algorithm, then we can get rid of database tracking outside of the algorithm.
let new_da_gas_price = if latest_da_recorded_block | ||
<= self.profitablility_totals.totaled_block_height() | ||
{ | ||
previous_gas_prices.da_gas_price |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the records are always behind for some period of time, we should always go into this branch. But since self.profitablility_totals.totaled_block_height()
depends on the number of self.update_totals
calls, it will not happen. But because of that, it creates very unintuitive code.
What do you think if we always call self.algorithm.calculate_da_gas_price
, but the self.total_reward()
and self.total_cost()
are different? We can achieve almost the same effect just by advancing reward and cost.
Because we always don't have live data from DA, we can predict the cost based on the previous blocks. We can calculate the da gas price per byte based on data from the past(total_cost/total_bytes). Using this price, we can calculate the cost for the upcoming block(txs_size * calculated_gas_price). The total_cost
is known_cost_from_the_past
+ cumulutive_predicted_cost
. Using total_cost
, you can calculate the next gas price, which will give you a reward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we always don't have live data from DA, we can predict the cost based on the previous blocks. We can calculate the da gas price per byte based on data from the past(total_cost/total_bytes). Using this price, we can calculate the cost for the upcoming block(txs_size * calculated_gas_price). The total_cost is known_cost_from_the_past + cumulutive_predicted_cost. Using total_cost, you can calculate the next gas price, which will give you a reward.
This is what Optimism does:
The L1 Data Fee is automatically charged for any transaction that is included in an OP Mainnet block. This fee is deducted directly from the address that sent the transaction. The exact amount paid depends on the estimated size of the transaction in bytes after compression, the current Ethereum gas price and/or blob gas price, and several small parameters.
We don't have the luxury of deducting from the address. And we decided to only have one gas price. So currently we calculate the gas price before we add txs to the block, when we don't know how many bytes the txs will take up in that block. Of course, we could use dry_run
to calculate the max fee, so that's still an option if we want to separate them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also relevant from Optimism:
It is currently not possible to limit the maximum L1 Data Fee that a transaction is willing to pay. This is the result of limitations in existing Ethereum transaction formats and tradeoffs that the OP Stack makes to retain better EVM equivalence. Work is underway to improve this situation with the introduction of a new standardized transaction type. For now, you should be aware that the L1 Data Fee can fluctuate with the Ethereum gas price.
) | ||
}; | ||
|
||
let block_fullness = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the previous block's fullness should be part of the GasProviderService
. On each block imported from the block importer, the service will update its internal fullness variables and dump new values into the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean GasProviderService
. Right now the producer, the txpool, and graphql each get a copy of the impl GasPriceProvider for StaticGasPrice
.
That's how I imagine it will still work, but just with this new FuelGasPriceProvider
instead of StaticGasPrice
. I'm assuming that all the GasPriceProvider
needs is a BlockHeight
and then it has access to all the DBs it needs to assemble the gas price. I don't know why it needs to store block_fullness
when it can just look up the block's data in its DB.
crates/fuel-core/src/service/adapters/fuel_gas_price_provider/algorithm_adapter.rs
Outdated
Show resolved
Hide resolved
// TODO: Can we make this mutable? The problem is that the `GasPriceProvider` isn't mut, so | ||
// we can't hold mutable references. Might be able to make `GasPriceProvider` `&mut self` but will | ||
// take some finangling | ||
fn store_gas_prices( | ||
&self, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it should be mutable. The gas price provider service should have exclusive ownership of the database(we can create a separate database for it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gas price provider service
I don't think of this as being a service. I just thing of it as being an adapter handed to the producer, txpool, and graphql like here:
fuel-core/crates/services/producer/src/block_producer.rs
Lines 124 to 127 in acd8d19
let gas_price = self | |
.gas_price_provider | |
.gas_price(height.into()) | |
.ok_or(anyhow!("No gas price found for block {height:?}"))?; |
moving_average_profit: RefCell<i64>, | ||
last_profit: RefCell<i64>, | ||
profit_slope: RefCell<i64>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need RefCell
? If you need to update the internal state, then let's use &mut
=)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Kinda related to the TODO
above.
let old = *self.moving_average_profit.borrow(); | ||
*self.moving_average_profit.borrow_mut() = (old | ||
* (self.moving_average_window - 1)) | ||
.saturating_add(new_profit) | ||
.saturating_div(self.moving_average_window); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it seems that after restarting the node self.moving_average_profit
and self.last_profit
will be zero, which means the algorithm will work differently. We need to persist all information required for the algorithm.
Hmm, it would be nice if we used one iteration formula to avoid a persistent state. But I assume you haven't found any suitable for us=)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe
old_da_gas_price: u64,
old_total_production_reward: u64,
old_total_da_recording_cost: u64,
can be stored inside of the algorithm and passed during creation. Then we should be able to calculate profit form them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great point. Configuring of that might be a pain if the services aren't running when we start the algorithm, but it would be a solution. But how do we know what values it died on?
Another solution is we have an assoc type for the algo trait that the algo returns with arbitrary metadata. The type needs to match some data storage on the provider at compilation time, and we can have that type be versioned so the data storage can still be backwards compatible. Seems like a lot of trouble lol.
impl<FB, DA, A, GP> FuelGasPriceProvider<FB, DA, A, GP>
where
FB: FuelBlockHistory,
DA: DARecordingCostHistory,
A: GasPriceAlgorithm<Metadata=<Self as GasPriceHistory>::Metadata>,
GP: GasPriceHistory,
{
I don't know if that is compatible with what you were saying about recording the gas price after we receive the block from the importer.
Of course we can add storage to the algorithm--which is just so many traits
🐢
🐢
🐢
🐢
I'd suggest that we just merge the algo abstraction into the provider, but I think that would make versioning harder.
I feel like there is a cleaner solution here.
} else if Self::asking_for_next_block(latest_block, requested_block_height) { | ||
let new_gas_prices = self.calculate_new_gas_price(latest_block)?; | ||
self.gas_price_history | ||
.store_gas_prices(requested_block_height, &new_gas_prices) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was store_gas_prices(latest_block)
before and the tests were still passing. Fixing it didn't break the tests either, so the coverage needs to be updated if we want to keep this approach.
## Version v0.29.0 ### Added - [#1889](#1889): Add new `FuelGasPriceProvider` that receives the gas price algorithm from a `GasPriceService` ### Changed - [#1942](#1942): Sequential relayer's commits. - [#1952](#1952): Change tip sorting to ratio between tip and max gas sorting in txpool - [#1960](#1960): Update fuel-vm to v0.53.0. ### Fixed - [#1950](#1950): Fix cursor `BlockHeight` encoding in `SortedTXCursor` ## What's Changed * Fix code coverage compilation and tests by @Dentosal in #1943 * Weekly `cargo update` by @github-actions in #1949 * Fix cursor block height decoding in SortedTXCursor by @AurelienFT in #1950 * Sequential relayer's commits by @xgreenx in #1942 * Add Gas Price Updater Service by @MitchTurner in #1938 * Change tip sorting to ratio between tip and max gas sorting in txpool by @AurelienFT in #1952 * deps(client): update eventsource-client to fix CVE(s) by @Br1ght0ne in #1954 * Update fuel-vm to v0.53.0 by @Dentosal in #1960 ## New Contributors * @AurelienFT made their first contribution in #1950 **Full Changelog**: v0.28.0...v0.29.0
Closed in favor of other smaller PRs |
Part of: #1624
Here is the current specs for the gas price algorithm:
https://www.notion.so/fuellabs/Gas-Price-Algo-V1-Specs-2e7a06263c4f46928b6d179e90a4ba1a
Introduce a new type to replace
StaticGasPrice
that will actually calculate new gas prices based on historical data.This work is largely exploratory since we don't know exactly what the algorithm will land on. But this PR includes work defining the architecture of our
FuelGasPriceProvider
as well as some exploratory work defining a proof-of-concept algorithm. This is the main reason this PR is still in "Draft".The current design separates the algorithm from the
FuelBlockProvider
. This helps identify what behavior is independent of the specifc algorithm we end up choosing and helps us determine the minimum set of variables the algorithm requires. Currently the algorithm looks like this:The actual impllementation of the
FuelGasPriceAlgo
for now is going to take inspiration from a PID control loop on the DA gas price side, but ideally as simple as possible and as deterministic as possible (we don't want to use floats in places likeBlockFullness
).Architecture Diagram (WIP, I'll try to update as it changes). CLICK IF NOT SHOWING UP PROPERLY IN DARK MODE:

Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]