feat: solo sequencer, testapp max blob size ldflags#3235
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces a Solo Sequencer (in-memory, single-leader) and makes DefaultMaxBlobSize overridable via ldflags; standardizes size-related types from int to uint64 across blob handling and batching logic; adds Dockerfile ldflags and CLI flag to enable solo sequencer; includes tests and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Solo as SoloSequencer
participant Exec as Executor
Client->>Solo: SubmitBatchTxs(batchID, txs)
Solo->>Solo: Validate batchID
Solo->>Solo: Append txs to in-memory queue
Solo-->>Client: SubmitBatchTxsResponse
Client->>Solo: GetNextBatch(batchID)
Solo->>Solo: Drain queue (atomic)
Solo->>Exec: GetExecutionInfo / MaxGas
Exec-->>Solo: ExecutionInfo
Solo->>Exec: FilterTxs(candidate txs)
Exec-->>Solo: validTxs + postponedTxs
Solo->>Solo: Re-queue postponedTxs (front)
Solo-->>Client: GetNextBatchResponse(validTxs, timestamp)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 57s —— View job Code Review
Overall the PR is clean and well-scoped. Two issues warrant attention before merge; the rest are minor. 🔴 Major Issues1. Missing If filterStatuses, err := s.executor.FilterTxs(ctx, txs, req.MaxBytes, maxGas, false)
if err != nil {
// ... fallback is fine
} else if len(filterStatuses) != len(txs) {
// executor violated its contract — fail loudly rather than panic later
return nil, fmt.Errorf("executor returned %d filter statuses for %d txs",
len(filterStatuses), len(txs))
}2. Silent ldflags fallback — if err != nil || v == 0 {
DefaultMaxBlobSize = 5 * 1024 * 1024 // 5 MB fallback
return
}A misconfigured ldflags override (e.g. if err != nil || v == 0 {
panic(fmt.Sprintf("invalid defaultMaxBlobSizeStr %q: expected a non-zero base-10 uint64, err=%v",
defaultMaxBlobSizeStr, err))
}🟡 Minor Issues3. CHANGELOG copy — Two small issues in the current text:
4. Missing doc comment on Per repo guidelines ("Document exported types and functions in Go"), the constructor needs a doc comment. The type already has one; the function doesn't. A one-liner is sufficient: // NewSoloSequencer creates a SoloSequencer with an empty in-memory transaction queue.5. Block time default change in The default block time silently changed from 10 ms → 100 ms. If this is intentional (seems like it's a separate/unrelated fix for the testapp), a changelog entry or PR comment explaining why would help reviewers. 6. Solo sequencer in non-aggregator mode
✅ Positive Observations
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3235 +/- ##
==========================================
+ Coverage 61.79% 61.91% +0.11%
==========================================
Files 120 122 +2
Lines 12670 12744 +74
==========================================
+ Hits 7830 7890 +60
- Misses 3962 3971 +9
- Partials 878 883 +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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CHANGELOG.md (1)
15-15:⚠️ Potential issue | 🟡 MinorUse hyphenated compound adjective:
in-memory.The compound adjective "in memory" should be hyphenated when it modifies a noun.
📝 Suggested wording fix
-- 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)🤖 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 entry text to use the hyphenated compound adjective "in-memory": replace the phrase "simple in memory single sequencer without force inclusion" with "simple in-memory single sequencer without force inclusion" in the line that starts with "Add solo sequencer (simple in memory single sequencer without force inclusion) [`#3235`]".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 14: Update the changelog entry to include the missing pronoun and clarify
preposition: change the line referencing DefaultMaxBlobSize to read something
like "Make it easier to override `DefaultMaxBlobSize` via ldflags" (ensure
`DefaultMaxBlobSize` and "ldflags" remain intact).
---
Duplicate comments:
In `@CHANGELOG.md`:
- Line 15: Update the changelog entry text to use the hyphenated compound
adjective "in-memory": replace the phrase "simple in memory single sequencer
without force inclusion" with "simple in-memory single sequencer without force
inclusion" in the line that starts with "Add solo sequencer (simple in memory
single sequencer without force inclusion) [`#3235`]".
🪄 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: 010beb3b-7b0a-4e64-af5e-17c3a87ecbd7
📒 Files selected for processing (2)
CHANGELOG.mdblock/internal/submitting/submitter.go
✅ Files skipped from review due to trivial changes (1)
- block/internal/submitting/submitter.go
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes / Improvements
Tests