Get rid of get_blobs_not_found and unify blob errors #2649

ndr-ds · 2024-10-18T18:38:25Z

Motivation

We've had these two different error variants for blobs for a while: BlobNotFoundOnRead and BlobsNotFound. The motivation was to differentiate between errors when doing a storage read, and everything else. In the current state of the code, I don't think this distinction is really giving us much, and it complicates the code more.

We also have this get_blobs_not_found function that abstracts away the different nested error types.

Proposal

Remove BlobNotFoundOnRead, and simplify things a bit. Wrote custom/manual From implementations for these different error types, so we can have a BlobsNotFound error everywhere and can get rid of get_blobs_not_found.
Fixes #2628

Test Plan

CI

Release Plan

Nothing to do / These changes follow the usual release cycle.

ndr-ds · 2024-10-18T18:38:38Z

Remove application registry #2650
Use ApplicationDescription blob type #2647
Create ApplicationDescription blob type #2612
Get rid of get_blobs_not_found and unify blob errors #2649 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ndr-ds and the rest of your teammates on Graphite

ma2bd · 2024-10-21T22:16:38Z

linera-execution/src/system.rs

    #[error("Oracle response mismatch")]
    OracleResponseMismatch,
    #[error("No recorded response for oracle query")]
    MissingOracleResponse,
 }

+impl SystemExecutionError {
+    pub fn get_blobs_not_found(&self) -> Option<Vec<BlobId>> {


In the rest of the code, we try to normalize the errors during conversion instead. In this case, I wonder why we have so many kinds of BlobsNotFound.

(There's also #2628 for this.)

Yes, I agree this is not the ideal end solution for this, this is just an intermediate step until we get to #2628

Let's add a TODO(#2628) to both get_blobs_not_found functions.

Talked with @afck a while back, I'm just gonna do #2628 here already

linera-core/src/updater.rs

afck

I'm not sure about this. It mainly shows that we have too many error types that are nested in too many ways. But cleaning those up is beyond the scope of this PR, of course.

I'm also a bit concerned about the fact that we now convert BlobsNotFound to the corresponding variant of the higher-level type implicitly everywhere, using ?: Should they really be treated all the same? E.g. aren't there cases where a validator should locally expect to have a particular blob—i.e. should respond with something like an "internal error"—that are different from other places where it's correct to respond with a BlobsNotFound to the client, so they can send them to us?

Anyway, I'm approving this for now because I don't have a simple better idea and I don't think it introduces new problems, but I'd prefer if we could get another opinion on this PR, by a second reviewer.

ndr-ds · 2024-11-08T19:47:43Z

Those are good points. I think right now though, AFAIU, the behavior is that we always return the error to the client, and fail on the client if the blob is actually not found. This is a behavior that could be looked into though, if we think it should be done differently 🤔

afck · 2024-11-13T09:30:10Z

linera-core/src/client/mod.rs

+            error => Self::RemoteNodeError(error),
+        }
+    }
+}


