-
Notifications
You must be signed in to change notification settings - Fork 117
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
chore(all): Cumulative branch - attempt 2 #2127
base: main
Are you sure you want to change the base?
Changes from 135 commits
7b1539d
af8871c
b463d5b
844694e
af9bab6
9829574
2b1eb76
3bf2b3e
ed5c3ae
8715a04
62cbd7b
b383b0a
9ced281
825d4d3
15e37d0
b1e1edb
7b3b4b3
ffec0b3
91ab836
eea8ead
49538dc
b303196
847cd52
020cb11
92bbfae
69efa86
8b87b65
be54ae7
196f1ec
55126cb
ce0d420
49b31a8
7975424
70bba14
0365941
da314f1
36041e8
13a93ea
f13cdf7
0c6e82a
20c881e
b053357
6b106b8
e447c20
cbc89a4
699df26
6c256d4
f386d51
b2bf298
08f00c2
3ee634d
e66ef6c
d4779f9
b2aa1af
745c396
5835638
d799879
36286bb
e48be9c
30eb657
b4a1077
783f59e
c5d6f0b
b7db920
f4a7d79
0f46172
22c7717
05cba80
443ac1b
10efd5e
099716d
151a533
9818c7e
160cc88
4a9fe1c
e048be4
a960d06
8bf34db
3985ad2
d90a95a
e3ec723
e17d29c
1bce2ee
90e9cd6
0c0b3d5
4964f98
14ea836
61e3cfe
11df8e5
8dfe59d
daa9262
92d494f
6286b20
41a7657
a1a3def
b395ac2
63e4db0
36fe774
e64bc74
c6c68c3
37542a0
ea69a35
af43b93
a728bcb
97fba5c
826351d
af8c5e0
023ebfd
d66b298
43b1eb4
52c7529
420f621
561f95f
09eee8e
11c551e
72539e0
62704e5
d8a267e
3603f6b
32fbbbc
54c9b75
2c36828
23340c6
f8d281c
3fe1f7d
b2433cf
01c9d2e
a9e31f8
24570b3
c5288a9
f75009a
c7777e9
3e84eb5
6e63668
8333e76
f954810
c3d7c90
4f42385
643c7ba
e48d051
1b058b4
2e7f198
2acfa5f
2811de8
2c7f448
14913e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -68,3 +68,8 @@ packages: | |||||||||||||||||||||
recursive: False | ||||||||||||||||||||||
with-expecter: true | ||||||||||||||||||||||
all: True | ||||||||||||||||||||||
github.com/berachain/beacon-kit/mod/state-transition/pkg/core: | ||||||||||||||||||||||
config: | ||||||||||||||||||||||
recursive: False | ||||||||||||||||||||||
with-expecter: true | ||||||||||||||||||||||
include-regex: ExecutionEngine | ||||||||||||||||||||||
Comment on lines
+71
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Fix indentation to maintain consistency. The configuration looks good, but the indentation appears to be using spaces while the rest of the file uses tabs. Please maintain consistent indentation throughout the file. - github.com/berachain/beacon-kit/mod/state-transition/pkg/core:
- config:
- recursive: False
- with-expecter: true
- include-regex: ExecutionEngine
+ github.com/berachain/beacon-kit/mod/state-transition/pkg/core:
+ config:
+ recursive: False
+ with-expecter: true
+ include-regex: ExecutionEngine 📝 Committable suggestion
Suggested change
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,11 +57,9 @@ func DefaultComponents() []any { | |
*BlobSidecar, *BlobSidecars, *Logger, | ||
], | ||
components.ProvideBlobProofVerifier, | ||
components.ProvideBlobVerifier[ | ||
*BeaconBlockHeader, *BlobSidecar, *BlobSidecars, | ||
], | ||
components.ProvideChainService[ | ||
*AvailabilityStore, *BeaconBlock, *BeaconBlockBody, | ||
*AvailabilityStore, | ||
*ConsensusBlock, *BeaconBlock, *BeaconBlockBody, | ||
*BeaconBlockHeader, *BeaconState, *BeaconStateMarshallable, | ||
*BlobSidecars, *BlockStore, *Deposit, *DepositStore, | ||
*ExecutionPayload, *ExecutionPayloadHeader, *Genesis, | ||
|
@@ -91,7 +89,7 @@ func DefaultComponents() []any { | |
], | ||
components.ProvideDepositStore[*Deposit], | ||
components.ProvideDispatcher[ | ||
*BeaconBlock, *BlobSidecars, *Genesis, *Logger, | ||
*ConsensusBlock, *BeaconBlock, *BlobSidecars, *Genesis, *Logger, | ||
], | ||
components.ProvideEngineClient[ | ||
*ExecutionPayload, *ExecutionPayloadHeader, *Logger, | ||
|
@@ -107,7 +105,8 @@ func DefaultComponents() []any { | |
components.ProvideReportingService[*Logger], | ||
components.ProvideCometBFTService[*Logger], | ||
components.ProvideServiceRegistry[ | ||
*AvailabilityStore, *BeaconBlock, *BeaconBlockBody, | ||
*AvailabilityStore, | ||
*ConsensusBlock, *BeaconBlock, *BeaconBlockBody, | ||
Comment on lines
+108
to
+109
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) LGTM: Consider documenting the parameter grouping pattern. The parameter additions to Consider adding a comment above the function to explain the parameter grouping pattern, making it easier for future maintainers to follow the same convention when adding new parameters. |
||
*BeaconBlockHeader, *BlockStore, *BeaconState, | ||
*BeaconStateMarshallable, *BlobSidecar, *BlobSidecars, | ||
*Deposit, *DepositStore, *ExecutionPayload, *ExecutionPayloadHeader, | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -202,7 +202,7 @@ start-erigon: ## start an ephemeral `erigon` node | |||||||||||||
docker run \ | ||||||||||||||
--rm -v $(PWD)/${TESTAPP_FILES_DIR}:/${TESTAPP_FILES_DIR} \ | ||||||||||||||
-v $(PWD)/.tmp:/.tmp \ | ||||||||||||||
thorax/erigon:latest init \ | ||||||||||||||
erigontech/erigon:latest init \ | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider using a specific version tag instead of 'latest'. Good move switching to the official - erigontech/erigon:latest init \
+ erigontech/erigon:v2.60.9 init \
...
- erigontech/erigon:latest \
+ erigontech/erigon:v2.60.9 \ Also applies to: 215-215 |
||||||||||||||
--datadir .tmp/erigon \ | ||||||||||||||
${ETH_GENESIS_PATH} | ||||||||||||||
Comment on lines
+205
to
207
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Consider using a specific version tag instead of 'latest'. The change to use the official - erigontech/erigon:latest init \
+ erigontech/erigon:v2.60.9 init \ 📝 Committable suggestion
Suggested change
|
||||||||||||||
|
||||||||||||||
|
@@ -212,7 +212,7 @@ start-erigon: ## start an ephemeral `erigon` node | |||||||||||||
-p 8551:8551 \ | ||||||||||||||
--rm -v $(PWD)/${TESTAPP_FILES_DIR}:/${TESTAPP_FILES_DIR} \ | ||||||||||||||
-v $(PWD)/.tmp:/.tmp \ | ||||||||||||||
thorax/erigon:latest \ | ||||||||||||||
erigontech/erigon:latest \ | ||||||||||||||
--http \ | ||||||||||||||
--http.addr 0.0.0.0 \ | ||||||||||||||
--http.api eth,net \ | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,7 +93,7 @@ node_settings: | |
max_memory: 2048 | ||
images: | ||
besu: hyperledger/besu:24.5.4 | ||
erigon: thorax/erigon:v2.60.1 | ||
erigon: erigontech/erigon:v2.60.9 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Document the repository change from 'thorax' to 'erigontech'. While switching to the official repository is a good practice, please document the rationale for this change in the commit message or PR description to help with future maintenance. |
||
ethereumjs: ethpandaops/ethereumjs:stable | ||
geth: ethereum/client-go:latest | ||
nethermind: nethermind/nethermind:latest | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,7 +39,7 @@ def start(persistent_peers, is_seed, validator_index, config_settings, app_setti | |
--beacon-kit.kzg.implementation={} \ | ||
--beacon-kit.engine.rpc-dial-url {} \ | ||
--beacon-kit.block-store-service.enabled \ | ||
--beacon-kit.node-api.enabled --beacon-kit.node-api.logging \ | ||
--beacon-kit.node-api.enabled --beacon-kit.node-api.logging --beacon-kit.node-api.address 0.0.0.0:3500 \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Consider parameterizing the node API configuration. The addition of the node API configuration is consistent with the test environment setup. However, binding to Consider:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) 💡 Codebase verification Based on the code analysis, I can now generate the final response: API security needs to be enhanced before exposing to external networks The current implementation:
While binding to
🔗 Analysis chainConsider security implications and configuration flexibility of the node API. The hardcoded API address
Let's verify if this API endpoint is protected elsewhere in the codebase: Let me analyze the search results to verify the security implications of exposing the node API:
[beacon-kit.node-api]
enabled = "false"
address = "0.0.0.0:3500"
Let me verify if there are any authentication/authorization mechanisms: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any authentication/security middleware for the node API
rg -A 5 "node-api"
Length of output: 50121 Script: #!/bin/bash
# Search for auth middleware or security-related code
rg -A 5 "middleware|auth|security" mod/node-api/
Length of output: 1302 |
||
--pruning=nothing \ | ||
{} {}".format(kzg_impl, "$BEACOND_ENGINE_DIAL_URL", seed_option, persistent_peers_option) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,17 +23,16 @@ package blockchain | |
import ( | ||
"context" | ||
|
||
payloadtime "github.com/berachain/beacon-kit/mod/beacon/payload-time" | ||
engineprimitives "github.com/berachain/beacon-kit/mod/engine-primitives/pkg/engine-primitives" | ||
) | ||
|
||
// sendPostBlockFCU sends a forkchoice update to the execution client. | ||
func (s *Service[ | ||
_, BeaconBlockT, _, _, BeaconStateT, _, _, _, _, _, | ||
_, ConsensusBlockT, _, _, _, BeaconStateT, _, _, _, _, _, | ||
]) sendPostBlockFCU( | ||
ctx context.Context, | ||
st BeaconStateT, | ||
blk BeaconBlockT, | ||
blk ConsensusBlockT, | ||
) { | ||
lph, err := st.GetLatestExecutionPayloadHeader() | ||
if err != nil { | ||
|
@@ -54,18 +53,19 @@ func (s *Service[ | |
// sendNextFCUWithAttributes sends a forkchoice update to the execution | ||
// client with attributes. | ||
func (s *Service[ | ||
_, BeaconBlockT, _, _, BeaconStateT, | ||
_, ConsensusBlockT, _, _, _, BeaconStateT, | ||
_, _, ExecutionPayloadHeaderT, _, _, | ||
]) sendNextFCUWithAttributes( | ||
ctx context.Context, | ||
st BeaconStateT, | ||
blk BeaconBlockT, | ||
blk ConsensusBlockT, | ||
lph ExecutionPayloadHeaderT, | ||
) { | ||
var err error | ||
beaconBlk := blk.GetBeaconBlock() | ||
|
||
Comment on lines
+64
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Consider adding a helper method for beacon block retrieval Multiple methods perform the same block retrieval operation. Consider extracting this into a helper method with consistent error handling. +func (s *Service[_, ConsensusBlockT, ...]) getBeaconBlockWithLogging(
+ blk ConsensusBlockT,
+) (BeaconBlockT, error) {
+ beaconBlk, err := blk.GetBeaconBlock()
+ if err != nil {
+ s.logger.Error("failed to get beacon block", "error", err)
+ return nil, err
+ }
+ return beaconBlk, nil
+} Also applies to: 105-106 Add error handling for GetBeaconBlock The -beaconBlk := blk.GetBeaconBlock()
+beaconBlk, err := blk.GetBeaconBlock()
+if err != nil {
+ s.logger.Error("failed to get beacon block", "error", err)
+ return
+}
|
||
stCopy := st.Copy() | ||
if _, err = s.stateProcessor.ProcessSlots( | ||
stCopy, blk.GetSlot()+1, | ||
if _, err := s.stateProcessor.ProcessSlots( | ||
stCopy, beaconBlk.GetSlot()+1, | ||
); err != nil { | ||
s.logger.Error( | ||
"failed to process slots in non-optimistic payload", | ||
|
@@ -74,13 +74,12 @@ func (s *Service[ | |
return | ||
} | ||
|
||
prevBlockRoot := blk.HashTreeRoot() | ||
payloadTime := blk.GetBody().GetExecutionPayload().GetTimestamp() | ||
if _, err = s.localBuilder.RequestPayloadAsync( | ||
prevBlockRoot := beaconBlk.HashTreeRoot() | ||
if _, err := s.localBuilder.RequestPayloadAsync( | ||
ctx, | ||
stCopy, | ||
blk.GetSlot()+1, | ||
payloadtime.Next(s.chainSpec, payloadTime), | ||
beaconBlk.GetSlot()+1, | ||
blk.GetNextPayloadTimestamp().Unwrap(), | ||
prevBlockRoot, | ||
lph.GetBlockHash(), | ||
lph.GetParentHash(), | ||
|
@@ -96,13 +95,15 @@ func (s *Service[ | |
// sendNextFCUWithoutAttributes sends a forkchoice update to the | ||
// execution client without attributes. | ||
func (s *Service[ | ||
_, BeaconBlockT, _, _, _, _, _, | ||
_, ConsensusBlockT, _, _, _, _, _, _, | ||
ExecutionPayloadHeaderT, _, PayloadAttributesT, | ||
]) sendNextFCUWithoutAttributes( | ||
ctx context.Context, | ||
blk BeaconBlockT, | ||
blk ConsensusBlockT, | ||
lph ExecutionPayloadHeaderT, | ||
) { | ||
beaconBlk := blk.GetBeaconBlock() | ||
|
||
Comment on lines
+105
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add error handling for GetBeaconBlock Similar to the previous method, add error handling for the GetBeaconBlock call. -beaconBlk := blk.GetBeaconBlock()
+beaconBlk, err := blk.GetBeaconBlock()
+if err != nil {
+ s.logger.Error("failed to get beacon block", "error", err)
+ return
+}
|
||
if _, _, err := s.executionEngine.NotifyForkchoiceUpdate( | ||
ctx, | ||
// TODO: Switch to New(). | ||
|
@@ -113,7 +114,7 @@ func (s *Service[ | |
SafeBlockHash: lph.GetParentHash(), | ||
FinalizedBlockHash: lph.GetParentHash(), | ||
}, | ||
s.chainSpec.ActiveForkVersionForSlot(blk.GetSlot()), | ||
s.chainSpec.ActiveForkVersionForSlot(beaconBlk.GetSlot()), | ||
), | ||
); err != nil { | ||
s.logger.Error( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Package path in mockery config needs correction
The ExecutionEngine interface is defined in
mod/beacon/blockchain
package, but the mockery configuration is targetinggithub.com/berachain/beacon-kit/mod/state-transition/pkg/core
. This mismatch means the mock generation will not find the intended interface..mockery.yaml
to target the correct location:github.com/berachain/beacon-kit/mod/beacon/blockchain
🔗 Analysis chain
LGTM! Configuration follows established patterns.
The new package configuration is well-structured and consistent with other entries. The settings are appropriate for generating focused mocks of execution engine interfaces.
Let's verify that the regex pattern matches the intended interfaces:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 92
Script:
Length of output: 524