-
Notifications
You must be signed in to change notification settings - Fork 85
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
Add HistoricalSummariesBlockProof for headers from Capella onwards #291
Conversation
41225d6
to
b0564a6
Compare
4d12b99
to
a250fee
Compare
history-network.md
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
75e9a54
to
d4d2cd9
Compare
bc3266a
to
c73d137
Compare
history/history-network.md
Outdated
BlockHeaderProof = Union[None, HistoricalHashesAccumulatorProof, HistoricalRootsBlockProof] | ||
# Proof that BeaconBlock root is part of HistoricalRoots and thus canonical | ||
# For Capella and onwards | ||
HistoricalSummariesProof = Vector[Bytes32, 13] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
history/history-network.md
Outdated
|
||
# Proof for EL BlockHeader for Capella and onwards | ||
HistoricalSummariesBlockProof = Container[ | ||
beaconBlockProof: BeaconBlockProof, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
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.
d4d2cd9
to
da43ef0
Compare
c73d137
to
79fa69d
Compare
There was a problem hiding this 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
history/history-network.md
Outdated
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adjusted in ec0200c
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
PR on top of #287
In draft as this would require a stable Portal beacon network deployed to have access to the
historical_summaries
.