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

GetBlobSidecars API update #8278

Merged
merged 9 commits into from
May 5, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ the [releases page](https://github.com/Consensys/teku/releases).
### Breaking Changes

### Additions and Improvements
- Added metadata fields to getBlobSidecars response.

### Bug Fixes
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,16 @@
"type" : "object",
"required" : [ "data" ],
"properties" : {
"version" : {
"type" : "string",
"enum" : [ "phase0", "altair", "bellatrix", "capella", "deneb", "electra" ]
},
"execution_optimistic" : {
"type" : "boolean"
},
"finalized" : {
"type" : "boolean"
},
"data" : {
"type" : "array",
"items" : {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,18 @@

import static tech.pegasys.teku.beaconrestapi.BeaconRestApiTypes.BLOB_INDICES_PARAMETER;
import static tech.pegasys.teku.beaconrestapi.BeaconRestApiTypes.PARAMETER_BLOCK_ID;
import static tech.pegasys.teku.ethereum.json.types.EthereumTypes.MILESTONE_TYPE;
import static tech.pegasys.teku.infrastructure.http.HttpStatusCodes.SC_OK;
import static tech.pegasys.teku.infrastructure.http.RestApiConstants.EXECUTION_OPTIMISTIC;
import static tech.pegasys.teku.infrastructure.http.RestApiConstants.FINALIZED;
import static tech.pegasys.teku.infrastructure.http.RestApiConstants.TAG_BEACON;
import static tech.pegasys.teku.infrastructure.json.types.CoreTypes.BOOLEAN_TYPE;
import static tech.pegasys.teku.infrastructure.json.types.SerializableTypeDefinition.listOf;

import com.fasterxml.jackson.core.JsonProcessingException;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.function.Function;
import tech.pegasys.teku.api.ChainDataProvider;
import tech.pegasys.teku.api.DataProvider;
import tech.pegasys.teku.infrastructure.async.SafeFuture;
Expand All @@ -38,6 +41,7 @@
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
import tech.pegasys.teku.spec.SpecMilestone;
import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
import tech.pegasys.teku.spec.datastructures.metadata.BlobSidecarsAndMetaData;
import tech.pegasys.teku.spec.schemas.SchemaDefinitionCache;
import tech.pegasys.teku.spec.schemas.SchemaDefinitionsDeneb;

Expand Down Expand Up @@ -75,7 +79,7 @@ private static EndpointMetadata createEndpointMetadata(final SchemaDefinitionCac
@Override
public void handleRequest(final RestApiRequest request) throws JsonProcessingException {
final List<UInt64> indices = request.getQueryParameterList(BLOB_INDICES_PARAMETER);
final SafeFuture<Optional<List<BlobSidecar>>> future =
final SafeFuture<Optional<BlobSidecarsAndMetaData>> future =
chainDataProvider.getBlobSidecars(request.getPathParameter(PARAMETER_BLOCK_ID), indices);

request.respondAsync(
Expand All @@ -86,21 +90,26 @@ public void handleRequest(final RestApiRequest request) throws JsonProcessingExc
.orElse(AsyncApiResponse.respondNotFound())));
}

private static SerializableTypeDefinition<List<BlobSidecar>> getResponseType(
private static SerializableTypeDefinition<BlobSidecarsAndMetaData> getResponseType(
final SchemaDefinitionCache schemaCache) {
final DeserializableTypeDefinition<BlobSidecar> blobSidecarType =
SchemaDefinitionsDeneb.required(schemaCache.getSchemaDefinition(SpecMilestone.DENEB))
.getBlobSidecarSchema()
.getJsonTypeDefinition();
return SerializableTypeDefinition.<List<BlobSidecar>>object()
return SerializableTypeDefinition.<BlobSidecarsAndMetaData>object()
.name("GetBlobSidecarsResponse")
.withField("data", listOf(blobSidecarType), Function.identity())
.withOptionalField("version", MILESTONE_TYPE, (b) -> Optional.of(b.getMilestone()))
Copy link
Contributor

@StefanBratanov StefanBratanov May 1, 2024

Choose a reason for hiding this comment

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

I don't think we need the OptionalField here for version, execution_optimistic and finalized. Can make it similar to GetBlock. The SerializableTypeDefinition is only used for serializing our fields which we know that are always present in our implementation.

Copy link
Member

Choose a reason for hiding this comment

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

The OpenAPI spec has defined them as optional. So I believe we need to use withOptionalField() to ensure we are compliant, don't we?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, if we want to deserialize as well. Otherwise, not needed, since we always add them, but not fuzzed about it. Probably defining them as optional makes it more clear.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking from the perspective of the open api spec that we generate. If we want to match the "official" spec, we would need the optional field so we would generate the open api schema spec as optional as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that makes sense.

.withOptionalField(
EXECUTION_OPTIMISTIC, BOOLEAN_TYPE, (b) -> Optional.of(b.isExecutionOptimistic()))
.withOptionalField(FINALIZED, BOOLEAN_TYPE, (b) -> Optional.of(b.isFinalized()))
.withField("data", listOf(blobSidecarType), BlobSidecarsAndMetaData::getData)
.build();
}

private static ResponseContentTypeDefinition<List<BlobSidecar>> getSszResponseType() {
final OctetStreamResponseContentTypeDefinition.OctetStreamSerializer<List<BlobSidecar>>
serializer = (data, out) -> data.forEach(blobSidecar -> blobSidecar.sszSerialize(out));
private static ResponseContentTypeDefinition<BlobSidecarsAndMetaData> getSszResponseType() {
final OctetStreamResponseContentTypeDefinition.OctetStreamSerializer<BlobSidecarsAndMetaData>
serializer =
(data, out) -> data.getData().forEach(blobSidecar -> blobSidecar.sszSerialize(out));

return new OctetStreamResponseContentTypeDefinition<>(serializer, __ -> Collections.emptyMap());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import tech.pegasys.teku.spec.SpecMilestone;
import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock;
import tech.pegasys.teku.spec.datastructures.metadata.BlobSidecarsAndMetaData;
import tech.pegasys.teku.spec.datastructures.metadata.ObjectAndMetaData;

class GetBlobSidecarsTest extends AbstractMigratedBeaconHandlerWithChainDataProviderTest {
Expand Down Expand Up @@ -61,7 +62,8 @@ void shouldReturnBlobSidecars()
handler.handleRequest(request);

assertThat(request.getResponseCode()).isEqualTo(SC_OK);
assertThat(request.getResponseBody()).isEqualTo(blobSidecars);
assertThat(((BlobSidecarsAndMetaData) request.getResponseBody()).getData())
.isEqualTo(blobSidecars);
}

@Test
Expand All @@ -82,8 +84,9 @@ void metadata_shouldHandle500() throws JsonProcessingException {
@Test
void metadata_shouldHandle200() throws IOException {
final List<BlobSidecar> blobSidecars = dataStructureUtil.randomBlobSidecars(3);

final String data = getResponseStringFromMetadata(handler, SC_OK, blobSidecars);
final BlobSidecarsAndMetaData blobSidecarsAndMetaData =
new BlobSidecarsAndMetaData(blobSidecars, SpecMilestone.DENEB, true, false, false);
final String data = getResponseStringFromMetadata(handler, SC_OK, blobSidecarsAndMetaData);
final String expected =
Resources.toString(
Resources.getResource(GetBlobSidecarsTest.class, "getBlobSidecars.json"), UTF_8);
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import tech.pegasys.teku.spec.datastructures.forkchoice.ProtoNodeData;
import tech.pegasys.teku.spec.datastructures.forkchoice.ReadOnlyForkChoiceStrategy;
import tech.pegasys.teku.spec.datastructures.lightclient.LightClientBootstrap;
import tech.pegasys.teku.spec.datastructures.metadata.BlobSidecarsAndMetaData;
import tech.pegasys.teku.spec.datastructures.metadata.BlockAndMetaData;
import tech.pegasys.teku.spec.datastructures.metadata.ObjectAndMetaData;
import tech.pegasys.teku.spec.datastructures.metadata.StateAndMetaData;
Expand Down Expand Up @@ -100,7 +101,7 @@ public ChainDataProvider(
combinedChainDataClient,
new BlockSelectorFactory(spec, combinedChainDataClient),
new StateSelectorFactory(spec, combinedChainDataClient),
new BlobSidecarSelectorFactory(combinedChainDataClient),
new BlobSidecarSelectorFactory(spec, combinedChainDataClient),
rewardCalculator);
}

Expand Down Expand Up @@ -179,7 +180,7 @@ public SafeFuture<Optional<ObjectAndMetaData<SignedBeaconBlock>>> getBlock(
return fromBlock(blockIdParam, Function.identity());
}

public SafeFuture<Optional<List<BlobSidecar>>> getBlobSidecars(
public SafeFuture<Optional<BlobSidecarsAndMetaData>> getBlobSidecars(
final String blockIdParam, final List<UInt64> indices) {
return blobSidecarSelectorFactory
.createSelectorForBlockId(blockIdParam)
Expand All @@ -188,7 +189,11 @@ public SafeFuture<Optional<List<BlobSidecar>>> getBlobSidecars(

public SafeFuture<Optional<List<BlobSidecar>>> getAllBlobSidecarsAtSlot(
final UInt64 slot, final List<UInt64> indices) {
return blobSidecarSelectorFactory.slotSelectorForAll(slot).getBlobSidecars(indices);
return blobSidecarSelectorFactory
.slotSelectorForAll(slot)
.getBlobSidecars(indices)
.thenApply(
maybeBlobSideCarsMetaData -> maybeBlobSideCarsMetaData.map(ObjectAndMetaData::getData));
}

public SafeFuture<Optional<ObjectAndMetaData<Bytes32>>> getBlockRoot(final String blockIdParam) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
import java.util.Optional;
import tech.pegasys.teku.infrastructure.async.SafeFuture;
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
import tech.pegasys.teku.spec.datastructures.metadata.BlobSidecarsAndMetaData;

public interface BlobSidecarSelector {
SafeFuture<Optional<List<BlobSidecar>>> getBlobSidecars(List<UInt64> indices);
SafeFuture<Optional<BlobSidecarsAndMetaData>> getBlobSidecars(List<UInt64> indices);
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,22 @@
import tech.pegasys.teku.api.AbstractSelectorFactory;
import tech.pegasys.teku.infrastructure.async.SafeFuture;
import tech.pegasys.teku.infrastructure.unsigned.UInt64;
import tech.pegasys.teku.spec.Spec;
import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;
import tech.pegasys.teku.spec.datastructures.blocks.SignedBeaconBlock;
import tech.pegasys.teku.spec.datastructures.blocks.SlotAndBlockRoot;
import tech.pegasys.teku.spec.datastructures.blocks.blockbody.versions.deneb.BeaconBlockBodyDeneb;
import tech.pegasys.teku.spec.datastructures.metadata.BlobSidecarsAndMetaData;
import tech.pegasys.teku.storage.client.ChainHead;
import tech.pegasys.teku.storage.client.CombinedChainDataClient;

public class BlobSidecarSelectorFactory extends AbstractSelectorFactory<BlobSidecarSelector> {

public BlobSidecarSelectorFactory(final CombinedChainDataClient client) {
private final Spec spec;

public BlobSidecarSelectorFactory(final Spec spec, final CombinedChainDataClient client) {
super(client);
this.spec = spec;
}

@Override
Expand All @@ -44,11 +50,23 @@ public BlobSidecarSelector blockRootSelector(final Bytes32 blockRoot) {
if (maybeSlot.isPresent()) {
final SlotAndBlockRoot slotAndBlockRoot =
new SlotAndBlockRoot(maybeSlot.get(), blockRoot);
return getBlobSidecars(slotAndBlockRoot, indices);
return getBlobSidecars(slotAndBlockRoot, indices)
.thenApply(blobSidecars -> addMetaData(blobSidecars, slotAndBlockRoot));
}
return client
.getBlockByBlockRoot(blockRoot)
.thenCompose(maybeBlock -> getBlobSidecarsForBlock(maybeBlock, indices));
.thenCompose(
maybeBlock -> {
if (maybeBlock.isEmpty()) {
return SafeFuture.completedFuture(Optional.empty());
}
final SignedBeaconBlock block = maybeBlock.get();
final SlotAndBlockRoot slotAndBlockRoot =
new SlotAndBlockRoot(block.getSlot(), blockRoot);
return getBlobSidecarsForBlock(maybeBlock, indices)
.thenApply(
blobSidecars -> addMetaData(blobSidecars, slotAndBlockRoot));
});
});
}

Expand All @@ -57,7 +75,13 @@ public BlobSidecarSelector headSelector() {
return indices ->
client
.getChainHead()
.map(head -> getBlobSidecars(head.getSlotAndBlockRoot(), indices))
.map(
head ->
getBlobSidecars(head.getSlotAndBlockRoot(), indices)
.thenApply(
blobSideCars ->
addMetaData(
blobSideCars, head.getSlotAndBlockRoot(), head.isOptimistic())))
.orElse(SafeFuture.completedFuture(Optional.empty()));
}

Expand All @@ -66,27 +90,58 @@ public BlobSidecarSelector genesisSelector() {
return indices ->
client
.getBlockAtSlotExact(GENESIS_SLOT)
.thenCompose(maybeGenesisBlock -> getBlobSidecarsForBlock(maybeGenesisBlock, indices));
.thenCompose(
maybeGenesisBlock ->
getBlobSidecarsForBlock(maybeGenesisBlock, indices)
.thenApply(
blobSidecars ->
addMetaData(
blobSidecars,
GENESIS_SLOT,
false,
true,
client.isFinalized(GENESIS_SLOT))));
}

@Override
public BlobSidecarSelector finalizedSelector() {
return indices ->
client
.getLatestFinalized()
.map(anchorPoint -> getBlobSidecars(anchorPoint.getSlotAndBlockRoot(), indices))
.map(
anchorPoint ->
getBlobSidecars(anchorPoint.getSlotAndBlockRoot(), indices)
.thenApply(
blobSideCars ->
addMetaData(
blobSideCars,
anchorPoint.getSlotAndBlockRoot(),
client.isChainHeadOptimistic())))
.orElse(SafeFuture.completedFuture(Optional.empty()));
}

@Override
public BlobSidecarSelector slotSelector(final UInt64 slot) {
return indices -> {
if (client.isFinalized(slot)) {
return getBlobSidecars(slot, indices);
return getBlobSidecars(slot, indices)
.thenApply(
blobSidecars ->
addMetaData(blobSidecars, slot, client.isChainHeadOptimistic(), true, true));
}
return client
.getBlockAtSlotExact(slot)
.thenCompose(maybeBlock -> getBlobSidecarsForBlock(maybeBlock, indices));
.thenCompose(
maybeBlock ->
getBlobSidecarsForBlock(maybeBlock, indices)
.thenApply(
blobSidecars ->
addMetaData(
blobSidecars,
slot,
client.isChainHeadOptimistic(),
false,
client.isFinalized(slot))));
};
}

Expand All @@ -96,7 +151,12 @@ public BlobSidecarSelector slotSelectorForAll(final UInt64 slot) {
.getAllBlobSidecars(slot, indices)
.thenApply(
blobSidecars ->
blobSidecars.isEmpty() ? Optional.empty() : Optional.of(blobSidecars));
blobSidecars.isEmpty()
? Optional.empty()
: addMetaData(
// We don't care about metadata since the api (teku only) that
// consumes the return value doesn't use it
Optional.of(blobSidecars), new SlotAndBlockRoot(slot, Bytes32.ZERO)));
}

private SafeFuture<Optional<List<BlobSidecar>>> getBlobSidecarsForBlock(
Expand Down Expand Up @@ -125,4 +185,60 @@ private SafeFuture<Optional<List<BlobSidecar>>> getBlobSidecars(
final UInt64 slot, final List<UInt64> indices) {
return client.getBlobSidecars(slot, indices).thenApply(Optional::of);
}

private Optional<BlobSidecarsAndMetaData> addMetaData(
final Optional<List<BlobSidecar>> maybeBlobSidecarList,
final SlotAndBlockRoot slotAndBlockRoot) {
if (maybeBlobSidecarList.isEmpty()) {
return Optional.empty();
}

final UInt64 slot = slotAndBlockRoot.getSlot();
final Bytes32 blockRoot = slotAndBlockRoot.getBlockRoot();
final Optional<ChainHead> maybeChainHead = client.getChainHead();
final boolean isFinalized = client.isFinalized(slot);
boolean isOptimistic;
boolean isCanonical = false;

if (maybeChainHead.isPresent()) {
ChainHead chainHead = maybeChainHead.get();
isOptimistic = chainHead.isOptimistic() || client.isOptimisticBlock(blockRoot);
isCanonical = client.isCanonicalBlock(slot, blockRoot, chainHead.getRoot());
} else {
// If there's no chain head, we assume the block is not optimistic and not canonical
isOptimistic = client.isOptimisticBlock(blockRoot);
}
return addMetaData(maybeBlobSidecarList, slot, isOptimistic, isCanonical, isFinalized);
}

private Optional<BlobSidecarsAndMetaData> addMetaData(
final Optional<List<BlobSidecar>> maybeBlobSidecarList,
final SlotAndBlockRoot slotAndBlockRoot,
final boolean isOptimistic) {
if (maybeBlobSidecarList.isEmpty()) {
return Optional.empty();
}
return addMetaData(
maybeBlobSidecarList,
slotAndBlockRoot.getSlot(),
isOptimistic,
true,
client.isFinalized(slotAndBlockRoot.getSlot()));
}

private Optional<BlobSidecarsAndMetaData> addMetaData(
final Optional<List<BlobSidecar>> maybeBlobSidecarList,
final UInt64 blockSlot,
final boolean executionOptimistic,
final boolean canonical,
final boolean finalized) {
return maybeBlobSidecarList.map(
blobSidecarList ->
new BlobSidecarsAndMetaData(
blobSidecarList,
spec.atSlot(blockSlot).getMilestone(),
executionOptimistic,
canonical,
finalized));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ protected ChainDataProvider setupBySpec(
this.blockSelectorFactory = spy(new BlockSelectorFactory(spec, mockCombinedChainDataClient));
this.stateSelectorFactory = spy(new StateSelectorFactory(spec, mockCombinedChainDataClient));
this.blobSidecarSelectorFactory =
spy(new BlobSidecarSelectorFactory(mockCombinedChainDataClient));
spy(new BlobSidecarSelectorFactory(spec, mockCombinedChainDataClient));
final ChainDataProvider provider =
new ChainDataProvider(
spec,
Expand Down
Loading