Add missing fulu presets to beacon config#16170
Conversation
|
it would be nice to replace the usages of |
|
the PR doesn't address one of the fields mentioned in the issue ( |
Those are compile time array bounds, so they need to be constants at compile time.
Added in the latest commit. |
| } | ||
|
|
||
| // Add derived values that are computed from other config values. | ||
| data["UPDATE_TIMEOUT"] = strconv.FormatUint( |
There was a problem hiding this comment.
even if this is a computed value I think it should be added to the config itself like the other ones, you can just note that it's also this calculation but typically config values are just hard coded
There was a problem hiding this comment.
for example
MAX_REQUEST_DATA_COLUMN_SIDECARS == MAX_REQUEST_BLOCKS_DENEB * NUMBER_OF_COLUMNS
but we have max request data column sidecars in the specs.
something feels off though.. I wonder if we need to add a unit test to make sure that this is equal it seems strange to add it if it's never used anywhere
config/params/config.go
Outdated
| FieldElementsPerExtBlob uint64 `yaml:"FIELD_ELEMENTS_PER_EXT_BLOB" spec:"true"` // FieldElementsPerExtBlob is the number of field elements in an extended blob. | ||
| KzgCommitmentsInclusionProofDepth uint64 `yaml:"KZG_COMMITMENTS_INCLUSION_PROOF_DEPTH" spec:"true"` // KzgCommitmentsInclusionProofDepth is the depth of the KZG commitments inclusion proof. | ||
| CellsPerExtBlob uint64 `yaml:"CELLS_PER_EXT_BLOB" spec:"true"` // CellsPerExtBlob is the number of cells in an extended blob. | ||
| NumberOfColumns uint64 `yaml:"NUMBER_OF_COLUMNS" spec:"true"` // NumberOfColumns is the number of data columns in the network. |
There was a problem hiding this comment.
I don't think we should return these configs for the sake of returning, we need to tie it into how they are actually used otherwise we're going to run into a bug down the road ( even though we set this we aren't using it or at least validating it)
There was a problem hiding this comment.
so for Number of Columns it actually got removed in
#16073
There was a problem hiding this comment.
we need to understand why and if adding it back in this way makes sense
|
https://github.com/offchainlabs/prysm/blob/2773bdef89ab460aea4353d4d98833a4eeaa4918/specrefs/presets.yml we need to update this as well afterwards |
| "FIELD_ELEMENTS_PER_CELL", // Configured as a constant in config/fieldparams/mainnet.go | ||
| "FIELD_ELEMENTS_PER_EXT_BLOB", // Configured in proto/ssz_proto_library.bzl | ||
| "GLOAS_FORK_EPOCH", | ||
| "GLOAS_FORK_VERSION", | ||
| "INCLUSION_LIST_SUBMISSION_DEADLINE", | ||
| "INCLUSION_LIST_SUBMISSION_DUE_BPS", | ||
| "KZG_COMMITMENTS_INCLUSION_PROOF_DEPTH", // Configured in proto/ssz_proto_library.bzl | ||
| "MAX_BYTES_PER_INCLUSION_LIST", | ||
| "MAX_REQUEST_BLOB_SIDECARS_FULU", | ||
| "MAX_REQUEST_INCLUSION_LIST", | ||
| "MAX_REQUEST_PAYLOADS", // Compile time constant on BeaconBlockBody.ExecutionRequests | ||
| "MIN_BUILDER_WITHDRAWABILITY_DELAY", | ||
| "NUMBER_OF_COLUMNS", // Configured as a constant in config/fieldparams/mainnet.go |
There was a problem hiding this comment.
These changes were defined elsewhere in the application. You've added them again so now we have the same constants twice. This is a bug risk if we ever needed to update or change the value, it may not be updated in all places. Can we avoid having the constants twice?
config/params/config.go
Outdated
| BalancePerAdditionalCustodyGroup uint64 `yaml:"BALANCE_PER_ADDITIONAL_CUSTODY_GROUP" spec:"true"` // BalancePerAdditionalCustodyGroup is the balance increment corresponding to one additional group to custody. | ||
|
|
||
| // Light client preset values | ||
| UpdateTimeout uint64 `yaml:"UPDATE_TIMEOUT" spec:"true"` // UpdateTimeout is SLOTS_PER_EPOCH * EPOCHS_PER_SYNC_COMMITTEE_PERIOD |
There was a problem hiding this comment.
talked offline but we can move this where the others are, makes sense to me
|
|
||
| // Add Fulu preset values. These are compile-time constants from fieldparams, | ||
| // not runtime configs, but are required by the /eth/v1/config/spec API. | ||
| data["NUMBER_OF_COLUMNS"] = convertValueForJSON(reflect.ValueOf(uint64(fieldparams.NumberOfColumns)), "NUMBER_OF_COLUMNS") |
There was a problem hiding this comment.
can you update the ethspecify locations? specref ymls
how you use it #15592 (comment)
<!-- Thanks for sending a PR! Before submitting: 1. If this is your first PR, check out our contribution guide here https://docs.prylabs.network/docs/contribute/contribution-guidelines You will then need to sign our Contributor License Agreement (CLA), which will show up as a comment from a bot in this pull request after you open it. We cannot review code without a signed CLA. 2. Please file an associated tracking issue if this pull request is non-trivial and requires context for our team to understand. All features and most bug fixes should have an associated issue with a design discussed and decided upon. Small bug fixes and documentation improvements don't need issues. 3. New features and bug fixes must have tests. Documentation may need to be updated. If you're unsure what to update, send the PR, and we'll discuss in review. 4. Note that PRs updating dependencies and new Go versions are not accepted. Please file an issue instead. 5. A changelog entry is required for user facing issues. --> **What type of PR is this?** Feature **What does this PR do? Why is it needed?** Added some missing constants of fulu to beacon config so that beacon api returns expected values **Which issues(s) does this PR fix?** Fixes #16138 **Other notes for review** **Acknowledgements** - [ ] I have read [CONTRIBUTING.md](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md). - [ ] I have included a uniquely named [changelog fragment file](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md#maintaining-changelogmd). - [ ] I have added a description with sufficient context for reviewers to understand this PR. - [ ] I have tested that my changes work as expected and I added a testing plan to the PR description (if applicable).
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Added some missing constants of fulu to beacon config so that beacon api returns expected values
Which issues(s) does this PR fix?
Fixes #16138
Other notes for review
Acknowledgements