Skip to content

Conversation

@Kukoomomo
Copy link
Contributor

@Kukoomomo Kukoomomo commented Jan 5, 2026

Summary by CodeRabbit

  • Refactor
    • Disabled BLS signature aggregation in the rollup signing mechanism. Signature fields now use placeholder values instead of aggregated signatures, with related code paths disabled.

✏️ Tip: You can customize this high-level summary in your review settings.

@Kukoomomo Kukoomomo requested a review from a team as a code owner January 5, 2026 06:31
@Kukoomomo Kukoomomo requested review from r3aker86 and removed request for a team January 5, 2026 06:31
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 5, 2026

📝 Walkthrough

Walkthrough

BLS signature aggregation in the rollup signing process has been disabled. BLS-related imports have been removed, signature aggregation logic has been commented out, and the signature field is now populated with a placeholder value instead of an aggregated signature.

Changes

Cohort / File(s) Summary
BLS Signature Disabling
tx-submitter/services/rollup.go
Removed bls12381 and blssignatures imports; disabled BLS signature aggregation logic in buildSignatureInput; replaced aggregated signature with placeholder value ([]byte("0x")); commented out signature derivation and encoding paths

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 No more BLS upon the chain,
We've bid those signatures goodbye,
A placeholder now fills the lane,
As "0x" hops swiftly by!
Simpler paths now lead us through,
This rollup's brand new point of view. 🌙

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Set bls sig to nil' directly describes the main change: disabling BLS signatures by setting them to placeholder values (nil/empty) in the rollup signing path.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3ee14af and 0b8e2cc.

📒 Files selected for processing (1)
  • tx-submitter/services/rollup.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: test
  • GitHub Check: check
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (go)
🔇 Additional comments (1)
tx-submitter/services/rollup.go (1)

1235-1270: Remove placeholder signature handling - coordinate BLS signature implementation across L1 and L2.

The BLS signature aggregation logic has been disabled on the L2 side (tx-submitter) by commenting out lines 1241-1253 and setting the signature to a placeholder value []byte("0x") (line 1267). However, the L1 contract's verifySignature() function (L1Staking.sol line 333-340) is currently a stub that always returns true with a TODO comment, so this change does not immediately break batch submissions.

However, several issues remain:

  1. Code inconsistency: The function still extracts signers from batch signatures (lines 1240-1251) and queries the signer bitmap (lines 1255-1262), but then discards the actual signature data entirely. This creates an inconsistent state where metadata about signers is preserved but cryptographic proof is absent.

  2. Incomplete BLS implementation: Both L1 and L2 have incomplete or placeholder BLS implementations:

    • L2 buildSignatureInput: BLS aggregation disabled, signature is placeholder
    • L1 verifySignature: Stub function with TODO comment, always returns true
    • This needs coordinated completion when actual BLS verification is enabled
  3. Commented code: The BLS logic is commented out rather than deleted, making the codebase harder to maintain and suggesting uncertainty about this architectural decision.

When the L1 verifySignature() function is actually implemented to validate BLS signatures, the current placeholder signature will cause all batch submissions to fail unless the L2 code is also updated to properly aggregate and include valid BLS signatures. Ensure this change is coordinated with the L1 contract update.

Likely an incorrect or invalid review comment.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

return nil, err
}
sigs = append(sigs, sig)
// sig, err := blssignatures.SignatureFromBytes(bz.Signature)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to delete?

aggregatedSig := blssignatures.AggregateSignatures(sigs)
blsSignature := bls12381.NewG1().EncodePoint(aggregatedSig)
// aggregatedSig := blssignatures.AggregateSignatures(sigs)
// blsSignature := bls12381.NewG1().EncodePoint(aggregatedSig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@curryxbo curryxbo self-requested a review January 6, 2026 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants