Add signature verifications #138
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Luca Georges Francois <[email protected]>
Signed-off-by: Luca Georges Francois <[email protected]>
Signed-off-by: Luca Georges Francois <[email protected]>
Signed-off-by: Luca Georges Francois <[email protected]>
Signed-off-by: Luca Georges Francois <[email protected]>
metachris
reviewed
Jun 7, 2022
|
|
||
| ok, err := types.VerifySignature(registration.Message, types.DomainBuilder, registration.Message.Pubkey[:], registration.Signature[:]) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusBadRequest) |
There was a problem hiding this comment.
we should probably log the error (with log.Error), for visibility for the operator
metachris
reviewed
Jun 7, 2022
| return | ||
| } | ||
| if !ok { | ||
| http.Error(w, errInvalidSignature.Error(), http.StatusBadRequest) |
There was a problem hiding this comment.
same as above. maybe even wrap the error and add a bit of context 🤔
There was a problem hiding this comment.
Totally agree, added in a1c0a4d
metachris
reviewed
Jun 7, 2022
| @@ -218,7 +228,7 @@ func (m *BoostService) handleGetHeader(w http.ResponseWriter, req *http.Request) | |||
| var wg sync.WaitGroup | |||
| for _, relay := range m.relays { | |||
| wg.Add(1) | |||
| go func(relayAddr string) { | |||
| go func(relayAddr string, relayPubKey types.PublicKey) { | |||
There was a problem hiding this comment.
do we want to pass in the relay alltogether instead of the individual fields?
There was a problem hiding this comment.
This way, we can only read the data. It slows down a little bit the process due to copies but honestly not that much.
|
very nice! 🙏 |
Signed-off-by: Luca Georges Francois <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #138 +/- ##
==========================================
- Coverage 71.56% 68.77% -2.80%
==========================================
Files 6 6
Lines 517 538 +21
==========================================
Hits 370 370
- Misses 121 137 +16
- Partials 26 31 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: Luca Georges Francois <[email protected]>
This was referenced Jun 9, 2022
Merged
metachris
approved these changes
Jun 9, 2022
|
merging into the develop branch |
metachris
pushed a commit
that referenced
this pull request
Jun 10, 2022
* Add signature verification for getHeader response Signed-off-by: Luca Georges Francois <[email protected]> * Remove typo Signed-off-by: Luca Georges Francois <[email protected]> * Add signature verification for registerValidator request Signed-off-by: Luca Georges Francois <[email protected]> * Update mergemock integration tests Signed-off-by: Luca Georges Francois <[email protected]> * Fix typo in flag Signed-off-by: Luca Georges Francois <[email protected]> * Add error logging Signed-off-by: Luca Georges Francois <[email protected]> * Add signature verification tests Signed-off-by: Luca Georges Francois <[email protected]>
metachris
added a commit
that referenced
this pull request
Jun 14, 2022
* Add signature verifications (#138) * Add signature verification for getHeader response Signed-off-by: Luca Georges Francois <[email protected]> * Remove typo Signed-off-by: Luca Georges Francois <[email protected]> * Add signature verification for registerValidator request Signed-off-by: Luca Georges Francois <[email protected]> * Update mergemock integration tests Signed-off-by: Luca Georges Francois <[email protected]> * Fix typo in flag Signed-off-by: Luca Georges Francois <[email protected]> * Add error logging Signed-off-by: Luca Georges Francois <[email protected]> * Add signature verification tests Signed-off-by: Luca Georges Francois <[email protected]> * Require the relay pubkey (#143) * mev-boost cli flags for setting genesis fork version, computing correct domain (#148) * better errors on failed signature validation (#151) Co-authored-by: Luca G.F <[email protected]>
screwyprof
pushed a commit
to screwyprof/mev-boost
that referenced
this pull request
Feb 3, 2023
* Add signature verifications (flashbots#138) * Add signature verification for getHeader response Signed-off-by: Luca Georges Francois <[email protected]> * Remove typo Signed-off-by: Luca Georges Francois <[email protected]> * Add signature verification for registerValidator request Signed-off-by: Luca Georges Francois <[email protected]> * Update mergemock integration tests Signed-off-by: Luca Georges Francois <[email protected]> * Fix typo in flag Signed-off-by: Luca Georges Francois <[email protected]> * Add error logging Signed-off-by: Luca Georges Francois <[email protected]> * Add signature verification tests Signed-off-by: Luca Georges Francois <[email protected]> * Require the relay pubkey (flashbots#143) * mev-boost cli flags for setting genesis fork version, computing correct domain (flashbots#148) * better errors on failed signature validation (flashbots#151) Co-authored-by: Luca G.F <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description:
This PR adds support for signature verifications in the following handlers :
registerValidatorgetHeadergetPayloadProblem(s) & goal(s):
See #95 for more context.
I have run these commands:
make lintmake testmake run-mergemock-integrationgo mod tidyAdditional comments:
For now, mergemock integration tests do not pass because the relay generates a random private key for each run and mev-boost does not have a way to know which one it is.
I've created a PR which allows to provide a private key to mergemock's relay.