Skip to content
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

fix(sidecar): local blob building #152

Merged
merged 10 commits into from
Jul 22, 2024
191 changes: 157 additions & 34 deletions bolt-sidecar/src/api/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use parking_lot::Mutex;
use reqwest::Url;
use serde::Deserialize;
use std::{sync::Arc, time::Duration};
use thiserror::Error;
use tokio::net::TcpListener;

use super::spec::{
Expand Down Expand Up @@ -140,15 +141,12 @@ where
// On ANY error, we fall back to locally built block
tracing::warn!(slot, elapsed = ?start.elapsed(), err = ?err, "Proxy error, fetching local payload instead");

let payload_and_bid = match server.payload_fetcher.fetch_payload(slot).await {
Some(payload_and_bid) => payload_and_bid,
None => {
// TODO: handle failure? In this case, we don't have a fallback block
// which means we haven't made any commitments. This means the EL should
// fallback to local block building.
tracing::error!("No local payload produced for slot {slot}");
return Err(BuilderApiError::FailedToFetchLocalPayload(slot));
}
let Some(payload_and_bid) = server.payload_fetcher.fetch_payload(slot).await else {
// TODO: handle failure? In this case, we don't have a fallback block
// which means we haven't made any commitments. This means the EL should
// fallback to local block building.
tracing::debug!("No local payload with commitments produced for slot {slot}");
return Err(BuilderApiError::FailedToFetchLocalPayload(slot));
};

let hash = payload_and_bid.bid.message.header.block_hash.clone();
Expand All @@ -168,7 +166,7 @@ where
meta: Default::default(),
};

tracing::info!(elapsed = ?start.elapsed(), %hash, number, "Returning locally built header");
tracing::info!(elapsed = ?start.elapsed(), %hash, number, ?versioned_bid, "Returning locally built header");
Ok(Json(versioned_bid))
}

Expand All @@ -195,30 +193,11 @@ where

// If we have a locally built payload, it means we signed a local header.
// Return it and clear the cache.
if let Some(payload) = server.local_payload.lock().take() {
let requested_block = &signed_blinded_block
.message
.body
.execution_payload_header
.block_hash;

// WARNING: this is an important check. If the local block does not match what the
// beacon node has signed, we are at risk of equivocation and slashing.
if payload.block_hash() != requested_block {
tracing::error!(
expected = %requested_block.to_string(),
have = %payload.block_hash().to_string(),
"Local block hash does not match requested block hash"
);

return Err(BuilderApiError::InvalidLocalPayloadBlockHash {
expected: requested_block.to_string(),
have: payload.block_hash().to_string(),
});
};

tracing::debug!("Local block found, returning: {payload:?}");
return Ok(Json(payload));
if let Some(local_payload) = server.local_payload.lock().take() {
check_locally_built_payload_integrity(&signed_blinded_block, &local_payload)?;

tracing::debug!("Valid local block found, returning: {local_payload:?}");
return Ok(Json(local_payload));
}

// TODO: how do we deal with failures here? What if we submit the signed blinded block but don't get a response?
Expand Down Expand Up @@ -285,3 +264,147 @@ where
async fn index() -> Html<&'static str> {
Html("Hello")
}

#[derive(Error, Debug, Clone)]
pub enum LocalPayloadIntegrityError {
#[error(
"Locally built payload does not match signed header.
{field_name} mismatch: expected {expected}, have {have}"
)]
FieldMismatch {
field_name: String,
expected: String,
have: String,
},
}

/// Helper macro to compare fields of the signed header and the local block.
macro_rules! assert_payload_fields_eq {
($expected:expr, $have:expr, $field_name:ident) => {
if $expected != $have {
tracing::error!(
field_name = stringify!($field_name),
expected = %$expected,
have = %$have,
"Local block does not match signed header"
);
return Err(LocalPayloadIntegrityError::FieldMismatch {
field_name: stringify!($field_name).to_string(),
expected: $expected.to_string(),
have: $have.to_string(),
});
}
};
}

