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 HistoricalRootsBlockProof for merge till capella blocks #287

Merged
merged 5 commits into from
Oct 25, 2024

Conversation

kdeme
Copy link
Collaborator

@kdeme kdeme commented Apr 17, 2024

The way for proving these blocks was already presented/discussed a while back and in the mean while we also have test vectors added thanks to @ogenev (ethereum/portal-spec-tests#9), so I suggest that we start thinking about a path for activating this in the network.

Aside from potentially adjusting the description in this PR and having it implemented in the clients, we will also need changes to the bridges to build these proofs and, more importantly, discuss on how we will get rid of the old headers without the proof that might already live on the network. Hence, keeping this in draft for now.

@kdeme kdeme marked this pull request as ready for review April 17, 2024 16:05
@kdeme kdeme marked this pull request as draft April 17, 2024 16:06
Comment on lines 168 to 173
BeaconBlockBodyProof = Vector[Bytes32, 8] # Proof that EL block_hash in ExecutionPayload is part of BeaconBlockBody
BeaconBlockHeaderProof = Vector[Bytes32, 3] # Proof that BeaconBlockBody root is part of BeaconBlockHeader
HistoricalRootsProof = Vector[Bytes32, 14] # Proof that BeaconBlockHeader root is part of HistoricalRoots

HistoricalRootsBlockProof = Container[
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not fully sure about the naming here yet, feedback requested :-). I went with the names I used also in the initial presentation.

Basically each proof is named for which structure "the leaf" is proven to be part of. But the inverse would work also (e.g. BlockHashProof instead of BeaconBlockBodyProof).

With this current approach, we can also have a naming clash in the future when we do the same for historical_summaries, as there we would have then the HistoricalSummariesProof, which also exists in our Portal beacon chain network (https://github.com/ethereum/portal-network-specs/blob/master/beacon-chain/beacon-network.md#historicalsummaries), but it is something different there (proof that historical summaries is part of beacon state).

history-network.md Outdated Show resolved Hide resolved
# Proof for EL BlockHeader after TheMerge until Capella
HistoricalRootsBlockProof = Container[
beaconBlockProof: BeaconBlockProof,
beaconBlocRoot: Bytes32,
Copy link
Member

Choose a reason for hiding this comment

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

Is the the beacon block that the proof is anchored to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I mentioned here in this link, #287 (comment), would it be more clear to go with object naming where the name of the proof is about what it proves (instead of "how")?

BlockProofHistoricalRoots = Container[
    beaconBlockProof: BeaconBlockProofHistoricalRoots, # Proof that the BeaconBlock is part of the historical_roots and thus part of the canonical chain
    beaconBlockRoot: Bytes32,
    executionBlockProof: ExecutionBlockProof, # Proof that EL BlockHash is part of the BeaconBlock
    slot: Slot
]

The reason I still attach the HistoricalRoots part at the end, is because there is also the HistoricalSummaries version. This would then be:

BlockProofHistoricalSummaries = Container[
    beaconBlockProof: BeaconBlockProofHistoricalSummaries, # Proof that the BeaconBlock is part of the historical_summaries and thus part of the canonical chain
    beaconBlockRoot: Bytes32,
    executionBlockProof: ExecutionBlockProof, # Proof that EL BlockHash is part of the BeaconBlock
    slot: Slot
]

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

I suspect my knowledge of the proving data structures here may not be adequate to provide fully informed feedback.

IIUC this gives us a proof format for blocks from the merge to Capella, during which the beacon chain was using HistoricalRoots, after which the beacon chain migrates to HistoricalSummaries.

I have an open question about what the HistoricalRootsBlockProof.beaconBlockRoot field represents. My assumption (which may be wrong) is that the field is the beacon block for which the proof is anchored.

Assuming I'm correct about these things, I have the following suggestion.

This fork happened a long time ago, and thus all of the proofs and data are generally frozen in history. What if we anchored all of these proofs to a fixed root that could be baked into every client, so that all blocks from this range would be able to be proven against a fixed root hash. That root hash would probably be either the last block before the fork or the first block after the fork, and then all of the proofs of this type could be proven against that singular block root without need to source data from another network.

@kdeme
Copy link
Collaborator Author

kdeme commented Sep 11, 2024

IIUC this gives us a proof format for blocks from the merge to Capella, during which the beacon chain was using HistoricalRoots, after which the beacon chain migrates to HistoricalSummaries.

That's right.

I have an open question about what the HistoricalRootsBlockProof.beaconBlockRoot field represents. My assumption (which may be wrong) is that the field is the beacon block for which the proof is anchored.

This is the root of the BeaconBlock of which the EL block hash is part of (BeaconBlock -> BeaconBlockBody -> ExecutionPayload).
The BeaconBlockProof is then the SSZ merkle proof that this EL block hash is part of the BeaconBlock with that root. So the verify part requires this proof + the BeaconBlock root as root and the EL block hash as leaf.

The other part of the verification is to make sure that the BeaconBlock used is canonical. This uses the historical_roots and works pretty much the same as our accumulator pre-merge. So the main difference here is that we get to proof if a BeaconBlock is canonical, and not an EL block. But as an EL block basically part of the BeaconBlock, we can used the above explained extra step to get also the EL block (hash) proven.

The historical_roots can/will be baked in the binary as it is frozen.

What if we anchored all of these proofs to a fixed root that could be baked into every client, so that all blocks from this range would be able to be proven against a fixed root hash. That root hash would probably be either the last block before the fork or the first block after the fork, and then all of the proofs of this type could be proven against that singular block root without need to source data from another network.

I don't think I fully understand this part. Is it still applicable with above explanation?

history/history-network.md Outdated Show resolved Hide resolved
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

All of my questions are generally answered and I also apologize for asking confusing questions. I think the biggest thing that this PR needs (which you already acquiesced to) is a more detailed explanation of the properties and structure of the proof. (and maybe a diagram).

Lets get this merged

@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-roots-block-proofs branch from 135b32e to fbb2a43 Compare September 17, 2024 15:30
@kdeme kdeme force-pushed the historical-roots-block-proofs branch from 135f222 to af07120 Compare September 17, 2024 15:39
@kdeme
Copy link
Collaborator Author

kdeme commented Sep 17, 2024

I have rebased the PR and updated it with renaming of the proofs + extra text clarification + some charts in 6d7a522, fbb2a43 and af07120.

@kdeme kdeme marked this pull request as ready for review September 17, 2024 16:49
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

The additional explanations and the charts make a big difference for me. Thank you.

@kdeme kdeme force-pushed the historical-roots-block-proofs branch from ce3dfe6 to 4aef56b Compare September 18, 2024 07:52
@kdeme kdeme requested review from acolytec3 and ogenev September 18, 2024 07:54
@kdeme
Copy link
Collaborator Author

kdeme commented Oct 3, 2024

This PR is blocked by a solution/action taken for removal on the None in the Union of the block proofs and also #337

@pipermerriam
Copy link
Member

Looks like we should probably:

  1. Leave the None in for now.
  2. Move to have support for the new content types at the position in the Union where they are in this PR.
  3. Wait until after Devcon and remove the none as part of Add ephemeral block headers to the history network spec #341

Clients can still accept headers without proofs. We'll end up with a network with some awkward mixed databases where some have proven headers and some have unproven headers for certain blocks. We'll clean it up later with #341 getting merged.

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

@kdeme
Copy link
Collaborator Author

kdeme commented Oct 21, 2024

I'm fine with merging this as is now as long as we later do still have a look at either:
a. Removing the None and thus shifting al the selector
b. or Removing the Union all together.

@KolbyML
Copy link
Member

KolbyML commented Oct 21, 2024

I'm fine with merging this as is now as long as we later do still have a look at either: a. Removing the None and thus shifting al the selector b. or Removing the Union all together.

Kim and I discussed this a bit offline. I am kind of a fan of b but we will have a wider discussion with the teams on this issue

@morph-dev
Copy link
Contributor

In general, it looks good to me.

Since there was some discussion about naming, I have an idea but I'm not sure if it was considered before.
How about naming top level proofs something like: BlockPreMergeProof and BlockPreCapellaProof? It makes it clear when each proof type should be used.

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.

6 participants