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

Add HistoricalSummariesBlockProof for headers from Capella onwards #291

Merged

Conversation

kdeme
Copy link
Collaborator

@kdeme kdeme commented Apr 19, 2024

PR on top of #287

In draft as this would require a stable Portal beacon network deployed to have access to the historical_summaries.

@kdeme kdeme force-pushed the historical-roots-block-proofs branch from 41225d6 to b0564a6 Compare April 22, 2024 09:19
@kdeme kdeme force-pushed the historical-summaries-block-proofs branch from 4d12b99 to a250fee Compare April 22, 2024 09:24
@@ -179,8 +181,21 @@ HistoricalRootsBlockProof = Container[
slot: Slot
]

BlockHeaderProof = Union[None, AccumulatorProof, HistoricalRootsBlockProof]
# BeaconState historical_summaries proof (for Capella onwards)
HistoricalSummariesProof = Vector[Bytes32, 13] # Proof that BeaconBlockHeader root is part of HistoricalSummaries
Copy link
Member

Choose a reason for hiding this comment

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

There is possible collusion with the proof for the HistoricalSummaries beacon content type in the beacon network here.

In Trin, I'm using HistoricalSummariesStateProof for the proof of the beacon content.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I know, I mentioned that name clash here: #287 (comment)

I might reverse the naming because of that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've altered the naming which should avoid the name clash and hopefully also be more clear.

BlockHeaderProof = Union[None, HistoricalHashesAccumulatorProof, HistoricalRootsBlockProof]
# Proof that BeaconBlock root is part of HistoricalRoots and thus canonical
# For Capella and onwards
HistoricalSummariesProof = Vector[Bytes32, 13]
Copy link
Contributor

Choose a reason for hiding this comment

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

What would you think about renaming this to something like HistoricalSummariesRootsProof or something similar so it's just 100% clear what it is? In parallel, I wonder if we could rename this beacon chain object to HistoricalSummariesWithStateProof to be 100% clear this is the proof that the summaries are rooted in the current beacon state.

Copy link
Member

Choose a reason for hiding this comment

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

In Trin we use HistoricalSummariesBlockProof and HistoricalSummariesStateProof. Happy to switch to whatever we decide.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the HistoricalSummariesBlockProof is the container below that holds both the BeaconBlockProof (i.e. the proof that an EL blockhash is part of a beacon block and then what is currently called thethe HistoricalSummariesProof which anchors the Beacon Block root in the HistoricalSummaries. As such, was thinking maybe we can call this second one "HistoricalSummariesRootProof" to reflect that we're anchoring the beacon block root in the particular block_summary_root for the period in which it falls.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've altered the naming which should avoid the name clash and hopefully also be more clear.


# Proof for EL BlockHeader for Capella and onwards
HistoricalSummariesBlockProof = Container[
beaconBlockProof: BeaconBlockProof,
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned on Discord, this type isn't valid for post Capella Beacon Blocks since the proof length is now 12 elements long. I'm assuming we should add a new postCapellaBeaconBlockProof type that's a vector of 12 elements

Copy link
Contributor

Choose a reason for hiding this comment

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

Another (preferred) idea is to change the type of BeaconBlockProof to a List[Bytes32, 12]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've made the change to List. I'm thinking however about also changing it to 1 same type for also the version that use historical_roots.

history/history-network.md Outdated Show resolved Hide resolved
@kdeme kdeme force-pushed the historical-roots-block-proofs branch from d4d2cd9 to da43ef0 Compare September 17, 2024 11:02
@kdeme kdeme force-pushed the historical-summaries-block-proofs branch from c73d137 to 79fa69d Compare September 18, 2024 09:05
@kdeme kdeme requested review from acolytec3 and ogenev September 20, 2024 14:00
@kdeme kdeme marked this pull request as ready for review October 21, 2024 18:53
Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

I would like to make it more clear the Portal Beacon Network isn't required and is one of many options to avoid additional confusion with EL teams for example.

It is easier to prevent confusion then to resolve it later on sorta like preventative care

This chain of two proofs allows for verifying that an EL `BlockHeader` is part of the canonical chain. The only requirement is having access to the beacon chain `historical_summaries`.
The `historical_summaries` is a [`BeaconState` field](https://github.com/ethereum/consensus-specs/blob/dev/specs/capella/beacon-chain.md#beaconstate) that was introduced since the Capella fork. It gets updated every period (8192 slots). The `BlockProofHistoricalSummaries` MUST be used to verify blocks from the Capella fork onwards.

The Portal beacon network [provides access](./beacon-chain/beacon-network.md#historicalsummaries) to an up to date `historical_summaries` object.
Copy link
Member

Choose a reason for hiding this comment

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

This can lead to confusion that the Portal Beacon Network is the only option to get these.

I think we should either

  • not mention the Portal Beacon Network
  • or mention multiple sources implementations could get this data, for example an EL client wouldn't run the Portal Beacon Network as it already has access to this data from their CL client.

The reason I mention this as it is a question I had to answer multiple times to EL layer teams and is consistently a cause of confusion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps I can alter it to:

"The historical_summaries can be taken from the CL BeaconState (Link to beacon API endpoint) or the Portal beacon network also provides access to an up to date historical_summaries object."

And hopefully in a secondary PR I can add a specific API endpoint for it :)

Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a good solution to me because then it is clear you aren't forced to use Portal beacon but it is 1 of 2 options, which should prevent confusion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

adjusted in ec0200c

@kdeme
Copy link
Collaborator Author

kdeme commented Oct 21, 2024

Again, I'd be fine with moving forward and merging this (after reviews), but with the same remark as here: #287 (comment)

And it also should be clear that we cannot really do the full validation yet for these type of headers as long as we do not have access to historical_summaries.

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit: looks good

@KolbyML KolbyML merged commit f5aed01 into historical-roots-block-proofs Oct 25, 2024
3 checks passed
KolbyML pushed a commit that referenced this pull request Oct 25, 2024
)

* Add HistoricalSummariesBlockProof for headers from Capella onwards

* Add more explanation + charts

* Adjust options for getting historical_summaries based on feedback
KolbyML pushed a commit that referenced this pull request Oct 25, 2024
)

* Add HistoricalSummariesBlockProof for headers from Capella onwards

* Add more explanation + charts

* Adjust options for getting historical_summaries based on feedback
@KolbyML KolbyML deleted the historical-summaries-block-proofs branch October 25, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants