gloas proposer preference apis#608
Conversation
| properties: | ||
| version: | ||
| type: string | ||
| enum: [gloas] |
There was a problem hiding this comment.
maybe fulu should be available too?
There was a problem hiding this comment.
is there any reason not to include fulu?
There was a problem hiding this comment.
the proposer preferences concept doesn't exist or make sense for Fulu, does it?
There was a problem hiding this comment.
There is no fulu type for SignedProposerPreferences, so I'd just keep gloas here
There was a problem hiding this comment.
yeah was just thinking from perspective of having to submit proposer preferences while still in fulu and having the ability to get what you submitted?
There was a problem hiding this comment.
while the clock slot is still fulu, the proposal_slot would be in gloas, we would use the message slot anyways to determine the fork? I guess the same question exists in gossip, which uses the gloas fork digest even if messages are broadcast before the fork (at least in lodestar)
the same question exists if we wanna extend the proposer preferences in the future but I think we should always use the message slot
|
Awesome! I ll close my PR! |
| From the Gloas fork onwards, beacon nodes use the set of validators that have submitted | ||
| subscriptions via this endpoint to identify which validators are using the node for duties, | ||
| and adjust the node's custody group count (CGC) accordingly. Validator clients SHOULD |
There was a problem hiding this comment.
historically CGC was up only, this implies it might be able to be reduced now?
There was a problem hiding this comment.
No I can try to reword it. I'm just implying that we should use this as a replacement to prepare beacon proposer endpoint for cgc purposes.
| - For validators served by the beacon node, if no signed proposer preferences are available | ||
| for a given proposal slot, the beacon node MAY fall back to its configured default fee | ||
| recipient and target gas limit when constructing or accepting bids for that validator. | ||
| Bids whose fee recipient or gas limit do not match a validator's preferences will be | ||
| rejected at gossip validation, so applying sensible local defaults reduces missed | ||
| proposals during the transition. |
There was a problem hiding this comment.
should (vs may) fall back? what would be the alternative?
There was a problem hiding this comment.
zeroed out i guess, in a sense that is also a local default so i'll change it to should
| produceBlockV | ||
| stateful | ||
| CGC | ||
| dedup |
There was a problem hiding this comment.
dedup is a horrible horrible word, can we just spell de-duplicate out instead?
| @@ -0,0 +1,47 @@ | |||
| get: | |||
| operationId: getPoolProposerPreferences | |||
There was a problem hiding this comment.
A bit pedantic of me, but I find it strange that there is a pool API for a message type that is not included in blocks. In my mind the /pool APIs are for messages that can be packed into blocks like attestations, payload attestations, sync committee messages, etc.
There was a problem hiding this comment.
Separately, why do beacon nodes have to store these messages at all? My understanding is that they exist mainly for the sake of builders, so there is limited value in a beacon node keeping track of them after validation (aside from the minimal amount of data that is required to be tracked to detect duplicates).
There was a problem hiding this comment.
I guess builders mostly use the SSE event, but also load from this pool to catch up on any events that happened in the recent past prior to their subscription starting?
There was a problem hiding this comment.
Yeah the SSE event is the most important one. I guess its not so important to catch up on any events cause the preferences are gossiped every epoch. So in the worse case a builder would have to wait 2 epochs before building their blocks which I think is fine?
There was a problem hiding this comment.
should i remove it from the pool namespace? i can do that. i just didn't think it should be under validator, the original proposal from bharath was under pool.
waiting for some more opinions here I can remove the endpoint in general if not very useful
There was a problem hiding this comment.
Separately, why do beacon nodes have to store these messages at all?
we do need to store them to be able to validate bids against target gas limit and fee recipient of the proposer
A bit pedantic of me, but I find it strange that there is a pool API for a message type that is not included in blocks.
in lodestar we currently call this ProposerPreferencesPool, but we can think about renaming this if we wanna keep that notion to only use pool for messages that are included in blocks
There was a problem hiding this comment.
for now i removed it from the pools namespace as suggested
There was a problem hiding this comment.
I'm not sure I see the utility of it either.
nflaig
left a comment
There was a problem hiding this comment.
just some initial comments, haven't done an in-depth review yet
| properties: | ||
| version: | ||
| type: string | ||
| enum: [gloas] |
There was a problem hiding this comment.
There is no fulu type for SignedProposerPreferences, so I'd just keep gloas here
| $ref: "../../../beacon-node-oapi.yaml#/components/schemas/Gloas.SignedProposerPreferences" | ||
| application/octet-stream: | ||
| schema: | ||
| description: "SSZ serialized `List[SignedProposerPreferences]` bytes. Use Accept header to choose this response type" |
There was a problem hiding this comment.
List requires a limit, need to think more about what the right limit here is, technically you can have proposer preferences from different branches, so 2 * SLOTS_PER_EPOCH might not be sufficient
| @@ -0,0 +1,47 @@ | |||
| get: | |||
| operationId: getPoolProposerPreferences | |||
There was a problem hiding this comment.
Separately, why do beacon nodes have to store these messages at all?
we do need to store them to be able to validate bids against target gas limit and fee recipient of the proposer
A bit pedantic of me, but I find it strange that there is a pool API for a message type that is not included in blocks.
in lodestar we currently call this ProposerPreferencesPool, but we can think about renaming this if we wanna keep that notion to only use pool for messages that are included in blocks
…n-APIs into proposer-preferences-apis
|
updated wording to fall back to parent gas limit |
| parameters: | ||
| - in: header | ||
| schema: | ||
| $ref: "../../beacon-node-oapi.yaml#/components/schemas/ConsensusVersion" | ||
| required: true | ||
| name: Eth-Consensus-Version | ||
| description: "The active consensus version to which the signed proposer preferences being submitted belongs." |
There was a problem hiding this comment.
Do we need to enforce the consensus version here?
There was a problem hiding this comment.
that's required to make the endpoint forward compatible, otherwise if SignedProposerPreferences format changes in the future (eg. in heze) you cannot signal to the receiver what's the data format you are sending (required by receiver to decode the data), the receiver could always compute it from the current clock slot in the case here, but since proposer preferences should be send 1 epoch before the fork (due to epoch lookahead), that's not a good option either.
…n-APIs into proposer-preferences-apis
**What type of PR is this?**
Feature
**What does this PR do? Why is it needed?**
- adding /eth/v1/validator/proposer_preferences POST endpoint, also
hooks up validator client
- adding `proposer_preferences` SSE event topic on /eth/v1/events
kurtosis setup
```
participants:
- el_type: ethrex
el_image: ethpandaops/ethrex:glamsterdam-devnet-4
el_extra_params:
- --http.api=eth,net,web3,admin
cl_type: prysm
cl_image: gcr.io/offchainlabs/prysm/beacon-chain:latest
vc_image: gcr.io/offchainlabs/prysm/validator:latest
supernode: true
count: 2
cl_extra_params:
- --subscribe-all-subnets
- --verbosity=debug
vc_extra_params:
- --enable-beacon-rest-api
- --verbosity=debug
- el_type: ethrex
el_image: ethpandaops/ethrex:glamsterdam-devnet-4
el_extra_params:
- --http.api=eth,net,web3,admin
cl_type: prysm
cl_image: gcr.io/offchainlabs/prysm/beacon-chain:latest
vc_image: gcr.io/offchainlabs/prysm/validator:latest
validator_count: 63
cl_extra_params:
- --verbosity=debug
vc_extra_params:
- --enable-beacon-rest-api
- --verbosity=debug
network_params:
fulu_fork_epoch: 0
gloas_fork_epoch: 2
seconds_per_slot: 6
genesis_delay: 40
additional_services:
- dora
global_log_level: debug
```
depends on OffchainLabs#16421 + adding
some connection changes for ptc duties
for more perfect run otherwise you will see some orphans
**Which issues(s) does this PR fix?**
implements ethereum/beacon-APIs#608 changes
note: this pr does not implement deprecation or the get proposer
preferences endpoint
**Other notes for review**
**Acknowledgements**
- [x] I have read
[CONTRIBUTING.md](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md).
- [x] I have included a uniquely named [changelog fragment
file](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md#maintaining-changelogmd).
- [x] I have added a description with sufficient context for reviewers
to understand this PR.
- [x] I have tested that my changes work as expected and I added a
testing plan to the PR description (if applicable).
---------
Co-authored-by: Jun Song <87601811+syjn99@users.noreply.github.com>
nflaig
left a comment
There was a problem hiding this comment.
LGTM, I really like all the notes for implementers, great work, thanks @james-prysm
**What type of PR is this?** Feature **What does this PR do? Why is it needed?** adds missed ssz support from ethereum/beacon-APIs#608 pr, and defaults validator client to use ssz for proposer preferences **Which issue(s) does this PR fix?** addresses partially ethereum/beacon-APIs#608 **Other notes for review** **Acknowledgements** - [x] I have read [CONTRIBUTING.md](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md). - [x] I have included a uniquely named [changelog fragment file](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md#maintaining-changelogmd). - [x] 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). --------- Co-authored-by: Jun Song <87601811+syjn99@users.noreply.github.com>
As per ethereum/beacon-APIs#608 > Validator clients SHOULD begin submitting signed proposer preferences one epoch before the Gloas fork activates, so the proposer preference caches kept by both beacon nodes and builders are warm at the fork boundary, letting builders prepare valid bids for the first Gloas slots. Also related to ethereum/consensus-specs#4947 but we already do this since we subscribe to new gossip topics 2 epochs before the fork (`SUBSCRIPTIONS_LOOKAHEAD_EPOCHS = 2`).
Supersedes #593 with ethereum/consensus-specs#5196, ethereum/consensus-specs#5236 added
Adds the proposer preferences API for Gloas, deprecates the legacy
fee-recipient/gas-limit endpoints, and tightens
beacon_committee_subscriptionsforCGC bookkeeping.
Added
POST /eth/v1/validator/proposer_preferences— VC submission of signedpreferences and publishes them on the
proposer_preferencesgossipsub topic.proposer_preferencesSSE event for messages passing gossip validation.Gloas.ProposerPreferences/Gloas.SignedProposerPreferencestypes, includingdependent_root(used for gossip dedup and validation).Deprecated
POST /eth/v1/validator/prepare_beacon_proposer— superseded by signed proposerpreferences. Pre-Gloas support is REQUIRED; post-Gloas the endpoint MAY be a no-op
or removed.
POST /eth/v1/validator/register_validator— fee recipient and gas limit move tosigned proposer preferences. Same pre/post-Gloas support rules apply.
Updated
POST /eth/v1/validator/beacon_committee_subscriptions— from Gloas onwards, BNsuse the subscription set to identify which validators are using the node and to
size the node's custody group count (CGC). VCs SHOULD submit one entry per
attached active validator per attestation duty, including when multiple
validators share the same
(slot, committee_index), so the BN tracks everyattached validator rather than only one per subnet.
Fixes #570