Skip to content

perf(protocol): optimize propose/prove gas via non-upgradeable checkers and new entry points#21502

Open
dantaik wants to merge 12 commits intomainfrom
dantaik/propose-whitelist-opt
Open

perf(protocol): optimize propose/prove gas via non-upgradeable checkers and new entry points#21502
dantaik wants to merge 12 commits intomainfrom
dantaik/propose-whitelist-opt

Conversation

@dantaik
Copy link
Copy Markdown
Member

@dantaik dantaik commented Mar 25, 2026

Summary

Reduces propose and prove gas costs through structural changes — non-upgradeable whitelists, new entry points, and inlined logic.

Gas results (vs main)

Metric main PR Saved
propose_single 71,167 63,490 -7,677 (-10.8%)
propose_forced_inclusion 59,032 56,397 -2,635 (-4.5%)
prove_single 81,678 76,665 -5,013 (-6.1%)
prove_batch_10 88,588 83,575 -5,013 (-5.7%)

New entry points (not on main)

Metric Gas vs propose() baseline
proposeDefault() (1 blob, zero params) 56,715 -3,295 (-5.5%)
proposeCompact() (1 blob, explicit params) 57,287 -2,723 (-4.5%)
proposeCompact() (3 blobs) 58,790 -2,711 (-4.4%) vs propose 3-blob

Key changes

  • proposeDefault() / proposeCompact() — new entry points that skip ABI encode/decode overhead for common propose cases
  • Non-upgradeable PreconfWhitelist (Ownable2Step) — eliminates delegatecall proxy overhead on propose, inlines checkProposer with gas-optimized assembly beacon root lookup
  • Non-upgradeable ProverWhitelist (Ownable2Step) — eliminates delegatecall proxy overhead on prove
  • Inline _buildProposal into _propose() — eliminates function call overhead
  • Gas benchmark testsInboxProposeGas.t.sol and PreconfWhitelistGas.t.sol with pre-filled ring buffer for realistic steady-state measurement
  • Deploy scripts updated for non-upgradeable checker constructors

Test plan

  • forge test --match-path "test/layer1/{core,preconf}/**" — 172 tests pass
  • forge build — zero errors
  • No changes to existing function ABIs (kept deprecatedInactiveSince placeholder for Go binding compat)
  • Gas benchmarks for proposeDefault() / proposeCompact() vs propose() in InboxProposeGas.t.sol

🤖 Generated with Claude Code

Client implementration

@davidtaikocha depends on proposal parameters, our client code shall choose between propose, proposeDefault and proposeCompact to save gas cost.

…rs and new entry points

- Add proposeDefault() and proposeCompact() entry points that skip ABI encode/decode overhead
- Convert PreconfWhitelist to non-upgradeable (Ownable), inline checkProposer with gas-optimized beacon root lookup
- Convert ProverWhitelist to non-upgradeable (Ownable), eliminate delegatecall proxy overhead
- Inline _buildProposal into shared _propose() and add early-return in _consumeForcedInclusions
- Update deploy scripts and tests for non-proxy whitelist contracts
- Add gas benchmark tests for propose variants and checkProposer

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dantaik and others added 2 commits March 25, 2026 17:25
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3bc01c5228

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

dantaik and others added 5 commits March 25, 2026 17:34
- Replace Ownable with Ownable2Step in both PreconfWhitelist and
  ProverWhitelist for safer two-step ownership transfers
- Fix DeployShastaContracts to pass locally deployed preconfWhitelist
  to MainnetInbox instead of orphaned config.preconfWhitelist

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… test comment

- Use 0x00 scratch space instead of free memory pointer in checkProposer
  assembly block for compiler-version robustness
- Add @dev warning to proposeCompact about missing deadline support
- Clarify gas test naming: 1-operator baseline hits slow path, explaining
  why 3-operator test is cheaper

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…I compat

Keep the deprecated uint32 field in the struct so the autogenerated
operators() getter returns the same 4-element tuple that existing Go
bindings (gen_preconf_whitelist.go) and rpc/methods.go expect.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ange

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ProverWhitelist and PreconfWhitelist are no longer upgradeable, so
storage layout tracking is unnecessary. Remove their _Layout.sol
files and entries from gen-layouts.sh.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@taikoxyz taikoxyz deleted a comment from chatgpt-codex-connector bot Mar 25, 2026
@taikoxyz taikoxyz deleted a comment from claude bot Mar 25, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dantaik
Copy link
Copy Markdown
Member Author

dantaik commented Mar 25, 2026

@claude please review

@claude
Copy link
Copy Markdown

claude bot commented Mar 25, 2026

Claude finished @dantaik's task in 2m 56s —— View job


PR Review

  • Reading changed files
  • Reviewing Inbox.sol changes
  • Reviewing PreconfWhitelist.sol changes
  • Reviewing ProverWhitelist.sol changes
  • Reviewing deploy scripts and tests

Overall this is a clean, well-targeted optimization PR. The gas savings are real and the changes are focused. A few things to address:

Bug: Dead validation in DeployShastaContracts

_validateConfig at line 74 requires config.preconfWhitelist != address(0), but _deploy now always creates a fresh PreconfWhitelist and never uses config.preconfWhitelist:

// _validateConfig (line 74) — still present
require(config.preconfWhitelist != address(0), "PRECONF_WHITELIST not set");

// _deploy (line 91) — ignores config.preconfWhitelist entirely
address preconfWhitelist = address(new PreconfWhitelist(config.contractOwner));

The old code passed config.preconfWhitelist directly to MainnetInbox. The diff shows that was replaced with the freshly-deployed address, but the DeploymentConfig field and its validation were not removed. Either remove the field + validation, or make deployment conditionally reuse the provided address (as DeployProtocolOnL1 does). Fix this →

Missing interface entries for new entry points

proposeDefault() and proposeCompact() are not declared in IInbox. Any consumer relying on the interface (ABI codegen, downstream contracts casting to IInbox) won't see them. Even if the intent is to keep these as implementation-level helpers, they deserve interface declarations so the ABI is authoritative. Fix this →

checkProposer assembly — silent fallback when no beacon root found

If the loop finds no valid beacon root (e.g. ts > current on all iterations), beaconRoot remains bytes32(0). This causes uint256(0) % count = 0, silently selecting the operator at index 0 instead of reverting. This is the same behavior as the original getBeaconBlockRootAtOrAfter, but worth an explicit comment in the hot path since the reader can't easily follow the fallback behavior through the assembly:

// After the assembly block
// Note: if beaconRoot == 0 (no root found), operator selection defaults to index 0.

_transferOwnership constructor pattern skips zero-address guard

Both PreconfWhitelist and ProverWhitelist use:

constructor(address _owner) {
    _transferOwnership(_owner);
}

This bypasses Ownable's constructor (which in OZ v5 includes a zero-address revert), so address(0) is accepted silently. Passing address(0) leaves the contract permanently unownable. The deploy scripts use real addresses so this is not a live risk, but a defensive zero-address check is cheap:

constructor(address _owner) {
    require(_owner != address(0));
    _transferOwnership(_owner);
}

Or use Ownable(_owner) in the initializer list if your OZ version supports it.

Minor observations

  • proposeCompact deadline warning: The NatSpec is clear about silent loss of deadline protection. Consider whether this needs an @dev notice in the interface too when it's added there.
  • Snapshot numbers look correct: proposeDefault at 56,715 vs propose_gas_baseline at 60,010 = ~3,300 gas saved (PR claims 2,000+, which is conservative). checkProposer_gas_3operators at 2,654 is impressively low.
  • Early-return in _consumeForcedInclusions: The available == 0 fast path is a nice improvement; production path avoids all the forced-inclusion computation.
  • DeploymentConfig.preconfWhitelist in DeployShastaContracts: see bug above — this should be resolved before the script is used for a mainnet deploy.

dantaik and others added 3 commits March 25, 2026 08:51
…s, deploy fix

- Add proposeDefault() and proposeCompact() to IInbox interface
- Add zero-address guard in ProverWhitelist and PreconfWhitelist constructors
- Make DeployShastaContracts conditionally deploy PreconfWhitelist (reuse
  config.preconfWhitelist if set, deploy fresh if zero)
- Add comment about beaconRoot == 0 fallback behavior in checkProposer

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dantaik
Copy link
Copy Markdown
Member Author

dantaik commented Mar 25, 2026

Thanks for the review! All four points addressed in 5cfc753:

  1. Dead validation in DeployShastaContracts — Fixed. Made deployment conditional: reuses config.preconfWhitelist if set, deploys fresh if zero. Removed the require != address(0) validation since the field is now optional.

  2. Missing interface entries — Fixed. Added proposeDefault() and proposeCompact() to IInbox with full NatSpec (including the deadline warning). Inbox implementations now use @inheritdoc IInbox.

  3. Silent beacon root fallback — Added explicit comment above the _latestActivationEpoch read: "if beaconRoot == 0 (no root found), operator selection defaults to index 0. This matches the behavior of the original getBeaconBlockRootAtOrAfter."

  4. Zero-address guard in constructors — Added require(_owner != address(0), ZeroAddress()) to both PreconfWhitelist and ProverWhitelist constructors.

@dantaik dantaik enabled auto-merge March 25, 2026 08:58
@claude
Copy link
Copy Markdown

claude bot commented Mar 25, 2026

Storage Layout Review for PR 21502 - Two _Layout.sol files deleted as part of architectural change from EssentialContract (upgradeable) to Ownable2Step (non-upgradeable). Deploy scripts confirm fresh non-proxy deployments so these changes are safe, but the new layouts are entirely incompatible with the old proxy layouts. ProverWhitelist: old layout had _provers at slot 251 and proverCount at slot 252; new layout starts at slot 0. PreconfWhitelist: old layout had operators/operatorMapping/packed-vars/ejecters at slots 251-254; new layout starts at slot 0 with deprecated vars removed. A proxy upgrade from old to new would corrupt all stored data. Warnings: (1) No new _Layout.sol files generated - confirm gen-layouts.sh intentionally excludes non-upgradeable contracts. (2) deprecatedInactiveSince retained in OperatorInfo for Go ABI compat. (3) operatorCount is uint8 max 255 operators. Safe changes: variable order preserved in both contracts, __gap arrays removed appropriately. Recommendations: confirm no production proxy points to old implementations; verify gen-layouts.sh exclusion is intentional; document that these contracts cannot be upgraded. Automated storage layout security review by Claude Code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants