fix(l2): rollup store SQLite migration for v9 to v10 upgrade#6514
fix(l2): rollup store SQLite migration for v9 to v10 upgrade#6514avilagaston9 merged 1 commit intomainfrom
Conversation
The DB_SCHEMA array runs as a single transaction during init_db(). On an existing v9 database the latest_sent table has two columns (_id, batch), but the INSERT statement tried to supply three values (including the new verified_at column added in v10). This caused a SQLite error that rolled back the entire schema transaction, preventing the subsequent ALTER TABLE migration from ever executing. By specifying column names in the INSERT, the statement works on both the old 2-column table and the new 3-column table (verified_at gets its DEFAULT 0). The ALTER TABLE migration then adds the column to existing databases as intended.
d59190e to
bd11d24
Compare
|
Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance. |
🤖 Kimi Code Reviewcrates/l2/storage/src/store_db/sql.rs This change follows SQL best practices by explicitly specifying column names rather than relying on positional ordering. This makes the schema initialization more robust against future column additions or reorderings. The behavior is functionally equivalent since No issues found. This is a correct code quality improvement. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
There was a problem hiding this comment.
Pull request overview
Fixes an L2 rollup store SQLite migration issue encountered when upgrading an existing v9 database to a v10 binary by making the latest_sent seed insert compatible with both the old (2-column) and new (3-column) table layouts.
Changes:
- Update the
latest_sentseed statement to use explicit column names ((_id, batch)) so it doesn’t fail on pre-migration schemas.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🤖 Claude Code ReviewNow I have the full context. Here is my review: Code Review: fix(l2): rollup store SQLite migration for v9 to v10 upgradeOverviewA one-line targeted fix for a startup panic when upgrading an L2 node from v9 to v10. The The Fix
-- Before
INSERT INTO latest_sent VALUES (0, 0, 0) ON CONFLICT(_id) DO NOTHING
-- After
INSERT INTO latest_sent (_id, batch) VALUES (0, 0) ON CONFLICT(_id) DO NOTHINGThis is correct and minimal. Using explicit column names ensures the statement is valid against both the old 2-column schema and the new 3-column schema — All three cases are covered:
Observations and ConcernsTwo other tables have the same latent risk. Lines 48 and 57 use the same positional -- line 48
INSERT INTO operation_count VALUES (0, 0, 0, 0) ON CONFLICT(_id) DO NOTHING
-- line 57
INSERT INTO latest_sent_to_aligned VALUES (0, 0, 0) ON CONFLICT(_id) DO NOTHINGIf either of these tables gains a column in a future version, the same startup panic will recur. Applying explicit column names consistently to all three singleton-row tables ( Silent error handling in let _ = self
.execute("ALTER TABLE latest_sent ADD COLUMN verified_at INT DEFAULT 0", ())
.await;The No automated migration test. The upgrade path (v9 schema → v10 binary) is exercised only by the manual steps in the PR description. A test that spins up an in-memory SQLite with the old 2-column SummaryThe fix is correct, surgically minimal, and clearly explained. The root cause is accurately diagnosed. No new issues are introduced. The main actionable takeaways are:
Verdict: Approve with the above as optional follow-up items. Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code ReviewNo findings. The change in sql.rs looks correct: inserting only I verified the SQL behavior directly with Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Brings in main commits since the prior merge: #6516 EIP-8025 compliance (Electra-aligned ExecutionRequests typed container in NewPayloadRequest, MAX_CONSOLIDATION_REQUESTS_PER_PAYLOAD corrected from 1 to 2, to_encoded_requests() helper for EIP-7685 bytes, removal of ExecutionPayloadHeader/NewPayloadRequestHeader, new byte-oriented execution_program entrypoint that decodes the wire format internally and returns valid: false instead of erroring on post-decode failures), #6463 BAL withdrawal reverse check (DB->BAL direction so a malicious builder can't omit a withdrawal recipient from the BAL), #6505 Kademlia k-bucket revert (PeerTableServer::spawn no longer takes a node_id), plus snap-sync observability + dashboards (#6470), pivot-update crash fix (#6475), weighted peer selection (#6428), txpool_contentFrom/txpool_inspect RPC (#6446), block-by-block exec fallback (#6464), Amsterdam EELS branch pin (#6495), and rollup store SQLite v9->v10 migration (#6514). Conflict resolutions: - crates/common/types/stateless_ssz.rs: this branch had already moved the EIP-8025 SSZ types out of crates/common/types/eip8025_ssz.rs into stateless_ssz.rs and tucked the native-rollup containers below them. Kept that layout, applied #6516's content updates to the EIP-8025 section (renamed spec-limit constants, ExecutionRequests typed container with to_encoded_requests, dropped header types and their tests), pulled in the EncodedRequests import, and kept both the new test_execution_requests_to_encoded_bytes and the branch's stateless round-trip tests. - crates/guest-program/src/l1/program.rs: adopted #6516's new execution_program(bytes: &[u8], crypto) API with the internal decode_eip8025 call, the validate_eip8025_execution helper, and the decode-failure test. Rewrote all `eip-8025` feature gates as `experimental-devnet` and all `eip8025_ssz::` paths as `stateless_ssz::` to match this branch's renames. - crates/guest-program/bin/{sp1,risc0,zisk,openvm}/src/main.rs: applied #6516's simplification (drop decode_eip8025 import, pass &input straight to execution_program) under the experimental-devnet feature gate. Also flipped the rkyv::rancor::Error import gate from the old `eip-8025` name to `experimental-devnet` so the non-devnet build still has the import it needs. - crates/prover/src/backend/exec.rs: kept #6516's updated comment ("raw input bytes" instead of "(NewPayloadRequest, ExecutionWitness)") under the experimental-devnet feature gate. Auto-merged regions checked: crates/vm/backends/levm/mod.rs picked up all of #6463's Part B (DB->BAL) reverse check intact, and cmd/ethrex/l2/initializers.rs picked up #6505's PeerTableServer::spawn signature change. Verified cargo fmt --all clean, cargo check --workspace clean, cargo check --workspace --tests clean, and cargo check -p ethrex-guest-program --features experimental-devnet --tests clean.
Motivation
When upgrading an ethrex L2 from v9 to v10, the v10 binary panics on startup with:
Description
v10 added a
verified_atcolumn to thelatest_senttable. TheDB_SCHEMAarray runs as a single transaction duringinit_db(). On an existing v9 database,CREATE TABLE IF NOT EXISTSis a no-op (table already exists with 2 columns), butINSERT INTO latest_sent VALUES (0, 0, 0)fails because it supplies 3 values for a 2-column table. This rolls back the entire schema transaction, so theALTER TABLEmigration that adds the column never executes.The fix uses explicit column names in the INSERT:
INSERT INTO latest_sent (_id, batch) VALUES (0, 0). This works on both the old 2-column table and the new 3-column table (verified_atgets itsDEFAULT 0).How to Test
latest_sent