Broadcast various requests as per #4684

[uvizhe]

Issue Addressed

Addresses #4684

Proposed Changes

Adds to a validator client --broadcast option taking multiple possible values.

Additional Info

I included a commit with multiple flags to make it possible to assess which variant fits better (because single option adds complexity to the code).

[CLAassistant]

All committers have signed the CLA.

[xrchz]

I think "none" may be a little confusing - is it not ok to have an empty string for that case?

[uvizhe]

@xrchz but isn't disabling broadcasting by supplying bare --broadcast flag more confusing?

[xrchz]

@xrchz but isn't disabling broadcasting by supplying bare --broadcast flag more confusing?

🤷‍♀️ I think it makes more sense that way

[michaelsproul]

I don't mind the --broadcast none syntax. Lighthouse doesn't have any other flags like --flag "", so I think that would be a bit strange

I'll try to review properly in the next little while, but seems fine at a glance

[michaelsproul]

@uvizhe Looks like cargo-fmt is failing, you can fix it with cargo fmt --all

[uvizhe]

@michaelsproul is it ok if I shorten 'sync-committee-messages' flag value to 'sync-committee' to match enum variant?

[michaelsproul]

@uvizhe yeah I think that's fine

[michaelsproul]

Looking good on the whole, just a few minor suggestions

[uvizhe]

@michaelsproul can we move further?

It's not that I'm in a hurry, I just want to make sure my part is done here.

[michaelsproul]

I think we're probably good to review and test this. It would be good if you could integrate it into one of the simulator tests

[michaelsproul]

I'm at devconnect right now but should have some time to review in a bit, or worst case once I'm back in 10 days

[uvizhe]

Regarding adding more tests, I'm afraid I can't do it in the scope of this issue. This would require mocking at least BeaconNodeHttpClient and depending on an approach (either integration test or a chain of unit tests) adding more helper code / tests somewhere. This is not well aligned with present tests, so all the negotiations can delay the PR for indefinite time.

[michaelsproul]

I think it would be relatively straightforward to adapt one of the existing simulator tests to use the broadcast flags. I can push a commit to do this shortly

[michaelsproul]

I pushed those tests, using some code from #4393 (thanks @macladson). That PR will conflict slightly with this one, but I think we should merge this first as it's simpler.

[uvizhe]

@michaelsproul, I'm not sure if you noticed my last force-push

[michaelsproul]

@uvizhe yeah I did thanks, will deploy this on some testnet infra next week then we can merge

I'm still on leave after devconnect and will be back next week

[xrchz]

before next release i hope 😁 and not too long before

[michaelsproul]

This is now running on some Goerli and Sepolia validators with --broadcast subscriptions,blocks,sync-committee and seems to be working well. I'll continue rolling it out on our infra ahead of the next release.

Let's merge!