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

Allow DA recorded blocks to come out-of-order #2415

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,14 @@ pub fn draw_bytes_and_cost_per_block(
Ok(())
}

fn one_gwei_i128() -> i128 {
i128::from(ONE_GWEI)
}

fn one_gwei_u128() -> u128 {
u128::from(ONE_GWEI)
}

pub fn draw_profit(
drawing_area: &DrawingArea<BitMapBackend, Shift>,
actual_profit: &[i128],
Expand All @@ -267,17 +275,15 @@ pub fn draw_profit(
const ACTUAL_PROFIT_COLOR: RGBColor = BLACK;
const PROJECTED_PROFIT_COLOR: RGBColor = RED;
const PESSIMISTIC_BLOCK_COST_COLOR: RGBColor = BLUE;
let actual_profit_gwei: Vec<_> = actual_profit
.into_iter()
.map(|x| x / ONE_GWEI as i128)
.collect();
let actual_profit_gwei: Vec<_> =
actual_profit.iter().map(|x| x / one_gwei_i128()).collect();
let projected_profit_gwei: Vec<_> = projected_profit
.into_iter()
.map(|x| x / ONE_GWEI as i128)
.iter()
.map(|x| x / one_gwei_i128())
.collect();
let pessimistic_block_costs_gwei: Vec<_> = pessimistic_block_costs
.into_iter()
.map(|x| x / ONE_GWEI as u128)
.iter()
.map(|x| x / one_gwei_u128())
.collect();
let min = *std::cmp::min(
actual_profit_gwei
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ impl Simulator {
// Increase to make the da price change faster
max_da_gas_price_change_percent: 10,
total_da_rewards_excess: 0,
da_recorded_block_height: 0,
// Change to adjust the cost per byte of the DA on block 0
latest_da_cost_per_byte: 0,
projected_total_da_cost: 0,
Expand Down Expand Up @@ -162,9 +161,8 @@ impl Simulator {
// Update DA blocks on the occasion there is one
if let Some((range, cost)) = da_block {
for height in range.to_owned() {
updater
.update_da_record_data(height..(height + 1), cost)
.unwrap();
let block_heights: Vec<u32> = (height..(height) + 1).collect();
updater.update_da_record_data(&block_heights, cost).unwrap();
actual_costs.push(updater.latest_known_total_da_cost_excess)
}
}
Expand Down
82 changes: 31 additions & 51 deletions crates/fuel-gas-price-algorithm/src/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@ use std::{
cmp::max,
collections::BTreeMap,
num::NonZeroU64,
ops::{
Div,
Range,
},
ops::Div,
};

#[cfg(test)]
Expand All @@ -16,14 +13,12 @@ mod tests;
pub enum Error {
#[error("Skipped L2 block update: expected {expected:?}, got {got:?}")]
SkippedL2Block { expected: u32, got: u32 },
#[error("Skipped DA block update: expected {expected:?}, got {got:?}")]
SkippedDABlock { expected: u32, got: u32 },
#[error("Could not calculate cost per byte: {bytes:?} bytes, {cost:?} cost")]
CouldNotCalculateCostPerByte { bytes: u128, cost: u128 },
#[error("Failed to include L2 block data: {0}")]
FailedTooIncludeL2BlockData(String),
#[error("L2 block expected but not found in unrecorded blocks: {0}")]
L2BlockExpectedNotFound(u32),
#[error("L2 block expected but not found in unrecorded blocks: {height}")]
L2BlockExpectedNotFound { height: u32 },
}

// TODO: separate exec gas price and DA gas price into newtypes for clarity
Expand Down Expand Up @@ -131,8 +126,6 @@ pub struct AlgorithmUpdaterV1 {
pub max_da_gas_price_change_percent: u16,
/// The cumulative reward from the DA portion of the gas price
pub total_da_rewards_excess: u128,
/// The height of the last L2 block recorded on the DA chain
pub da_recorded_block_height: u32,
/// The cumulative cost of recording L2 blocks on the DA chain as of the last recorded block
pub latest_known_total_da_cost_excess: u128,
/// The predicted cost of recording L2 blocks on the DA chain as of the last L2 block
Expand Down Expand Up @@ -307,11 +300,11 @@ impl core::ops::Deref for ClampedPercentage {
impl AlgorithmUpdaterV1 {
pub fn update_da_record_data(
&mut self,
height_range: Range<u32>,
range_cost: u128,
heights: &[u32],
netrome marked this conversation as resolved.
Show resolved Hide resolved
recording_cost: u128,
) -> Result<(), Error> {
if !height_range.is_empty() {
self.da_block_update(height_range, range_cost)?;
if !heights.is_empty() {
self.da_block_update(heights, recording_cost)?;
self.recalculate_projected_cost();
}
Ok(())
Expand Down Expand Up @@ -514,48 +507,35 @@ impl AlgorithmUpdaterV1 {

fn da_block_update(
&mut self,
height_range: Range<u32>,
range_cost: u128,
heights: &[u32],
recording_cost: u128,
) -> Result<(), Error> {
let expected = self.da_recorded_block_height.saturating_add(1);
let first = height_range.start;
if first != expected {
Err(Error::SkippedDABlock {
expected,
got: first,
})
} else {
let last = height_range.end.saturating_sub(1);
let range_bytes = self.drain_l2_block_bytes_for_range(height_range)?;
let new_cost_per_byte: u128 = range_cost.checked_div(range_bytes).ok_or(
Error::CouldNotCalculateCostPerByte {
bytes: range_bytes,
cost: range_cost,
},
)?;
self.da_recorded_block_height = last;
let new_da_block_cost = self
.latest_known_total_da_cost_excess
.saturating_add(range_cost);
self.latest_known_total_da_cost_excess = new_da_block_cost;
self.latest_da_cost_per_byte = new_cost_per_byte;
Ok(())
}
let recorded_bytes = self.drain_l2_block_bytes_for_heights(heights)?;
let new_cost_per_byte: u128 = recording_cost.checked_div(recorded_bytes).ok_or(
Error::CouldNotCalculateCostPerByte {
bytes: recorded_bytes,
cost: recording_cost,
},
)?;
let new_da_block_cost = self
.latest_known_total_da_cost_excess
.saturating_add(recording_cost);
self.latest_known_total_da_cost_excess = new_da_block_cost;
self.latest_da_cost_per_byte = new_cost_per_byte;
Ok(())
}

fn drain_l2_block_bytes_for_range(
fn drain_l2_block_bytes_for_heights(
&mut self,
height_range: Range<u32>,
heights: &[u32],
) -> Result<u128, Error> {
let mut total: u128 = 0;
for expected_height in height_range {
let (actual_height, bytes) = self
.unrecorded_blocks
.pop_first()
.ok_or(Error::L2BlockExpectedNotFound(expected_height))?;
if actual_height != expected_height {
return Err(Error::L2BlockExpectedNotFound(expected_height));
}
for expected_height in heights {
let bytes = self.unrecorded_blocks.remove(expected_height).ok_or(
Error::L2BlockExpectedNotFound {
height: *expected_height,
},
)?;
Comment on lines +539 to +543
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need an error here? We could log error here and use 0=)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's up to us. If this errors we have a serious problem with our code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the committer has problems and submits several bundles per same height(they can in theory because they do re-bundle), it will break the node.

Copy link
Member Author

Choose a reason for hiding this comment

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

That can't happen if it's finalized before they report to us.

total = total.saturating_add(bytes as u128);
}
Ok(total)
Expand All @@ -566,7 +546,7 @@ impl AlgorithmUpdaterV1 {
let projection_portion: u128 = self
.unrecorded_blocks
.iter()
.map(|(_, &bytes)| (bytes as u128))
.map(|(_, &bytes)| u128::from(bytes))
.fold(0_u128, |acc, n| acc.saturating_add(n))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need to iterate each time here? Just curios why not to use structure that does accounting internally and updates values once per add/remove events.

Copy link
Member Author

Choose a reason for hiding this comment

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

This gets called semi-infrequently (only when we get a batch back from the committer). It could become more frequent I guess.

Yes. An alternative would be having the "total unrecorded bytes" tracked and when we remove blocks we subtract their bytes from that total. Then this would just be a multiplication of that. I can add that if you prefer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

.saturating_mul(self.latest_da_cost_per_byte);
self.projected_total_da_cost = self
Expand Down
8 changes: 0 additions & 8 deletions crates/fuel-gas-price-algorithm/src/v1/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ pub struct UpdaterBuilder {
l2_block_capacity_threshold: u8,

total_rewards: u128,
da_recorded_block_height: u32,
da_cost_per_byte: u128,
project_total_cost: u128,
latest_known_total_cost: u128,
Expand Down Expand Up @@ -63,7 +62,6 @@ impl UpdaterBuilder {
l2_block_capacity_threshold: 50,

total_rewards: 0,
da_recorded_block_height: 0,
da_cost_per_byte: 0,
project_total_cost: 0,
latest_known_total_cost: 0,
Expand Down Expand Up @@ -133,11 +131,6 @@ impl UpdaterBuilder {
self
}

fn with_da_recorded_block_height(mut self, da_recorded_block_height: u32) -> Self {
self.da_recorded_block_height = da_recorded_block_height;
self
}

fn with_da_cost_per_byte(mut self, da_cost_per_byte: u128) -> Self {
self.da_cost_per_byte = da_cost_per_byte;
self
Expand Down Expand Up @@ -184,7 +177,6 @@ impl UpdaterBuilder {
l2_block_fullness_threshold_percent: self.l2_block_capacity_threshold.into(),
total_da_rewards_excess: self.total_rewards,

da_recorded_block_height: self.da_recorded_block_height,
latest_da_cost_per_byte: self.da_cost_per_byte,
projected_total_da_cost: self.project_total_cost,
latest_known_total_da_cost_excess: self.latest_known_total_cost,
Expand Down
Loading
Loading