fix: kurtosis blob preconfirmation flow #146
Conversation
WalkthroughThe recent changes enhance transaction handling across multiple components of the codebase, improving the calculation of transaction costs, debugging capabilities, and the robustness of proof verification processes. Notably, the updates ensure better handling of EIP-4844 transactions, enhance logging for easier issue tracing, and optimize transaction unmarshalling and marshalling procedures. These improvements enhance code maintainability, usability, and overall performance in a more complex fee environment. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Outside diff range comments (3)
bolt-sidecar/src/common.rs (2)
Line range hint
53-55:
Improve error handling for nonce validation.Consider providing more context in the error messages for nonce validation.
- return Err(ValidationError::NonceTooLow(account_state.transaction_count, transaction.nonce())); + return Err(ValidationError::NonceTooLow { + expected: account_state.transaction_count, + found: transaction.nonce(), + });
Line range hint
62-64:
Improve error handling for balance validation.Consider providing more context in the error messages for balance validation.
- return Err(ValidationError::InsufficientBalance); + return Err(ValidationError::InsufficientBalance { + required: max_transaction_cost(transaction), + available: account_state.balance, + });bolt-sidecar/src/state/execution.rs (1)
Line range hint
379-379:
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.
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (5)
- bolt-sidecar/src/common.rs (1 hunks)
- bolt-sidecar/src/state/execution.rs (3 hunks)
- builder/builder/utils_test.go (1 hunks)
- mev-boost-relay/services/api/proofs.go (2 hunks)
- mev-boost/server/service.go (2 hunks)
Additional comments not posted (10)
mev-boost-relay/services/api/proofs.go (1)
59-62: Handle potential nil pointer dereference.Ensure that
tx.HashTreeRoot()does not cause a nil pointer dereference.bolt-sidecar/src/common.rs (1)
43-45: Ensure correct handling of EIP-4844 transactions.Verify that the EIP-4844 transaction handling logic is correct and does not introduce errors.
bolt-sidecar/src/state/execution.rs (3)
248-249: Approved: Addition of debug log statement.The debug log statement provides useful information for tracing the validation process of the base fee.
328-328: Approved: Addition of debug log statement.The debug log statement provides useful information for tracing the validation process of the blob base fee.
365-365: Approved: Addition of trace log statement.The trace log statement provides essential information for understanding the state transitions within the execution context.
mev-boost/server/service.go (5)
25-25: Approved: Addition ofgethTypesimport.The import is necessary for handling Ethereum transactions in the updated implementation.
380-385: Approved: Unmarshalling raw transaction intogethTypes.Transaction.Unmarshalling the raw transaction into a
gethTypes.Transactionobject allows for more robust handling of the transaction data. The error handling ensures that any issues encountered during this process are logged appropriately.
387-392: Approved: Stripping transaction of blob sidecar and marshalling back into binary format.Stripping the transaction of its blob sidecar and marshalling it back into binary format ensures that the transaction data is handled correctly without unnecessary data transformations. The error handling ensures that any issues encountered during this process are logged appropriately.
393-395: Approved: Converting marshalled transaction back intoTransactionobject and computing hash tree root.Converting the marshalled transaction back into a
Transactionobject and computing its hash tree root is necessary for verifying the inclusion proof. The error handling ensures that any issues encountered during this process are logged appropriately.
377-395: Approved: Improved transaction handling process.The overall changes reflect a shift towards a more efficient and error-resilient transaction processing mechanism within the inclusion proof verification workflow.
|
|
||
| // TODO: this is pretty inefficient, we should work with the transaction already | ||
| // parsed without the blob here to avoid unmarshalling and marshalling again |
There was a problem hiding this comment.
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.
|
|
||
| // TODO: this is pretty inefficient, we should work with the transaction already | ||
| // parsed without the blob here to avoid unmarshalling and marshalling again |
There was a problem hiding this comment.
Consider optimizing the transaction processing.
The TODO comment indicates that the current approach is inefficient. Consider processing the transaction without unmarshalling and marshalling again.
| withoutBlob, err := transaction.WithoutBlobTxSidecar().MarshalBinary() | ||
| if err != nil { | ||
| log.WithError(err).Error("error marshalling transaction without blob tx sidecar") | ||
| return err | ||
| } |
There was a problem hiding this comment.
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.
| 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 | |
| } |
| transaction := new(types.Transaction) | ||
| err := transaction.UnmarshalBinary(constraint.Tx) | ||
| if err != nil { | ||
| log.WithError(err).Error("error unmarshalling transaction while verifying proofs") | ||
| return err | ||
| } |
There was a problem hiding this comment.
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.
| 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 | |
| } |
| /// | ||
| /// - 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`. |
There was a problem hiding this comment.
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);
+/// ```| // 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 |
There was a problem hiding this comment.
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.
| // 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. |
| byteTxs := make([]*common.HexBytes, 0, 5) | ||
| err := json.Unmarshal([]byte(raw), &byteTxs) | ||
| require.NoError(t, err) | ||
| require.Equal(t, len(byteTxs), 5) |
There was a problem hiding this comment.
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.
| 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) |
| withoutBlob, err := constraint.Tx.WithoutBlobTxSidecar().MarshalBinary() | ||
| if err != nil { | ||
| t.Logf("error marshalling transaction without blob tx sidecar: %v", err) | ||
| } |
There was a problem hiding this comment.
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.
| 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) | |
| } |
| payloadIndexes := make([]int, len(chosenConstraintTransactions)) | ||
| for i, chosenConstraint := range chosenConstraintTransactions { | ||
| for j, payloadTransaction := range payloadTransactions { | ||
| if chosenConstraint.Hash() == payloadTransaction.Hash() { | ||
| payloadIndexes[i] = j | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| 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()] | |
| } |
| // 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) | ||
| } |
There was a problem hiding this comment.
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.
| // 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) | |
| } | |
| } |
|
merging to move forward with testing blobs. adding an issue to fix the multiproofs separately. #148 |
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation