Stubbed implementation of GetLightClientBootstrap #6384

robzajac · 2022-11-05T15:40:09Z

PR Description

This PR is a base implementation of GetLightClientBootstrap with return 501;. It is intended to validate the interface.

Fixed Issue(s)

fixes #6287

Documentation

I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

I thought about adding a changelog entry, and added one if I deemed necessary.

CLAassistant · 2022-11-05T15:40:21Z

All committers have signed the CLA.

robzajac · 2022-11-05T15:41:31Z

...rc/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/beacon/GetLightClientBootstrap.java

+    request.respondError(501, "Not implemented");
+  }
+
+  private static SerializableTypeDefinition<ObjectAndMetaData<LightClientBootstrap>>


Unsure whether to use ObjectAndMetadata here given that this API only requires version and not executionOptimistic or canonical

ObjectAndMetadata is generally what's coming out of the ChainDataClient, so using that should be ok, but it's also just an interface you're using if we end up needing to change it so it's not locked in stone...

It will still have all fields consumers need, and the two extra that they don't. As long as clients don't have strict response validation (i.e. allowing additive fields) then it should be okay.

...est/java/tech/pegasys/teku/beaconrestapi/handlers/v1/beacon/GetLightClientBootstrapTest.java

data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/BeaconRestApiTypes.java

robzajac · 2022-11-05T15:46:13Z

ethereum/spec/src/main/java/tech/pegasys/teku/spec/schemas/SchemaDefinitionsAltair.java

