Skip to content
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

Return HTTP 404 for pruned blob requests #6331

Merged
merged 1 commit into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions beacon_node/http_api/src/block_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,10 +274,27 @@ impl BlockId {
warp_utils::reject::custom_not_found(format!("beacon block with root {}", root))
})?;

// Error if the block is pre-Deneb and lacks blobs.
let blob_kzg_commitments = block.message().body().blob_kzg_commitments().map_err(|_| {
warp_utils::reject::custom_bad_request(
"block is pre-Deneb and has no blobs".to_string(),
)
})?;

// Return the `BlobSidecarList` identified by `self`.
let blob_sidecar_list = chain
.get_blobs(&root)
.map_err(warp_utils::reject::beacon_chain_error)?;
let blob_sidecar_list = if !blob_kzg_commitments.is_empty() {
chain
.store
.get_blobs(&root)
.map_err(|e| warp_utils::reject::beacon_chain_error(e.into()))?
.ok_or_else(|| {
warp_utils::reject::custom_not_found(format!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good change to me.

I'm not sure if we want to introduce new status codes, but there's also 410 Gone which would allow us to differentiate between block not found and expired blobs, without having to parse response message - but I'm also happy with 404.

In PeerDAS we'd also need a error response for nodes that are not capable of serving blobs data (custody columns < 64), and I think we might be returning 404 too? 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

410 sounds good, but we would need to add it to the spec: https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Beacon/getBlobSidecars.

Maybe we can advocate for that, while returning 404 in the meantime?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

"no blobs stored for block {root}"
))
})?
} else {
BlobSidecarList::default()
};

let blob_sidecar_list_filtered = match indices.indices {
Some(vec) => {
Expand Down
117 changes: 117 additions & 0 deletions beacon_node/http_api/tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1667,6 +1667,93 @@ impl ApiTester {
self
}

/// Test fetching of blob sidecars that are not available in the database due to pruning.
///
/// If `zero_blobs` is false, test a block with >0 blobs, which should be unavailable.
/// If `zero_blobs` is true, then test a block with 0 blobs, which should still be available.
pub async fn test_get_blob_sidecars_pruned(self, zero_blobs: bool) -> Self {
// Prune all blobs prior to the database's split epoch.
let store = &self.chain.store;
let split_epoch = store.get_split_slot().epoch(E::slots_per_epoch());
let force_prune = true;
self.chain
.store
.try_prune_blobs(force_prune, split_epoch)
.unwrap();

let oldest_blob_slot = store.get_blob_info().oldest_blob_slot.unwrap();

assert_ne!(
oldest_blob_slot, 0,
"blob pruning should have pruned some blobs"
);

// Find a block with either 0 blobs or 1+ depending on the value of `zero_blobs`.
let mut test_slot = None;
for slot in 0..oldest_blob_slot.as_u64() {
let block_id = BlockId(CoreBlockId::Slot(Slot::new(slot)));
let (block, _, _) = block_id.blinded_block(&self.chain).unwrap();
let num_blobs = block.num_expected_blobs();

if (zero_blobs && num_blobs == 0) || (!zero_blobs && num_blobs > 0) {
test_slot = Some(Slot::new(slot));
break;
}
}
let test_slot = test_slot.expect(&format!(
"should be able to find a block matching zero_blobs={zero_blobs}"
));

match self
.client
.get_blobs::<E>(CoreBlockId::Slot(test_slot), None)
.await
{
Ok(result) => {
if zero_blobs {
assert_eq!(
&result.unwrap().data[..],
&[],
"empty blobs are always available"
);
} else {
assert_eq!(result, None, "blobs should have been pruned");
}
}
Err(e) => panic!("failed with non-404 status: {e:?}"),
}

self
}

pub async fn test_get_blob_sidecars_pre_deneb(self) -> Self {
let oldest_blob_slot = self.chain.store.get_blob_info().oldest_blob_slot.unwrap();
assert_ne!(
oldest_blob_slot, 0,
"oldest_blob_slot should be non-zero and post-Deneb"
);
let test_slot = oldest_blob_slot - 1;
assert!(
!self
.chain
.spec
.fork_name_at_slot::<E>(test_slot)
.deneb_enabled(),
"Deneb should not be enabled at {test_slot}"
);

match self
.client
.get_blobs::<E>(CoreBlockId::Slot(test_slot), None)
.await
{
Ok(result) => panic!("queries for pre-Deneb slots should fail. got: {result:?}"),
Err(e) => assert_eq!(e.status().unwrap(), 400),
}

self
}

pub async fn test_beacon_blocks_attestations(self) -> Self {
for block_id in self.interesting_block_ids() {
let result = self
Expand Down Expand Up @@ -6846,6 +6933,36 @@ async fn get_blob_sidecars() {
.await;
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn get_blob_sidecars_pruned() {
let mut config = ApiTesterConfig::default();
config.spec.altair_fork_epoch = Some(Epoch::new(0));
config.spec.bellatrix_fork_epoch = Some(Epoch::new(0));
config.spec.capella_fork_epoch = Some(Epoch::new(0));
config.spec.deneb_fork_epoch = Some(Epoch::new(0));

ApiTester::new_from_config(config)
.await
.test_get_blob_sidecars_pruned(false)
.await
.test_get_blob_sidecars_pruned(true)
.await;
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn get_blob_sidecars_pre_deneb() {
let mut config = ApiTesterConfig::default();
config.spec.altair_fork_epoch = Some(Epoch::new(0));
config.spec.bellatrix_fork_epoch = Some(Epoch::new(0));
config.spec.capella_fork_epoch = Some(Epoch::new(0));
config.spec.deneb_fork_epoch = Some(Epoch::new(1));

ApiTester::new_from_config(config)
.await
.test_get_blob_sidecars_pre_deneb()
.await;
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn post_validator_liveness_epoch() {
ApiTester::new()
Expand Down
Loading