Skip to content

fix(op-node): Fix Channel Out MaxSize Underflow#5187

Merged
mergify[bot] merged 14 commits intodevelopfrom
refcell/fix/channelo
Mar 21, 2023
Merged

fix(op-node): Fix Channel Out MaxSize Underflow#5187
mergify[bot] merged 14 commits intodevelopfrom
refcell/fix/channelo

Conversation

@refcell
Copy link
Contributor

@refcell refcell commented Mar 17, 2023

Description

Fixes CLI-3649

Context in: #5186. In short, the maxSize parameter of the channel out's OutputFrame function is unchecked so the 23 frame overhead size will underflow, resulting in a nearly max uint64 frame size when a maxSize parameter of < 23 is provided.

@changeset-bot
Copy link

changeset-bot bot commented Mar 17, 2023

⚠️ No Changeset found

Latest commit: ba3e76e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@refcell refcell self-assigned this Mar 17, 2023
@refcell refcell requested a review from sebastianst March 17, 2023 18:55
@netlify
Copy link

netlify bot commented Mar 17, 2023

Deploy Preview for opstack-docs ready!

Name Link
🔨 Latest commit ba3e76e
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/6419db393e878300070dcf2d
😎 Deploy Preview https://deploy-preview-5187--opstack-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@refcell refcell requested a review from a team as a code owner March 17, 2023 19:41
@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #5187 (c237957) into develop (568f080) will decrease coverage by 4.03%.
The diff coverage is 100.00%.

❗ Current head c237957 differs from pull request most recent head ba3e76e. Consider uploading reports for the commit ba3e76e to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5187      +/-   ##
===========================================
- Coverage    41.60%   37.58%   -4.03%     
===========================================
  Files          358      216     -142     
  Lines        22393    18193    -4200     
  Branches       776        0     -776     
===========================================
- Hits          9317     6837    -2480     
+ Misses       12368    10686    -1682     
+ Partials       708      670      -38     
Flag Coverage Δ
bedrock-go-tests 37.58% <100.00%> (-0.11%) ⬇️
contracts-bedrock-tests ?
contracts-tests ?
core-utils-tests ?
dtl-tests ?
fault-detector-tests ?
sdk-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
op-node/rollup/derive/channel_out.go 77.97% <100.00%> (+0.13%) ⬆️

... and 154 files with indirect coverage changes

Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

2 small nits, looks good otherwise ✨

@refcell refcell requested a review from sebastianst March 17, 2023 21:32
Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

There's a lot of nice cleanup changes in here, but those are unrelated to primary concern of the PR which is fixing the MaxSizeUnderflow. Those changes should be split out to a new PR.

@refcell refcell requested a review from trianglesphere March 17, 2023 23:42
@mergify
Copy link
Contributor

mergify bot commented Mar 20, 2023

Hey @refcell! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Mar 20, 2023
@refcell refcell requested a review from sebastianst March 20, 2023 19:14
Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

Looking good now! Would be nice to still see the remaining tests that got refactored also get a proper test name.

@refcell refcell requested a review from sebastianst March 21, 2023 14:50
@sebastianst sebastianst dismissed trianglesphere’s stale review March 21, 2023 14:54

review got addressed

@sebastianst sebastianst requested review from trianglesphere and removed request for trianglesphere March 21, 2023 15:25
@mergify
Copy link
Contributor

mergify bot commented Mar 21, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Contributor

mergify bot commented Mar 21, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify mergify bot merged commit 0aab6a5 into develop Mar 21, 2023
@mergify mergify bot deleted the refcell/fix/channelo branch March 21, 2023 16:46
@mergify mergify bot removed the on-merge-train label Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants