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

feat: add debug_getHistoricalSummaries endpoint #7245

Draft
wants to merge 2 commits into
base: unstable
Choose a base branch
from

Conversation

acolytec3
Copy link

Motivation

The Portal Network needs a way to access the HistoricalSummaries property on the BeaconState for post Capella beacon state since the HistoricalSummaries are used to anchor EL block header proofs to the beacon state roots in light client updates that the serve as the source of truth for verifying the authenticity of post capella EL block headers. This currently requires downloading the entire Beacon State from a beacon node willing to serve the debug endpoints and then throwing away everything except for the desired data (or else downloading an ERA file from a willing endpoint). It is desirable to be able to access this data more efficiently directly from beacon nodes.

Description

This adds a new getHistoricalSummaries endpoint to the debug namespace on the REST API that serves the HistoricalSummaries field from the current head BeaconState as SSZ bytes. We only need the most current one for use in constructing proofs of recent EL block headers embedded in a beacon block.

This mirrors similar experimental work being done in the nimbus client

Steps to test or reproduce

git checkout historicalSummaries-endpoint
./lodestar beacon \
--rest.namespace='*' \
--checkpointSyncUrl=https://beaconstate.info \

In a separate terminal

curl -X GET --header "Accept: application/octet-stream" "http://127.0.0.1:9596/eth/v1/debug/historical_summaries" > ./hs.ssz

Then, use a script like below to verify that historical summaries are correctly returned

    import { ssz } from '@lodestar/types'

    const summariesBytes = fs.readFileSync('./test/util/hs.ssz')
    const summaries = ssz.capella.BeaconState.fields.historicalSummaries.deserialize(summariesBytes)
    console.log(summaries)

@acolytec3 acolytec3 requested a review from a team as a code owner November 26, 2024 20:50
@CLAassistant
Copy link

CLAassistant commented Nov 26, 2024

CLA assistant check
All committers have signed the CLA.

@acolytec3 acolytec3 marked this pull request as draft November 26, 2024 20:50
@@ -196,5 +203,15 @@ export function getDefinitions(_config: ChainForkConfig): RouteDefinitions<Endpo
timeoutMs: 5 * 60 * 1000,
},
},
getHistoricalSummaries: {
url: "/eth/v1/debug/historical_summaries",
Copy link
Member

Choose a reason for hiding this comment

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

If this endpoint is useful for Portal it makes sense to standardize it on the beacon-api or at least get rough consensus there

if we wanna add a non-standardized api we should rather go with v0 or add to lodestar namespace instead of debug

Copy link
Author

Choose a reason for hiding this comment

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

Fair point and this is early days still. I looked at the Nimbus PR again and it looks like they moved it under a client specific namespace so maybe I should mirror this on the lodestar side for now?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it would be better to move the api there but I am also fine with v0, although it's not clear if this should be in debug or beacon namespace

Copy link
Author

Choose a reason for hiding this comment

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

Understood. I've switched to v0 for the moment. I chatted @deme who's working on the Nimbus equivalent and he's hoping to get a PR for the consensus specs to further general conversation there sometime soon.

resp: {
data: ssz.capella.HistoricalSummaries,
meta: EmptyMetaCodec,
onlySupport: WireFormat.ssz,
Copy link
Member

Choose a reason for hiding this comment

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

any reason why this shouldn't also support JSON?

Copy link
Author

Choose a reason for hiding this comment

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

No specific reason other than it's most useful for portal clients to download the ssz encoded variant as it requires less work to integrate it into our own work. Will add a JSON option as well.

Copy link
Member

@nflaig nflaig Nov 27, 2024

Choose a reason for hiding this comment

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

Generally we support both return formats and the client decides based on Accept header, if header is not present we default to JSON

@g11tech g11tech changed the title Add debug_getHistoricalSummaries endpoint feat: Add debug_getHistoricalSummaries endpoint Nov 27, 2024
@nflaig nflaig changed the title feat: Add debug_getHistoricalSummaries endpoint feat: add debug_getHistoricalSummaries endpoint Nov 27, 2024
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm, we can always move the endpoint once we have it in beacon apis, CI running

@@ -85,5 +86,21 @@ export function getDebugApi({
},
};
},
async getHistoricalSummaries(_, context) {
const {state} = await getStateResponseWithRegen(chain, "head");
Copy link
Member

@nflaig nflaig Nov 27, 2024

Choose a reason for hiding this comment

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

imo hard coding this to head is not great, we should probably align this with other apis which take a state_id parameter

Copy link
Contributor

Choose a reason for hiding this comment

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

correct, stateid in the route and corresponding change here makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants