Dynamic relay configuration (fixed)

[screwyprof]

📝 Summary

This PR implements Dynamic Relay Configuration Proposal.

At the moment 2 RCPs are implemented:

• File
• JSON API

---

✅ I have run these commands

• make lint
• make test-race
• go mod tidy

[screwyprof]

Hey guys, It's been a while since Capella/Shanghai were merged, any news or ETA?

[metachris]

I like it a lot! Left some minor comments on the registerValidator concurrency handling. The other changes to service.go look good.

Will review the rest soon.

[metachris]

Hey Max, time for an update!

I had this PR on the back of my mind for a while, and did a few partial reviews already.

Then there was a lot of work due to the unbundling attack, and the Shapella upgrade in the week after.

Generally the PR looks good, but I am having a hard time with because of it’s size, and the depth of the changes. That makes it very hard to reason about and ensuring it’s correctness, and it's very important to be careful with merging these things.

For adding such an amount of changes, it would be good to get review and sign-off from multiple core contributors, who I believe are running into the same problem when reviewing this PR - that it’s hard to verify because of it’s size.

My suggestion would be to break up this PR into multiple smaller ones. For instance, start with only the container, or whatever you think would be a good first step. Do you have an idea how it could be broken up effectively? That way it will be easier to review each of the PRs thoroughly and can move this forward.

[beetrootkid]

Generally the PR looks good, but I am having a hard time with because of it’s size, and the depth of the changes. That makes it very hard to reason about and ensuring it’s correctness, and it's very important to be careful with merging these things.

For adding such an amount of changes, it would be good to get review and sign-off from multiple core contributors, who I believe are running into the same problem when reviewing this PR - that it’s hard to verify because of it’s size.

My suggestion would be to break up this PR into multiple smaller ones. For instance, start with only the container, or whatever you think would be a good first step. Do you have an idea how it could be broken up effectively? That way it will be easier to review each of the PRs thoroughly and can move this forward.

Hi Chris. Thanks a lot for the response. I must say I'm a little frustrated since we did check and walk you through it multiple times as we were developing the PR. To be asked three months after submitting the PR to make such a fundamental change is a real challenge. I'm not even clear how "splitting it up" would actually create less of a burden in terms of reviews since for each part, overall context would have to be retained by the reviewer.

We have invested a significant amount of time in delivering this change for mev-boost and the community and it would be a shame for it to be stopped here after all the hard work so far. I think we'll have to regroup internally and figure out what is possible given your feedback and our own roadmap/resource demands.

[ralexstokes]

I think we all agree more flexible relay configuration is a very nice enhancement to this software; that being said, I need to echo metachris that this software has become quite important to the Ethereum ecosystem and so changes need to be merged carefully.

I hear you on the back and forth and understand how frustrating that can be. But I do find it hard to review something this large and so some suggestions:

• I see some whitespace edits, can we pull these out into a separate PR?
• Can we start with just the core changes in a single PR? Which seem to be the Configurator and which every implementation is the simplest (I think just the config file provider, right?)

Once we merge in the skeleton, we can add fancier features like additional configuration providers that pull in more concepts like an API provider

[beetrootkid]

Hi @metachris and @ralexstokes . Thanks a lot for your additional feedback and pointers. We've had a chance to discuss this now internally and wanted to follow up on a couple of things:

1. Can you and the team confirm that you actually want this feature in mev-boost? Regardless of how the PR is sliced and diced, it would be good if you could confirm that this is something you want to and will add.
2. Given that we've provided very detailed and clear documentation, is it possible for you to suggest how exactly you'd like the PR to be split? I know @ralexstokes you've made some suggestions above. Could you add a little more detail and perhaps an indication of how many PRs you'd think we should aim for?

Thanks a lot.

[metachris]

@beetrootkid

Re 1 - i think the question is more whether there is broader interest in this feature. i.e. have other staking pools expressed interest? Feels like this functionality could be useful to others, but i haven't seen that expressed. Have you?

Re 2 - there already were some suggestions how to break it up, and i think as developers you could make the most educated suggestion about this. Anything from 2 to 4 PRs sound like a reasonable number.

Alex is out this week. Pinging @jtraglia and @michaelneuder for thoughts

[jtraglia]

Yeah, I share the same sentiment as the others. This is a beast of a PR and I wouldn't feel comfortable merging it right now, but it is a feature I'd be willing to add. I gave an initial review a while back with mostly positive feedback, but did mention my discomforts with its size. I also think it has a better chance of making it into this repo if it were broken up into more manageable bites. I'm not sure how you should do that exactly, but I'm sure it's possible.

[metachris]

this PR was closed automatically (and by accident) when removing the main branch, sorry about that!

[metachris]

As discussed in the comments, please break this up into smaller changes 🙏

[metachris]

Closing this for now since the last comments were over a month ago. Happy to reopen / continue the conversation if there's interest @beetrootkid. Thanks for the effort