-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix revert in accordance with comments #114
base: main
Are you sure you want to change the base?
Conversation
Delta Summary - Kotlin Code Coverage
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
=========================================
Coverage 70.54% 70.54%
Complexity 1042 1042
=========================================
Files 282 282
Lines 11310 11310
Branches 1028 1080 +52
=========================================
Hits 7979 7979
Misses 2861 2861
Partials 470 470
|
@@ -165,6 +165,10 @@ func (bm *BlobMaker) Write(rlpBlock []byte, forceReset bool) (ok bool, err error | |||
// 1. underlying writer error (shouldn't happen we use a simple in memory buffer) | |||
// 2. we exceed the maximum input size of 2Mb (shouldn't happen either) | |||
// In both cases, we can't do anything, so we reset the state. | |||
if innerErr := bm.compressor.Revert(); innerErr != nil { |
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.
Do we know if there are implications with the coordinator if we change that?
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.
And what are the differences between compressor.Revert()
and header.removeLastBlock()
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 already the expected behavior of the compressor. But in any case I think it's not expected to ever happen right @gbotrel?
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.
bm.compressor
only handles the "payload" part of the blob, but every block has some representation in the block header as well (how many blocks for each conflated batch) so if we want to remove a block we have to revert on both header and payload.
As per auditor feedback https://consensys.slack.com/archives/C07771UFHUG/p1725977141736399, even if unreachable, the code section should match its comments and reset.