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

Allow light client to verify signatures at period boundary #2805

Merged
merged 1 commit into from
Jun 16, 2022
Merged

Allow light client to verify signatures at period boundary #2805

merged 1 commit into from
Jun 16, 2022

Conversation

etan-status
Copy link
Contributor

@etan-status etan-status commented Jan 17, 2022

As the sync committee signs the previous block, the situation arises at
every sync committee period boundary, that the new sync committee signs
a block in the previous sync committee period. The light client cannot
reliably detect this condition (e.g., assume that this is the case when
it is currently on the last slot of a sync committee period), because
the last couple slots of a sync committee period may not have a block.

For example, when receiving a LightClientUpdate that is constructed
as in the following illustration, it is unknown whether sync_aggregate
was signed by the current or next sync committee at attested_header.

                       
        slot N           N + 1   |            N + 2   (slot not sent!)
                                 |
  +-----------------+     \ /    |     +----------------+
  | attested_header | <--- X ----|---- | sync_aggregate |
  +-----------------+     / \    |     +----------------+
                        missed   |
                                 |
                          sync committee
                          period boundary

This patch addresses this edge case by including the slot at which the
sync_aggregate was created into the LightClientUpdate object.

Note that the signature_slot cannot be trusted beyond the purpose of
signature verification, as it could be manipulated to any other slot
within the same sync committee period and fork version, without making
the sync_aggregate invalid.

@etan-status etan-status marked this pull request as draft January 17, 2022 13:44
@etan-status etan-status marked this pull request as ready for review January 18, 2022 15:12
@etan-status etan-status changed the title Allow light client to verify signatures at period bounary Allow light client to verify signatures at period boundary Jan 18, 2022
@etan-status
Copy link
Contributor Author

Rebased to dev, also fixing a recent regression in get_sync_aggregate when requesting a signature from state.next_sync_committee that was introduced in #2826.

@etan-status
Copy link
Contributor Author

Rebased to dev.

@etan-status
Copy link
Contributor Author

Change method in which information about signing sync committee is included into the LightClientUpdate (Devconnect AMS feedback - see #2802 (comment))

Before:

  • If update.next_sync_committee is empty, then update.sync_aggregate was signed by store.next_sync_committee, otherwise it was signed by store.current_sync_committee.

After:

  • update.signature_slot contains the slot at which the update.sync_aggregate was signed. If that slot is in the store.finalized_header.slot's sync committee period, the update was signed by store.current_sync_committee. If it's in the next one, it was signed by store.next_sync_committee.

@etan-status etan-status marked this pull request as draft May 2, 2022 09:45
@etan-status
Copy link
Contributor Author

  • Fix get_sync_aggregate test function to use correct fork digest at fork boundary
  • Removed unused block_root argument from get_sync_aggregate
  • Return fork_digest / signature_slot that is used by get_sync_aggregate, for clarity

As the sync committee signs the previous block, the situation arises at
every sync committee period boundary, that the new sync committee signs
a block in the previous sync committee period. The light client cannot
reliably detect this condition (e.g., assume that this is the case when
it is currently on the last slot of a sync committee period), because
the last couple slots of a sync committee period may not have a block.

For example, when receiving a `LightClientUpdate` that is constructed
as in the following illustration, it is unknown whether `sync_aggregate`
was signed by the current or next sync committee at `attested_header`.

```

        slot N           N + 1   |            N + 2   (slot not sent!)
                                 |
  +-----------------+     \ /    |     +----------------+
  | attested_header | <--- X ----|---- | sync_aggregate |
  +-----------------+     / \    |     +----------------+
                        missed   |
                                 |
                          sync committee
                          period boundary
```

This patch addresses this edge case by including the slot at which the
`sync_aggregate` was created into the `LightClientUpdate` object.

Note that the `signature_slot` cannot be trusted beyond the purpose of
signature verification, as it could be manipulated to any other slot
within the same sync committee period and fork version, without making
the `sync_aggregate` invalid.
@ralexstokes
Copy link
Member

I have not done a review of the changes but just going off the PR description:

it seems like we could use the current_slot input to validate_light_client_update to determine which sync committee period we are in, and then avoid adding more data to the light client update

what do you think about this?

@etan-status
Copy link
Contributor Author

The current_slot input is based off local wall clock, which may be several days in the future in case of syncing historic data.

@ralexstokes
Copy link
Member

ah good point, then I think adding the target slot makes a lot of sense :)

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

Sorry for the super late review @etan-status. Well done. LGTM!

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

Successfully merging this pull request may close these issues.

4 participants