/// Perform some integrity checks on the locally built payload.
/// This is to ensure that the beacon node will accept the header that was signed
/// when we submit the full payload.
#[inline]
fn check_locally_built_payload_integrity(
signed_blinded_block: &SignedBlindedBeaconBlock,
local_payload: &GetPayloadResponse,
) -> Result<(), LocalPayloadIntegrityError> {
let header_signed_by_cl = &signed_blinded_block.message.body.execution_payload_header;
let local_execution_payload = local_payload.execution_payload();

assert_payload_fields_eq!(
&header_signed_by_cl.block_hash,
local_execution_payload.block_hash(),
BlockHash
);

assert_payload_fields_eq!(
header_signed_by_cl.block_number,
local_execution_payload.block_number(),
BlockNumber
);

assert_payload_fields_eq!(
&header_signed_by_cl.state_root,
local_execution_payload.state_root(),
StateRoot
);

assert_payload_fields_eq!(
&header_signed_by_cl.receipts_root,
local_execution_payload.receipts_root(),
ReceiptsRoot
);

assert_payload_fields_eq!(
&header_signed_by_cl.prev_randao,
local_execution_payload.prev_randao(),
PrevRandao
);

assert_payload_fields_eq!(
&header_signed_by_cl.gas_limit,
&local_execution_payload.gas_limit(),
GasLimit
);

assert_payload_fields_eq!(
&header_signed_by_cl.gas_used,
&local_execution_payload.gas_used(),
GasUsed
);

assert_payload_fields_eq!(
&header_signed_by_cl.timestamp,
&local_execution_payload.timestamp(),
Timestamp
);

assert_payload_fields_eq!(
&header_signed_by_cl.extra_data,
local_execution_payload.extra_data(),
ExtraData
);

assert_payload_fields_eq!(
&header_signed_by_cl.base_fee_per_gas,
local_execution_payload.base_fee_per_gas(),
BaseFeePerGas
);

assert_payload_fields_eq!(
&header_signed_by_cl.parent_hash,
local_execution_payload.parent_hash(),
ParentHash
);

assert_payload_fields_eq!(
&header_signed_by_cl.fee_recipient,
local_execution_payload.fee_recipient(),
FeeRecipient
);

assert_payload_fields_eq!(
&header_signed_by_cl.logs_bloom,
local_execution_payload.logs_bloom(),
LogsBloom
);

assert_payload_fields_eq!(
&header_signed_by_cl.blob_gas_used,
&local_execution_payload.blob_gas_used().unwrap_or_default(),
BlobGasUsed
);

assert_payload_fields_eq!(
&header_signed_by_cl.excess_blob_gas,
&local_execution_payload
.excess_blob_gas()
.unwrap_or_default(),
ExcessBlobGas
);

// TODO: Sanity check: recalculate transactions and withdrawals roots
// and assert them against the header

// TODO: Sanity check: verify the validator signature
// signed_blinded_block.verify_signature()?;

Ok(())
}
8 changes: 4 additions & 4 deletions bolt-sidecar/src/api/spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ pub enum BuilderApiError {
Timeout(#[from] tokio::time::error::Elapsed),
#[error("Invalid fork: {0}")]
InvalidFork(String),
#[error("Invalid local payload block hash. expected: {expected}, got: {have}")]
InvalidLocalPayloadBlockHash { expected: String, have: String },
#[error("Locally-built payload does not match expected signed header")]
LocalPayloadIntegrity(#[from] super::builder::LocalPayloadIntegrityError),
#[error("Generic error: {0}")]
Generic(String),
}
Expand Down Expand Up @@ -109,8 +109,8 @@ impl IntoResponse for BuilderApiError {
BuilderApiError::InvalidFork(err) => {
(StatusCode::BAD_REQUEST, Json(err)).into_response()
}
BuilderApiError::InvalidLocalPayloadBlockHash { .. } => {
(StatusCode::BAD_REQUEST, self.to_string()).into_response()
BuilderApiError::LocalPayloadIntegrity(err) => {
(StatusCode::BAD_REQUEST, err.to_string()).into_response()
Comment on lines +112 to +113
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Response handling for LocalPayloadIntegrity is not tested.

The implementation for BuilderApiError::LocalPayloadIntegrity correctly provides a BAD_REQUEST response with the error message. However, there are no test cases found for BuilderApiError or LocalPayloadIntegrity in the codebase, indicating that the response handling is not adequately tested.

  • Please add test cases for BuilderApiError::LocalPayloadIntegrity to ensure the response handling is verified.
Analysis chain

LGTM! Verify the response handling for LocalPayloadIntegrity.

The IntoResponse implementation for LocalPayloadIntegrity correctly provides a BAD_REQUEST response with the error message.

However, ensure that the response handling for LocalPayloadIntegrity is correctly implemented and tested.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the response handling for `LocalPayloadIntegrity` in the codebase.

# Test: Search for the response handling of `LocalPayloadIntegrity`. Expect: Proper implementation and testing.
rg --type rust 'LocalPayloadIntegrity'

Length of output: 483


Script:

#!/bin/bash
# Description: Verify the response handling and testing for `LocalPayloadIntegrityError`.

# Search for test cases related to `LocalPayloadIntegrityError`.
rg --type rust 'LocalPayloadIntegrityError' --glob '*test*'

# Search for response handling in the codebase.
rg --type rust 'into_response' -A 5

Length of output: 3474


Script:

#!/bin/bash
# Description: Verify the testing for `BuilderApiError::LocalPayloadIntegrity`.

# Search for test cases related to `BuilderApiError`.
rg --type rust 'BuilderApiError' --glob '*test*'

# Search for test cases related to `LocalPayloadIntegrity`.
rg --type rust 'LocalPayloadIntegrity' --glob '*test*'

Length of output: 103


Script:

#!/bin/bash
# Description: List all test files in the codebase to verify the presence of tests.

# List all test files in the codebase.
fd --type f --glob '*test*.rs'

Length of output: 62


Script:

#!/bin/bash
# Description: Verify the presence of tests for `BuilderApiError` and `LocalPayloadIntegrity` in `bolt-sidecar/src/test_util.rs`.

# Search for `BuilderApiError` in the test file.
rg --type rust 'BuilderApiError' bolt-sidecar/src/test_util.rs

# Search for `LocalPayloadIntegrity` in the test file.
rg --type rust 'LocalPayloadIntegrity' bolt-sidecar/src/test_util.rs

Length of output: 131

}
BuilderApiError::Generic(err) => {
(StatusCode::INTERNAL_SERVER_ERROR, Json(err)).into_response()
Expand Down
12 changes: 6 additions & 6 deletions bolt-sidecar/src/builder/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use alloy_primitives::U256;
use blst::min_pk::SecretKey;
use ethereum_consensus::{
crypto::PublicKey,
crypto::{KzgCommitment, PublicKey},
deneb::mainnet::ExecutionPayloadHeader,
ssz::prelude::{List, MerkleizationError},
};
Expand Down Expand Up @@ -95,13 +95,13 @@ impl LocalBuilder {

/// Build a new payload with the given transactions. This method will
/// cache the payload in the local builder instance, and make it available
///
pub async fn build_new_local_payload(
&mut self,
template: &BlockTemplate,
) -> Result<(), BuilderError> {
let transactions = template.as_signed_transactions();
let blobs_bundle = template.as_blobs_bundle();
let kzg_commitments = blobs_bundle.commitments.clone();

// 1. build a fallback payload with the given transactions, on top of
// the current head of the chain
Expand Down Expand Up @@ -132,7 +132,7 @@ impl LocalBuilder {
);

// 3. sign the bid with the local builder's BLS key
let signed_bid = self.create_signed_builder_bid(value, eth_header)?;
let signed_bid = self.create_signed_builder_bid(value, eth_header, kzg_commitments)?;

// 4. prepare a get_payload response for when the beacon node will ask for it
let Some(get_payload_res) =
Expand All @@ -159,20 +159,20 @@ impl LocalBuilder {

/// transform a sealed header into a signed builder bid using
/// the local builder's BLS key.
///
/// TODO: add blobs bundle
fn create_signed_builder_bid(
&self,
value: U256,
header: ExecutionPayloadHeader,
blob_kzg_commitments: Vec<KzgCommitment>,
) -> Result<SignedBuilderBid, BuilderError> {
// compat: convert from blst to ethereum consensus types
let pubkey = self.secret_key.sk_to_pk().to_bytes();
let consensus_pubkey = PublicKey::try_from(pubkey.as_slice()).expect("valid pubkey bytes");
let blob_kzg_commitments = List::try_from(blob_kzg_commitments).expect("valid list");

let message = BuilderBid {
header,
blob_kzg_commitments: List::default(),
blob_kzg_commitments,
public_key: consensus_pubkey,
value,
};
Expand Down
12 changes: 8 additions & 4 deletions bolt-sidecar/src/builder/payload_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ impl FallbackPayloadBuilder {
pub struct Context {
extra_data: Bytes,
base_fee: u64,
blob_gas_used: u64,
excess_blob_gas: u64,
prev_randao: B256,
fee_recipient: Address,
Expand All @@ -91,7 +92,6 @@ pub struct Hints {
pub gas_used: Option<u64>,
pub receipts_root: Option<B256>,
pub logs_bloom: Option<Bloom>,
pub blob_gas_used: Option<u64>,
pub state_root: Option<B256>,
pub block_hash: Option<B256>,
}
Expand Down Expand Up @@ -173,8 +173,13 @@ impl FallbackPayloadBuilder {
latest_block.header.blob_gas_used.unwrap_or_default(),
) as u64;

let blob_gas_used = transactions
.iter()
.fold(0, |acc, tx| acc + tx.blob_gas_used().unwrap_or_default());

let ctx = Context {
base_fee,
blob_gas_used,
excess_blob_gas,
parent_beacon_block_root,
prev_randao,
Expand All @@ -198,7 +203,7 @@ impl FallbackPayloadBuilder {
let header = build_header_with_hints_and_context(&latest_block, &hints, &ctx);

let sealed_header = header.seal_slow();
let sealed_block = SealedBlock::new(sealed_header.clone(), body.clone());
let sealed_block = SealedBlock::new(sealed_header, body.clone());

let block_hash = hints.block_hash.unwrap_or(sealed_block.hash());

Expand Down Expand Up @@ -359,7 +364,6 @@ pub(crate) fn build_header_with_hints_and_context(
let gas_used = hints.gas_used.unwrap_or_default();
let receipts_root = hints.receipts_root.unwrap_or_default();
let logs_bloom = hints.logs_bloom.unwrap_or_default();
let blob_gas_used = hints.blob_gas_used.unwrap_or_default();
let state_root = hints.state_root.unwrap_or_default();

Header {
Expand All @@ -379,7 +383,7 @@ pub(crate) fn build_header_with_hints_and_context(
mix_hash: context.prev_randao,
nonce: BEACON_NONCE,
base_fee_per_gas: Some(context.base_fee),
blob_gas_used: Some(blob_gas_used),
blob_gas_used: Some(context.blob_gas_used),
excess_blob_gas: Some(context.excess_blob_gas),
parent_beacon_block_root: Some(context.parent_beacon_block_root),
extra_data: context.extra_data.clone(),
Expand Down
8 changes: 8 additions & 0 deletions bolt-sidecar/src/primitives/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,14 @@ impl GetPayloadResponse {
GetPayloadResponse::Deneb(payload) => payload.execution_payload.block_hash(),
}
}

pub fn execution_payload(&self) -> &ExecutionPayload {
match self {
GetPayloadResponse::Capella(payload) => payload,
GetPayloadResponse::Bellatrix(payload) => payload,
GetPayloadResponse::Deneb(payload) => &payload.execution_payload,
}
}
}

/// A struct representing the current chain head.
Expand Down
8 changes: 5 additions & 3 deletions bolt-spammer/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ pub fn generate_random_tx() -> TransactionRequest {

/// Generate random transaction with blob (eip4844)
pub fn generate_random_blob_tx() -> TransactionRequest {
let sidecar: SidecarBuilder<SimpleCoder> = SidecarBuilder::from_slice(b"Blobs are fun!");
let random_bytes = thread_rng().gen::<[u8; 32]>();
let sidecar: SidecarBuilder<SimpleCoder> = SidecarBuilder::from_slice(random_bytes.as_slice());
let sidecar: BlobTransactionSidecar = sidecar.build().unwrap();

let dead_address = Address::from_str(DEAD_ADDRESS).unwrap();
Expand All @@ -35,10 +36,11 @@ pub fn generate_random_blob_tx() -> TransactionRequest {
.with_chain_id(KURTOSIS_CHAIN_ID)
.with_value(U256::from(100))
.with_max_fee_per_blob_gas(100u128)
.max_fee_per_gas(100u128)
.max_priority_fee_per_gas(50u128)
.max_fee_per_gas(NOICE_GAS_PRICE)
.max_priority_fee_per_gas(NOICE_GAS_PRICE / 10)
.with_gas_limit(1_000_000u128)
.with_blob_sidecar(sidecar)
.with_input(random_bytes)
}

pub fn prepare_rpc_request(method: &str, params: Vec<Value>) -> Value {
Expand Down
Loading