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: resolve the errors in GetLightClientUpdatesByRange function #14171

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

rupam-04
Copy link

@rupam-04 rupam-04 commented Jul 2, 2024

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

  1. Remove this line
  2. Return HandleError in places where continue is used inside if err != nil
  3. Make this requirement 1 vote and not 66%

Which issues(s) does this PR fix?

Fixes #14170

Other notes for review

I am yet to add unit tests for the 3rd change

@rupam-04 rupam-04 requested a review from a team as a code owner July 2, 2024 14:59
@CLAassistant
Copy link

CLAassistant commented Jul 2, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@rkapka rkapka left a comment

Choose a reason for hiding this comment

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

Let's not merge without tests to make sure we don't forget about them

beacon-chain/rpc/eth/light-client/handlers.go Outdated Show resolved Hide resolved
beacon-chain/rpc/eth/light-client/handlers.go Show resolved Hide resolved
beacon-chain/rpc/eth/light-client/handlers.go Show resolved Hide resolved
beacon-chain/rpc/eth/light-client/handlers.go Show resolved Hide resolved
@rupam-04
Copy link
Author

rupam-04 commented Jul 2, 2024

Let's not merge without tests to make sure we don't forget about them

tests added! Pls let me know if any more changes are required

@rupam-04 rupam-04 requested a review from rkapka July 2, 2024 21:31
}

if syncAggregate.SyncCommitteeBits.Count()*3 < config.SyncCommitteeSize*2 {
if syncAggregate.SyncCommitteeBits.Count() < 1 {

Choose a reason for hiding this comment

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

I'm not sure if we even should filter out the blocks with zero SyncComitteeBits. since the light clients themselves check that.
But according to the specs, this will not count as a valid update anyway, so maybe we could save them the trouble of computation and just don't send them invalid updates.
Unless there is another use for this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

The full node spec does not require to send only updates with 1 or more participants, but since it's the sync committee that provides an update, having 0 participants essentially means that the sync committee did not give an update.

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.

some issues in the GetLightClientUpdatesByRange function
4 participants