prysmaticlabs / prysm

Go implementation of Ethereum proof of stake
https://www.offchainlabs.com
GNU General Public License v3.0
3.47k stars 1.01k forks source link

Too Hard to Add/Modify API Schema in Prysm #10149

Closed rauljordan closed 2 years ago

rauljordan commented 2 years ago

🚀 Feature Request

Description

It is really hard to add a new API endpoint, add a new protobuf type, or change anything about our existing protobuf schemas. Adding a simple, new message in protobuf in a regular file such as proto/prysm/v1alpha1/attestation.proto leads to massive changelog of files when generating Go code for the diff

diff --git a/proto/prysm/v1alpha1/attestation.proto b/proto/prysm/v1alpha1/attestation.proto
index 9906adc64..c6dc9a7ef 100644
--- a/proto/prysm/v1alpha1/attestation.proto
+++ b/proto/prysm/v1alpha1/attestation.proto
@@ -24,6 +24,10 @@ option java_outer_classname = "AttestationProto";
 option java_package = "org.ethereum.eth.v1alpha1";
 option php_namespace = "Ethereum\\Eth\\v1alpha1";

+message SpecialAttestation {
+    bytes aggregation_bits = 1 [(ethereum.eth.ext.ssz_max) = "2048", (ethereum.eth.ext.cast_type) = "github.com/prysmaticlabs/go-bitfield.Bitlist"];
+}
+
 message Attestation {
     // A bitfield representation of validator indices that have voted exactly
     // the same vote and have been aggregated into this attestation.

...leads to:

$ ./hack/update-go-pbs.sh && git status
On branch develop
Your branch is up to date with 'origin/develop'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
    modified:   proto/eth/service/beacon_chain_service.pb.gw.go
    modified:   proto/eth/service/beacon_debug_service.pb.gw.go
    modified:   proto/eth/service/validator_service.pb.gw.go
    modified:   proto/eth/v1/attestation.pb.gw.go
    modified:   proto/eth/v1/beacon_block.pb.gw.go
    modified:   proto/eth/v1/beacon_chain.pb.gw.go
    modified:   proto/eth/v1/beacon_state.pb.gw.go
    modified:   proto/eth/v1/events.pb.gw.go
    modified:   proto/eth/v1/node.pb.gw.go
    modified:   proto/eth/v1/validator.pb.gw.go
    modified:   proto/eth/v2/beacon_block.pb.gw.go
    modified:   proto/eth/v2/beacon_state.pb.gw.go
    modified:   proto/eth/v2/sync_committee.pb.gw.go
    modified:   proto/eth/v2/validator.pb.gw.go
    modified:   proto/eth/v2/version.pb.gw.go
    modified:   proto/prysm/v1alpha1/attestation.pb.go
    modified:   proto/prysm/v1alpha1/attestation.pb.gw.go
    modified:   proto/prysm/v1alpha1/attestation.proto
    modified:   proto/prysm/v1alpha1/beacon_block.pb.gw.go
    modified:   proto/prysm/v1alpha1/beacon_state.pb.gw.go
    modified:   proto/prysm/v1alpha1/finalized_block_root_container.pb.gw.go
    modified:   proto/prysm/v1alpha1/p2p_messages.pb.gw.go
    modified:   proto/prysm/v1alpha1/powchain.pb.gw.go
    modified:   proto/prysm/v1alpha1/sync_committee.pb.gw.go
    modified:   proto/prysm/v1alpha1/sync_committee_mainnet.go
    modified:   proto/prysm/v1alpha1/sync_committee_minimal.go
    modified:   proto/testing/gocast.go

Most of these edits affect imports and minor formatting issues, but they happen every time one wants to edit a protobuf. Moreover, adding new files is non-trivial because our build system, Bazel, relies on special configuration files called BUILD.bazel in every directory to figure out how to build the associated protobufs. These require manual edits, where we a developer has to update the associated BUILD.bazel file when creating a .proto file. Adding dependencies is non-trivial, and is error-prone. This makes it really hard for someone to add or modify endpoints, types, and API schemas in Prysm.

Describe the solution you'd like

  1. Fix our ./hack/update-go-pbs.sh file so it works without generating a big changelog
  2. Add documentation on a step-by-step guide to adding a new API endpoint with gRPC protobufs in Prysm and generate Go code using our update-go-pbs.sh script
  3. Add documentation on how to edit existing .proto files and generate Go code using our update-go-pbs.sh script
  4. Find ways of removing the need for developers to manually edit BUILD.bazel

To solve the last point, (4), we can probably add thorough documentation on

  1. What BUILD.bazel files are
  2. The structure of BUILD.bazel files in our proto/ directory and their common elements
  3. How to modify BUILD.bazel files when you are adding (a) a new .proto file, or (b) adding Go or protobuf dependencies to your file
prestonvanloon commented 2 years ago

Fix our ./hack/update-go-pbs.sh file so it works without generating a big changelog

How is this achieved? I think that big change log is due to dependencies or changes included that didn't run update-go-pbs.sh so all of that data is stale. I ran it on develop with no change and i see changes like a dependency was updated but none of the pbs updated.

diff --git a/proto/eth/service/node_service.pb.go b/proto/eth/service/node_service.pb.go
index 992a6ad1e..348419a69 100755
--- a/proto/eth/service/node_service.pb.go
+++ b/proto/eth/service/node_service.pb.go
@@ -1,6 +1,6 @@
 // Code generated by protoc-gen-go. DO NOT EDIT.
 // versions:
-//     protoc-gen-go v1.25.0
+//     protoc-gen-go v1.27.1
rauljordan commented 2 years ago

It might be system-dependent. On my linux machine, running the scripts always ends with a huge change-log. However, I think the more important issue is in how difficult it is to modify the BUILD files manually and how someone making small changes, adding a dep, etc. would have to deal with the intricacies of the BUILD files in our proto/ folder

rauljordan commented 2 years ago

The go pbs script is no longer as annoying as it used to be. Closing