Add fee recip. to GET validator/blocks

[paulhauner]

Assuming that #178 merges, there is a race-condition regarding the VC communicating the suggested_fee_recipient (SFR) to the BN.

If the VC has not been able to call the prepare_beacon_proposer endpoint prior to block production (i.e., GET validator/blocks), the BN won't know the SFR to pass to the execution node. In such a case, the BN must choose to either:

1. Raise an error and refuse to produce a block.
2. Use the SFR value provided to the BN via CLI/config (if any).
3. Use some default value (e.g., zero-address).

This means that diligent users should have some "catch all" SFR address provided to the BN to cover this edge-case.

A more robust and user-friendly alternative is to add an optional suggested_fee_recipient query parameter to the GET validator/blocks endpoint. This ensures that a properly configured VC will always communicate the appropriate SFR to the BN. A diligent user no longer requires the "catch all" SFR on the BN.

[mcdee]

Could this be solved by requiring a validator client to call prepare_beacon_proposer prior to requesting a block? It seems a cleaner solution than the conflicts that could occur when we're specifying the suggested fee recipient in multiple places.

The call to prepare_beacon_proposers should return pretty much immediately, so I don't see this requirement slowing down block production much. Or am I missing something?

[ajsutton]

Load balancing makes that a bit more complex - the validator client may have called prepare_beacon_proposer but it went to a different backend server (fail over setups would have the same issue). There's currently a few other race condition type situations where this might happen even with a simpler setup. Those could certainly be closed off but we already have to have a bunch of logic in the beacon node to ensure the EL is prepared ahead of time it seems a shame to have to duplicate that in the VC to ensure the beacon node is prepared.

Currently Teku does require prepare_beacon_proposer to be called or it will just fail to produce the block, but it's not entirely ideal. If we do make it strictly required we'd need a specific error code to tell the VC that prepare hadn't been called so that it knows it has to go back and call it then retry.

[mcdee]

I would suggest that a simple load balancer is not really suitable for multiple beacon nodes. Calls like prepare_beacon_proposer and subscriptions, for example, should be sent to all nodes to allow for failover without interruption.

Would there be complicated logic in the validator client to handle this? Would it not just be a case of a rough flow of "fetch proposer duties, send prepare_beacon_proposer, schedule per-duty proposals" for each epoch?

At first glance it seems that enforcing the requirement to call prepare_beacon_proposer and throwing a specific error from the beacon node back to the validator client if not is reasonable, but I'm by no means up to speed with the subtleties of the merge work to date so am likely unaware of the issues involved. I would, however, like to avoid sending the same (nor not the same!) data twice in two calls if possible, especially as this would perhaps make prepare_beacon_proposer appear to be extraneous to someone not versed in the details of VC->BN->EL communications for blocks.

[ajsutton]

I would suggest that a simple load balancer is not really suitable for multiple beacon nodes. Calls like prepare_beacon_proposer and subscriptions, for example, should be sent to all nodes to allow for failover without interruption.

Subscriptions have been handled in the past by using options like --subscribe-all-subnets, so this would be the first time we're directly breaking the ability to use load balancing which does seem like a shame.

Would there be complicated logic in the validator client to handle this? Would it not just be a case of a rough flow of "fetch proposer duties, send prepare_beacon_proposer, schedule per-duty proposals" for each epoch?

Then you have to handle beacon node restarts, validators being added mid-epoch, potentially right before they are due to propose a block. It's not overly common to hit situations where prepare wasn't called so a specific error code that lets the vc know it has to call prepare, then create block again is probably ok but still more complex than just making the info required available on the first call. It's not that it's impossible to handle, just that it's going to wind up looking an awful lot like the code we already have in the BN to ensure prepare is called.

I do understand your concerns about providing the information in two calls but that seems like it could be easily resolved by being explicit that the fee recipient from the get block call is only used if prepare wasn't previously called. And really as long as the BN used one of the two values it shouldn't really matter which one.

The cognitive dissonance I'm having here is that we created prepare_beacon_proposer API on the basis that the beacon node would be responsible for managing the block preparation but now we're finding that actually BOTH the BN and VC are having to manage the prepare/create sequence just in slightly different forms.

[paulhauner]

I would, however, like to avoid sending the same (nor not the same!) data twice in two calls if possible, especially as this would perhaps make prepare_beacon_proposer appear to be extraneous to someone not versed in the details of VC->BN->EL communications for blocks.

I also see the desire to avoid duplicate information, however it does prevent missed blocks in circumstances that users are likely to (infrequently) encounter on mainnet. In my opinion, the benefits outweigh the costs.

Whilst it does add some complexity to the API, it also removes a big ugly problem that GET validator/blocks doesn't have enough information to actually produce a block (without burning block fees).

The cognitive dissonance I'm having here is that we created prepare_beacon_proposer API on the basis that the beacon node would be responsible for managing the block preparation but now we're finding that actually BOTH the BN and VC are having to manage the prepare/create sequence just in slightly different forms.

With prepare_beacon_proposer the BN is still responsible for ensuring forkchoiceUpdated is called with the right params at the right time. Although the VC is now passing an extra field to the BN, the BN is still insulating the VC from a lot of detail about timing and re-orgs. In essence, I don't think we've negated all our hard work 😅

[ajsutton]

With prepare_beacon_proposer the BN is still responsible for ensuring forkchoiceUpdated is called with the right params at the right time. Although the VC is now passing an extra field to the BN, the BN is still insulating the VC from a lot of detail about timing and re-orgs. In essence, I don't think we've negated all our hard work

I meant that to be in support of this change - the VC can still just call get block and is fully insulated for managing prepare. Without this change the VC is still insulated from a bunch of details of the EL but is back to having the burden of guaranteeing prepare is called in all cases.

[mcdee]

In this proposal, if a fee recipient is not already known then a block will be returned only after an execution payload has been built, which could take a significant amount of time (an additional half a second? second? not sure here, but it already takes long enough to receive pure consensus blocks). I get that the idea the BN should reject the request, force the VC to send a prepare_beacon_proposer and then resubmit the request for the block then seems counter-productive here, especially given the fact that I already worry about time taken, but it at least tells you that the BN didn't have a fee recipient for the proposer and allows a level of error reporting and hence correction.

If, for example, there was a bug in a validator client's implementation of prepare_beacon_proposer such that it was never called, or only ever called with the first element, or whatever, then under the current proposal this would end up going unreported to the VC. Similarly with a restarted beacon node, but at least with an error the VC can resend its prepare for all known validators in the epoch, avoiding potentially hitting this problem several times in an epoch.

I agree that having the fee recipient in the validator/blocks call would be nice, I suppose my thinking is that it's either good enough for generating blocks, in which case we can throw out prepare_beacon_proposer, or it isn't, in which case we shouldn't have it in the call. And although having some sort of "base payload" able to be created quickly could be a middle ground wouldn't that require the execution layer to be far more aware of block timings than it is today? But, again, I'm nowhere near deep enough in to the merge specs to know if something like a base payload is viable with the current architecture. I do believe that my concerns about the use of the API are valid, but again I'm not involved enough to know for sure so will leave this to those that are.

[paulhauner]

I agree that having the fee recipient in the validator/blocks call would be nice, I suppose my thinking is that it's either good enough for generating blocks, in which case we can throw out prepare_beacon_proposer, or it isn't, in which case we shouldn't have it in the call.

It might be "good enough for generating blocks", but it's far from optimal. The purpose of prepare_beacon_proposer is to give the EL a look-ahead before block production so they can start building an optimal transactions set given the details of the parent payload. The ELs already have this in PoW and they're regressing if we don't give them the look-ahead. Using the fee recipient on GET validator/blocks is almost guaranteed to be worse, but it'll still produce a useful block for the consensus layer and maybe a useful block for the exec layer.

Taking it a step back, I would say that @mcdee wants a GET validator/blocks will allow an advanced VC implementation to be maximally flexible:

• Feedback from GET validator/blocks if the fee recipient wasn't already known.
• The ability to fail quickly if the fee recipient wasn't known (don't produce a block).

I want a GET validator/blocks that will make a simple VC implementation maximally live:

• The ability to provide fee_recipient to GET validator/blocks and produce blocks even if prepare_beacon_proposer buggy, without needing to explicitly handle this not-unlikely edge-case.

Perhaps something that meets all of these requirements is:

1. There is an optional suggested_fee_recipient: Address query on the GET validator/blocks request
   • If the query param is provided and the fee recipient isn't already known, produce a block.
   • If the query param is not provided and the fee recipient isn't already known, don't produce a block. Return some well-defined error.
2. There is a optional fee_recipient_known: bool "metadata" (data) in the GET validator/blocks response which can tell the VC if the default value was used.

On a side note, I also propose that BNs should be making scary logs if they produce a block with a fee recipient that wasn't already known via prepare_beacon_proposer. This would help both simple and advanced impls.

[mcdee]

My apologies @paulhauner just realised I haven't replied to this. I think that your suggestion gives us the best of all worlds. It gives validator clients flexibility to follow either route, and the information they need if they think they're supplying fee recipient information but not actually.

The only thing I'm wondering about is if the response metadata should be expanded to include the additional situation where prepare_payload provides one fee recipient and blocks provides a different one. Or this could be considered an error, perhaps? Not sure what we should be doing in that situation.