Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
EIP-3030: BLS Remote Signer HTTP API Standard #3030
EIP-3030: BLS Remote Signer HTTP API Standard #3030
Changes from 13 commits
08093ed
1b6590d
2102d09
0a39bab
c5df99f
6c30758
5b9ed19
fbf2b9c
4e8f773
61195ad
eadb63c
28cc951
cb13fd1
c145013
c625262
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
There is no need for authentication if the requestor is not privileged. And it wouldn't be privileged if you removed the need for the signer to trust the
signingRoot
.So in essence, you now force the validator to store some sealer-credential, instead of a sealing-key -- just swapping one set of secrets for another.
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.
I agree. This rationale should be more emphatic on the point about swapping of secrets, since it beats the purpose of this API. Please notice that the following paragraph advises the reader to explore the use of network Access Control Lists (ACLs).
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.
I'd argue that pre-image validation is such a core feature that the post-image shouldn't even be sent to the signer. The signer should always be forced to calculate the hash-to-sign locally, given the input + info about signing request (maybe using mimetypes to signal what type of signing is requested)
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.
With this spec, as is, you cement the behaviour of a standard-compliant signer to sign blind checks. Which seems like a very bad idea, IMO
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.
We can do the following then:
domain
, thetype
of message to be produced (randao, block, or attestation), and the specificdata
per type of message.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.
You can also remove the
signingRoot
parameter. Btw, can you point me towards info about what thesigningRoot
consists of? Is it the ssz-hash of a full block? or a header? How large is the preimage?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.
The
signingRoot
is the string representation of a SHA256 Hash. The result of the Ethereum 2.0 functioncompute_signing_root
.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.
I see. So, in practice, what's the size of the
ssz_object
? Why not send it in it's entirety?And depending on the size of that -- if it's extremely large, perhaps only portions + proofs/hashes for missing parts?
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.
Fyi this is Prysm's definition. We have a field for the beacon object for additional verifications such as slashing prevention.
https://github.com/prysmaticlabs/prysm/blob/master/proto/validator/accounts/v2/keymanager.proto#L53
It's not a mandatory field, the remote can ignore it
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.
there s one scenario in my head where having the signingRoot together with all the needed fields to recompute it might come in handy, tamper detection. if the sent root and the calculated root mismatch then you have a big problem
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.
In that case, someone is tampering with the traffic. So if they can modify one (e.g. the signingRoot, to sign something else), why wouldn't they tamper with both?
On the one hand, there's a vague argument "it might be handy". So the arguments for including it are:
The arguments against including it are:
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.
well, you are right of course :D thanks ,
pushing for everything to get signeres to not sign anything blindly should be the the goal
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 supports both gRPC and JSON over HTTP. There's a grpc<->json/http proxy. We will update our documentations