feat: enable post-capella HistoricalSummary Header validation #1774

KolbyML · 2025-04-22T22:18:26Z

What was wrong?

HistoricalSummary HeaderWithProof validation wasn't enabled

How was it fixed?

Enabled it, the E2HS bridge has no way to verify post-capella HeaderWithProofs as it doesn't have access to a provider and we don't want to give it access to one. So the assumption is the the E2HS bridge is downloading the files from a trusted source.

For this to work, it requires the BeaconNetwork or validation will fail

morph-dev

Few comments. Would like @ogenev to take a look as well.

morph-dev · 2025-04-23T11:37:57Z

crates/validation/src/header_validator.rs

+    /// The historical summaries are provided by the header oracle.
+    HeaderOracle(Arc<RwLock<HeaderOracle>>),
+    /// The historical summaries are provided by passed historical summaries.
+    HistoricalSummaries(HistoricalSummaries),


How exactly is this used? Only in tests or?
Can/should we make this a collection?
Either HashMap<u64, HistoricalSummaries> or Vec<(u64, HistoricalSummaries)> (the u64 would be epoch).

let historical_summaries = HistoricalSummaries::from_ssz_bytes(&historical_summaries_bytes) .expect("cannot decode HistoricalSummaries bytes");

I don't think we should/need to make this a collect, as HistoricalSummaries are indexed by 0, they are also only like 40k bytes, super tiny in terms of test data we will add. So I don't think we need to hyper optimize this to reduce our test data sizes, as this is a drop in the bucket compared to our real offenders. We also only need to store 1 HistoricalSummaries for all of our HistoricalSummaries tests, we would just update it over time

I think I'm not familiar with how exactly historical summaries work, so correctly me if I'm wrong.

The get_historical_summary function accepts epoch for a reason. So whatever historical summaries we use, we should know that they are valid only up to some slot.

After reading a bit about it, I agree that we probably don't need collection. But I would still include epoch.
I would personally prefer: HistoricalSummaries { epoch: u64, historical_summaries: HistoricalSummaries } but I'm not against simple HistoricalSummaries(u64, HistoricalSummaries(.

This way, the get_historical_summary can return descriptive error, which should be much better than what would happen otherwise (I guess error while checking merkle proof).

I don't understand why we would need to return the epoch. get_historical_summary()) returns the HistoricalSummaries for the given epoch or a more up to date HistoricalSummaries. HistoricalSummaries are already proven, get_historical_summary() will error if it can't get the HistoricalSummaries of at least the given epoch.

Also I am a little confused what you are referring to now.

what HistoricalSummariesProvider::get_historical_summary() returns? I don't think it makes sense to return it here as if we got the epoch we know it contains the expected data for that epoch

what HeaderOracle::get_historical_summary() returns? For our usecase it doesn't matter what epoch of HistoricalSummaries the database returns, as it either returns the requested version or a more up to date version

morph-dev · 2025-04-23T11:44:45Z

crates/validation/src/header_validator.rs

+        header_validator
+            .validate_header_with_proof(
+                &hwp,
+                HistoricalSummariesProvider::HeaderOracle(Arc::new(RwLock::new(


nit: If I'm not mistaken, since this is pre-merge, this can be HistoricalSummariesProvider::None

morph-dev · 2025-04-23T11:45:05Z

crates/validation/src/header_validator.rs

-            .validate_header_with_proof(&header_with_proof)
+            .validate_header_with_proof(
+                &header_with_proof,
+                HistoricalSummariesProvider::HeaderOracle(Arc::new(RwLock::new(


morph-dev · 2025-04-23T11:45:53Z

crates/validation/src/header_validator.rs

-            .validate_header_with_proof(&hwp)
+            .validate_header_with_proof(
+                &hwp,
+                HistoricalSummariesProvider::HeaderOracle(Arc::new(RwLock::new(


morph-dev · 2025-04-23T11:45:59Z

crates/validation/src/header_validator.rs

-            .validate_header_with_proof(&future_hwp)
+            .validate_header_with_proof(
+                &future_hwp,
+                HistoricalSummariesProvider::HeaderOracle(Arc::new(RwLock::new(


morph-dev · 2025-04-23T11:47:46Z

crates/validation/src/header_validator.rs

@@ -153,15 +201,21 @@ impl HeaderValidator {
        let gen_index = EPOCH_SIZE + block_root_index;
        let historical_summary_index =


Are you sure that this index works both pre and post Dencun? Do we have tests that cover all forks?

Are you sure that this index works both pre and post Dencun?

The index calculation starts at the epoch of the CAPELLA_FORK_EPOCH, so you are basically just calculating 8192 slot increments from the fork. The intervals of HistoricalSummaries remain constant from this point.

Do we have tests that cover all forks?

We don't have proof generation yet, I am assuming that is something you would need to add over time

Sorry, I marked wrong lines.

I think that calling Self::verify_beacon_block_proof few lines above wouldn't work correctly. It even says that it's for pre-capella. This is primary reason why I want @ogenev to take a look at this, as I believe he originally implemented it.

And I'm not comfortable with us shipping code that doesn't have a single test for all forks. I would rather wait with this PR until we have those tests vectors and we can do it properly.

I think that calling Self::verify_beacon_block_proof few lines above wouldn't work correctly. It even says that it's for pre-capella. This is primary reason why I want @ogenev to take a look at this, as I believe he originally implemented it.

https://github.com/ethereum/trin/pull/1748/files#diff-06484484ad3bff928c828d2fc2013746b289cb23a51c53c42bab5a13966f87a9R105-R123

The comment is just out of date, the beacon_block_body_root_proof doesn't change between forks as since in the PR lines linked above, the beacon_block_body_root_proof proof is the same format for bellatrix, capella, deneb

I am looking forward to @ogenev review as well.

And I'm not comfortable with us shipping code that doesn't have a single test for all forks.

We already have tests for Capella. ^ I think it is unreasonable to expect tests for all forks currently and this request is a little unreasonable.

I would rather wait with this PR until we have those tests vectors and we can do it properly.

I don't think it would make a difference, as if there was a mistake Validation would fail and that would be our worst case, the best case is things just work. We have been waiting for a lot of this code for quite some time. We can always fix things in the future, so I think this fear is unwarranted in these specific circumstances.

Currently we have nothing, so there is only one place to go and that is up. The world won't end, we will have tests for all forks before a user even touches Trin.

Me and Milos talked in dm's. He didn't see we already have tests for capella, I will double check the tests, and see if I can use the test data from ethereum/portal-spec-tests#45 to test deneb, Milos didn't care if he had tests for pectra yet.

I will do this and ping when I have some time

I think that calling Self::verify_beacon_block_proof few lines above wouldn't work correctly. It even says that it's for pre-capella. This is primary reason why I want @ogenev to take a look at this, as I believe he originally implemented it.

This should work for post-capella (including pectra) because the indexes don't change, it verifies that the execution block header is included in the beacon block.

However, we need to add some tests before pushing to prod.

morph-dev · 2025-04-23T11:50:18Z

crates/validation/src/header_validator.rs

+    pub async fn validate_header_with_proof(
+        &self,
+        header_with_proof: &HeaderWithProof,
+        historical_summaries_provider: HistoricalSummariesProvider,


Any reason we need to pass HistoricalSummariesProvider here, instead of in constructor?

If I'm not mistaken, real usage will use Oracle, tests would use either constant or none, and bridge would use none. But none of these cases require different value between calls.

KolbyML · 2025-04-23T16:54:38Z

@morph-dev ready for another look. @ogenev do you think you will be able to review this PR before you go on leave?

morph-dev · 2025-04-23T19:09:52Z

crates/validation/src/header_validator.rs

+    HistoricalSummaries(HistoricalSummaries),
+    /// We don't have historical summaries, for example the E2HS bridge doesn't use a provider and
+    /// hence can't get HistoricalSummaries. HistoricalSummary proof validation will fail.
+    #[default]


I don't think this should be default. Probably this type shouldn't implement Default.

The same applies for HeaderValidator. Few places that currently use default HeaderValidator should explicitly create one with HistoricalSummariesProvider::None.

If you want "easy" way to create HeaderValidator without HistoricalSummariesProvider, then you can have constructor that would be documented: HeaderValidator::new_without_historical_summaries().

morph-dev · 2025-04-23T19:13:32Z

bin/e2hs-writer/src/reader.rs

+            true => Some(Arc::new(
+                lookup_epoch_acc(
+                    epoch_index,
+                    &HeaderValidator::default().pre_merge_acc,


Why don't we just use PreMergeAccumulator::default()?

HeaderValidator in the future would need to support multiple networks, PreMergeAccumulator currently only supports mainnet, but I don't see why we wouldn't add sepolia in the future

I will make the recommended change for the time being

morph-dev · 2025-04-23T19:44:10Z

crates/validation/src/header_validator.rs

+    /// The historical summaries are provided by the header oracle.
+    HeaderOracle(Arc<RwLock<HeaderOracle>>),
+    /// The historical summaries are provided by passed historical summaries.
+    HistoricalSummaries(HistoricalSummaries),


I think I'm not familiar with how exactly historical summaries work, so correctly me if I'm wrong.

The get_historical_summary function accepts epoch for a reason. So whatever historical summaries we use, we should know that they are valid only up to some slot.

After reading a bit about it, I agree that we probably don't need collection. But I would still include epoch.
I would personally prefer: HistoricalSummaries { epoch: u64, historical_summaries: HistoricalSummaries } but I'm not against simple HistoricalSummaries(u64, HistoricalSummaries(.

This way, the get_historical_summary can return descriptive error, which should be much better than what would happen otherwise (I guess error while checking merkle proof).

morph-dev · 2025-04-23T19:49:39Z

crates/validation/src/header_validator.rs

@@ -153,15 +201,21 @@ impl HeaderValidator {
        let gen_index = EPOCH_SIZE + block_root_index;
        let historical_summary_index =


Sorry, I marked wrong lines.

I think that calling Self::verify_beacon_block_proof few lines above wouldn't work correctly. It even says that it's for pre-capella. This is primary reason why I want @ogenev to take a look at this, as I believe he originally implemented it.

And I'm not comfortable with us shipping code that doesn't have a single test for all forks. I would rather wait with this PR until we have those tests vectors and we can do it properly.

ogenev

Did an initial review, other than some naming comments I don't see any major issues.

However, we should add deneb tests before merging to make sure that everything works as expected (those validation has not been tested on Deneb but in theory should work).

ogenev · 2025-04-24T09:35:20Z

crates/validation/src/header_validator.rs

@@ -128,13 +183,11 @@ impl HeaderValidator {
    }

    /// A method to verify the chain of proofs for post-Capella execution headers.
-    #[allow(dead_code)] // TODO: Remove this when used
-    fn verify_post_capella_header(
+    async fn verify_historical_summaries(


I'm confused by this name, the goal here is to verify the post-capella execution header against historical summaries, not to verify the historical summaries?

ogenev · 2025-04-24T09:44:21Z

crates/validation/src/header_validator.rs

            }
        }
    }

    /// A method to verify the chain of proofs for post-merge/pre-Capella execution headers.
-    fn verify_post_merge_pre_capella_header(
+    fn verify_historical_roots(


This name is confusing, we don't verify historical roots, we verify the header against historical roots.

ogenev · 2025-04-24T09:56:28Z

crates/validation/src/header_validator.rs

@@ -153,15 +201,21 @@ impl HeaderValidator {
        let gen_index = EPOCH_SIZE + block_root_index;
        let historical_summary_index =


I think that calling Self::verify_beacon_block_proof few lines above wouldn't work correctly. It even says that it's for pre-capella. This is primary reason why I want @ogenev to take a look at this, as I believe he originally implemented it.

This should work for post-capella (including pectra) because the indexes don't change, it verifies that the execution block header is included in the beacon block.

However, we need to add some tests before pushing to prod.

ogenev · 2025-04-24T10:31:17Z

@morph-dev ready for another look. @ogenev do you think you will be able to review this PR before you go on leave?

Yeah, even if I go to leave sooner, I will find time to review it.

KolbyML requested review from ogenev and morph-dev April 22, 2025 22:18

KolbyML self-assigned this Apr 22, 2025

KolbyML force-pushed the capella-proofs branch 3 times, most recently from 4dfa0bb to 4f2dd61 Compare April 23, 2025 01:26

feat: enable post-capella HistoricalSummary Header validation

4f2dd61

morph-dev reviewed Apr 23, 2025

View reviewed changes

fix: address PR concerns

371a083

KolbyML requested a review from morph-dev April 23, 2025 16:54

morph-dev reviewed Apr 23, 2025

View reviewed changes

fix: address PR concerns

7bf5d49

KolbyML force-pushed the capella-proofs branch from b690876 to 7bf5d49 Compare April 23, 2025 20:27

ogenev reviewed Apr 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable post-capella HistoricalSummary Header validation #1774

feat: enable post-capella HistoricalSummary Header validation #1774

KolbyML commented Apr 22, 2025

morph-dev left a comment

morph-dev Apr 23, 2025

KolbyML Apr 23, 2025 •

edited

Loading

morph-dev Apr 23, 2025

KolbyML Apr 23, 2025

morph-dev Apr 23, 2025

morph-dev Apr 23, 2025

morph-dev Apr 23, 2025

morph-dev Apr 23, 2025

morph-dev Apr 23, 2025

KolbyML Apr 23, 2025

morph-dev Apr 23, 2025

KolbyML Apr 23, 2025 •

edited

Loading

KolbyML Apr 23, 2025 •

edited

Loading

KolbyML Apr 23, 2025

ogenev Apr 24, 2025

morph-dev Apr 23, 2025

KolbyML commented Apr 23, 2025

morph-dev Apr 23, 2025

morph-dev Apr 23, 2025

KolbyML Apr 23, 2025

KolbyML Apr 23, 2025

morph-dev Apr 23, 2025

morph-dev Apr 23, 2025

ogenev left a comment

ogenev Apr 24, 2025

ogenev Apr 24, 2025

ogenev Apr 24, 2025

ogenev commented Apr 24, 2025

		@@ -153,15 +201,21 @@ impl HeaderValidator {
		let gen_index = EPOCH_SIZE + block_root_index;
		let historical_summary_index =

feat: enable post-capella HistoricalSummary Header validation #1774

Are you sure you want to change the base?

feat: enable post-capella HistoricalSummary Header validation #1774

Conversation

KolbyML commented Apr 22, 2025

What was wrong?

How was it fixed?

morph-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

KolbyML Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML commented Apr 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogenev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogenev commented Apr 24, 2025

KolbyML Apr 23, 2025 •

edited

Loading

KolbyML Apr 23, 2025 •

edited

Loading

KolbyML Apr 23, 2025 •

edited

Loading