Skip to content
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

Merged
merged 2 commits into from
Oct 1, 2023

Conversation

arnetheduck
Copy link
Contributor

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.

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.
@rolfyone
Copy link
Collaborator

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...

@arnetheduck
Copy link
Contributor Author

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.

@rolfyone
Copy link
Collaborator

Im not saying it has to go that way, just pointing out that this is technically breaking. Other teams should state their preference please.

@michaelsproul
Copy link
Collaborator

We haven't finished implementing v3 yet so there's no breakage for us. We're happy with a mandatory consensus value.

@james-prysm
Copy link
Contributor

internally we aren't using it, if vouch is using it it would break them but we can add it mandatory

@g11tech
Copy link
Contributor

g11tech commented Sep 21, 2023

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 👍

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rolfyone
Copy link
Collaborator

@mcdee will wait on your opinion before i merge

@mcdee
Copy link
Contributor

mcdee commented Sep 21, 2023

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
@mcdee
Copy link
Contributor

mcdee commented Sep 21, 2023

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.

@rkapka
Copy link
Collaborator

rkapka commented Sep 21, 2023

My understanding is that the consensus value will be the same as total in the response of https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Rewards/getBlockRewards. Is this correct?

@arnetheduck
Copy link
Contributor Author

of the the current block packing algorithms take this into account

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.

Is this correct?

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.

@michaelsproul
Copy link
Collaborator

As a follower of the v3 API and rewards API I agree we should define the consensus block value as equal to the total returned by the rewards API

@mcdee
Copy link
Contributor

mcdee commented Sep 21, 2023

computing the value of a block is separate from the packing process

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).

As a follower of the v3 API and rewards API I agree we should define the consensus block value as equal to the total returned by the rewards API

Having the equality between the two API endpoints works, and avoids having to define the value explicitly in the API doc.

@arnetheduck
Copy link
Contributor Author

calculate the value of the block for this change to the API

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.

@rkapka
Copy link
Collaborator

rkapka commented Sep 25, 2023

Why do we set the following headers when the response object also has these values?

  • Eth-Execution-Payload-Blinded
  • Eth-Execution-Payload-Value
  • Eth-Consensus-Payload-Value

@mcdee
Copy link
Contributor

mcdee commented Sep 25, 2023

Why do we set the following headers when the response object also has these values?

  1. allows obtaining the information without requiring partial parsing of the JSON
  2. available for SSZ responses, where the response object does not contain this data

@Falehfale Falehfale mentioned this pull request Sep 28, 2023
Closed
@rolfyone rolfyone merged commit c7ac49e into ethereum:master Oct 1, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants