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: kurtosis blob preconfirmation flow #146

Merged
merged 5 commits into from
Jul 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions bolt-sidecar/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,20 @@ pub fn calculate_max_basefee(current: u128, block_diff: u64) -> Option<u128> {
}

/// Calculates the max transaction cost (gas + value) in wei.
///
/// - For EIP-1559 transactions: `max_fee_per_gas * gas_limit + tx_value`.
/// - For legacy transactions: `gas_price * gas_limit + tx_value`.
/// - For EIP-4844 blob transactions: `max_fee_per_gas * gas_limit + tx_value +
/// max_blob_fee_per_gas * blob_gas_used`.
Comment on lines +32 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve documentation clarity.

The documentation for the max_transaction_cost function is clear but can be further improved by adding examples.

/// - For legacy transactions: `gas_price * gas_limit + tx_value`.
/// - For EIP-4844 blob transactions: `max_fee_per_gas * gas_limit + tx_value +
///   max_blob_fee_per_gas * blob_gas_used`.
+/// 
+/// # Examples
+/// 
+/// ```rust
+/// let tx = PooledTransactionsElement::new(...);
+/// let cost = max_transaction_cost(&tx);
+/// ```

pub fn max_transaction_cost(transaction: &PooledTransactionsElement) -> U256 {
let gas_limit = transaction.gas_limit() as u128;

let fee_cap = transaction.max_fee_per_gas();
let fee_cap = fee_cap + transaction.max_priority_fee_per_gas().unwrap_or(0);
let mut fee_cap = transaction.max_fee_per_gas();
fee_cap += transaction.max_priority_fee_per_gas().unwrap_or(0);

Comment on lines +40 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify the fee cap calculation.

The fee cap calculation can be made clearer by explicitly stating the logic.

-  let mut fee_cap = transaction.max_fee_per_gas();
-  fee_cap += transaction.max_priority_fee_per_gas().unwrap_or(0);
+  let mut fee_cap = transaction.max_fee_per_gas() + transaction.max_priority_fee_per_gas().unwrap_or(0);
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let mut fee_cap = transaction.max_fee_per_gas();
fee_cap += transaction.max_priority_fee_per_gas().unwrap_or(0);
let mut fee_cap = transaction.max_fee_per_gas() + transaction.max_priority_fee_per_gas().unwrap_or(0);

if let Some(eip4844) = transaction.as_eip4844() {
fee_cap += eip4844.max_fee_per_blob_gas + eip4844.blob_gas() as u128;
}

U256::from(gas_limit * fee_cap) + transaction.value()
}
Expand Down
4 changes: 4 additions & 0 deletions bolt-sidecar/src/state/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,8 @@ impl<C: StateFetcher> ExecutionState<C> {
let max_basefee = calculate_max_basefee(self.basefee, slot_diff)
.ok_or(ValidationError::MaxBaseFeeCalcOverflow)?;

tracing::debug!(%slot_diff, basefee = self.basefee, %max_basefee, "Validating basefee");

// Validate the base fee
if !req.validate_basefee(max_basefee) {
return Err(ValidationError::BaseFeeTooLow(max_basefee));
Expand Down Expand Up @@ -323,6 +325,7 @@ impl<C: StateFetcher> ExecutionState<C> {
let max_blob_basefee = calculate_max_basefee(self.blob_basefee, slot_diff)
.ok_or(ValidationError::MaxBaseFeeCalcOverflow)?;

tracing::debug!(%max_blob_basefee, blob_basefee = blob_transaction.transaction.max_fee_per_blob_gas, "Validating blob basefee");
if blob_transaction.transaction.max_fee_per_blob_gas < max_blob_basefee {
return Err(ValidationError::BlobBaseFeeTooLow(max_blob_basefee));
}
Expand Down Expand Up @@ -359,6 +362,7 @@ impl<C: StateFetcher> ExecutionState<C> {

let accounts = self.account_states.keys().collect::<Vec<_>>();
let update = self.client.get_state_update(accounts, block_number).await?;
tracing::trace!(%slot, ?update, "Applying execution state update");

self.apply_state_update(update);

Expand Down
121 changes: 96 additions & 25 deletions builder/builder/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,44 +24,115 @@ func TestGenerateMerkleMultiProofs(t *testing.T) {

raw := `["0x03f9029c01830299f184b2d05e008507aef40a00832dc6c09468d30f47f19c07bccef4ac7fae2dc12fca3e0dc980b90204ef16e845000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000001e0000000000000000000000000000000000000000000000000000000000000018000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000633b68f5d8d3a86593ebb815b4663bcbe0302e31382e302d64657600000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c000000000000000000000000000000000000000000000000000000000000000e00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004109de8da2a97e37f2e6dc9f7d50a408f9344d7aa1a925ae53daf7fbef43491a571960d76c0cb926190a9da10df7209fb1ba93cd98b1565a3a2368749d505f90c81c000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c0843b9aca00e1a00141e3a338e30c49ed0501e315bcc45e4edefebed43ab1368a1505461d9cf64901a01e8511e06b17683d89eb57b9869b96b8b611f969f7f56cbc0adc2df7c88a2a07a00910deacf91bba0d74e368d285d311dc5884e7cfe219d85aea5741b2b6e3a2fe", "0x02f873011a8405f5e10085037fcc60e182520894f7eaaf75cb6ec4d0e2b53964ce6733f54f7d3ffc880b6139a7cbd2000080c080a095a7a3cbb7383fc3e7d217054f861b890a935adc1adf4f05e3a2f23688cf2416a00875cdc45f4395257e44d709d04990349b105c22c11034a60d7af749ffea2765","0xf8708305dc6885029332e35883019a2894500b0107e172e420561565c8177c28ac0f62017f8810ffb80e6cc327008025a0e9c0b380c68f040ae7affefd11979f5ed18ae82c00e46aa3238857c372a358eca06b26e179dd2f7a7f1601755249f4cff56690c4033553658f0d73e26c36fe7815", "0xf86c0785028fa6ae0082520894098d880c4753d0332ca737aa592332ed2522cd22880d2f09f6558750008026a0963e58027576b3a8930d7d9b4a49253b6e1a2060e259b2102e34a451d375ce87a063f802538d3efed17962c96fcea431388483bbe3860ea9bb3ef01d4781450fbf", "0x02f87601836384348477359400850517683ba883019a28943678fce4028b6745eb04fa010d9c8e4b36d6288c872b0f1366ad800080c080a0b6b7aba1954160d081b2c8612e039518b9c46cd7df838b405a03f927ad196158a071d2fb6813e5b5184def6bd90fb5f29e0c52671dea433a7decb289560a58416e"]`

byteTxs := make([]*common.HexBytes, 0, 3)
byteTxs := make([]*common.HexBytes, 0, 5)
err := json.Unmarshal([]byte(raw), &byteTxs)
require.NoError(t, err)
require.Equal(t, len(byteTxs), 5)
Comment on lines +27 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure proper error handling for JSON unmarshalling.

Add error handling for the JSON unmarshalling step.

-  err := json.Unmarshal([]byte(raw), &byteTxs)
-  require.NoError(t, err)
+  err := json.Unmarshal([]byte(raw), &byteTxs)
+  if err != nil {
+    t.Fatalf("error unmarshalling JSON: %v", err)
+  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
byteTxs := make([]*common.HexBytes, 0, 5)
err := json.Unmarshal([]byte(raw), &byteTxs)
require.NoError(t, err)
require.Equal(t, len(byteTxs), 5)
byteTxs := make([]*common.HexBytes, 0, 5)
err := json.Unmarshal([]byte(raw), &byteTxs)
if err != nil {
t.Fatalf("error unmarshalling JSON: %v", err)
}
require.Equal(t, len(byteTxs), 5)


payloadTransactions := common.Map(byteTxs, func(rawTx *common.HexBytes) *types.Transaction {
transaction := new(types.Transaction)
err = transaction.UnmarshalBinary([]byte(*rawTx))
return transaction
})

transactionsRaw := new([]string)
err = json.Unmarshal([]byte(raw), transactionsRaw)
require.NoError(t, err)
require.Equal(t, payloadTransactions[0].Type(), uint8(3))
require.Equal(t, payloadTransactions[1].Type(), uint8(2))
Comment on lines +38 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

Add context to type assertions.

Include context in the type assertions to help with debugging.

-  require.Equal(t, payloadTransactions[0].Type(), uint8(3))
-  require.Equal(t, payloadTransactions[1].Type(), uint8(2))
+  require.Equal(t, payloadTransactions[0].Type(), uint8(3), "first transaction type mismatch")
+  require.Equal(t, payloadTransactions[1].Type(), uint8(2), "second transaction type mismatch")
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
require.Equal(t, payloadTransactions[0].Type(), uint8(3))
require.Equal(t, payloadTransactions[1].Type(), uint8(2))
require.Equal(t, payloadTransactions[0].Type(), uint8(3), "first transaction type mismatch")
require.Equal(t, payloadTransactions[1].Type(), uint8(2), "second transaction type mismatch")


constraints := make(types.HashToConstraintDecoded, 3)
constraints[payloadTransactions[0].Hash()] = &types.ConstraintDecoded{Tx: payloadTransactions[0]}
constraints[payloadTransactions[1].Hash()] = &types.ConstraintDecoded{Tx: payloadTransactions[1]}
constraints[payloadTransactions[2].Hash()] = &types.ConstraintDecoded{Tx: payloadTransactions[2]}
// try out all combinations of "constraints":
// e.g. only [0], then [0, 1], then [1] etc...
// and log which ones are failing and which ones are not
Comment on lines +41 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify the constraints testing logic.

Add comments to clarify the logic for testing various combinations of constraints.

-  // try out all combinations of "constraints":
-  // e.g. only [0], then [0, 1], then [1] etc...
-  // and log which ones are failing and which ones are not
+  // Try out all combinations of "constraints":
+  // e.g., only [0], then [0, 1], then [1], etc.
+  // Log which combinations are failing and which ones are not.
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// try out all combinations of "constraints":
// e.g. only [0], then [0, 1], then [1] etc...
// and log which ones are failing and which ones are not
// Try out all combinations of "constraints":
// e.g., only [0], then [0, 1], then [1], etc.
// Log which combinations are failing and which ones are not.

for i := 1; i < len(payloadTransactions)+1; i++ {
t.Logf("--- Trying with %d constraints\n", i)
for _, chosenConstraintTransactions := range combinations(payloadTransactions, i) {
// find the index of the chosen constraints inside payload transactions for debugging
payloadIndexes := make([]int, len(chosenConstraintTransactions))
for i, chosenConstraint := range chosenConstraintTransactions {
for j, payloadTransaction := range payloadTransactions {
if chosenConstraint.Hash() == payloadTransaction.Hash() {
payloadIndexes[i] = j
break
}
}
}
Comment on lines +48 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimize the payload index finding logic.

The logic for finding the payload indexes can be optimized for better readability and performance.

-  for i, chosenConstraint := range chosenConstraintTransactions {
-    for j, payloadTransaction := range payloadTransactions {
-      if chosenConstraint.Hash() == payloadTransaction.Hash() {
-        payloadIndexes[i] = j
-        break
-      }
-    }
-  }
+  payloadIndexMap := make(map[common.Hash]int)
+  for j, payloadTransaction := range payloadTransactions {
+    payloadIndexMap[payloadTransaction.Hash()] = j
+  }
+  for i, chosenConstraint := range chosenConstraintTransactions {
+    payloadIndexes[i] = payloadIndexMap[chosenConstraint.Hash()]
+  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
payloadIndexes := make([]int, len(chosenConstraintTransactions))
for i, chosenConstraint := range chosenConstraintTransactions {
for j, payloadTransaction := range payloadTransactions {
if chosenConstraint.Hash() == payloadTransaction.Hash() {
payloadIndexes[i] = j
break
}
}
}
payloadIndexes := make([]int, len(chosenConstraintTransactions))
payloadIndexMap := make(map[common.Hash]int)
for j, payloadTransaction := range payloadTransactions {
payloadIndexMap[payloadTransaction.Hash()] = j
}
for i, chosenConstraint := range chosenConstraintTransactions {
payloadIndexes[i] = payloadIndexMap[chosenConstraint.Hash()]
}


inclusionProof, root, err := CalculateMerkleMultiProofs(payloadTransactions, constraints)
require.NoError(t, err)
rootHash := root.Hash()
constraints := make(types.HashToConstraintDecoded)
for _, tx := range chosenConstraintTransactions {
constraints[tx.Hash()] = &types.ConstraintDecoded{Tx: tx}
}

hashesBytes := make([][]byte, len(inclusionProof.MerkleHashes))
for i, hash := range inclusionProof.MerkleHashes {
hashesBytes[i] = (*hash)[:]
}
leavesBytes := make([][]byte, len(constraints))
for i := 0; i < len(constraints); i++ {
tx := Transaction([]byte(*byteTxs[i]))
root, err := tx.HashTreeRoot()
require.NoError(t, err)
leavesBytes[i] = root[:]
inclusionProof, root, err := CalculateMerkleMultiProofs(payloadTransactions, constraints)
require.NoError(t, err)
rootHash := root.Hash()

leaves := make([][]byte, len(constraints))

i := 0
for _, constraint := range constraints {
if constraint == nil || constraint.Tx == nil {
t.Logf("nil constraint or transaction!")
}

// Compute the hash tree root for the raw preconfirmed transaction
// and use it as "Leaf" in the proof to be verified against

withoutBlob, err := constraint.Tx.WithoutBlobTxSidecar().MarshalBinary()
if err != nil {
t.Logf("error marshalling transaction without blob tx sidecar: %v", err)
}
Comment on lines +78 to +81
Copy link
Contributor

Choose a reason for hiding this comment

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

Add error context to the log message.

When logging errors, include context about the transaction to help with debugging.

-  t.Logf("error marshalling transaction without blob tx sidecar: %v", err)
+  t.Logf("error marshalling transaction without blob tx sidecar: %v, transaction: %v", err, constraint.Tx)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
withoutBlob, err := constraint.Tx.WithoutBlobTxSidecar().MarshalBinary()
if err != nil {
t.Logf("error marshalling transaction without blob tx sidecar: %v", err)
}
withoutBlob, err := constraint.Tx.WithoutBlobTxSidecar().MarshalBinary()
if err != nil {
t.Logf("error marshalling transaction without blob tx sidecar: %v, transaction: %v", err, constraint.Tx)
}


tx := Transaction(withoutBlob)
txHashTreeRoot, err := tx.HashTreeRoot()
if err != nil {
t.Logf("error calculating hash tree root: %v", err)
}
Comment on lines +84 to +87
Copy link
Contributor

Choose a reason for hiding this comment

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

Add error context to the log message.

When logging errors, include context about the transaction to help with debugging.

-  t.Logf("error calculating hash tree root: %v", err)
+  t.Logf("error calculating hash tree root: %v, transaction: %v", err, constraint.Tx)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
txHashTreeRoot, err := tx.HashTreeRoot()
if err != nil {
t.Logf("error calculating hash tree root: %v", err)
}
txHashTreeRoot, err := tx.HashTreeRoot()
if err != nil {
t.Logf("error calculating hash tree root: %v, transaction: %v", err, constraint.Tx)
}


leaves[i] = txHashTreeRoot[:]
i++
}

hashes := make([][]byte, len(inclusionProof.MerkleHashes))
for i, hash := range inclusionProof.MerkleHashes {
hashes[i] = []byte(*hash)
}
indexes := make([]int, len(inclusionProof.GeneralizedIndexes))
for i, index := range inclusionProof.GeneralizedIndexes {
indexes[i] = int(index)
}

ok, err := fastSsz.VerifyMultiproof(rootHash[:], hashes, leaves, indexes)
if err != nil {
t.Logf("error verifying merkle proof: %v", err)
}

if !ok {
t.Logf("FAIL with txs: %v", payloadIndexes)
} else {
t.Logf("SUCCESS with txs: %v", payloadIndexes)
}
}
}
indicesInt := make([]int, len(inclusionProof.GeneralizedIndexes))
for i, index := range inclusionProof.GeneralizedIndexes {
indicesInt[i] = int(index)
}

// Function to generate combinations of a specific length
func combinations[T any](arr []T, k int) [][]T {
var result [][]T
n := len(arr)
data := make([]T, k)
combine(arr, data, 0, n-1, 0, k, &result)
return result
}
Comment on lines +116 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve code clarity for the combinations function.

Add comments to clarify the logic of the combinations function.

// Function to generate combinations of a specific length
func combinations[T any](arr []T, k int) [][]T {
+  // Initialize the result slice to store combinations
  var result [][]T
  n := len(arr)
+  // Create a temporary slice to store the current combination
  data := make([]T, k)
+  // Generate combinations using the helper function
  combine(arr, data, 0, n-1, 0, k, &result)
  return result
}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Function to generate combinations of a specific length
func combinations[T any](arr []T, k int) [][]T {
var result [][]T
n := len(arr)
data := make([]T, k)
combine(arr, data, 0, n-1, 0, k, &result)
return result
}
// Function to generate combinations of a specific length
func combinations[T any](arr []T, k int) [][]T {
// Initialize the result slice to store combinations
var result [][]T
n := len(arr)
// Create a temporary slice to store the current combination
data := make([]T, k)
// Generate combinations using the helper function
combine(arr, data, 0, n-1, 0, k, &result)
return result
}


// Helper function to generate combinations
func combine[T any](arr, data []T, start, end, index, k int, result *[][]T) {
if index == k {
tmp := make([]T, k)
copy(tmp, data)
*result = append(*result, tmp)
return
}

fastSsz.VerifyMultiproof(rootHash, hashesBytes, leavesBytes, indicesInt)
for i := start; i <= end && end-i+1 >= k-index; i++ {
data[index] = arr[i]
combine(arr, data, i+1, end, index+1, k, result)
}
Comment on lines +125 to +137
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve code clarity for the combine function.

Add comments to clarify the logic of the combine function.

// Helper function to generate combinations
func combine[T any](arr, data []T, start, end, index, k int, result *[][]T) {
  if index == k {
+    // Combination of the required length is generated, add it to the result
    tmp := make([]T, k)
    copy(tmp, data)
    *result = append(*result, tmp)
    return
  }

+  // Generate combinations by including elements from the array
  for i := start; i <= end && end-i+1 >= k-index; i++ {
    data[index] = arr[i]
    combine(arr, data, i+1, end, index+1, k, result)
  }
}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Helper function to generate combinations
func combine[T any](arr, data []T, start, end, index, k int, result *[][]T) {
if index == k {
tmp := make([]T, k)
copy(tmp, data)
*result = append(*result, tmp)
return
}
fastSsz.VerifyMultiproof(rootHash, hashesBytes, leavesBytes, indicesInt)
for i := start; i <= end && end-i+1 >= k-index; i++ {
data[index] = arr[i]
combine(arr, data, i+1, end, index+1, k, result)
}
// Helper function to generate combinations
func combine[T any](arr, data []T, start, end, index, k int, result *[][]T) {
if index == k {
// Combination of the required length is generated, add it to the result
tmp := make([]T, k)
copy(tmp, data)
*result = append(*result, tmp)
return
}
// Generate combinations by including elements from the array
for i := start; i <= end && end-i+1 >= k-index; i++ {
data[index] = arr[i]
combine(arr, data, i+1, end, index+1, k, result)
}
}

}
19 changes: 18 additions & 1 deletion mev-boost-relay/services/api/proofs.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"time"

"github.com/attestantio/go-eth2-client/spec/phase0"
"github.com/ethereum/go-ethereum/core/types"
fastSsz "github.com/ferranbt/fastssz"
"github.com/flashbots/mev-boost-relay/common"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -39,7 +40,23 @@ func verifyInclusionProof(log *logrus.Entry, transactionsRoot phase0.Root, proof

// Compute the hash tree root for the raw preconfirmed transaction
// and use it as "Leaf" in the proof to be verified against
tx := Transaction(constraint.Tx)

// TODO: this is pretty inefficient, we should work with the transaction already
// parsed without the blob here to avoid unmarshalling and marshalling again
Comment on lines +43 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider optimizing the transaction processing.

The TODO comment indicates that the current approach is inefficient. Consider processing the transaction without unmarshalling and marshalling again.

transaction := new(types.Transaction)
err := transaction.UnmarshalBinary(constraint.Tx)
if err != nil {
log.WithError(err).Error("error unmarshalling transaction while verifying proofs")
return err
}
Comment on lines +46 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

Add error context to the log message.

When logging errors, include context about the transaction to help with debugging.

-  log.WithError(err).Error("error unmarshalling transaction while verifying proofs")
+  log.WithError(err).WithField("transaction", constraint.Tx).Error("error unmarshalling transaction while verifying proofs")
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
transaction := new(types.Transaction)
err := transaction.UnmarshalBinary(constraint.Tx)
if err != nil {
log.WithError(err).Error("error unmarshalling transaction while verifying proofs")
return err
}
transaction := new(types.Transaction)
err := transaction.UnmarshalBinary(constraint.Tx)
if err != nil {
log.WithError(err).WithField("transaction", constraint.Tx).Error("error unmarshalling transaction while verifying proofs")
return err
}


withoutBlob, err := transaction.WithoutBlobTxSidecar().MarshalBinary()
if err != nil {
log.WithError(err).Error("error marshalling transaction without blob tx sidecar")
return err
}
Comment on lines +53 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

Add error context to the log message.

When logging errors, include context about the transaction to help with debugging.

-  log.WithError(err).Error("error marshalling transaction without blob tx sidecar")
+  log.WithError(err).WithField("transaction", transaction).Error("error marshalling transaction without blob tx sidecar")
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
withoutBlob, err := transaction.WithoutBlobTxSidecar().MarshalBinary()
if err != nil {
log.WithError(err).Error("error marshalling transaction without blob tx sidecar")
return err
}
withoutBlob, err := transaction.WithoutBlobTxSidecar().MarshalBinary()
if err != nil {
log.WithError(err).WithField("transaction", transaction).Error("error marshalling transaction without blob tx sidecar")
return err
}


tx := Transaction(withoutBlob)
txHashTreeRoot, err := tx.HashTreeRoot()
if err != nil {
return ErrInvalidRoot
Expand Down
19 changes: 18 additions & 1 deletion mev-boost/server/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
eth2ApiV1Capella "github.com/attestantio/go-eth2-client/api/v1/capella"
eth2ApiV1Deneb "github.com/attestantio/go-eth2-client/api/v1/deneb"
"github.com/attestantio/go-eth2-client/spec/phase0"
gethTypes "github.com/ethereum/go-ethereum/core/types"
fastSsz "github.com/ferranbt/fastssz"
"github.com/flashbots/go-boost-utils/ssz"
"github.com/flashbots/go-boost-utils/types"
Expand Down Expand Up @@ -373,7 +374,23 @@ func (m *BoostService) verifyInclusionProof(responsePayload *BidWithInclusionPro

// Compute the hash tree root for the raw preconfirmed transaction
// and use it as "Leaf" in the proof to be verified against
tx := Transaction(constraint.Tx)

// TODO: this is pretty inefficient, we should work with the transaction already
// parsed without the blob here to avoid unmarshalling and marshalling again
Comment on lines +377 to +379
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider prioritizing the optimization mentioned in the TODO.

The current approach of unmarshalling and marshalling transactions is inefficient and can be optimized to avoid unnecessary data transformations.

transaction := new(gethTypes.Transaction)
err := transaction.UnmarshalBinary(constraint.Tx)
if err != nil {
log.WithError(err).Error("error unmarshalling transaction while verifying proofs")
return err
}

withoutBlob, err := transaction.WithoutBlobTxSidecar().MarshalBinary()
if err != nil {
log.WithError(err).Error("error marshalling transaction without blob tx sidecar")
return err
}

tx := Transaction(withoutBlob)
txHashTreeRoot, err := tx.HashTreeRoot()
if err != nil {
return errInvalidRoot
Expand Down
Loading