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

Remove fork_version from LightClientUpdate #2916

Merged
merged 3 commits into from
Jun 27, 2022
Merged

Remove fork_version from LightClientUpdate #2916

merged 3 commits into from
Jun 27, 2022

Conversation

etan-status
Copy link
Contributor

The fork_version field in LightClientUpdate can be derived from the
update.signature_slot value by consulting the locally configured fork
schedule. The light client already needs access to the fork schedule to
determine the GeneralizedIndex values used for merkle proofs, and the
memory layouts of the structures (including LightClientUpdate). The
fork_version itself is network dependent and doesn't reveal that info.

The `fork_version` field in `LightClientUpdate` can be derived from the
`update.signature_slot` value by consulting the locally configured fork
schedule. The light client already needs access to the fork schedule to
determine the `GeneralizedIndex` values used for merkle proofs, and the
memory layouts of the structures (including `LightClientUpdate`). The
`fork_version` itself is network dependent and doesn't reveal that info.
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.

👍 on adding this helper.

Some suggestions about where to put it down and tests.

specs/capella/beacon-chain.md Outdated Show resolved Hide resolved
@ralexstokes
Copy link
Member

iirc having the fork_version here means the update is "self-contained" wrt verification

imagine there is some relayer node that does not have a locally configured fork schedule, they can still determine if this is a valid update or not (and hence to pass along or not) but it requires having the fork version to compute the right domain

note this "relayer" could even be a smart contract where its better UX to not need to also bring along a fork schedule as ambient knowledge

@wemeetagain
Copy link
Contributor

I think having the update self-contained wrt verification doesn't buy us much here. I'd argue that there is already ambient knowledge required in the current protocol, which amounts to knowing the config+preset, genesis values, and having fork awareness of the beacon state layout. So if we can derive the fork schedule from the config, we're not really requiring additional knowledge.

Also would mention that the sync spec gives just one possible concrete protocol for syncing, but different relayers/consumers will have different requirements. The protocol likely doesn't need to support the lowest-common-denominator, but rather to just support the simplest approach.

@etan-status
Copy link
Contributor Author

etan-status commented Jun 21, 2022

imagine there is some relayer node that does not have a locally configured fork schedule, they can still determine if this is a valid update or not (and hence to pass along or not) but it requires having the fork version to compute the right domain

It is not self-contained, if you think about future forks that may change the GeneralizedIndex used in the proofs due to BeaconState becoming bigger, or even deliberate changes to the structures such as integrating an EL state root into it. The fork version itself is not enough to determine what data type is being used, the client needs to know that mapping locally.

The "relayer" could also just re-add the fork version if its consumers depend on it (and would likely also have to add more metadata than just that).

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.

LGTM

@ralexstokes @wemeetagain
Do people still have objections? Please let us know. :)


@with_all_phases
@spec_state_test_with_matching_config
def test_override_config_fork_epoch(spec, state):
Copy link
Contributor

Choose a reason for hiding this comment

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

very thoughtful @etan-status 👍

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.

None yet

4 participants