Actually, I think this is a conversion we should not make, since we will want to distinguish: If the error is that the remote node doesn't have a blob, we need to send it. If the error is that we don't have a blob, we need to request it or fail. (See #2871.)

ma2bd · 2024-11-13T16:52:34Z

linera-core/src/client/mod.rs

-                    } else {
+                Err(LocalNodeError::BlobsNotFound(blob_ids)) => {
+                    let blobs = remote_node.try_download_blobs(blob_ids.as_slice()).await;
+                    if blobs.len() != blob_ids.len() {


this is weird but changed in the next PR, right?

I think @afck had a PR that fixed this behavior, so this is already gone in my local version of this PR. Will update it in a bit!

Didn't I already merge that? I think try_download_blobs returns None if it couldn't download all requested blobs? So I think you can remove this comparison.

I think that's what I said 😅 already have that done locally, but haven't updated the PR yet

ndr-ds · 2024-11-13T20:58:04Z

Merge activity

Nov 13, 3:58 PM EST: A user started a stack merge that includes this pull request via Graphite.
Nov 13, 3:58 PM EST: A user merged this pull request with Graphite.

ndr-ds force-pushed the 10-18-blobnotfoundonread_-_blobsnotfound branch 2 times, most recently from c5174be to b49501f Compare October 18, 2024 20:12

This was referenced Oct 18, 2024

Create ApplicationDescription blob type #2612

Closed

Use ApplicationDescription blob type #2647

Closed

ndr-ds marked this pull request as ready for review October 18, 2024 21:14

ndr-ds requested review from afck, jvff and ma2bd October 18, 2024 21:15

ndr-ds mentioned this pull request Oct 19, 2024

Remove application registry #2650

Closed

ma2bd reviewed Oct 21, 2024

View reviewed changes

afck reviewed Oct 22, 2024

View reviewed changes

linera-core/src/updater.rs Outdated Show resolved Hide resolved

ndr-ds force-pushed the 10-18-blobnotfoundonread_-_blobsnotfound branch 2 times, most recently from 78912fd to bb341a0 Compare October 23, 2024 13:19

ndr-ds force-pushed the 10-18-blobnotfoundonread_-_blobsnotfound branch 2 times, most recently from 84566e6 to ca23cb5 Compare November 7, 2024 00:08

ndr-ds requested review from afck and ma2bd November 7, 2024 01:49

afck approved these changes Nov 8, 2024

View reviewed changes

afck mentioned this pull request Nov 8, 2024

Simplify and comment some blob-related code. #2849

Merged

ndr-ds force-pushed the 10-18-blobnotfoundonread_-_blobsnotfound branch 2 times, most recently from 0e1d67d to e9a3222 Compare November 8, 2024 19:41

afck reviewed Nov 13, 2024

View reviewed changes

afck self-requested a review November 13, 2024 09:30

ndr-ds changed the title ~~BlobNotFoundOnRead -> BlobsNotFound~~ Get rid of get_blobs_not_found and unify blob errors Nov 13, 2024

ma2bd reviewed Nov 13, 2024

View reviewed changes

BlobNotFoundOnRead -> BlobsNotFound

b550792

ndr-ds force-pushed the 10-18-blobnotfoundonread_-_blobsnotfound branch from e9a3222 to b550792 Compare November 13, 2024 19:13

afck approved these changes Nov 13, 2024

View reviewed changes

ndr-ds merged commit 143a051 into main Nov 13, 2024
24 checks passed

ndr-ds deleted the 10-18-blobnotfoundonread_-_blobsnotfound branch November 13, 2024 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of get_blobs_not_found and unify blob errors #2649

Get rid of get_blobs_not_found and unify blob errors #2649

ndr-ds commented Oct 18, 2024 •

edited

Loading

ndr-ds commented Oct 18, 2024 •

edited

Loading

ma2bd Oct 21, 2024 •

edited

Loading

afck Oct 22, 2024

ndr-ds Oct 23, 2024

afck Oct 28, 2024

ndr-ds Nov 5, 2024

afck left a comment

ndr-ds commented Nov 8, 2024

afck Nov 13, 2024

ma2bd Nov 13, 2024

ndr-ds Nov 13, 2024

afck Nov 13, 2024

ndr-ds Nov 13, 2024

ndr-ds commented Nov 13, 2024 •

edited

Loading

Get rid of get_blobs_not_found and unify blob errors #2649

Get rid of get_blobs_not_found and unify blob errors #2649

Conversation

ndr-ds commented Oct 18, 2024 • edited Loading

Motivation

Proposal

Test Plan

Release Plan

ndr-ds commented Oct 18, 2024 • edited Loading

ma2bd Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afck left a comment

Choose a reason for hiding this comment

ndr-ds commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndr-ds commented Nov 13, 2024 • edited Loading

Merge activity

ndr-ds commented Oct 18, 2024 •

edited

Loading

ndr-ds commented Oct 18, 2024 •

edited

Loading

ma2bd Oct 21, 2024 •

edited

Loading

ndr-ds commented Nov 13, 2024 •

edited

Loading