feat: solo sequencer, testapp max blob size ldflags#3235
feat: solo sequencer, testapp max blob size ldflags#3235julienrbrt wants to merge 10 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a solo sequencer—a lightweight, in-memory single sequencer—alongside changes that make Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SoloSequencer
participant Executor
Client->>SoloSequencer: SubmitBatchTxs(transactions)
SoloSequencer->>SoloSequencer: Validate batch ID
SoloSequencer->>SoloSequencer: Append to queue
SoloSequencer-->>Client: SubmitBatchTxsResponse
Client->>SoloSequencer: GetNextBatch()
SoloSequencer->>SoloSequencer: Drain queue
SoloSequencer->>Executor: GetExecutionInfo(MaxGas)
Executor-->>SoloSequencer: ExecutionInfo
SoloSequencer->>Executor: FilterTxs(transactions)
Executor-->>SoloSequencer: valid txs + postponed txs
SoloSequencer->>SoloSequencer: Re-queue postponed txs
SoloSequencer-->>Client: GetNextBatchResponse(validTxs)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task in 2m 43s —— View job Code Review:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3235 +/- ##
==========================================
+ Coverage 61.67% 61.79% +0.11%
==========================================
Files 120 122 +2
Lines 12635 12709 +74
==========================================
+ Hits 7793 7853 +60
- Misses 3968 3977 +9
- Partials 874 879 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/sequencers/solo/sequencer.go (1)
35-46: Add a doc comment for exported constructorNewSoloSequencer.The exported constructor should be documented for lint/readability consistency.
As per coding guidelines "Document exported types and functions in Go".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sequencers/solo/sequencer.go` around lines 35 - 46, Add a doc comment above the exported constructor NewSoloSequencer describing what it returns and the purpose of its parameters (logger zerolog.Logger, id []byte, executor execution.Executor) and any important behavior (e.g., initializes an empty queue). Mention that it constructs and returns a *SoloSequencer. Place the comment directly above the NewSoloSequencer function so it satisfies Go doc/lint rules.pkg/sequencers/solo/sequencer_test.go (1)
49-219: Refactor repetitive cases into table-driven tests.The validation-path tests are currently split into many near-identical functions; table-driven form would reduce duplication and align with repo test conventions.
As per coding guidelines "Use table-driven tests in Go unit tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sequencers/solo/sequencer_test.go` around lines 49 - 219, Several test functions (e.g., TestSoloSequencer_SubmitBatchTxs_InvalidID, TestSoloSequencer_SubmitBatchTxs_EmptyBatch, TestSoloSequencer_GetNextBatch_EmptyQueue, TestSoloSequencer_GetNextBatch_InvalidID) are repetitive; convert them into table-driven tests. Create one or more table-driven test functions that iterate cases (name, setup using newTestSequencer, inputs for SubmitBatchTxs/GetNextBatch/VerifyBatch, expected error or result) and run subtests with t.Run; keep existing helpers and unique symbols (newTestSequencer, SubmitBatchTxs, GetNextBatch, VerifyBatch, GetDAHeight, SetDAHeight) to locate behavior, and assert expected outcomes per case to remove duplicated test bodies while preserving assertions and mocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@block/internal/common/consts.go`:
- Around line 14-20: The init function currently swallows ParseUint errors and
zero values for defaultMaxBlobSizeStr and silently falls back to 5MiB; change it
to fail-fast by replacing the silent fallback with a startup failure that
includes the offending value and parse error: in init(), after calling
strconv.ParseUint(defaultMaxBlobSizeStr, 10, 64), if err != nil or v == 0, call
log.Fatalf or panic with a clear message (e.g., include defaultMaxBlobSizeStr
and err) instead of setting DefaultMaxBlobSize to 5*1024*1024 so malformed or
zero ldflags overrides immediately stop startup; keep setting DefaultMaxBlobSize
= v on success.
In `@CHANGELOG.md`:
- Line 15: Update the changelog line that reads "Add solo sequencer (simple in
memory single sequencer without force inclusion) [`#3235`]" to use the hyphenated
compound adjective "in-memory" (e.g., "Add solo sequencer (simple in-memory
single sequencer without force inclusion) [`#3235`]") to match project style and
consistency.
In `@pkg/sequencers/solo/sequencer.go`:
- Around line 94-113: The code assumes s.executor.FilterTxs returned a
filterStatuses slice matching len(txs); add a strict length check after calling
s.executor.FilterTxs to guard against mismatches: if len(filterStatuses) !=
len(txs) log an error via s.logger and fail early by returning an error (or the
enclosing function's appropriate error response) instead of proceeding to index
into filterStatuses; update any fallback branch that previously created a
default filterStatuses to use the same length check/early-return behavior so you
never iterate with mismatched lengths (refer to s.executor.FilterTxs,
filterStatuses, txs, and the switch on
execution.FilterOK/FilterPostpone/FilterRemove).
---
Nitpick comments:
In `@pkg/sequencers/solo/sequencer_test.go`:
- Around line 49-219: Several test functions (e.g.,
TestSoloSequencer_SubmitBatchTxs_InvalidID,
TestSoloSequencer_SubmitBatchTxs_EmptyBatch,
TestSoloSequencer_GetNextBatch_EmptyQueue,
TestSoloSequencer_GetNextBatch_InvalidID) are repetitive; convert them into
table-driven tests. Create one or more table-driven test functions that iterate
cases (name, setup using newTestSequencer, inputs for
SubmitBatchTxs/GetNextBatch/VerifyBatch, expected error or result) and run
subtests with t.Run; keep existing helpers and unique symbols (newTestSequencer,
SubmitBatchTxs, GetNextBatch, VerifyBatch, GetDAHeight, SetDAHeight) to locate
behavior, and assert expected outcomes per case to remove duplicated test bodies
while preserving assertions and mocks.
In `@pkg/sequencers/solo/sequencer.go`:
- Around line 35-46: Add a doc comment above the exported constructor
NewSoloSequencer describing what it returns and the purpose of its parameters
(logger zerolog.Logger, id []byte, executor execution.Executor) and any
important behavior (e.g., initializes an empty queue). Mention that it
constructs and returns a *SoloSequencer. Place the comment directly above the
NewSoloSequencer function so it satisfies Go doc/lint rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1495280-da0b-4921-aaf1-4261263baed5
📒 Files selected for processing (15)
CHANGELOG.mdapps/testapp/Dockerfileapps/testapp/cmd/root.goapps/testapp/cmd/run.goblock/internal/common/consts.goblock/internal/da/client.goblock/internal/submitting/batching_strategy.goblock/internal/submitting/batching_strategy_test.goblock/internal/submitting/da_submitter.goblock/internal/submitting/submitter.goblock/internal/syncing/syncer_forced_inclusion_test.gopkg/sequencers/solo/README.mdpkg/sequencers/solo/sequencer.gopkg/sequencers/solo/sequencer_test.gotest/e2e/failover_e2e_test.go
💤 Files with no reviewable changes (1)
- test/e2e/failover_e2e_test.go
| func init() { | ||
| v, err := strconv.ParseUint(defaultMaxBlobSizeStr, 10, 64) | ||
| if err != nil || v == 0 { | ||
| DefaultMaxBlobSize = 5 * 1024 * 1024 // 5 MB fallback | ||
| return | ||
| } | ||
| DefaultMaxBlobSize = v |
There was a problem hiding this comment.
Fail fast on an invalid -X blob-size override.
Right now a malformed or zero defaultMaxBlobSizeStr silently resets to 5 MiB. That makes a broken build look healthy and can nullify the new ldflags-based override until runtime traffic starts failing. Please surface this as a startup failure instead of quietly falling back.
Suggested fix
-import "strconv"
+import (
+ "fmt"
+ "strconv"
+)
@@
func init() {
v, err := strconv.ParseUint(defaultMaxBlobSizeStr, 10, 64)
if err != nil || v == 0 {
- DefaultMaxBlobSize = 5 * 1024 * 1024 // 5 MB fallback
- return
+ panic(fmt.Sprintf("invalid defaultMaxBlobSizeStr %q: expected a non-zero base-10 uint64", defaultMaxBlobSizeStr))
}
DefaultMaxBlobSize = v
}As per coding guidelines, "Validate all inputs from external sources in Go code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@block/internal/common/consts.go` around lines 14 - 20, The init function
currently swallows ParseUint errors and zero values for defaultMaxBlobSizeStr
and silently falls back to 5MiB; change it to fail-fast by replacing the silent
fallback with a startup failure that includes the offending value and parse
error: in init(), after calling strconv.ParseUint(defaultMaxBlobSizeStr, 10,
64), if err != nil or v == 0, call log.Fatalf or panic with a clear message
(e.g., include defaultMaxBlobSizeStr and err) instead of setting
DefaultMaxBlobSize to 5*1024*1024 so malformed or zero ldflags overrides
immediately stop startup; keep setting DefaultMaxBlobSize = v on success.
| ### Changes | ||
|
|
||
| - Make easier to override `DefaultMaxBlobSize` by ldflags [#3235](https://github.com/evstack/ev-node/pull/3235) | ||
| - Add solo sequencer (simple in memory single sequencer without force inclusion) [#3235](https://github.com/evstack/ev-node/pull/3235) |
There was a problem hiding this comment.
Use hyphenated compound adjective: in-memory.
Minor copy fix for consistency with the rest of the docs/changelog wording.
Suggested wording tweak
-- Add solo sequencer (simple in memory single sequencer without force inclusion) [`#3235`](https://github.com/evstack/ev-node/pull/3235)
+- Add solo sequencer (simple in-memory single sequencer without force inclusion) [`#3235`](https://github.com/evstack/ev-node/pull/3235)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Add solo sequencer (simple in memory single sequencer without force inclusion) [#3235](https://github.com/evstack/ev-node/pull/3235) | |
| - Add solo sequencer (simple in-memory single sequencer without force inclusion) [`#3235`](https://github.com/evstack/ev-node/pull/3235) |
🧰 Tools
🪛 LanguageTool
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...ll/3235) - Add solo sequencer (simple in memory single sequencer without force in...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 15, Update the changelog line that reads "Add solo
sequencer (simple in memory single sequencer without force inclusion) [`#3235`]"
to use the hyphenated compound adjective "in-memory" (e.g., "Add solo sequencer
(simple in-memory single sequencer without force inclusion) [`#3235`]") to match
project style and consistency.
| filterStatuses, err := s.executor.FilterTxs(ctx, txs, req.MaxBytes, maxGas, false) | ||
| if err != nil { | ||
| s.logger.Warn().Err(err).Msg("failed to filter transactions, proceeding with unfiltered") | ||
| filterStatuses = make([]execution.FilterStatus, len(txs)) | ||
| for i := range filterStatuses { | ||
| filterStatuses[i] = execution.FilterOK | ||
| } | ||
| } | ||
|
|
||
| var validTxs [][]byte | ||
| var postponedTxs [][]byte | ||
| for i, status := range filterStatuses { | ||
| switch status { | ||
| case execution.FilterOK: | ||
| validTxs = append(validTxs, txs[i]) | ||
| case execution.FilterPostpone: | ||
| postponedTxs = append(postponedTxs, txs[i]) | ||
| case execution.FilterRemove: | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against malformed FilterTxs result length before indexing.
If filterStatuses length diverges from len(txs), Line 108/Line 110 can panic or produce partial output. Add a strict length check and fail early.
Suggested fix
import (
"bytes"
"context"
"errors"
+ "fmt"
"sync"
"sync/atomic"
"time"
@@
filterStatuses, err := s.executor.FilterTxs(ctx, txs, req.MaxBytes, maxGas, false)
if err != nil {
s.logger.Warn().Err(err).Msg("failed to filter transactions, proceeding with unfiltered")
filterStatuses = make([]execution.FilterStatus, len(txs))
for i := range filterStatuses {
filterStatuses[i] = execution.FilterOK
}
+ } else if len(filterStatuses) != len(txs) {
+ return nil, fmt.Errorf("executor returned %d filter statuses for %d txs", len(filterStatuses), len(txs))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filterStatuses, err := s.executor.FilterTxs(ctx, txs, req.MaxBytes, maxGas, false) | |
| if err != nil { | |
| s.logger.Warn().Err(err).Msg("failed to filter transactions, proceeding with unfiltered") | |
| filterStatuses = make([]execution.FilterStatus, len(txs)) | |
| for i := range filterStatuses { | |
| filterStatuses[i] = execution.FilterOK | |
| } | |
| } | |
| var validTxs [][]byte | |
| var postponedTxs [][]byte | |
| for i, status := range filterStatuses { | |
| switch status { | |
| case execution.FilterOK: | |
| validTxs = append(validTxs, txs[i]) | |
| case execution.FilterPostpone: | |
| postponedTxs = append(postponedTxs, txs[i]) | |
| case execution.FilterRemove: | |
| } | |
| } | |
| filterStatuses, err := s.executor.FilterTxs(ctx, txs, req.MaxBytes, maxGas, false) | |
| if err != nil { | |
| s.logger.Warn().Err(err).Msg("failed to filter transactions, proceeding with unfiltered") | |
| filterStatuses = make([]execution.FilterStatus, len(txs)) | |
| for i := range filterStatuses { | |
| filterStatuses[i] = execution.FilterOK | |
| } | |
| } else if len(filterStatuses) != len(txs) { | |
| return nil, fmt.Errorf("executor returned %d filter statuses for %d txs", len(filterStatuses), len(txs)) | |
| } | |
| var validTxs [][]byte | |
| var postponedTxs [][]byte | |
| for i, status := range filterStatuses { | |
| switch status { | |
| case execution.FilterOK: | |
| validTxs = append(validTxs, txs[i]) | |
| case execution.FilterPostpone: | |
| postponedTxs = append(postponedTxs, txs[i]) | |
| case execution.FilterRemove: | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/sequencers/solo/sequencer.go` around lines 94 - 113, The code assumes
s.executor.FilterTxs returned a filterStatuses slice matching len(txs); add a
strict length check after calling s.executor.FilterTxs to guard against
mismatches: if len(filterStatuses) != len(txs) log an error via s.logger and
fail early by returning an error (or the enclosing function's appropriate error
response) instead of proceeding to index into filterStatuses; update any
fallback branch that previously created a default filterStatuses to use the same
length check/early-return behavior so you never iterate with mismatched lengths
(refer to s.executor.FilterTxs, filterStatuses, txs, and the switch on
execution.FilterOK/FilterPostpone/FilterRemove).
2 participants
Summary by CodeRabbit
Release Notes
New Features
--solo-sequencerCLI flag to enable solo sequencer modeDocumentation
Chores