linera_core::client: batch downloading of missing blobs (#4755)

[ma2bd]

Motivation

Batch downloading missing blobs

Proposal

Backport #4755

Test Plan

CI

[afck]

Sorry, I just realized this is wrong. We have to revert (or fix) the original PR instead:

store_blobs only stores blobs if it has already seen proof for them, so process_certificate must be called first, otherwise the blob won't get written.

[afck]

We could instead call process_certificate in a loop for all the certificates, and make a note of which one caused the first BlobsNotFound error. Then collect all BlobsNotFound errors, download the blobs in a batch, store_blobs, and then call process_certificate in a loop again, starting from the first height where we got an error.

[afck]

Sorry, @Twey, @ma2bd, I missed that the fallback was already implemented! That fixes it, of course. (Although I think what I'm describing above might be a bit simpler in some cases.)

[afck]

(I'd prefer error or result.)

[Twey]

error would be incorrect. result is a good suggestion, but too late :)

[Twey]

Suggested approach in #4770.

[ma2bd]

Remote compatibility tests are not passing (twice). We may want to investigate

[ma2bd]

nit: Can we use let .. = for complex expressions in argument position?

[Twey]

I don't love the added mental load of introducing unnecessary variables. It makes it harder to read the code from top to bottom since you don't know how the thing is going to be used until later, compared to having the top-level/motivating call (store_blobs here) first.

It's sometimes worth it if it's really unclear what the value of the expression is without a name (though that's a bit of a code smell for the names of the functions &c. involved in the expressions), but in this case I think it's pretty obvious that this is a set of blobs.

[Twey]

(best of both worlds is obviously Haskell-esque

    storeBlobs blobs
  where
    blobs = …

😄)

[ma2bd]

Our brains don't seem to work in the same way because for me this style hurts a lot

[ma2bd]

The intermediate variable (when named appropriately) acts as a summary of the complex expression.

let tasks = something_complicated(args).await?; // <-- can fail, can yield to another thread
process_tasks(tasks); // <- do the thing

[Twey]

Values are protocols between a constructor and a consumer. That means there are two information flows that you want to make clear at the point of seeing a value: one is upstream ‘what the value is’; the other is downstream ‘how the value is going to be used’. Usually ‘what the value is’ is evident from ‘how the value is constructed’; sometimes there are also non-obvious invariants that aren't clear from the way the value is constructed, and then IMO it's worth giving the expression a name via variable assignment, named function argument, et cetera, though a better solution (IMO) is to make the constructing variable clearer, e.g. by extracting it into a function whose name or (better) type expresses the invariants.

In your example the problem is that tasks doesn't really tell you what the tasks are for. Are we going to count them? Track their progress? That doesn't just make it harder to see what the function as a whole does, it also might have a bearing on what the tasks themselves actually are above and beyond what's captured in the name: if we're planning to run the tasks then it's implied that they're in a runnable state (not errored, exited, et cetera). In a small two-line example like this you can quickly look at the next line to see how the information is flowing, but as functions get bigger it's easy to lose track, because you've essentially decomposed a tree:

A
| \
B  C
|  |\
D  E F

into a (backwards!) list of edges:

C -> (E, F)
D -> B
A -> (B, C)

(and usually with more noise between them!). Now you have to pay attention to and remember all the names in order to be able to mentally reconstruct the (tree-shaped) information flow of the data, rather than it being inherent in the syntax.

Concretely, in this case, I'd probably call the variable blobs. But that defers the information about why we're interested in having a set of blobs (to store them)! You could call the variable blobs_to_pass_to_store_blobs but that's a little silly to do for everything, and to get the same level of information that's given by having syntactic nesting that reflects the flow of information you need to do this transitively:

let blob_ids_to_filter_to_download_to_store = get_blob_ids();
let filtered_blob_ids_to_download_to_store = filter(blob_ids_to_filter_to_store);
let filtered_downloaded_blobs_to_store = download(filtered_blob_ids_to_download_to_store);
store(filtered_downloaded_blobs_to_store);

In the case where the expression is very opaque, a quick fix can be to give it a descriptive name that describes its value:

let blobs = mysterious_function_that_could_return_anything();

This still obscures the information flow, but it's probably worth it if you don't want to refactor/rename the mysterious function into something more readable (a named function argument gives you the best of both worlds here). But in the case where the expression directly suggests what's being returned, e.g. when you're mapping a function called try_download_blob, declaring that it returns ‘blobs’ is information-free.

There's another argument in your comment about specifically the ? and .await, (I guess) implying that any expression ending with either of those should be assigned to a variable to make them ‘loud, explicit syntax’ as Stroustrup would say. I think this is somewhat a matter of opinion and culture, so I won't try to argue it from first principles, but Rust-the-language has made the decision that these operations are unsurprising enough today to be worthy of the terser syntax after extensive discussion (rather than, say, try or await statements/blocks) and I think idiomatic Rust shouldn't second-guess that.

[deuszx]

Why is this PR not being merged? RN we have a divergence between conway and main.

[Twey]

The remote compatibility test failure doesn't look related to this at all:

2025-10-09T22:32:32.550373Z ERROR linera: Error is Failed to close chain

Caused by:
    chain client error: Signer doesn't have key to sign for chain 726e27a961a72a7e32064a9bc7a68bbf6f825e07ae8b0282698a0215615bab85

[deuszx]

Are we going to merge this or not? AFAIU, it's on main anyway.