-
Notifications
You must be signed in to change notification settings - Fork 258
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
extend REST config with more spec constants #2962
Conversation
Depends on #2958 |
RPC @ Prater:
|
REST @ Prater:
|
beacon_chain/rpc/rest_config_api.nim
Outdated
@@ -22,22 +22,16 @@ proc installConfigApiHandlers*(router: var RestRouter, node: BeaconNode) = | |||
( | |||
CONFIG_NAME: | |||
const_preset, | |||
|
|||
# https://github.com/ethereum/consensus-specs/blob/v1.1.1/presets/mainnet/phase0.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reordered these sections to be consistent with the order in the spec.
Makes it easier to avoid missing out adding new constants in the future, as the problem will be seen when updating the spec URLs over time. Also, makes it easier for developer manually looking into the JSON output to compare it against the spec as it is the same order.
# GENESIS_SLOT | ||
# GENESIS_EPOCH | ||
# FAR_FUTURE_EPOCH | ||
# BASE_REWARDS_PER_EPOCH | ||
# DEPOSIT_CONTRACT_TREE_DEPTH | ||
# JUSTIFICATION_BITS_LENGTH | ||
# ENDIANNESS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about these. As they are phase0 ones and were not needed so far, I assume these were left out intentionally, so I did not add them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also left out on prysm
beacon_chain/rpc/rest_config_api.nim
Outdated
# TIMELY_SOURCE_FLAG_INDEX | ||
# TIMELY_TARGET_FLAG_INDEX | ||
# TIMELY_HEAD_FLAG_INDEX | ||
# TIMELY_SOURCE_WEIGHT | ||
# TIMELY_TARGET_WEIGHT | ||
# TIMELY_HEAD_WEIGHT | ||
# SYNC_REWARD_WEIGHT | ||
# PROPOSER_WEIGHT | ||
# WEIGHT_DENOMINATOR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, these are not part of the preset, so I only added the DOMAIN
ones as they were added for phase0 in the past.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prysm
is including these, with the exception of PARTICIPATION_FLAG_WEIGHTS
further down. Will also include them for compatibility.
BLS_WITHDRAWAL_PREFIX*: byte = 0 | ||
ETH1_ADDRESS_WITHDRAWAL_PREFIX*: byte = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙃
This extended config is now properly validated by Lodestar's light client demo site https://github.com/ChainSafe/eth2-light-client-demo/tree/dapplion/update-to-prater after adjusting it to handle CONSTANT_CASE in /config/spec. Will submit a PR to Lodestar regarding uppercase once LGPL CLA has been cleared by legal. |
beacon_chain/rpc/rpc_config_api.nim
Outdated
|
||
# https://github.com/ethereum/consensus-specs/blob/v1.1.1/presets/mainnet/phase0.yaml | ||
"MAX_COMMITTEES_PER_SLOT": | ||
$MAX_COMMITTEES_PER_SLOT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why for RPC we need $
and for REST we need Base10.toString
. I kept it as is for now, out of scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json-rpc uses some random, but particular encoding that others have come to rely upon - we must make sure that we don't change the formatting in any way between releases.
rest has a standard-defined output format, so the rest implementation uses more precise definitions that correspond to that standard - ie $
is some random string format for a type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that makes sense, thanks!
let configFlag = | ||
info.MAX_VALIDATORS_PER_COMMITTEE != MAX_VALIDATORS_PER_COMMITTEE or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, it felt quite random which of the constants were being checked. I extended this to cover all the constants exchanged via the config REST/RPC API that are hard-coded and not overridable using a dynamic configuration in Nimbus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @cheatfate , have to be careful on these comparisons, as this breaks compatibility with other clients that do not include the new extra constants as part of their config response.
Reverting this file!
Update regarding licensing: The light client component of Lodestar is Apache licensed. Only other parts of Lodestar are LGPL licensed, so going ahead and submitting PR there to fix their config/spec endpoint as well. |
JSON-RPC is expected to only return v1.0.0 constants. Updated the PR accordingly. New RPC output:
|
beacon_chain/rpc/rest_config_api.nim
Outdated
@@ -20,24 +20,19 @@ proc installConfigApiHandlers*(router: var RestRouter, node: BeaconNode) = | |||
cachedConfigSpec = | |||
RestApiResponse.prepareJsonResponse( | |||
( | |||
# https://github.com/ethereum/consensus-specs/blob/v1.0.0/configs/mainnet/phase0.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one no longer exists in newer spec versions. There is now a PRESET_BASE
though. I guess we keep this around for backwards compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prysm
also left this constant in, but on Prater, for them it's value is prater
and in nimbus it's mainnet
.
Rebased on The additional constants introduced with this are:
|
9aa9f5b
to
c973cc0
Compare
Added the constants from Now, compared to Prysm stable:
|
DOMAIN_CONTRIBUTION_AND_PROOF: | ||
"0x" & ncrutils.toHex( | ||
uint32(DOMAIN_CONTRIBUTION_AND_PROOF).toBytesLE()), | ||
# PARTICIPATION_FLAG_WEIGHTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also left out on prysm
let svalue = reader.readValue(string) | ||
try: | ||
value = parse(svalue, UInt256, 10) | ||
except AssertionError, RangeError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mratsim This is the location with the unexpected AssertionError
. There is a reachable path for that one when processing a string starting with 0x
, 0o
or 0b
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been fixed to use ValueError
now.
|
|
## UInt256 | ||
proc writeValue*(w: var JsonWriter[RestJson], value: UInt256) {. | ||
raises: [IOError, Defect].} = | ||
writeValue(w, toString(value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you verified that decimal encoding is appropriate? There are some traditions in the Ethereum world to use hexademically encoded big integers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spec:
Values are returned with following format:
- any value starting with 0x in the spec is returned as a hex string
- numeric values are returned as a quoted integer
It seems to not depend on "big integer" for the config/spec but instead on whether the spec defines it as 0x or not.
There is still the general problem that config/spec is underspecified, it's just a map of random constants.
Lodestar accepts it in the current form (their light client requires the server to provide it).
Prysm is still on the older TRANSITION_TOTAL_DIFFICULTY which was a smaller number than 256 bit.
I guess for now it's okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
645ce8a
to
7a00fd4
Compare
7a00fd4
to
eb38543
Compare
Updated links for 1.1.2 as well (no changes upstream) |
@cheatfate, can you comment regarding how this breaks the validator client expectations? |
Every client |
Some more context: An earlier commit on this PR included additional changes to RPC and in validator client logic.
Regarding VC, these were the incorrect changes: 3999285#diff-0efa923f42e20f0ff6915fd060c9eb33ac1d997af5bd003871f22da721886a17 The only remaining changes here are additional returned constants in the REST API for compatibility with Lodestar light client demo. |
Because And use |
#2764 has the skip-unknown-stuff patch |
Thanks for the additional background on Presto. I was not aware of it erroring out when encountering unknown fields. Good catch. I think that the fix from @arnetheduck may be sufficient to fix this problem for the spec response as well (although I'm not sure it is correct to blindly apply Note that without having the proper The introduction of a new sub-type was also what @zah recommended to me via DM. I'll create a separate PR for that, and turn this one back to "draft" for now, until new spec fields can be safely introduced that are not yet advertised by all the major clients. |
eb38543
to
de46e7b
Compare
(updated links to 1.1.3) |
de46e7b
to
86e0ab8
Compare
./rest_utils, | ||
../beacon_node, ../networking/eth2_network, | ||
../consensus_object_pools/[blockchain_dag, exit_pool, spec_cache], | ||
../validators/validator_duties, | ||
../spec/[eth2_merkleization, forks, network], | ||
../spec/datatypes/[phase0, altair], | ||
./rest_utils | ||
../spec/datatypes/[phase0, altair] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This re-ordering avoids a compilation error on i386 that occurs otherwise, ever since this PR has been rebased on top of #2997
As of #3005, the validator client now uses |
5d4dc50
to
531d80a
Compare
So far, the REST config response did not include all spec constants. The spec for `/eth/v1/config/spec` defines that the response should include constants for all hard forks known by the beacon node. This patch extends the corresponding response to include more constants.
531d80a
to
dae875a
Compare
So far, the REST/RPC config response did not include all spec constants.
The spec for
/eth/v1/config/spec
defines that the response shouldinclude constants for all hard forks known by the beacon node. This
patch extends the corresponding responses to include more constants.