fix(devnet): better support of multiple preconfs in blocks #162
Conversation
WalkthroughThe recent updates enhance various components of the project, focusing on improved transaction handling and configuration adjustments. The functionality for sending preconfirmations has been refined with new count parameters, allowing users to specify the number of preconfirmations. Additionally, configuration values and dependencies have been updated to improve performance and scalability. Collectively, these changes streamline operations and strengthen the overall robustness of the system. Changes
Assessment against linked issues
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 (
|
thedevbirb
changed the title
thedevbirb
added
C: mev-boost
thedevbirb
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range comments (1)
mev-boost/server/service.go (1)
Line range hint
399-407:
Increment loop variable only once.The loop variable
ishould be incremented only once per iteration.- leaves[i] = txHashTreeRoot[:] - i++ + leaves[i] = txHashTreeRoot[:]
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (3)
builder/go.sumis excluded by!**/*.summev-boost-relay/go.sumis excluded by!**/*.summev-boost/go.sumis excluded by!**/*.sum
Files selected for processing (18)
- Justfile (2 hunks)
- bolt-sidecar/src/config/mod.rs (1 hunks)
- bolt-spammer/src/main.rs (2 hunks)
- bolt-spammer/src/utils.rs (2 hunks)
- builder/Dockerfile (1 hunks)
- builder/Dockerfile.alltools (1 hunks)
- builder/builder/utils.go (2 hunks)
- builder/core/types/constraints.go (2 hunks)
- builder/go.mod (3 hunks)
- builder/miner/worker.go (2 hunks)
- mev-boost-relay/go.mod (2 hunks)
- mev-boost-relay/services/api/proofs.go (2 hunks)
- mev-boost-relay/services/api/service.go (1 hunks)
- mev-boost-relay/services/api/types.go (2 hunks)
- mev-boost/Dockerfile (2 hunks)
- mev-boost/go.mod (3 hunks)
- mev-boost/server/constraints.go (5 hunks)
- mev-boost/server/service.go (2 hunks)
Additional comments not posted (45)
mev-boost/Dockerfile (1)
2-2: Verify compatibility with the new base image.The base image has been updated from
golang:1.21-alpinetogolang:1.22-alpine. Ensure that the new Go version does not introduce any breaking changes or incompatibilities.Verification successful
Compatibility with Go 1.22 verified.
The
go.modfiles for the relevant modules (builder,mev-boost,mev-boost-relay) specifygo 1.22, indicating that the codebase is intended to be compatible with Go 1.22. No immediate concerns regarding dependencies were found.
builder/go.modmev-boost/go.modmev-boost-relay/go.modScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify compatibility with the new base image. # Test: Build the Docker image and run the application. Expect: No errors. docker build -t mev-boost-test -f mev-boost/Dockerfile . docker run --rm mev-boost-test /app/mev-boost --versionLength of output: 264
Script:
#!/bin/bash # Search for Go version-specific code or dependencies in the codebase. rg 'go1\.(2[0-2]|1[0-9])' . # Locate the go.mod file to check for any specific version constraints or dependencies. fd go.modLength of output: 2451
Script:
#!/bin/bash # Inspect the go.mod files for version constraints and dependencies. # Output the contents of the go.mod files to check for any specific version constraints or dependencies. cat builder/go.mod cat mev-boost/go.mod cat mev-boost-relay/go.modLength of output: 17614
builder/Dockerfile.alltools (1)
7-7: Verify compatibility with the new base image.The base image has been updated from
golang:1.21-alpinetogolang:1.22-alpine. Ensure that the new Go version does not introduce any breaking changes or incompatibilities.Verification successful
The verification confirms compatibility with the new base image.
The upgrade from
golang:1.21-alpinetogolang:1.22-alpinedoes not introduce any breaking changes or incompatibilities based on the codebase analysis.
- The codebase does not contain Go version-specific dependencies or build constraints related to Go 1.21.
- No Go version-specific features or APIs that might be impacted by the upgrade were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify compatibility with the new base image. # Test: Build the Docker image and run the application. Expect: No errors. docker build -t geth-alltools-test -f builder/Dockerfile.alltools . docker run --rm geth-alltools-test /usr/local/bin/geth versionLength of output: 282
Script:
#!/bin/bash # Description: Check for Go version-specific features or dependencies in the codebase that might be impacted by the upgrade to Go 1.22. # Find all Go module files to check for dependencies that might be affected by Go version changes. fd -e go.mod --exec cat {} \; # Search for any Go version-specific build constraints in the codebase. rg '// +build go1.21' -A 2Length of output: 63
Script:
#!/bin/bash # Description: Check for Go version-specific features or APIs used in the codebase that might be impacted by the upgrade to Go 1.22. # Search for Go version-specific features or APIs in the codebase. rg 'go1\.21' -A 2Length of output: 1802
builder/Dockerfile (1)
7-7: Verify compatibility with the new base image.The base image has been updated from
golang:1.21-alpinetogolang:1.22-alpine. Ensure that the new Go version does not introduce any breaking changes or incompatibilities.bolt-spammer/src/utils.rs (2)
22-22: LGTM! The gas limit change is appropriate.The new gas limit of 21,000 is more realistic for standard ETH transfers.
41-41: LGTM! The gas limit change is appropriate.The new gas limit of 42,000 is more realistic for blob transactions.
mev-boost-relay/services/api/proofs.go (3)
27-28: Parsing constraints decoded.The
ParseConstraintsDecodedfunction is used to parse the newhashToConstraintsinput. Ensure that this function is correctly implemented and used.
31-31: Initializing leaves array.The leaves array is initialized with the length of the parsed constraints. Ensure that this initialization is correct and efficient.
38-38: Marshalling transaction without blob.The transaction is marshalled without the blob sidecar. Ensure that this operation is correct and efficient.
builder/core/types/constraints.go (6)
4-4: Importing sort package.The
sortpackage is imported for sorting operations. Ensure that the import is necessary and used correctly.
34-42: Processing constraints and accumulating gas requirements.The function processes constraints and accumulates their gas requirements. Ensure that this logic is correct and efficient.
44-49: Sorting constraints by index.The indexed constraints are sorted in ascending order. Ensure that the sorting operation is correct and efficient.
50-59: Sorting unindexed constraints by nonce and hash.The unindexed constraints are sorted first by nonce and then by hash. Ensure that the sorting operation is correct and efficient.
61-61: Returning processed constraints and gas requirements.The function returns the processed constraints and their gas requirements. Ensure that the returned values are correct and used appropriately.
26-26: New functionParseConstraintsDecodedadded.The function processes constraints and returns multiple outputs. Ensure that the function is correctly implemented and used.
mev-boost-relay/services/api/types.go (3)
48-48: LGTM!The type alias
HashToConstraintDecodedis straightforward and correctly defined.
49-52: LGTM!The struct
ConstraintDecodedis well-defined and correctly encapsulates the necessary fields.
55-92: LGTM!The function
ParseConstraintsDecodedis well-structured and follows a clear logic for sorting and concatenating constraints. The use of slices and sorting functions is appropriate.bolt-spammer/src/main.rs (2)
36-37: LGTM!The new field
countin the structOptsis appropriately defined with the correct attributes for command-line parsing.
58-98: LGTM!The changes to the
mainfunction are well-structured and correctly implement the logic for sending multiple transactions. The use of the loop and the increment of the nonce are appropriate.mev-boost/go.mod (3)
3-3: LGTM!The update to the Go version from 1.21 to 1.22 is appropriate and ensures compatibility with newer features and improvements.
72-72: LGTM!The update to the
fastsszdependency is likely to include bug fixes or enhancements relevant to serialization in the context of Ethereum-related operations.
31-31: LGTM!The addition of the
dotdependency suggests an expansion of functionality, potentially related to graph representation or visualization.mev-boost/server/constraints.go (8)
4-8: LGTM! Import statements updated correctly.The addition of the
slicespackage and aliasingcommonasgethCommonare appropriate for the changes made in the file.
46-46: LGTM! UpdatedConstraintCachestruct for consistency.The
constraintsfield now usesgethCommon.Hash, ensuring uniformity in hash type usage.
52-52: LGTM! UpdatedNewConstraintCachefunction for consistency.The cache now uses
gethCommon.Hash, aligning with the updatedConstraintCachestruct.
61-61: LGTM! UpdatedAddInclusionConstraintmethod for consistency.The method now uses
gethCommon.Hash, ensuring uniformity in hash type usage.
84-84: LGTM! UpdatedAddInclusionConstraintsmethod for consistency.The method now uses
gethCommon.Hash, ensuring uniformity in hash type usage.
101-101: LGTM! UpdatedGetmethod for consistency.The method now returns a map with
gethCommon.Hash, aligning with the updatedConstraintCachestruct.
106-106: LGTM! UpdatedFindTransactionByHashmethod for consistency.The method now uses
gethCommon.Hash, ensuring uniformity in hash type usage.
123-161: LGTM! AddedParseConstraintsDecodedfunction.The function is well-structured and efficiently processes and sorts constraints using modern Go practices.
mev-boost-relay/go.mod (2)
48-48: LGTM! Added new indirect dependency.The addition of
github.com/emicklei/dot v1.6.2might introduce useful functionality related to dot language parsing or graph visualization.
95-95: LGTM! Updated dependency version.The update to
github.com/ferranbt/fastsszfromv0.1.3tov0.1.4-0.20240724090034-31cd371f8688likely includes important improvements or new features.Justfile (2)
113-122: LGTM! Addedsend-preconf-countfunction.The function is well-structured and extends the existing functionality to support multiple preconfirmations using the
--countparameter.
133-143: LGTM! Addedsend-blob-preconf-countfunction.The function is well-structured and extends the existing functionality to support multiple blob preconfirmations using the
--countparameter.builder/builder/utils.go (3)
159-160: Improved parameter naming.Renaming the parameter from
constraintstoHashToConstraintDecodedenhances readability and clarity.
161-162: Efficient constraint handling.The logic for parsing and concatenating constraints improves the organization and handling of constraints within the function.
192-193: Simplified loop declaration.Using the index variable
idirectly in the loop declaration simplifies the control flow and enhances readability.builder/go.mod (3)
3-3: Go version update.Updating the Go version from 1.20 to 1.22 ensures the project benefits from the latest features, performance improvements, and bug fixes.
29-29: Dependency update.Updating the
fastsszdependency to a newer version may include important fixes or performance improvements.
89-89: New indirect dependency.The addition of the
github.com/emicklei/dotdependency suggests new functionality or features that require this package.bolt-sidecar/src/config/mod.rs (1)
149-149: Increased default value formax_commitments_per_slot.Increasing the default value from
6to128significantly enhances the capacity for commitments per slot, likely improving performance and scalability.mev-boost/server/service.go (2)
346-348: Log a warning when no constraints are found for the slot.The log message provides useful feedback when constraints are absent, improving error handling.
386-398: Optimize transaction encoding.The TODO comment suggests optimizing the transaction encoding process to avoid unnecessary unmarshalling and marshalling.
- // TODO: this is pretty inefficient, we should work with the transaction already - // parsed without the blob here to avoid unmarshalling and marshalling againLikely invalid or redundant comment.
builder/miner/worker.go (2)
1036-1039: Encapsulate constraint parsing logic intypes.ParseConstraintsDecoded.The refactoring to use
types.ParseConstraintsDecoded(constraints)improves code readability and maintainability. Ensure thatParseConstraintsDecodedhandles all edge cases correctly.
1164-1164: Usecommon.Shiftfor removing elements.The change from
common.Poptocommon.Shiftimplies a shift in the method of accessing elements, potentially altering the order in which constraints are processed. Verify that this change does not introduce any unintended side effects.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- builder/builder/utils.go (2 hunks)
- mev-boost/server/service.go (2 hunks)
Additional comments not posted (9)
builder/builder/utils.go (5)
159-159: Parameter name change improves clarityThe parameter
constraintshas been renamed toHashToConstraintDecoded, which clarifies its role in the function.
161-162: Efficiently parse and concatenate constraintsThe function now parses
HashToConstraintDecodedintoconstraintsOrderedByIndexandconstraintsWithoutIndex, and concatenates them into a singleconstraintsslice. This improves the organization of constraint handling.
192-193: Directly reference transactions for accuracyThe loop now directly references the transaction (
tx) associated with the constraint, which increases the accuracy of the transaction verification againstpayloadTransactions.
195-197: Error handling for missing preconfirmed transactionsThe error logging within the loop has been updated to reflect the new variable names, ensuring that error messages are both consistent and informative. Specifically, the error messages now reference
tx.Hash()instead of the previoushash, providing clearer context when a preconfirmed transaction is not found.
204-204: Redundant increment statementThe increment statement
i++is redundant as the loop declaration already handles the index increment.- i++Likely invalid or redundant comment.
mev-boost/server/service.go (4)
346-346: Check for existence of inclusion constraintsA new check has been introduced to verify the existence of inclusion constraints linked to a specific slot, with a warning logged if none are found. This addition improves the robustness of error handling by offering clearer feedback when constraints are missing.
367-381: Organized mapping structure for constraintsThe logic for processing the inclusion constraints has been restructured. The previous implementation directly created a slice of byte arrays (
leaves) from the inclusion constraints. This has been replaced by a more organized mapping structure,hashToConstraint, which links transaction hashes to their decoded constraints. This change enhances the organization of constraints and facilitates subsequent processing by parsing the decoded constraints into a structured format.
386-393: Optimize transaction processingThe transaction unmarshalling process has been updated to ensure that each constraint's transaction is unmarshalled and stored in the new mapping. The error handling during unmarshalling remains intact, ensuring that any encountered issues are logged appropriately.
384-385: Remove redundant lineThe line initializing the
leavesslice is redundant and can be removed.- leaves := make([][]byte, len(constraints))Likely invalid or redundant comment.
| .await?; | ||
|
|
||
| info!("Response: {:?}", response.text().await?); | ||
| for i in 0..opts.count { |
There was a problem hiding this comment.
there is no delay between spamming? should we do something like 100 ms? just an idea
There was a problem hiding this comment.
For the devnet I think there is no reason to add a delay, but yeah I should bring these changes for helder testnet and it might be worth to add configurable delay between preconfs, with a 100ms default
There was a problem hiding this comment.
Actually, should I do that for Helder or wait for #163 to be done?
There was a problem hiding this comment.
waiting is fine, it's not a priority
| // ParseConstraintsDecoded receives a map of constraints and | ||
| // - creates a slice of constraints sorted by index | ||
| // - creates a slice of constraints without index sorted by nonce and hash | ||
| // Returns the concatenation of the slices | ||
| func ParseConstraintsDecoded(constraints HashToConstraintDecoded) []*ConstraintDecoded { |
There was a problem hiding this comment.
can you add a test for this fn? either in the relay or mev-boost is fine
There was a problem hiding this comment.
Done! Lmk if it's good enough
…s for multiple preconfs
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- Justfile (1 hunks)
Additional comments not posted (1)
Justfile (1)
115-123: Good enhancement! Consider setting a default value for thecountparameter.The addition of the
countparameter improves the flexibility of the command. To ensure clarity, consider setting a default value for thecountparameter within the command itself.- send-blob-preconf count='1': + send-blob-preconf count='1'='1':Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- mev-boost/server/constraints_test.go (1 hunks)
Additional comments not posted (3)
mev-boost/server/constraints_test.go (3)
11-22: LGTM! Initialization of raw transaction data is clear and well-documented.The raw transaction data is correctly initialized and documented with comments.
26-44: LGTM! Decoding process and error handling are correct.The decoding process uses
hex.DecodeStringandtx.UnmarshalBinary, followed by error handling withrequire.NoError. The logic for setting the index for the first two transactions is correct.
46-52: LGTM! Verification process is clear and correctly validates the expected values.The verification process checks the nonce and hash values of the parsed constraints using
require.Equal.
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range comments (1)
mev-boost-relay/services/api/proofs.go (1)
Line range hint
46-50:
Handle potential errors inHashTreeRoot.The function should log the error with more context to aid debugging.
- if err != nil { - return ErrInvalidRoot - } + if err != nil { + log.WithError(err).WithField("index", i).Error("error computing hash tree root") + return ErrInvalidRoot + }
| // | ||
| // NOTE: assumes constraints transactions are already without blobs | ||
| func verifyInclusionProof(log *logrus.Entry, transactionsRoot phase0.Root, proof *common.InclusionProof, hashToConstraints HashToConstraintDecoded) error { |
There was a problem hiding this comment.
Clarify the note about blobs.
The note about constraints transactions being without blobs should be clarified or moved to a more appropriate place in the documentation.
-// NOTE: assumes constraints transactions are already without blobs
+// NOTE: This function assumes that the constraints transactions are already without blobs.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.
| // | |
| // NOTE: assumes constraints transactions are already without blobs | |
| func verifyInclusionProof(log *logrus.Entry, transactionsRoot phase0.Root, proof *common.InclusionProof, hashToConstraints HashToConstraintDecoded) error { | |
| // | |
| // NOTE: This function assumes that the constraints transactions are already without blobs. | |
| func verifyInclusionProof(log *logrus.Entry, transactionsRoot phase0.Root, proof *common.InclusionProof, hashToConstraints HashToConstraintDecoded) error { |
| for i, constraint := range constraints { | ||
| if constraint == nil { | ||
| return ErrNilConstraint |
There was a problem hiding this comment.
Handle potential nil constraints.
The loop checks for nil constraints, but it should log the error with more context to aid debugging.
- if constraint == nil {
- return ErrNilConstraint
- }
+ if constraint == nil {
+ log.WithField("index", i).Error("nil constraint found")
+ return ErrNilConstraint
+ }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.
| for i, constraint := range constraints { | |
| if constraint == nil { | |
| return ErrNilConstraint | |
| for i, constraint := range constraints { | |
| if constraint == nil { | |
| log.WithField("index", i).Error("nil constraint found") | |
| return ErrNilConstraint | |
| } |
| encoded, err := constraint.Tx.MarshalBinary() | ||
| if err != nil { | ||
| log.WithError(err).Error("error marshalling transaction without blob tx sidecar") | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
Improve error logging for marshalling transactions.
When logging errors, include the index of the constraint to provide more context.
- log.WithError(err).Error("error marshalling transaction without blob tx sidecar")
+ log.WithError(err).WithField("index", i).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.
| encoded, err := constraint.Tx.MarshalBinary() | |
| if err != nil { | |
| log.WithError(err).Error("error marshalling transaction without blob tx sidecar") | |
| return err | |
| } | |
| encoded, err := constraint.Tx.MarshalBinary() | |
| if (err != nil) { | |
| log.WithError(err).WithField("index", i).Error("error marshalling transaction without blob tx sidecar") | |
| return err | |
| } |
| func verifyInclusionProof(log *logrus.Entry, transactionsRoot phase0.Root, proof *common.InclusionProof, constraints map[phase0.Hash32]*Constraint) error { | ||
| // | ||
| // NOTE: assumes constraints transactions are already without blobs | ||
| func verifyInclusionProof(log *logrus.Entry, transactionsRoot phase0.Root, proof *common.InclusionProof, hashToConstraints HashToConstraintDecoded) error { |
There was a problem hiding this comment.
Tip
Codebase Verification
Update the function call to use hashToConstraints.
Ensure that all calls to verifyInclusionProof are updated to use the new hashToConstraints parameter instead of the old constraints parameter.
mev-boost/server/service.gomev-boost-relay/services/api/service.go
Analysis chain
Update the function call to use hashToConstraints.
Ensure that all calls to verifyInclusionProof are updated to use the new hashToConstraints parameter instead of the old constraints parameter.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the updated `verifyInclusionProof` function.
# Test: Search for the function usage. Expect: Only occurances of the new signature.
rg --type go -A 5 $'verifyInclusionProof'
Length of output: 2270
| constraints := ParseConstraintsDecoded(hashToConstraints) | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure ParseConstraintsDecoded handles errors.
The function ParseConstraintsDecoded in mev-boost-relay/services/api/types.go does not handle any potential errors during parsing and sorting. Consider adding error handling to ensure robustness.
mev-boost-relay/services/api/types.go:ParseConstraintsDecoded
Analysis chain
Ensure ParseConstraintsDecoded handles errors.
The function ParseConstraintsDecoded should handle any potential errors during parsing. Verify its implementation to ensure robustness.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of `ParseConstraintsDecoded`.
# Test: Search for the implementation of `ParseConstraintsDecoded`. Expect: Proper error handling.
rg --type go 'func ParseConstraintsDecoded'
Length of output: 456
Script:
#!/bin/bash
# Extract the full implementation of `ParseConstraintsDecoded` from `mev-boost-relay/services/api/types.go`
rg --type go -A 20 'func ParseConstraintsDecoded' mev-boost-relay/services/api/types.go
Length of output: 1005
Script:
#!/bin/bash
# Extract the full implementation of `ParseConstraintsDecoded` from `mev-boost-relay/services/api/types.go`
rg --type go -A 50 'func ParseConstraintsDecoded' mev-boost-relay/services/api/types.go
Length of output: 1450
This PR introduces a better support for multiple (>2) preconfirmations in a block, which was mostly not working before. This has been achieved with the following steps:
--countflag; an appropriatejustfilecommand has been introducedThe code has been tested in the follwing scenarios:
closes #148
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
Dependency Updates
Refactor
Tests