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

Block headers returning incorrect public key. #660

Open
mcdee opened this issue Nov 11, 2024 · 5 comments
Open

Block headers returning incorrect public key. #660

mcdee opened this issue Nov 11, 2024 · 5 comments

Comments

@mcdee
Copy link

mcdee commented Nov 11, 2024

Describe the bug

The spec at https://ethereum.github.io/builder-specs/#/Builder/getHeader states that the pubkey field in the message struct is the public key of the builder, however the public key of the relay is being returned in this field.

Steps to reproduce

  1. Request a header with /eth/v1/builder/header/{slot}/{parent_hash}/{pubkey}
  2. Check the pubkey field of the response

Expected behavior

The pubkey field should be the public key of the builder, not the relay.

@metachris
Copy link
Collaborator

I think there was a reason why the relay key is there, I'll try to look it up and report back. Not sure if it might break something.

From an Ethereum perspective, the term "builder" in the builder-specs refers to the Builder Network in general. As in, a block coming from the builder network. The source of the block is the relay, and it seems generally okay for the relay pubkey to be in there.

What's the benefit you see on having the builder pubkey in there?

@mcdee
Copy link
Author

mcdee commented Nov 13, 2024

The source of the block is the relay, and it seems generally okay for the relay pubkey to be in there.

The pubkey supplied by the data APIs for the call is that of the builder, not the relay. The relay public key is already provided in its definition URL, for example at the top of https://boost-relay.flashbots.net/ I don't think that "builder" is particularly ambiguous in the spec.

What's the benefit you see on having the builder pubkey in there?

It allows us to differentiate between bids from certain builders. We have seen issues where builders provided an incorrect bid value and want to filter them out, for example.

There is also an issue where, with the public key being in the response, clients are using that public key to verify the signature rather than the one in the URL. This renders the whole idea of signature verification useless, as any key could be used to sign the bid as long as the attacking public key is in place, which opens up the validators to a MiTM attack.

@ralexstokes
Copy link
Collaborator

+1 to metachris

it is confusing but the idea is that the builder specs refers to this "abstract" builder in some places

given that this API refers to communication between the proposer and relay (via mev-boost), the idea is that the pubkey in the response is that of the relay as the response here is signed by the relay (and follows the pattern from the consensus-specs that the signature over a message includes the public key as the last field of the message)

relays should expose the data APIs which provide bid data and can be used to correlate a particular bid with a particular builder

one reason to have bids signed by relays is to lay the groundwork for some kind of "relay fault proof", i.e. in the event of a relay failure clients could conceivably include the signed response from this getHeader call as evidence that a relay failed to release a block (for example)

@mcdee
Copy link
Author

mcdee commented Nov 22, 2024

given that this API refers to communication between the proposer and relay (via mev-boost), the idea is that the pubkey in the response is that of the relay as the response here is signed by the relay (and follows the pattern from the consensus-specs that the signature over a message includes the public key as the last field of the message)

I'm not sure to what this is referring. As far as I'm aware, the main messages signed by validators within the consensus specs (attestations, proposals, sync committee messages, voluntary exit, ...) don't include the validator public key.

And as mentioned above, the public key should not be propagated in the same message as the signature. Verifying that signature s relates to public key p provides no useful information in itself unless there is some other knowledge about p. My impression was that this is provided in the relay URL (e.g. the URL at the top of https://boost-relay.flashbots.net/).

one reason to have bids signed by relays is to lay the groundwork for some kind of "relay fault proof", i.e. in the event of a relay failure clients could conceivably include the signed response from this getHeader call as evidence that a relay failed to release a block (for example)

I'm not arguing against this, I agree that bids should be signed by relays. What I'm saying is that the public key in the bid should not be the one used to verify the signature, because as it stands a relay could sign every single request with a different private key if it felt like it and as long as it passed the public key derived from said private key in each header the signature check would pass.

@ralexstokes
Copy link
Collaborator

ralexstokes commented Nov 25, 2024

one example is SignedVoluntaryExit, VoluntaryExit has the validator index instead (just to save space vs writing the entire public key) but same idea

--

yes, I agree the public key in the message should be verified against the expected value (e.g. from the relay public key), secure implementations of mev-boost in this context would check this

--

right, so I guess you'd like to just remove the public key from the message, or instead use it for the builder's public key

I don't think its urgent as you can still use the relay data APIs to correlate the end-to-end flow, but the specs could be changed to include the builder's public key

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

No branches or pull requests

3 participants