Skip to content

Conversation

@yeguclu
Copy link

@yeguclu yeguclu commented Jan 16, 2026

📝 Summary

Unify optimistic v1 and v3 submissions to use BidAdjustmentDataV3 format, removing BidAdjustmentDataV1 and intermediate conversion methods.

💡 Motivation and Context

For payout transactions, gas_used may differ from gas_limit. BidAdjustmentDataV3(https://docs.ultrasound.money/builders/optimistic-v3) format relaxes the gas_limit == gas_used assumption by explicitly providing placeholder_gas_used, allowing the relay to use the actual gas consumed.

In addition to optimistic v3 submissions, there already is a support for using the BidAdjustmentDataV3 with optimistic v1 submissions. Currently in rbuilder codebase, v1 submissions used BidAdjustmentDataV1 while v3 submissions used BidAdjustmentDataV2. Changing those submission types to use v3 data allows relays to use the actual gas consumed, preventing adjustment failures when gas_used differs from gas_limit, and will increase builder adjustment revenues.

Therefore we can simplify the codebase by:

  • Removing BidAdjustmentDataV1 entirely
  • Removing the intermediate BidAdjustmentData struct and conversion methods
  • Using BidAdjustmentDataV3 directly for both optimistic submission methods

✅ I have completed the following steps:

  • [✅ ] Run make lint
  • [✅ ] Run make test
  • Added tests (if applicable)

@yeguclu yeguclu changed the title refactor: update optimistic v3 adjustments to use adjustment data v3 feat: update optimistic v3 adjustments to use adjustment data v3 Jan 22, 2026
@yeguclu yeguclu changed the title feat: update optimistic v3 adjustments to use adjustment data v3 feat: update optimistic submissions to use adjustment data v3 Jan 22, 2026
@yeguclu yeguclu changed the title feat: update optimistic submissions to use adjustment data v3 feat: update both optimistic submission methods to use adjustment data v3 Jan 22, 2026
@metachris
Copy link
Contributor

@claude review

@claude
Copy link
Contributor

claude bot commented Jan 27, 2026

Claude finished @metachris's task in 4m 46s —— View job


Code Review Complete

  • Gather context and read repository guidelines
  • Review changed files
  • Analyze code quality and patterns
  • Check for bugs, security issues, and edge cases
  • Provide review feedback

Summary

This PR simplifies the bid adjustment code by consolidating BidAdjustmentDataV1, BidAdjustmentDataV2, and intermediate BidAdjustmentData into a single BidAdjustmentDataV3 struct. The key addition is the placeholder_gas_used field which allows relays to use actual gas consumed rather than assuming gas_limit == gas_used.

Key findings:

  • One potential issue identified with unconditional expect() evaluation (see inline comment)
  • Field naming should be verified against relay API specs
  • Overall the refactoring is clean and well-motivated

I've posted a detailed summary comment and 3 inline comments on specific code locations.

.executed_tx_infos
.last()
.map(|tx_info| tx_info.space_used.gas)
.expect("payout transaction must exist");
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential panic in edge cases

This expect() is now evaluated unconditionally, whereas previously the bid adjustment data was constructed lazily inside the iterator. If executed_tx_infos is ever empty (e.g., in testing or edge cases), this will panic.

Consider deferring this calculation to only when bid_adjustment_state_proofs is non-empty, or using unwrap_or(0) if a default is acceptable:

Suggested change
.expect("payout transaction must exist");
.expect("payout transaction must exist");

Alternatively, you could compute this lazily:

let placeholder_gas_used = LazyCell::new(|| {
    self.executed_tx_infos
        .last()
        .map(|tx_info| tx_info.space_used.gas)
        .expect("payout transaction must exist")
});

This preserves the original behavior where the expect would only be evaluated when bid adjustments are actually created.

/// adjusting payments to contract addresses.
pub placeholder_receipt_proof: Vec<Bytes>,
/// New in V2: Logs bloom accrued until but not including the last (payment) transaction.
pub el_placeholder_receipt_proof: Vec<Bytes>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Field naming change

The field was renamed from placeholder_receipt_proof to el_placeholder_receipt_proof. While this naming is more consistent with the EL/CL prefix convention used elsewhere in V3, ensure this matches the exact field name expected by the relay API. The UltraSound docs should be the source of truth for field names.

The el_ prefix makes sense as this is an execution layer proof (vs consensus layer), similar to el_transactions_root and cl_placeholder_transaction_proof.

}
}
/// Gas used by the placeholder (payout) transaction. Required for V3 to relax the
/// gas_limit == gas_used requirement.
Copy link
Contributor

Choose a reason for hiding this comment

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

New V3 field - verify relay compatibility

The placeholder_gas_used field is the key addition for V3 format. This is correctly documented as relaxing the gas_limit == gas_used requirement.

Please verify:

  1. This field name matches exactly what the relay expects
  2. The relay version you're targeting supports this field (ensure relay is updated to accept V3 format if not already)

@claude
Copy link
Contributor

claude bot commented Jan 27, 2026

Code Review Summary

Overview

This PR simplifies the bid adjustment code by unifying optimistic v1 and v3 submissions to use a single BidAdjustmentDataV3 format. The changes remove ~90 lines of code by eliminating BidAdjustmentDataV1, BidAdjustmentDataV2, and the intermediate BidAdjustmentData struct with its conversion methods.

What Changed

  1. adjustment.rs: Consolidated to a single BidAdjustmentDataV3 struct with a new placeholder_gas_used field
  2. submit_block.rs: Updated to use BidAdjustmentDataV3 for optimistic v1 submissions
  3. submit_header.rs: Updated to use BidAdjustmentDataV3 for optimistic v3 header submissions
  4. building/mod.rs: Now constructs BidAdjustmentDataV3 directly with the actual gas used from the payout transaction
  5. relay_submit.rs: Simplified by removing .into_v1() and .into_v2() conversions

Strengths

  • Code simplification: Removes duplicate data structures and conversion methods
  • Correct gas handling: Uses actual gas_used from transaction execution instead of gas_limit, which is the key benefit of V3 format
  • Consistent naming: EL/CL prefixes are now consistent across the struct

Items to Address

I've left inline comments on the following:

  1. Potential panic (building/mod.rs:1148): The expect("payout transaction must exist") is now evaluated unconditionally. Consider deferring the calculation or using lazy evaluation.

  2. Field naming verification (adjustment.rs:42, 46): The field names should be verified against the relay API specification to ensure compatibility.

Questions

  • Has this been tested against an actual relay to confirm the V3 format is accepted correctly?
  • Are there any relays that might still expect the old V1/V2 format?

Verdict

The changes look good overall. The simplification is well-motivated and the implementation is clean. Just verify the inline comments are addressed and confirm relay compatibility before merging.

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.

2 participants