Fix nil batchFetcher dereference in FillInBatchGasFields#4540
Open
joshuacolvin0 wants to merge 15 commits intomasterfrom
Open
Fix nil batchFetcher dereference in FillInBatchGasFields#4540joshuacolvin0 wants to merge 15 commits intomasterfrom
joshuacolvin0 wants to merge 15 commits intomasterfrom
Conversation
… is nil Guard against nil batchFetcher in FillInBatchGasFields before passing it to FromFallibleBatchFetcher, which wraps a nil function in a non-nil closure that bypasses the downstream nil check and panics when called. Also skip FillInBatchGasFields in ParseIncomingL1Message when batchFetcher is nil. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4540 +/- ##
==========================================
- Coverage 34.52% 34.08% -0.45%
==========================================
Files 498 497 -1
Lines 59092 59169 +77
==========================================
- Hits 20400 20165 -235
- Misses 35067 35439 +372
+ Partials 3625 3565 -60 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
❌ 11 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
tsahee
requested changes
Mar 23, 2026
| if err != nil { | ||
| return nil, err | ||
| if batchFetcher != nil { | ||
| err = msg.FillInBatchGasFields(batchFetcher) |
Contributor
There was a problem hiding this comment.
nil check should be in FillInBatchGasFields, so that if the fields need to be filled the function will error out and not silently create wrong messages.
Both FillInBatchGasFields and FillInBatchGasFieldsWithParentBlock now return an error when batchFetcher is nil and the message kind is BatchPostingReport, instead of silently leaving gas fields unpopulated. Move the nil guard to the caller in delayed.go and pass the fetcher directly in inbox_tracker.go. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Let the nil check live in FillInBatchGasFieldsWithParentBlock so that FillInBatchGasFields delegates without duplicating the logic. This way a nil fetcher only errors when the message is a BatchPostingReport and the fields actually need to be filled, rather than being checked upfront. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Remove caller-side nil guards for batchFetcher Now that FillInBatchGasFieldsWithParentBlock handles nil batchFetcher internally, remove the redundant nil guards in delayed.go and transaction_streamer.go so that all call sites go through the centralized check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Provide real fetcher in reorg path and add tests for fetcher error paths Wire up a real batchFetcher in the reorg-resequence LookupMessagesInRange call so BatchPostingReport messages get their gas fields filled. Add tests for fetcher error fallback: when LegacyBatchGasCost is already set the function succeeds (pre-arbos50 compat), and when neither field is set the fetcher error is propagated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Add tests for pre-set BatchDataStats and truncated L2msg edge cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test that FillInBatchGasFields populates BatchDataStats and LegacyBatchGasCost with a working fetcher, and that it passes msg.Header.BlockNumber as parentChainBlockNumber through the FromFallibleBatchFetcher wrapper. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ntBlock Include parentChainBlockNumber in the nil-fetcher error and add batchNum and err to the batch data fetch warning for easier debugging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bragaigor
previously approved these changes
Mar 30, 2026
- Add ErrNilBatchFetcher, ErrBatchHashMismatch, ErrParseBatchPostingReport sentinel errors with %w wrapping for programmatic error checking - Remove redundant nil guard in delayed.go since FillInBatchGasFieldsWithParentBlock handles non-BatchPostingReport - Add edge-case tests: nil fetcher data, empty batch, idempotent calls, WithParentBlock nil data, and all non-BatchPostingReport message kinds - Add warning logs for swallowed FillInBatchGasFields errors in replay binary and GetMessage; fix misleading reorg log message - Use log.Error instead of log.Warn in replay binary (log level filtering) - Provide stub batchFetcher in init.go to satisfy non-nil contract Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Provide non-nil batchFetcher in system tests that call LookupMessagesInRange - Add delayedMessagesRead to replay log.Error for debuggability - Trim verbose comments in init.go and remove redundant comment block in transaction_streamer.go - Minor test comment fixes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
@copilot please review |
…, revert test stubs to nil Agent-Logs-Url: https://github.com/OffchainLabs/nitro/sessions/184c0914-dd59-4497-a54d-b33a83f9d832 Co-authored-by: bragaigor <5835477+bragaigor@users.noreply.github.com>
I found and fixed a CI failure introduced by the previous commit. The root cause was that Fix in commit
|
Copilot
AI
changed the title
Fix nil batchFetcher dereference
Fix nil batchFetcher dereference in FillInBatchGasFields
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FillInBatchGasFields(nil)panicked forBatchPostingReportmessages becauseFromFallibleBatchFetcher(nil)produced a non-nil closure that bypassed the downstream nil guard and panicked on invocation.Changes
FillInBatchGasFields: explicit nil check — returnsErrNilBatchFetcherforBatchPostingReport, nil for all other kinds; only wraps non-nil fetchers viaFromFallibleBatchFetcherFillInBatchGasFieldsWithParentBlock: restores original nil→skip behavior (return nil) soLookupMessagesInRangecallers that don't need batch gas fields can safely pass nil without failing on incidentalBatchPostingReportmessages in the same blockarbnode/inbox_tracker.go,arbnode/transaction_streamer.go: pass real fetchers instead of nil at call sites that processBatchPostingReportmessagescmd/nitro/init/init.go: defensive stub fetcher for the deployment block (noBatchPostingReportexpected there); updated commentErrNilBatchFetcher,ErrBatchHashMismatch,ErrParseBatchPostingReportwith%wwrapping for programmatic checkingGetMessagewithoutinboxReader)Behavioral contract
No consensus changes — replay binary behavior is identical.