@@ -41,6 +42,7 @@ public class SchemaDefinitionsAltair extends AbstractSchemaDefinitions {
  private final ContributionAndProofSchema contributionAndProofSchema;
  private final SignedContributionAndProofSchema signedContributionAndProofSchema;
  private final MetadataMessageSchemaAltair metadataMessageSchema;
+  private final LightClientBootstrapSchema lightClientBootstrapSchema;


Added only for Altair schema definitions. Question whether the schema should be added for all consensus versions (i.e. lift up into SchemaDefinitions) (though there is no change from Altair) or whether it should only be specified in SchemaDefinitionsAltair and used this way.

https://github.com/ethereum/beacon-APIs/blob/master/apis/beacon/light_client/bootstrap.yaml#L31

We'll want to pull it into SchemaDefinitions I think, rather than hard references to Altair, as that creates the altair schema definitions even though maybe only this one is used... @ajsutton may have opinions here so tagging

Given the light client types are defined in the spec they will be versioned with hard forks and so need to come from SchemaDefinitions. They will still just wind up being added to SchemaDefinitionsAltair because SchemaDefinitionsBellatrix extends from that and doesn't need to make any changes so will just inherit the Altair definitions.

Since these types are new in Altair I think you've actually got it right that they get added to SchemaDefinitionsAltair - they're then inherited by Bellatrix and they don't need to be in Phase0.

Not entirely sure that comment made sense... SchemaDefinitions is an interface that only contains the types that are common to all hard forks. For things new in Altair like this you'd call SchemaDefinitions.toVersionAltair() to get a SchemaDefinitionsAltair and then can use these new methods.

And Bellatrix inherits all the Altair types, then just redefines a few, so SchemaDefinitionsBellatrix extends SchemaDefinitionsAltair so these methods will also be available there (and Capella extends Bellatrix).

Admittedly I am still a bit confused. Since SchemaDefinitions contains types for all consensus versions (including phase0), I think we wouldn't lift LightClientBootstrap there. The spec asks only for an Altair.LightClientBootstrap return type on this endpoint. The other endpoint I can see that has a similar treatment is produceSyncCommitteeContribution. This makes me inclined to retrieve the SchemaDefinitionsAltair in the endpoint with SchemaDefinitionsAltair.required(schemaDefinitionCache.getSchemaDefinition(SpecMilestone.ALTAIR)) and from there .getLightClientBootstrapSchema()

Sorry should have responded to this earlier, but it looks like you worked it out and done the right thing. The new method only gets added to SchemaDefinitionsAltair. We can be a bit sloppy with terminology when saying things should be added to "SchemaDefinitions" - we mean to the schema definitions for the first milestone it's relevant to.

rolfyone · 2022-11-06T21:36:31Z

...ec/src/main/java/tech/pegasys/teku/spec/datastructures/lightclient/LightClientBootstrap.java

+public class LightClientBootstrap
+    extends Container3<LightClientBootstrap, BeaconBlockHeader, SyncCommittee, SszBytes32Vector> {
+
+  // TODO: Lift into SpecConfigAltair


Yep I think moving this will be a whole thing...

It's a little involved to create an SSZ object against the schema for the correct milestone, but it's worthwhile in case it ever changes...
Initially a good object to look at would probably be ExecutionPayload which exists from Bellatrix... it's not broken down into versions because there's only 1, although it may start moving very soon because there's changes marked for capella...

It might actually be worth that becoming it's own smallish PR as they're a little involved.

@rolfyone Can this one be treated like SYNC_COMMITTEE_SIZE from SyncCommittee? Another Altair-specific parameter. The field would move up to SpecConfigAltair, and then I believe it should be read from config, or a static constants file. But I'm having trouble seeing where the config is parsed (for e.g. SYNC_COMMITTEE_SIZE).

I have an example impl on a separate branch but it fails the SpecConfigBuilder validation, making me believe the field is not being successfully read from config.

On second pass, I got it working (was missing a method in the Builder) robzajac@6b1b6a4

Just awaiting a second opinion on whether we want this in config this way, and then can write it into this branch.

Moved constant up to config in the latest push.

robzajac · 2022-11-10T20:41:03Z

@rolfyone @ajsutton Addressed comments & outstanding todos, do you mind taking another look?
The acceptance test failure is new. Unable to replicate locally, taking another look.

ajsutton

Looking really good. I think the key thing is just to provide a default so we don't break backwards compatibility with the config endpoint.

...ec/src/main/java/tech/pegasys/teku/spec/datastructures/lightclient/LightClientBootstrap.java

ajsutton · 2022-11-11T04:02:00Z

ethereum/spec/src/main/java/tech/pegasys/teku/spec/schemas/SchemaDefinitionsAltair.java

@@ -41,6 +42,7 @@ public class SchemaDefinitionsAltair extends AbstractSchemaDefinitions {
  private final ContributionAndProofSchema contributionAndProofSchema;
  private final SignedContributionAndProofSchema signedContributionAndProofSchema;
  private final MetadataMessageSchemaAltair metadataMessageSchema;
+  private final LightClientBootstrapSchema lightClientBootstrapSchema;


Sorry should have responded to this earlier, but it looks like you worked it out and done the right thing. The new method only gets added to SchemaDefinitionsAltair. We can be a bit sloppy with terminology when saying things should be added to "SchemaDefinitions" - we mean to the schema definitions for the first milestone it's relevant to.

ajsutton · 2022-11-11T04:11:28Z

ethereum/spec/src/test/resources/tech/pegasys/teku/spec/config/bellatrixForkSpec.json

@@ -79,6 +79,7 @@
  "SLOTS_PER_EPOCH": "32",
  "SLOTS_PER_HISTORICAL_ROOT": "8192",
  "SYNC_COMMITTEE_SIZE": "512",
+  "SYNC_COMMITTEE_BRANCH_LENGTH": "5",


Sadly we can't just update these test files (including altairForkSpec.json and mergeForkSpec.json). They're a capture of the actual response the beacon node will give from the /eth/v1/node/config endpoint which the standalone validator client uses to load the spec from the beacon node. It looks like adding this new property as a required field to Altair is breaking backwards compatibility (which is what the SpecConfigLoaderTest.shouldLoadMainnetPreservingBackwardsCompatibilityWithRestApi test is checking for.

We probably need to provide a default value in SpecConfigBuilder instead of requiring the field be set. Could just use the mainnet value as the default which I think would be reasonable. It's not particularly ideal but better than breaking backwards compatibility since people don't always update the VC and BN together. The default probably never gets used anyway since the VC doesn't use it and the built-in presets will provide it for the BN.

Thanks @ajsutton I think that makes sense. I set a default in the builder, similar treatment as updateTimeout. To check my understanding, is there a point at which the validators are updated to receive the new configs? Or will we continue to use the default for some time?

The validator client will load the config from the beacon node every time it starts up, so the real value should be picked up quite quickly but it's possible the VC is running against a beacon node that doesn't get updated or against a different client which doesn't yet have light client support and this config item. So we'll probably wind up keeping the default around pretty much forever (or at least until the next hard fork if all clients include it by then since that forces everyone to update). For the vast majority of setups though, they'll start loading the real value very quickly.

ajsutton · 2022-11-11T04:16:46Z

Oh I think that acceptance test failure is likely related to the config backwards compatibility issue as well. It's running the latest version of the validator client (with these changes) against an old release of the beacon node and it's failing to start, likely because of the incorrect config.

To run locally you'll likely need to run ./gradlew distDocker to build the docker images from the latest code including your changes (otherwise it likely winds up running the latest version from master without your changes). Then you can run the acceptance test in IntelliJ like a normal test.

./gradlew acceptanceTest should automatically build the docker image but it runs all the tests so takes a fair while which is less useful for debugging stuff.

…200 response type.

Signed-off-by: Paul Harris <paul.harris@consensys.net>

robzajac · 2022-11-11T14:31:46Z

@ajsutton Recreated locally & pushed up a fix 🙏

ajsutton

LGTM. Thanks for your patience on this and sorry we've been slow with reviews.

robzajac commented Nov 5, 2022

View reviewed changes

rolfyone reviewed Nov 6, 2022

View reviewed changes

robzajac force-pushed the light-client-bootstrap-endpoint branch 3 times, most recently from 4603123 to 7525630 Compare November 10, 2022 19:32

robzajac marked this pull request as ready for review November 10, 2022 20:39

ajsutton reviewed Nov 11, 2022

View reviewed changes

robzajac and others added 7 commits November 11, 2022 15:15

Base implementation of GetLightClientBootstrap, modulo validation of …

ea71c56

…200 response type.

fix 200 test, and moved bootstrap to the route for lightclient.

ca54f4f

Signed-off-by: Paul Harris <paul.harris@consensys.net>

update child field schema for lightclientbootstrap

13018f8

fix integration test failures

4203d1c

Move sync committee constant to config

1b76b0b

More configs

39f939c

Remove constant from forkspec. Refactor LightClientBootstrapSchema

03d27e3

robzajac force-pushed the light-client-bootstrap-endpoint branch from 56b631f to 03d27e3 Compare November 11, 2022 14:15

ajsutton approved these changes Nov 13, 2022

View reviewed changes

Merge branch 'master' into light-client-bootstrap-endpoint

d4b3735

ajsutton merged commit 56ed542 into Consensys:master Nov 14, 2022

robzajac deleted the light-client-bootstrap-endpoint branch November 14, 2022 14:13

robzajac mentioned this pull request Nov 17, 2022

Add (experimental) light client server functionality #4230

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stubbed implementation of GetLightClientBootstrap #6384

Stubbed implementation of GetLightClientBootstrap #6384

robzajac commented Nov 5, 2022 •

edited

Loading

CLAassistant commented Nov 5, 2022 •

edited

Loading

robzajac Nov 5, 2022

rolfyone Nov 6, 2022

robzajac Nov 8, 2022

robzajac Nov 5, 2022

robzajac Nov 5, 2022

rolfyone Nov 6, 2022

ajsutton Nov 6, 2022

ajsutton Nov 6, 2022

robzajac Nov 8, 2022

ajsutton Nov 11, 2022

rolfyone Nov 6, 2022

robzajac Nov 8, 2022

robzajac Nov 9, 2022

robzajac Nov 10, 2022

robzajac commented Nov 10, 2022

ajsutton left a comment

ajsutton Nov 11, 2022

ajsutton Nov 11, 2022

robzajac Nov 11, 2022

ajsutton Nov 13, 2022

ajsutton commented Nov 11, 2022

robzajac commented Nov 11, 2022 •

edited

Loading

ajsutton left a comment

Stubbed implementation of GetLightClientBootstrap #6384

Stubbed implementation of GetLightClientBootstrap #6384

Conversation

robzajac commented Nov 5, 2022 • edited Loading

PR Description

Fixed Issue(s)

Documentation

Changelog

CLAassistant commented Nov 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robzajac commented Nov 10, 2022

ajsutton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajsutton commented Nov 11, 2022

robzajac commented Nov 11, 2022 • edited Loading

ajsutton left a comment

Choose a reason for hiding this comment

robzajac commented Nov 5, 2022 •

edited

Loading

CLAassistant commented Nov 5, 2022 •

edited

Loading

robzajac commented Nov 11, 2022 •

edited

Loading