Track superseded mempool errors separately

[tilacog]

Description

Mempools::execute() races configured mempools concurrently and returns the first one that succeeds. Previously, errors from mempools that lost the race were counted as real failures, even though the overall submission was successful. Dropped mempools were never recorded.

This skewed mempool counts by both:

• counting errors alongside a successful submission, and
• omitting counts for dropped mempools.

This PR keeps the racing behavior but changes how outcomes interact with the metrics.

Changes

Behavior

• On first success, score a Success metric for the winning mempool and Superseded for every other configured mempool.
• On all-failed, score a Failed metric for each one of them.

Code specific (all in crates/driver)

• domain/mempools.rs:

  • Introduce a private Outcome enum, representing the bare outcome of a mempool submission.
  • Mempools::execute defers updating the metrics after select_ok resolves (once it has all the relevant Outcomes).

• infra/observe/mod.rs:

  • Logs emission and metric scoring functions were split, effectivelly replacing mempool_executed with two narrower helpers:
    • mempool_log: emits only the per-attempt log line, no metrics.
    • mempool_submission_result: increases the metric driver_mempool_submission counter (and the ..._blocks_passed and counter when timing is known) using the final, reclassified label produced by update_metrics.

How to test

Existing driver unit tests cover the race semantics; this PR does not change the externally observable submission outcome, only how observation is sequenced and labeled. To verify manually:

1. Run the driver against a config with at least two mempools.
2. Trigger a settlement that succeeds via the public mempool.
3. Confirm Prometheus shows one result="Success" increment for the winner and one result="Superseded" increment for the loser; no Revert/Expired/Other from the loser.
4. Trigger a settlement that fails on every mempool and confirm each mempool gets its own non-Superseded failure label.

Alert query update needed when deploying

Per-mempool success counts both wins and races-lost (so happy and failure paths both emit N events for N configured mempools, keeping the ratio symmetric). Superseded stays a separate label so dashboards can still distinguish wins from race-losses (and race-losses-that-errored) per mempool.

sum by (network) (increase(driver_mempool_submission{cow_fi_environment="prod",result=~"Success|Superseded"}[2h]))
/
sum by (network) (increase(driver_mempool_submission{cow_fi_environment="prod",result!="Disabled"}[2h])) < 0.6

[fleupold]

Is there a reason you are not using the PR template for the description?

I agree with the change, however I'd like to suggest that we interpret "superseeded" events as success wrt. how you envision to change the metric. A superseeded submission should be considered a successful one.

This way we receive N (# of mempool) events in the happy case, and N events in the failure case allowing us to keep our alert metric as a ratio of successful to failed ones (otherwise failed events would be weighted N times more than successful ones).

[tilacog]

Is there a reason you are not using the PR template for the description?

Apologies, I was in a rush and didn't account for that. I've updated the description to match the template.

I agree with the change, however I'd like to suggest that we interpret "superseeded" events as success wrt. how you envision to change the metric. A superseeded submission should be considered a successful one.

This way we receive N (# of mempool) events in the happy case, and N events in the failure case allowing us to keep our alert metric as a ratio of successful to failed ones (otherwise failed events would be weighted N times more than successful ones).

Agree. I've adjusted the suggested metric.

[gemini-code-assist]

Code Review

This pull request refactors the mempool execution logic to use FuturesUnordered, enabling more detailed tracking of success, failure, and superseded states. It also adds a blocks_passed method to the Error enum for improved block-level timing metrics. A high-severity logic error was identified where disabled mempools are incorrectly reported as 'Superseded' if another mempool wins the race, which would artificially inflate success rate metrics. A correction was suggested to preserve the 'Disabled' status during the racing process.

[tilacog]

Ok, here's what I landed on after addressing @MartinquaXD's comments:

I didn't use a Dashmap or Hashmap because hashing Mempool isn't straightforward (due to an float field in Config), so I kept a vector of pairs instead (Mempool, Outcome) and handled them by position.

I had to use FuturesUnordered instead of select_ok because sending submit's results out of the async closure wasn't possible due to lifetime constraints, plus mempool::Error isn't Clone since anyhow::Error isn't either... and I didn't want to add more complexity there.

So I followed the suggestions as closely as I could:

I used an Outcome enum to capture (and own) errors and submissions results by consuming the results from submit calls, and processed them in their metrics pipeline, finally recreating the returning result to avoid breaking the contract with execute callers (currently just Competition::process_settle_request). I added a few unit tests for that part since they were easy to write.

I think this is in good shape to merge. If we don't see any correctness issues, I'd recommend merging right away and improving the code later, because I think this will help with our current alerts on high mempool submission errors.

[MartinquaXD]

It seems like logging the submission outcome and updating the labels in a single function lands you in hot water. I think the least invasive solution would be:

1. keep using select_ok()
2. log result in select_ok()
3. have 1 generic function that increments the metric for a given mempool based on the given Outcome.

[MartinquaXD]

AFAICS there is no need to introduce the FuturesUnordered. Using select_ok() protects us from the case where there is already some future that returned success but some other futures gets stuck and doesn't finish.

        let mut stats: Vec<Outcome> = std::iter::repeat_with(|| Outcome::Pending)
            .take(self.mempools.len())
            .collect();

        let res = select_ok(
            self.mempools
                .iter()
                .zip(stats.iter_mut())
                .map(|(mempool, stat)| {
                    async move {
                        let result = self
                            .submit(mempool, settlement, submission_deadline, mode)
                            .instrument(tracing::info_span!("mempool", kind = mempool.to_string()))
                            .await;
                        *stat = Outcome::from(&result);
                        result
                    }
                    .boxed()
                }),
        )
        .await;

        self.update_metrics(settlement, &stats);

        Ok(res?.0.tx_hash)

[tilacog]

Good call, thanks!
I really missed the short-circuiting logic between the last commits.

(I'll revisit this note after considering your suggestion for splitting logs and metrics, so what follows is just the initial part of the fix)

I'm afraid we really need FuturesUnordered here, under the premise that Outcome owns submit results. The reason is we can’t create an Outcome from a &result inside an async task sent to select_ok, because:

• It’s short-lived, so we can’t borrow it.
• We can’t clone it, since anyhow::Error isn’t Clone.

The current approach moves error values back and forth. Using FuturesUnordered lets us pull results into the caller’s scope, move them into Outcome, and then extract them again to build the expected return for execute.

To preserve the safety you mentioned, I added a small follow-up in 1cf8033 to exit the while let loop early, reproducing the previous select_ok behavior.

Let me know if I’m missing anything!

[tilacog]

Martin and I had a quick sync, and we decided on the this next set of changes:

1. We will split the code paths for logs and metrics. This will simplify the creation of Outcome values, which can then become stateless enums (meaning they can be assigned from within the select_ok! task scope. (So no need for FuturesUnordered anymore)
2. We should log all errors, even if their submissions turn out to be superseded in the round.
3. It's OK to use mempool names as their IDs (we already do this for the metric labels)
4. After all that, we can simplify the stats vector creation.

[MartinquaXD]

This function looks very complicated. Wouldn't something like this also work?

        let some_submission_successful = stats
            .iter()
            .any(|outcome| matches!(outcome, Outcome::Success(_)));

        for (mempool, outcome) in self.mempools.iter().zip_eq(stats.iter()) {
            match outcome {
                Outcome::Failed(error) if some_submission_successful => {
                    // if there was a successful submission we assume the errors are
                    // false-positivies due to race conditions during the
                    // submission.
                    observe::mempool_outcome(mempool, Outcome::Superseeded),
                }
                _ => observe::mempool_outcome(mempool, outcome),
            }
        }

[tilacog]

Thanks for the suggestion. Addressed in 3551bb6

[MartinquaXD]

You could have a single function like metric_label(&Outcome) which contains those strings inside and calls err.metric_label().

[tilacog]

Addressed in 5eb4b93 & f6cb0a3

[MartinquaXD]

The diff is still larger than anticipated but that seems due to some existing code being moved into separate functions and some re-formatting which is alright.
The actual logic change is now very small.

Already approving as the last comments are mostly nits.

[MartinquaXD]

This string literal has the risk of drifting. I think the less error prone option would be to rename Pending to Superseded and do Outcome::Superseded.metric_label() instead of the string literal here.

[tilacog]

Renamed in 807c325

[MartinquaXD]

Given that we only capture the FailureReason as a label to increase metrics with having a full enum here seems a little much. Directly storing a String or &'static str in the Failed variant seems to be sufficient.

[tilacog]

Addressed in bf24685

[MartinquaXD]

nit: pet peeve of Jose's is that there should not be a bunch of other stuff between struct and impl blocks (in this case of Mempools).

[tilacog]

Addressed in fb4d44e