Skip to content

make delivered payload optional #667

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

dvush
Copy link
Contributor

@dvush dvush commented Jul 22, 2025

πŸ“ Summary

πŸ’‘ Motivation and Context


βœ… I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

@Copilot Copilot AI review requested due to automatic review settings July 22, 2025 09:07
@dvush dvush requested a review from ZanCorDX as a code owner July 22, 2025 09:07
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR makes the delivered payload optional in the historical data fetching process. It adds a configuration option to allow fallback behavior when payload delivery information cannot be retrieved from relays.

  • Added a configurable flag require_delivered_payload_from_relays to control whether payload delivery is mandatory
  • Implemented fallback logic that generates a fake delivered payload when relay data is unavailable
  • Added error handling and warning logging for failed payload delivery fetches

block_hash: block.header.hash,
builder_pubkey: Default::default(),
proposer_pubkey: Default::default(),
// insignificant random address
Copy link
Preview

Copilot AI Jul 22, 2025

Choose a reason for hiding this comment

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

The comment 'insignificant random address' is vague and doesn't explain why this specific address was chosen or its purpose. Consider explaining what this address represents or documenting the criteria for its selection.

Suggested change
// insignificant random address
// Placeholder address for testing purposes; has no functional significance

Copilot uses AI. Check for mistakes.

Comment on lines +340 to +341
// assume that payload is generated 3 seconds after the slot timestamp
timestamp_ms: (block.header.timestamp + 3) * 1000,
Copy link
Preview

Copilot AI Jul 22, 2025

Choose a reason for hiding this comment

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

The hardcoded 3-second offset appears to be a magic number. Consider defining this as a named constant with documentation explaining the rationale behind this timing assumption.

Suggested change
// assume that payload is generated 3 seconds after the slot timestamp
timestamp_ms: (block.header.timestamp + 3) * 1000,
// assume that payload is generated after a fixed offset defined by PAYLOAD_GENERATION_OFFSET_SECONDS
timestamp_ms: (block.header.timestamp + PAYLOAD_GENERATION_OFFSET_SECONDS) * 1000,

Copilot uses AI. Check for mistakes.

Comment on lines +323 to +325
fn generate_fake_delivered_payload(block: &Block) -> BuilderBlockReceived {
BuilderBlockReceived {
slot: 0,
Copy link
Preview

Copilot AI Jul 22, 2025

Choose a reason for hiding this comment

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

Using a hardcoded slot value of 0 in the fake payload may not accurately represent the actual slot for the block. Consider calculating or deriving the appropriate slot value based on the block timestamp and genesis time.

Suggested change
fn generate_fake_delivered_payload(block: &Block) -> BuilderBlockReceived {
BuilderBlockReceived {
slot: 0,
fn generate_fake_delivered_payload(block: &Block, genesis_time: u64, slot_duration: u64) -> BuilderBlockReceived {
let slot = (block.header.timestamp - genesis_time) / slot_duration;
BuilderBlockReceived {
slot,

Copilot uses AI. Check for mistakes.

@dvush dvush merged commit 96bde39 into develop Aug 2, 2025
3 checks passed
@dvush dvush deleted the backtest_do_not_require_deliveder_payload_2025-07-17 branch August 2, 2025 09:42
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