Actionable comments posted: 0

♻️ Duplicate comments (2)

coordinator/internal/logic/provertask/batch_prover_task.go (2)

224-228: 🛠️ Refactor suggestion

Add null check for OpenVMChunkProof metadata

When accessing fields from openvmProof.MetaData.ChunkInfo, there's no check to ensure it's not nil, which could lead to a panic.

if openvmProof, ok := proof.(*message.OpenVMChunkProof); ok {
+	if openvmProof.MetaData.ChunkInfo == nil {
+		return nil, fmt.Errorf("missing ChunkInfo in OpenVMChunkProof for chunk hash: %s", chunk.Hash)
+	}
	chunkInfo.InitialBlockNumber = openvmProof.MetaData.ChunkInfo.InitialBlockNumber
	chunkInfo.BlockCtxs = openvmProof.MetaData.ChunkInfo.BlockCtxs
	chunkInfo.TxDataLength = openvmProof.MetaData.ChunkInfo.TxDataLength
}

---

291-298: 🛠️ Refactor suggestion

Add safety checks and use constants for BlobDataProof parsing

The code directly accesses byte ranges in dbBatch.BlobDataProof without validating its length or using named constants for offsets, which could lead to panic if the input is malformed.

// Memory layout of `BlobDataProof`: used in Codec.BlobDataProofForPointEvaluation()
// | z       | y       | kzg_commitment | kzg_proof |
// |---------|---------|----------------|-----------|
// | bytes32 | bytes32 | bytes48        | bytes48   |
+const (
+	blobDataProofMinLength = 160 // Minimum length for valid BlobDataProof
+	kzgCommitmentOffset    = 64  // Start offset for KZG commitment
+	kzgProofOffset         = 112 // Start offset for KZG proof
+	byte48Length           = 48  // Length of Byte48 fields
+)
+
+// Ensure BlobDataProof has sufficient length to extract all fields
+if len(dbBatch.BlobDataProof) < blobDataProofMinLength {
+	return nil, fmt.Errorf("BlobDataProof length insufficient: got %d bytes, need at least %d bytes",
+		len(dbBatch.BlobDataProof), blobDataProofMinLength)
+}
+
taskDetail.KzgProof = message.Byte48{Big: hexutil.Big(*new(big.Int).SetBytes(dbBatch.BlobDataProof[kzgProofOffset:kzgProofOffset+byte48Length]))}
taskDetail.KzgCommitment = message.Byte48{Big: hexutil.Big(*new(big.Int).SetBytes(dbBatch.BlobDataProof[kzgCommitmentOffset:kzgCommitmentOffset+byte48Length]))}

🧹 Nitpick comments (6)

common/types/message/message.go (6)

1-11: Add appropriate import sorting for math/big

The import for math/big is added but not sorted according to standard Go conventions (standard libraries first, then third-party imports).

import (
	"encoding/json"
	"errors"
	"fmt"
-	"math/big"
+	"math/big"

	"github.com/scroll-tech/go-ethereum/common"
	"github.com/scroll-tech/go-ethereum/common/hexutil"
)

---

48-54: Clarify comment on ForkName field usage

The comment for the ForkName field is ambiguous. Improve the documentation to clarify the expected values and usage.

type ChunkTaskDetail struct {
-	// use one of the string of EuclidFork / EuclidV2Fork
+	// ForkName must be one of the constants: EuclidFork or EuclidV2Fork
	ForkName         string        `json:"fork_name"`
	BlockHashes      []common.Hash `json:"block_hashes"`
	PrevMsgQueueHash common.Hash   `json:"prev_msg_queue_hash"`
}

---

56-74: Fix typo in Byte48 MarshalText comment

There's a typo in the comment for the MarshalText method.

func (e Byte48) MarshalText() ([]byte, error) {
	i := e.ToInt()
-	// overrite encode big
+	// override encode big
	if sign := i.Sign(); sign < 0 {
		// sanity check
		return nil, errors.New("Byte48 must be positive integer")
	} else {
		s := i.Text(16)
		if len(s) > 96 {
-			return nil, errors.New("integer Exceed 384bit")
+			return nil, errors.New("integer exceeds 384 bits")
		}
		return []byte(fmt.Sprintf("0x%0*s", 96, s)), nil
	}
}

---

121-135: Add documentation for new ChunkInfo fields

The new fields added to the ChunkInfo struct lack documentation explaining their purpose and usage.

type ChunkInfo struct {
	ChainID            uint64           `json:"chain_id"`
	PrevStateRoot      common.Hash      `json:"prev_state_root"`
	PostStateRoot      common.Hash      `json:"post_state_root"`
	WithdrawRoot       common.Hash      `json:"withdraw_root"`
	DataHash           common.Hash      `json:"data_hash"`
	IsPadding          bool             `json:"is_padding"`
	TxBytes            []byte           `json:"tx_bytes"`
	TxBytesHash        common.Hash      `json:"tx_data_digest"`
	PrevMsgQueueHash   common.Hash      `json:"prev_msg_queue_hash"`
+	// Hash of the message queue after processing the chunk
	PostMsgQueueHash   common.Hash      `json:"post_msg_queue_hash"`
+	// Total length of transaction data in bytes
	TxDataLength       uint64           `json:"tx_data_length"`
+	// Initial L2 block number in the chunk
	InitialBlockNumber uint64           `json:"initial_block_number"`
+	// Context information for each block in the chunk
	BlockCtxs          []BlockContextV2 `json:"block_ctxs"`
}

---

137-144: Add documentation for BlockContextV2 struct

The new BlockContextV2 struct lacks documentation explaining its purpose and how it differs from any potential previous version.

+// BlockContextV2 contains metadata about a block in the EuclidV2 fork,
+// providing context for verification and execution
type BlockContextV2 struct {
+	// Unix timestamp of the block
	Timestamp uint64      `json:"timestamp"`
+	// Base fee per gas in the block
	BaseFee   hexutil.Big `json:"base_fee"`
+	// Gas limit of the block
	GasLimit  uint64      `json:"gas_limit"`
+	// Number of transactions in the block
	NumTxs    uint16      `json:"num_txs"`
+	// Number of L1 messages in the block
	NumL1Msgs uint16      `json:"num_l1_msgs"`
}

---

330-339: Document purpose of new OpenVMBatchInfo message queue fields

The new fields PrevMsgQueueHash and PostMsgQueueHash lack documentation about their purpose.

type OpenVMBatchInfo struct {
	ParentBatchHash  common.Hash `json:"parent_batch_hash"`
	ParentStateRoot  common.Hash `json:"parent_state_root"`
	StateRoot        common.Hash `json:"state_root"`
	WithdrawRoot     common.Hash `json:"withdraw_root"`
	BatchHash        common.Hash `json:"batch_hash"`
	ChainID          uint64      `json:"chain_id"`
+	// Hash of the message queue before processing the batch
	PrevMsgQueueHash common.Hash `json:"prev_msg_queue_hash"`
+	// Hash of the message queue after processing the batch
	PostMsgQueueHash common.Hash `json:"post_msg_queue_hash"`
}

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4c4cff0 and 80af426.

⛔ Files ignored due to path filters (1)

• zkvm-prover/Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (3)

• common/types/message/message.go (7 hunks)
• coordinator/internal/logic/provertask/batch_prover_task.go (6 hunks)
• zkvm-prover/Cargo.toml (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• zkvm-prover/Cargo.toml

🧰 Additional context used 🧬 Code Definitions (2) coordinator/internal/logic/provertask/batch_prover_task.go (3)

coordinator/internal/types/auth.go (1)

• Hash (134-141)

common/types/message/message.go (11)

• Halo2ChunkProof (168-178)
• ChunkInfo (121-135)
• OpenVMChunkProof (310-318)
• ProofTypeBatch (43-43)
• ChunkProof (153-155)
• BatchTaskDetail (100-110)
• EuclidV2Fork (15-15)
• EuclidV2ForkNameForProver (18-18)
• EuclidFork (14-14)
• EuclidForkNameForProver (17-17)
• Byte48 (57-59)

coordinator/internal/orm/batch.go (2)

• Batch (18-72)
• Batch (80-82)

common/types/message/message.go (1)

coordinator/internal/types/auth.go (1)

• Hash (134-141)

🔇 Additional comments (9)

common/types/message/message.go (6)

13-19: Excellent addition of fork constants for proper versioning

The renaming of euclidFork to EuclidFork enhances public accessibility, and the addition of EuclidV2 constants establishes proper versioning support. This change enables better backward compatibility and smooth transitions between fork versions.

---

76-97: Well-implemented custom JSON marshaling for Byte48

The implementation of custom unmarshaling is thorough with proper validation for string format, hex decoding, and byte length. Using this custom approach for 48-byte values handles the 384-bit integers that exceed the default hexutil.Big limit of 256 bits.

---

100-110: Add documentation comment for BlobBytes encoding

The BlobBytes field lacks documentation about its encoding expectations. Past review comments highlighted the need for this clarification.

type BatchTaskDetail struct {
	// use one of the string of EuclidFork / EuclidV2Fork
	ForkName        string       `json:"fork_name"`
	ChunkInfos      []*ChunkInfo `json:"chunk_infos"`
	ChunkProofs     []ChunkProof `json:"chunk_proofs"`
	BatchHeader     interface{}  `json:"batch_header"`
+	// BlobBytes contains the raw blob data in binary format (not base64 encoded)
	BlobBytes       []byte       `json:"blob_bytes"`
	KzgProof        Byte48       `json:"kzg_proof,omitempty"`
	KzgCommitment   Byte48       `json:"kzg_commitment,omitempty"`
	ChallengeDigest common.Hash  `json:"challenge_digest,omitempty"`
}

---

158-165: Good implementation of fork-aware proof creation

The update to include both EuclidFork and EuclidV2Fork in the switch case maintains backward compatibility while supporting the new fork version.

---

191-199: Consistent fork handling in BatchProof creation

The pattern of supporting both forks in the switch statement is well-maintained across all proof creation functions.

---

248-256: Consistent approach for BundleProof creation

The update to include both EuclidFork and EuclidV2Fork in the switch case maintains the same pattern used in other proof creation functions, ensuring consistency across the codebase.

coordinator/internal/logic/provertask/batch_prover_task.go (3)

219-223: Improved variable naming and null check

The variable rename from haloProot to halo2Proof improves code readability, and the null check for halo2Proof.ChunkInfo prevents potential null pointer dereference.

---

249-251: Good addition of debug logs

Adding debug logs with task details improves observability and will be helpful during troubleshooting sessions. The correct task type message.ProofTypeBatch.String() is now used in the log message.

---

261-271: Well-structured fork name mapping

The implementation correctly maps internal fork names to the prover-specific fork names, maintaining a clear separation between internal and external representations.

Actionable comments posted: 0

🧹 Nitpick comments (1)

rollup/go.mod (1)

3-4: Toolchain Declaration Simplification

The removal of the explicit toolchain declaration (previously something like toolchain go1.22.2) in favor of the simpler go 1.22 declaration reduces verbosity. Please ensure that this change aligns with your build and tooling expectations, especially if any tooling-specific features were previously tied to the removed declaration.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 38af170 and f045984.

⛔ Files ignored due to path filters (3)

• coordinator/go.sum is excluded by !**/*.sum
• go.work.sum is excluded by !**/*.sum
• rollup/go.sum is excluded by !**/*.sum

📒 Files selected for processing (2)

• coordinator/go.mod (1 hunks)
• rollup/go.mod (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• coordinator/go.mod

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: tests

🔇 Additional comments (1)

rollup/go.mod (1)

14-14: Dependency Version Update for da-codec

The version of github.com/scroll-tech/da-codec has been updated to v0.1.3-0.20250401062930-9f9f53898493. Please verify that this updated version is compatible with the rest of your modules and that there are no breaking changes. It would be beneficial to cross-check against any upstream release notes or test the integration in your CI to ensure system stability.