-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Allow DA recorded blocks to come out-of-order #2415
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
Changes from 8 commits
448d8c2
c5866a3
c54f098
fcf4477
159d5b4
ba080ff
d2b3c28
128a9ba
5f19932
922f3e0
d7e9bdd
12c517e
f8dca06
2bb86fe
60771e9
03da963
ea14e6f
b615b33
cfd1a71
d983891
4c2b844
48fe978
b578a8e
7ca55c7
6ac41de
f2af4aa
cc90cbd
7ac01aa
9f78fa6
a2c0125
57ed9db
eff76bf
4d5409a
4e8df28
757896a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,10 +3,7 @@ use std::{ | |||||
cmp::max, | ||||||
collections::BTreeMap, | ||||||
num::NonZeroU64, | ||||||
ops::{ | ||||||
Div, | ||||||
Range, | ||||||
}, | ||||||
ops::Div, | ||||||
}; | ||||||
|
||||||
#[cfg(test)] | ||||||
|
@@ -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:?}")] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated, but this could be just |
||||||
L2BlockExpectedNotFound { height: u32 }, | ||||||
} | ||||||
|
||||||
// TODO: separate exec gas price and DA gas price into newtypes for clarity | ||||||
|
@@ -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 | ||||||
|
@@ -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(()) | ||||||
|
@@ -514,48 +507,32 @@ 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_range(heights)?; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just:
Suggested change
It'll save us from renaming the function when we change the datatype again. However, the intention here could be to not relate to the Either way, just a small detail. |
||||||
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( | ||||||
&mut self, | ||||||
height_range: Range<u32>, | ||||||
) -> Result<u128, Error> { | ||||||
fn drain_l2_block_bytes_for_range(&mut self, heights: &[u32]) -> Result<u128, Error> { | ||||||
netrome marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
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, | ||||||
}, | ||||||
)?; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been thinking about the (very unlikely) case that our gas price metadata is "ahead" of the chain on restart. This could in theory result in a bunch of the blocks being missing from the Since the precision of the algorithm isn't that important, I think the simplest solution would be to just ignore re-reported blocks. In that case we don't want it throwing an error here. So I'm going to change this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmmmmmmm.... Well it's kinda weird. We don't get the values for each committed block, we just get a value for the entire bundle. This means, for example if the We can take 1/8 of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In terms of the algorithm, I think the best approach here is to ignore the batch. The problem is if it ever occurs, we will carry those There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Nevermind. We can both remove the blocks and ignore. I think that's the best. |
||||||
total = total.saturating_add(bytes as u128); | ||||||
} | ||||||
Ok(total) | ||||||
|
@@ -566,7 +543,7 @@ impl AlgorithmUpdaterV1 { | |||||
let projection_portion: u128 = self | ||||||
.unrecorded_blocks | ||||||
.iter() | ||||||
.map(|(_, &bytes)| (bytes as u128)) | ||||||
.map(|(_, &bytes)| bytes as u128) | ||||||
netrome marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
.fold(0_u128, |acc, n| acc.saturating_add(n)) | ||||||
.saturating_mul(self.latest_da_cost_per_byte); | ||||||
self.projected_total_da_cost = self | ||||||
|
Uh oh!
There was an error while loading. Please reload this page.