Refactor & Fix Bugs in Payload Selection Logic

[ethDreamer]

Issue Addressed

The existing payload selection logic is very difficult to reason about and there are two different bugs:

// This is the logic before this PR
fn select_payload() {
    if builder_enabled {
        match chain_health {
            healthy => {
                (local_result, relay) = fetch_local_and_builder_payloads_concurrently()
                local = match local_result? {
                   // ^ this line is a bug as it will cause the function to error if local fails but relay doesn't
                   ...
                }
                return match (relay, local) {
                    (Err, Ok) => Payload::Local,
                    (Ok(None), Ok) => Payload::Local,
                    (Ok(Some(relay)), Ok(local)) => {
                        if !always_prefer_builder_payload {
                            if local_value > relay_value {
                                return Payload::Local
                            } else if should_override_builder && percentage < ignore_builder_override_suggestion_threshold {
                                return Payload::Local
                            }
                        }
                        match verify_builder_bid(relay) {
                            Ok(()) => Payload::Builder,
                            Err(not_invalid) => Payload::Local,
                            Err(invalid) => Payload::Local,
                        }
                    }
                    (Ok(Some(relay)), Err) => {
                        match verify_builder_bid(relay) {
                            Ok(()) => Payload::Builder,
                            Err(not_invalid) => Payload::Builder, // this is a bug actually, see below
                            Err(invalid) => Error,
                        }
                    }
                    (Err, Err) => Error,
                    (Ok(None), Err) => Error,
                }
            }
            premerge | optimistic | unhealthy => (),
        }
    }
    Payload::Local
}

fn verify_builder_bid(relay) {
    if relay.value() < builder_profit_threshold {
        return Err(not_invalid)
        // this is a bug because all other error validations haven't run yet so we don't know if it's really invalid
    }
    if relay.actual_payload_validity_checks() {
        return Err(invalid)
    }
    Ok(())
}

I have refactored this code for simplicity and fixing bugs:

fn select_payload() {
    if !builder_enabled {
        return Payload::Local;
    }
    if chain_health != healthy {
        return Payload::Local;
    }
    
    // bug fixed here so the function doesn't return early if local fetch fails
    (local, relay) = fetch_local_and_builder_payloads_concurrently();
    match (relay, local) {
        (Err, Ok(local)) => Payload::Local,
        (Ok(None), Ok(local)) => Payload::Local,
        (Err, Err) => Error,
        (Ok(None), Err) => Error,
        Ok(Some(relay), Ok(local)) => {
            if verify_builder_bid(relay).is_err() {
                return Payload::Local,
            }
            if always_prefer_builder_payload {
                return Payload::Builder,
            }
            if local_value > relay_value {
                return Payload::Local,
            }
            if relay_value < builder_profit_threshold {
                return Payload::Local,
            }
            if should_override_builder && percentage < ignore_builder_override_suggestion_threshold {
                return Payload::Local
            }
            Payload::Builder
        }
        Ok(Some(relay), Err) => {
            if verify_builder_bid(relay).is_err() {
                Error
            } else {
                Payload::Builder,
            }
        }
    }
}

fn verify_builder_bid(relay) {
    if relay.actual_payload_validity_checks() {
        return Err(invalid)
    }
    Ok(())
}

[michaelsproul]

Nice fixes and a much clearer refactor!

I just had one suggestion about naming.