-
Notifications
You must be signed in to change notification settings - Fork 827
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upload proposals' blobs separately. #3204
Conversation
pub async fn maybe_insert(&mut self, blob: &Blob) -> Result<(), ViewError> { | ||
let blob_id = blob.id(); | ||
if let Some(maybe_blob) = self.pending_blobs.get_mut(&blob_id).await? { | ||
if maybe_blob.is_none() { | ||
*maybe_blob = Some(blob.clone()); | ||
} | ||
} | ||
Ok(()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be useful to signal somehow if the blob was not inserted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so: We could then return to the client whether the blob was inserted in any of the collections. I'll do that in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, not sure it would be useful at all:
HandlePendingBlob
currently returns a ChainInfo
. I'm not sure it would be more useful to replace this with a bool
, or worth it to add another response struct that contains both a bool
and a ChainInfo
.
Because on the client side, there isn't necessarily an expectation that we get a true
response: E.g. other clients might currently be sending the same validated block certificate and also upload the blobs, so we would get back false
because now the validator already had it.
Anyway, happy to discuss this next week!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking more along the lines of: if we try to insert an unexpected blob, maybe we'd like to know? Even if just to log it on the validator side, because that indicates either malicious behavior or a bug. It shouldn't happen, but if it does happen, it might be better to know than just silently ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I'll look into it.
I don't think we can 100% rule out false positives, but at least for now it should be fine to log a warning.
…#3244) ## Motivation We currently don't detect if the blob in `HandlePendingBlob` was needed at all. (See [this discussion](#3204 (comment))) ## Proposal Return an error if no pending block needed it. ## Test Plan CI should catch regressions. ## Release Plan - Nothing to do / These changes follow the usual release cycle. ## Links - Discussion: #3204 (comment) - [reviewer checklist](https://github.com/linera-io/linera-protocol/blob/main/CONTRIBUTING.md#reviewer-checklist)
Motivation
Including blobs with the gRPC message that contains a block proposal or certificate severely limits the total size of the blobs. (See #3048.)
Proposal
Remove the
blobs
field from block proposals, and handle blobs separately, with one message each.I added a
maximum_published_blobs
limit per block, and limited the number of pending proposals with blobs to 1 for permissionless chains. (See #3203.)Test Plan
The tests have been updated where necessary. Otherwise they already cover different scenarios involving proposals with blobs.
Release Plan
Links