-
Notifications
You must be signed in to change notification settings - Fork 174
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
blocksv3: add consensus value #358
Conversation
In order to compare blocks coming from two sources, a validator client must consider both consensus and execution profit - without consensus reward information, the VC must fall back on unreliable heuristics that can be biased towards inefficient consensus packing, specially when execution payloads are the same which they are likely to be if the block producer is using the same mev relay for both beacon nodes. The consensus computation is currently not a necessary part of block construction but given that blocks are constructed over a state, the information necessary to compute the is already present at block construction time. This PR suggests a mandatory value to make validator client multiplexing easier at the expense of beacon node complexity - if instead it is optional, we are back to where we started where blocks cannot be compared across implementations. The value has to be trusted, ie the BN could be report an inaccurate value but this is not a new risk.
This being 'required' means it's basically a breaking change. That's potentially ok, but it'll lead to some chaos for anyone that's already implemented v3 without it. Because this is only used on devnets currently It's not impossible, I think ideally these new ones would be non required and therefore not a breaking change... |
Considering that the execution value is not fit for purpose without a corresponding consensus value, I consider this a bugfix that's part of the process of creating the API in the first place - my understanding is that nobody has really gone through with v3 yet and we're still in the early experiments phase, a point at which I think it's generally convenient to be lenient. Perhaps we should mark new API experimental and prone to breakage for this reason? An api that hasn't been hardened by a few rounds of implementation usually has small omissions like this. |
Im not saying it has to go that way, just pointing out that this is technically breaking. Other teams should state their preference please. |
We haven't finished implementing v3 yet so there's no breakage for us. We're happy with a mandatory consensus value. |
internally we aren't using it, if vouch is using it it would break them but we can add it mandatory |
lodestar has v3 implemented on a branch and we intend to include it in final deneb cut, but surely can include consensus value. mandatory is good for us as well 👍 |
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.
lgtm
@mcdee will wait on your opinion before i merge |
I'd be okay with this if the definition of the consensus value is fully defined. Specifically, we would need to ensure that the value of included attestations is correct. Attestations within an aggregate that have already been included in prior blocks in the chain are valued at 0, so knowledge of the past 64 slots' on-chain attestations are required to calculate this accurately. I do not know if all of the the current block packing algorithms take this into account, so I would like to hear a confirmation from client teams that they can provide this value accurately before we make it a mandatory requirement for the endpoint to provide. I'd also like to see the wording tightened up to reflect this. |
1 similar comment
I'd be okay with this if the definition of the consensus value is fully defined. Specifically, we would need to ensure that the value of included attestations is correct. Attestations within an aggregate that have already been included in prior blocks in the chain are valued at 0, so knowledge of the past 64 slots' on-chain attestations are required to calculate this accurately. I do not know if all of the the current block packing algorithms take this into account, so I would like to hear a confirmation from client teams that they can provide this value accurately before we make it a mandatory requirement for the endpoint to provide. I'd also like to see the wording tightened up to reflect this. |
My understanding is that the consensus value will be the same as |
computing the value of a block is separate from the packing process - it's something you do after you've packed your block using the same spec functions that are used to compute the reward in the state transition. Smart clients might want to take into account history and profit during packing process, but this is not a requirement / orthogonal to providing the value - afair, the eth value is mostly useful for comparing recent attestations from a different fork vs old attestations from the canonical fork but this point of view might be out of date. We don't have any requirements or tests that clients pack optimal blocks, in part because doing so is computationally expensive in edge cases, but also because the packing depends on client options (like subscribe-all-subnets). As @michaelsproul points out, today we have the undesireable combination of clients packing suboptimal blocks and vc:s picking these poorly packed blocks when better ones exist locally in a multi-client-bn setup.
Presumably yes ;) I haven't followed the rewards api closely but it seems like a reasonable interpretation - I can't imagine why it wouldn't be the same. |
As a follower of the v3 API and rewards API I agree we should define the consensus block value as equal to the |
Yeah, the point was that if the packing process isn't aware of the prior history of the chain then it will equally fail to be able to calculate the value of the block for this change to the API (or will need to be written separately, which could be a chunk of work for client teams).
Having the equality between the two API endpoints works, and avoids having to define the value explicitly in the API doc. |
Well.. not really, as I pointed out above: the packing can be done independently of the final block and its eth value - packing is the process of selecting attestations for the block - it is unaware of anything else that goes into the block and can be done purely based on heuristics, bitset coverage and other tricks. The vast majority of profitable attestations are not in the history: they're from the previous slot and history doesn't matter for them - a better packing algorithm might look deeper and do more tricks, but critically, it doesn't need to know anything about eth value (which depends on validator counts and a bunch of other things). Computing the value of the block is done while applying the block to the state in order to compute the new state root - it's done after the attestations have been chosen and after the other components of the block are selected (slashings, exits and so on). It is completely independent of packing. This is where history must be used (in the form of a prestate for the block), because it also is needed for other things. |
Why do we set the following headers when the response object also has these values?
|
|
In order to compare blocks coming from two sources, a validator client must consider both consensus and execution profit - without consensus reward information, the VC must fall back on unreliable heuristics that can be biased towards inefficient consensus packing, specially when execution payloads are the same which they are likely to be if the block producer is using the same mev relay for both beacon nodes.
The consensus computation is currently not a necessary part of block construction but given that blocks are constructed over a state, the information necessary to compute the is already present at block construction time.
This PR suggests a mandatory value to make validator client multiplexing easier at the expense of beacon node complexity - if instead it is optional, we are back to where we started where blocks cannot be compared across implementations.
The value has to be trusted, ie the BN could be report an inaccurate value but this is not a new risk.