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

Add source param to getLightClientOptimisticUpdate #288

Closed
wants to merge 4 commits into from

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Jan 14, 2023

Route getLightClientOptimisticUpdate exposes a LightClientOptimisticUpdate produced with the SyncAggregate of the head block. This PR allows to customize this behaviour to support consumers that need a more "timely" SyncAggregate.

Proposed values of source param:

  • block (default): Get SyncAggregate from current's head block body, body.sync_aggregate

  • pool_contribution_and_proof: Construct SyncAggregate from aggregating SignedContributionAndProof objects in the node's pool received from the gossip topic sync_committee_contribution_and_proof. MUST aggregate messages with message.contribution.slot == current_slot and message.contribution.beacon_block_root == head_root.

  • pool_committee_message: Construct SyncAggregate from aggregating SyncCommitteeMessage objects in the node's pool received from the gossip topics sync_committee_{subnet_id}. MUST aggregate messages withslot == current_slot and beacon_block_root == head_root. After receiving a signatures request, the node SHOULD subscribe to all sync committee subnets.

Since source param is optional and defaults to block I guess this is non-breaking change with the current rules of the repo right?

types/api.yaml Outdated

SyncAggregateSource:
type: string
enum: ["block", "signedcontributionandproof_pool", "synccommitteemessage_pool"]
Copy link
Collaborator

@rolfyone rolfyone Jan 15, 2023

Choose a reason for hiding this comment

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

these are really hard to read...

given block means block.body.sync_aggregate, maybe it makes sense to do something like
pool_committee_message
pool_contribution_and_proof

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Picked an awful name to trigger suggestions 😄

@rolfyone
Copy link
Collaborator

Who are the right people to decide on this vs. the alternative you mentioned? I'm not really across the space, and would prefer the right people ultimately make the decision on this one, i'm happy to approve and merge if there's agreement...

@dapplion
Copy link
Collaborator Author

Who are the right people to decide on this vs. the alternative you mentioned? I'm not really across the space, and would prefer the right people ultimately make the decision on this one, i'm happy to approve and merge if there's agreement...

Let's sit on it for 2 more weeks to collect input during interop

@etan-status
Copy link
Contributor

I don't like the side effect of GET request to "MUST subscribe to topics and aggregate" which is a heavy operation.

The functionality is already available using the validator namespace, and is more explicit (turn on subscription, then just ask for producing contributions whenever one has interest in it). This makes it explicit to express intent, instead of having nasty side effect to GET requests.

For application, they typically have a certain minimum percentage of signatures they want to ensure, so this new endpoint doesn't help that much either because application doesn't know when that threshold is reached, or it waits longer than necessary to ask the BN, so the API is not really optimal for that use case either.

@dapplion
Copy link
Collaborator Author

dapplion commented Dec 8, 2023

I don't like the side effect of GET request to "MUST subscribe to topics and aggregate" which is a heavy operation.

The functionality is already available using the validator namespace, and is more explicit (turn on subscription, then just ask for producing contributions whenever one has interest in it). This makes it explicit to express intent, instead of having nasty side effect to GET requests.

Agree with @etan-status 's points, so closing

@dapplion dapplion closed this Dec 8, 2023
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.

3 participants