Skip to content

Conversation

nidhi-singh02
Copy link
Contributor

@nidhi-singh02 nidhi-singh02 commented Apr 17, 2025

Node Syncing endpoint implemented along with E2E tests.

https://ethereum.github.io/beacon-APIs/?urls.primaryName=v3.1.0#/Node/getSyncingStatus

Copy link

codecov bot commented Apr 17, 2025

Codecov Report

❌ Patch coverage is 11.76471% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.64%. Comparing base (9fb50c1) to head (917c398).

Files with missing lines Patch % Lines
node-api/handlers/node/sync.go 0.00% 26 Missing ⚠️
consensus/cometbft/service/abci.go 0.00% 2 Missing ⚠️
node-api/backend/backend.go 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2693      +/-   ##
==========================================
- Coverage   60.64%   60.64%   -0.01%     
==========================================
  Files         354      355       +1     
  Lines       16673    16685      +12     
==========================================
+ Hits        10111    10118       +7     
- Misses       5773     5779       +6     
+ Partials      789      788       -1     
Files with missing lines Coverage Δ
node-api/handlers/node/handler.go 100.00% <100.00%> (ø)
node-api/handlers/node/version.go 0.00% <ø> (ø)
node-core/components/api_handlers.go 100.00% <100.00%> (ø)
consensus/cometbft/service/abci.go 35.92% <0.00%> (-0.72%) ⬇️
node-api/backend/backend.go 50.00% <0.00%> (-2.78%) ⬇️
node-api/handlers/node/sync.go 0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nidhi-singh02
Copy link
Contributor Author

nidhi-singh02 commented Apr 17, 2025

I wanted to investigate why e2e tests are failing on the build (without any changes in PR related to kurtosis or e2e testing as of now), so wanted to run the tests locally on my machine to see if the behaviour is same across.
Unable to run e2e tests locally, trying through main branch. Updated docker desktop and restarting machine didn't help. Trying other ways.

nidhi-singh02 added 6 commits May 5, 2025 11:38
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
nidhi-singh02 added 2 commits May 5, 2025 14:53
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
@nidhi-singh02
Copy link
Contributor Author

Was able to fix the E2E issue where tests were not starting. Now getting assertion error in staking test

=== NAME  TestBeaconKitE2ESuite/TestDepositRobustness
    e2e_staking_test.go:274: 
        	Error Trace:	/home/runner/work/beacon-kit/beacon-kit/testing/e2e/e2e_staking_test.go:274
        	Error:      	Not equal: 
        	            	expected: 10000000000000
        	            	actual  : 6032000000000    	          

When running it on local machine, all the tests runs fine.

Signed-off-by: nidhi-singh02 <[email protected]>
@nidhi-singh02 nidhi-singh02 marked this pull request as ready for review May 6, 2025 06:36
@nidhi-singh02 nidhi-singh02 requested a review from a team as a code owner May 6, 2025 06:36
@nidhi-singh02 nidhi-singh02 requested a review from a team May 6, 2025 06:36

// GetCometNode returns the concrete CometBFT node.
func (s *Service) GetCometNode() *node.Node {
return s.node
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should we rename this consensusInstance or something like that?

baseHeight := blockStore.Base()

response := nodetypes.SyncingData{
HeadSlot: latestHeight,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would comment here that this relays on the fact that there is a one to one matching among comet blocks and payloads. Should this ever change in the future, this would not be correct anymore

Comment on lines +53 to +54
IsOptimistic: true,
ELOffline: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure how do we set these data by only looking at CometBFT.
In fact all of these data seems to me related to beaconKit state only.
Even the isSyncing quantity can be stored in `consensus/cometbft/service object by only looking at the SyncingToHeight attribute in FinalizeBlockRequest.

Copy link
Collaborator

@abi87 abi87 left a comment

Choose a reason for hiding this comment

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

It seems to me the content of this API should be served by looking at BeaconKit data only, not querying cometBFT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants