Add storage and resend for validators preferences #149

0xpanoramix · 2022-06-10T14:42:04Z

Description:

This PR adds a way to send validators preferences at a regular interval to the relays.
It follows the implementation proposed in #141 and is part of the steps to achieve v1 as described in #95.

Problem(s) & goal(s):

//

Additional context & references:

This PR adds a method to handle the logic of sending the preferences to the relays, which is used by both the standard handler and a new method used to send the already registered prefs at a provided interval.

Several questions still remains regarding the implementation, see #141 for more.

I have run these commands:

make lint
make test
make run-mergemock-integration
go mod tidy

metachris · 2022-06-13T12:08:33Z

Very nice, and thoughtful! I noticed that registerValidatorAtInterval is not yet used, but it's still marked as wip so I'm sure you're aware ^^

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

Ruteri · 2022-06-14T08:35:18Z

cmd/mev-boost/main.go

+	defaultRelayTimeoutMs            = getEnvInt("RELAY_TIMEOUT_MS", 2000) // timeout for all the requests to the relay
+	defaultRelayCheck                = os.Getenv("RELAY_STARTUP_CHECK") != ""
+	defaultGenesisForkVersion        = getEnv("GENESIS_FORK_VERSION", "")
+	defaultRegisterValidatorInterval = getEnvInt("REGISTER_VALIDATOR_INTERVAL_SEC", 5)


Let's think about a sane default, if all of the validators send their registration every 5 seconds it will effectively be a DDoS attack on the unfortunate relay on the receiving end
Slot is 12 seconds, that's a sane lower bound
An epoch is 384 seconds, and validators are asked to re-register once per epoch so it's an upper bound
I think something like a fourth (96s) of an epoch is already frequent enough

even resending once an epoch should be fine (and instantly on receiving)

I agree with your comments, but it seems like it's a bad idea to let the user decide of the interval in the end, wdyt ?

Ruteri · 2022-06-14T08:46:36Z

server/service.go

+		select {
+		case <-done:
+			return
+		case <-ticker.C:


One issue that arises is that we can send twice in a short interval (if a registration request comes near the timer tick)
In this approach it's not easy to solve it, and I think we could come at this from a different angle:

make registerValidatorAtInterval the only place responsible for sending the registration

listen on both the timer, and a new channel that accepts new registration requests

on new registration request, send the preferences and reset the timer

This has some nice benefits:

m.vp is now an object local to registerValidatorAtInterval

validator preferences struct can become just a plain old map

no need for mutex

after a registration you will correctly wait until the next interval

The only downside is that you'll need to return the registration status back over a channel (or a callback), but it should be okay.

We can submit a PR to this PR if you prefer not implementing it

Totally agree with this comment ! Thanks a lot for raising the issue and proposing an alternative 👍
I can make the updates don't worry.

Added in de04d2f

cc @Ruteri

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

Ruteri · 2022-06-14T11:37:19Z

server/service.go

+	// Send the payload to the goroutine responsible for handling the resend at interval
+	m.newRegistrationsRequests <- payload
+	// Block until we get the number of successful requests back from this goroutine
+	numSuccessRequestsToRelay := <-m.numSuccessRequestsToRelay


Once context (timeout) handling is implemented this will become a race, the result channel (or callback) should be specific to the request.
I think pushing a pair of (payload, result chan) would be a clean way to solve it that will also work nicely with context timeouts.

I really like the idea of pushing the response channel, I've added this in 2d26f24

Ruteri · 2022-06-14T11:44:03Z

server/service.go

+
+	// Used to stop registerValidatorAtInterval
+	done chan bool
+	// Used by registerValidator to share new incoming registration request with the goroutine holding the ticker


It'd be great to avoid fields shared across requests as they usually introduce a race

Ruteri · 2022-06-14T11:45:47Z

server/service.go

@@ -56,10 +57,17 @@ type BoostService struct {
 	serverTimeouts       HTTPServerTimeouts

 	httpClient http.Client
+
+	// Used to stop registerValidatorAtInterval
+	done chan bool


Another way to implement shutdowns will be to have a context with cancel, and storing the cancel function.
Since we are not using contexts yet this is good too.

I agree, that's what they've been made after all.
Fixed in a2bb054

Ruteri · 2022-06-14T11:46:27Z

server/service.go

@@ -110,10 +122,15 @@ func (m *BoostService) StartHTTPServer() error {
 		IdleTimeout:       m.serverTimeouts.Idle,
 	}

+	// Start separate process to send validator preferences at regular interval.
+	go m.registerValidatorAtInterval(time.Second*384, m.done)


I'm guessing this should be registerValidatorIntervalSec

Missed that, thanks ! Fixed in e1d6572.

metachris · 2022-06-14T12:12:45Z

server/service.go

@@ -384,3 +435,7 @@ func (m *BoostService) handleGetPayload(w http.ResponseWriter, req *http.Request
 		return
 	}
 }
+
+func (m *BoostService) shutdown() {
+	m.done <- true


using a context would be better because if there's multiple things to shut down, this channel would only work for one of them. could be okay for now, and we can also do this in another PR.

Same as above, fixed in a2bb054

metachris · 2022-06-14T12:14:57Z

server/service.go

+		case <-done:
+			// mev-boost has probably stopped
+			return
+		case payload = <-m.newRegistrationsRequests:


this will overwrite the previous payload, but it's conceivable that there's multiple validators using one BN that would then overwrite each others registrations. perhaps better to make this additive?

I'm actually not sure about it, the expectation is to have a single BN connecting to mev-boost, and forcing this to be additive would mean you cannot ever remove a registration.

I've implemented the merging process between the old (local payload list) and the new one (received from the channel) but not pushed it, so you'll just have to tell me !

@Ruteri expectation is a single BN, but this single BN could have many validator clients connected! We can not ignore this case.

One approach could be to add a time to each registration and remove old ones.

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

metachris · 2022-06-14T13:51:58Z

server/service.go

+			return
+		case srr := <-m.registrationsRequests:
+			// registerValidator has received new registrations and forwards them to here
+			srr.numSuccessRequestsToRelay <- m.sendValidatorPreferences(log, srr.preferences)


payload isn't updated

👍
Fixed in a3d5a4b

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

metachris · 2022-06-14T14:49:37Z

It was closed because the temporary develop branch was deleted after merging into main. Please point that PR towards the main branch, I seem to not be able to do it from my user, and can't reopen until then :)

0xpanoramix changed the title ~~Add storage for validator preferences~~ Add storage and resend for validator preferences Jun 10, 2022

0xpanoramix changed the title ~~Add storage and resend for validator preferences~~ Add storage and resend for validators preferences Jun 10, 2022

0xpanoramix mentioned this pull request Jun 10, 2022

Store validator preferences and resend them at a regular interval #141

Closed

0xpanoramix added 3 commits June 13, 2022 17:53

Add storage for validator preferences

f4480a3

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

Add method to send validator preferences at regular interval

dc2b14d

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

Add flag for interval duration used to resend validator preferences

7ca7520

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

0xpanoramix marked this pull request as ready for review June 13, 2022 16:17

Add time indication in flag

dfe1e5a

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

Ruteri reviewed Jun 14, 2022

View reviewed changes

0xpanoramix added 3 commits June 14, 2022 11:58

Increase interval for sending validator preferences

3d78915

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

Use shared channels to register validator preferences

de04d2f

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

Add shutdown method to stop boost goroutine

616013f

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

Ruteri reviewed Jun 14, 2022

View reviewed changes

metachris reviewed Jun 14, 2022

View reviewed changes

0xpanoramix added 3 commits June 14, 2022 14:17

Use proper value for register validator preferences

e1d6572

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

Replace done channel with context

a2bb054

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

Replace registrations channels with a safer one

2d26f24

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

metachris reviewed Jun 14, 2022

View reviewed changes

Update register validators payload with new value

a3d5a4b

Signed-off-by: Luca Georges Francois <luca.georges-francois@epitech.eu>

metachris deleted the branch flashbots:develop June 14, 2022 14:48

metachris closed this Jun 14, 2022

metachris mentioned this pull request Jun 14, 2022

v1 implementation tasks #95

Closed

7 tasks

0xpanoramix mentioned this pull request Jun 16, 2022

Add storage and resend for validators preferences #153

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add storage and resend for validators preferences #149

Add storage and resend for validators preferences #149

0xpanoramix commented Jun 10, 2022

metachris commented Jun 13, 2022

Ruteri Jun 14, 2022

metachris Jun 14, 2022

0xpanoramix Jun 14, 2022

Ruteri Jun 14, 2022

0xpanoramix Jun 14, 2022

0xpanoramix Jun 14, 2022

Ruteri Jun 14, 2022

0xpanoramix Jun 14, 2022

Ruteri Jun 14, 2022 •

edited

Loading

Ruteri Jun 14, 2022 •

edited

Loading

0xpanoramix Jun 14, 2022

Ruteri Jun 14, 2022

0xpanoramix Jun 14, 2022

metachris Jun 14, 2022

0xpanoramix Jun 14, 2022

metachris Jun 14, 2022

Ruteri Jun 14, 2022

0xpanoramix Jun 14, 2022

metachris Jun 14, 2022

metachris Jun 14, 2022

0xpanoramix Jun 14, 2022

metachris commented Jun 14, 2022

Add storage and resend for validators preferences #149

Add storage and resend for validators preferences #149

Conversation

0xpanoramix commented Jun 10, 2022

metachris commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ruteri Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

Ruteri Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metachris commented Jun 14, 2022

Ruteri Jun 14, 2022 •

edited

Loading

Ruteri Jun 14, 2022 •

edited

Loading