Avoid blob downloads for availability checks#3
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2381cf3dd8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if base.ends_with("/vh") { | ||
| return format!("{base}/{normalized_hash}"); | ||
| } | ||
|
|
||
| vec![ | ||
| format!("{base}/blob/{normalized_hash}"), | ||
| format!("{base}/vh/{normalized_hash}"), | ||
| ] | ||
| format!("{base}/vh/{normalized_hash}") |
There was a problem hiding this comment.
/blob-suffixed PODA URLs correctly
This URL builder now only treats /vh as a terminal path segment, so a configured base like https://.../blob (which was explicitly supported before) is transformed into https://.../blob/vh/<hash>. get_blob_from_cloud uses this directly, causing cloud retrieval to fail for existing deployments that still provide a /blob base even when the blob is present at /blob/<hash>.
Useful? React with 👍 / 👎.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c2db62c17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .map_or(true, Self::check_vh_value_exists) | ||
| } | ||
| _ => true, | ||
| } |
There was a problem hiding this comment.
check_vh_value_exists currently defaults unrecognized payloads to true, so null or unexpected object shapes from /check_vh are interpreted as “blob exists”. In deployments where the endpoint returns null/nonstandard JSON for missing hashes, blob_exists and finality fallback can incorrectly report availability and stop retrying even though the blob is absent. The default branch here should be fail-closed (false or an error) instead of optimistic true.
Useful? React with 👍 / 👎.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 189acdf250
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| format!("{base}/blob/{normalized_hash}"), | ||
| format!("{base}/vh/{normalized_hash}"), | ||
| ] | ||
| format!("{base}/vh/{normalized_hash}") |
There was a problem hiding this comment.
/blob base when building cloud fetch URL
When poda_url is configured with a /blob suffix (which was supported before this change), this builder now produces .../blob/vh/{hash} instead of .../blob/{hash}. That makes get_blob_from_cloud hit a non-existent path and fail all cloud blob downloads for those existing configurations.
Useful? React with 👍 / 👎.
| fn poda_check_vh_batch_url(&self) -> String { | ||
| let mut base = self.poda_url.trim_end_matches('/'); | ||
|
|
||
| for suffix in ["/vh", "/check_vh"] { |
There was a problem hiding this comment.
/blob URLs before calling check_vh
The batch fallback URL stripping only handles /vh and /check_vh, so a configured base like https://host/blob now becomes https://host/blob/check_vh. That breaks cloud existence/finality fallback for prior /blob deployments and can incorrectly report blobs as unavailable/not final even when they exist in PODA.
Useful? React with 👍 / 👎.
|
@codex review we dont support a blob endpoint |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 189acdf250
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| || msg.contains("\"code\":-32602") | ||
| { |
There was a problem hiding this comment.
blobs_exist now always sends getnevmblobdata with [versionhash, false], but any "code":-32602 error is treated as “blob not found” and sent to cloud fallback. If a deployed node only supports the older one-argument signature, every lookup will return -32602, so on-node blobs can be reported unavailable unless PODA also has them. This is a behavior regression from the prior single-call path and can produce false negatives in availability/finality decisions.
Useful? React with 👍 / 👎.
| } | ||
| return Ok(values.iter().map(Self::check_vh_value_exists).collect()); | ||
| } | ||
| return Ok(aggregate_result(value)); |
There was a problem hiding this comment.
check_vh results per hash
When check_vh returns an object under result/results (for example a hash→bool map), this branch collapses it through aggregate_result(value) instead of mapping by hash. Because check_vh_value_exists on a plain map object returns false, all requested hashes are marked unavailable. That yields incorrect cloud fallback results for valid map-style batch responses.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 189acdf250
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| || msg.contains("\"code\":-32602") | ||
| { |
There was a problem hiding this comment.
blobs_exist now always sends getnevmblobdata with [versionhash, false], but any "code":-32602 error is treated as “blob not found” and sent to cloud fallback. If a deployed node only supports the older one-argument signature, every lookup will return -32602, so on-node blobs can be reported unavailable unless PODA also has them. This is a behavior regression from the prior single-call path and can produce false negatives in availability/finality decisions.
Useful? React with 👍 / 👎.
| } | ||
| return Ok(values.iter().map(Self::check_vh_value_exists).collect()); | ||
| } | ||
| return Ok(aggregate_result(value)); |
There was a problem hiding this comment.
check_vh results per hash
When check_vh returns an object under result/results (for example a hash→bool map), this branch collapses it through aggregate_result(value) instead of mapping by hash. Because check_vh_value_exists on a plain map object returns false, all requested hashes are marked unavailable. That yields incorrect cloud fallback results for valid map-style batch responses.
Useful? React with 👍 / 👎.
|
@codex review getnevmblobdata(versionhash, false)No one-arg fallback, no compatibility shim. For the two new PR issues:
So I won’t add old-node fallback or |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 189acdf250
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn truthy_check_vh_response(response: &str) -> bool { | ||
| !matches!( | ||
| response.trim().to_ascii_lowercase().as_str(), | ||
| "0" | "false" | "missing" | "not_found" | "not found" | "notfound" | ||
| ) |
There was a problem hiding this comment.
The string parser currently treats every value except a short denylist as exists=true, so responses like "", "blob not found", or other backend error text will be interpreted as available. In this commit, blob_exists and both finality checks rely on check_vh, so this can incorrectly mark missing blobs as present/finalized when the cloud endpoint returns descriptive non-boolean strings instead of strict booleans.
Useful? React with 👍 / 👎.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review remember no /blob, no one-arg node compatibility, POST /check_vh with aggregate bool or same-length bool array. |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
getnevmblobdatawithgetdata=falsefor availability checks so node lookups do not download blob bytes.blobs_existfor up to 32 version hashes using one JSON-RPC batch request and one PODAcheck_vhPOST-array fallback for missing hashes./vhonly and bump crate metadata and lockfile to0.1.12.Test plan
cargo fmtcargo testcargo check