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

Make lightclient patch pass the linter #2133

Merged
merged 10 commits into from
Nov 17, 2020

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Nov 15, 2020

Make #2130 executable + pass the linter.
Note: this PR doesn't enable lightclient fork spec tests.

@hwwhww hwwhww force-pushed the executable-lightclient-spec branch from 92f0663 to a4c6c47 Compare November 15, 2020 13:46
@@ -70,15 +70,55 @@ This is a standalone beacon chain patch adding light client support via sync com
#### `BeaconBlockBody`

```python
class BeaconBlockBody(phase0.BeaconBlockBody):
Copy link
Collaborator

Choose a reason for hiding this comment

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

class BeaconBlockBody(phase0.BeaconBlockBody): is so much cleaner than repeating all the properties!

@protolambda: Can we do some Python magic here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

With python we can do a lot, but the question is if we should. Being implicit would mean we need to spec appending of fields. And then sometimes appending is not the approach we want. Or functionality is not sequential, and we remove a field.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason why I did the inheriting from phase0 in the original PR instead of repeating properties is not just to cut down the number of lines of code, it's also to make the patch to the spec truly not sequential (ie. if it's implemented after sharding it could still be included as is). This would make it easier for us to work on light client stuff, the merge and sharding in parallel. So it would be really nice if we can make the inheritance work!

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds great, but then do what we want some diamond structure? I.e. phase 0 type inherited by various phase 1 types, and then recombined in the complete phase 1 type? This becomes "the diamond problem" once we start overriding things in different types, which we then need to reconsolidate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I don't support the concept of "phases" anymore, so... 😄

Realistically, once some patch gets implemented, we could just quickly rewrite the other patches to point to the previous patch as a base (eg. s/phase0./lightclient_fork./g), and when writing each individual patch make sure that it is valid under any combination of the other patches.

This does of course assume that the absolute number of patches that are treated this way (as opposed to just being PRs to existing spec as all the beacon chain changes pre-launch were) needs to be very small.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, implemented it and opened a PR to my SSZ library that we use here in the spec, with support for all this (inheritance, mixins, diamonds): protolambda/remerkleable#8

Please give it a review, and let's discuss on next spec call. It seems small, but it's quite a step away from bare struct definitions.

@hwwhww
Copy link
Contributor Author

hwwhww commented Nov 16, 2020

Updates:

Co-authored-by: vbuterin <[email protected]>
@vbuterin vbuterin merged commit 4df3547 into vbuterin-patch-2 Nov 17, 2020
@hwwhww hwwhww deleted the executable-lightclient-spec branch November 17, 2020 13:42
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.

4 participants