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(state): validate state content by anchoring it to the state root #1377

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

morph-dev
Copy link
Collaborator

@morph-dev morph-dev commented Aug 13, 2024

What was wrong?

We currently don't anchor state content to the chain. We did this in the past as history network (and fetching block header) wasn't reliable, but we should start doing it.

Related issue: #1305

How was it fixed?

Fetch the block header from the network and validate the content based on the state_root.

To-Do

@morph-dev morph-dev changed the title State root validation feat(state): validate state content by anchoring it to the state root Aug 13, 2024
@morph-dev morph-dev force-pushed the state_root_validation branch 4 times, most recently from 1e310f8 to 0097de2 Compare August 16, 2024 10:05
@morph-dev morph-dev marked this pull request as ready for review August 16, 2024 10:55
@morph-dev morph-dev self-assigned this Aug 16, 2024
@morph-dev morph-dev added this to the Devcon 2024 milestone Aug 16, 2024
@morph-dev
Copy link
Collaborator Author

morph-dev commented Aug 16, 2024

Ideally, this should be merged after ethereum/hive#1152 and ethereum/portal-spec-tests#23 (otherwise, state hive tests will start failing).

@KolbyML
Copy link
Member

KolbyML commented Aug 16, 2024

I don't think we should enable this validation until we have live history having 100% coverage

@KolbyML
Copy link
Member

KolbyML commented Aug 16, 2024

One concern is what if the user isn't running the history network, there doesn't appear to be any measures in place to prevent against this error case

@KolbyML
Copy link
Member

KolbyML commented Aug 16, 2024

If we did want to enable it, we should enable it by block ranges over time

@morph-dev
Copy link
Collaborator Author

morph-dev commented Aug 16, 2024

Considering that in short/mid term we only plan to gossip only first 1'000'000 blocks and the most recent ones (head state), we are already covered with history network (4444 + latest). And for anything else, having history at 100% should be higher priority than having state at 100% (and also much easier).

In addition, I would say it makes sense for state bridge to first gossip block header to history network, then process that block (execute + gossip), and we avoid this problem altogether.

While I think I gave good arguments, I'm also fine with having a startup flag that can disable this check (or at least attempt it, and if we can't fine the header consider that proof is anchored). But I would leave the check enabled by default (at least for now).


One concern is what if the user isn't running the history network, there doesn't appear to be any measures in place to prevent against this error case

This is good argument, but we will always have this issue (now or later). There are few simple ways around it:

  • my preference: print error message at startup and shut down
  • run history network but without storage.

@KolbyML
Copy link
Member

KolbyML commented Aug 16, 2024

One concern is what if the user isn't running the history network, there doesn't appear to be any measures in place to prevent against this error case

This is good argument, but we will always have this issue (now or later). There are few simple ways around it:

  • my preference: print error message at startup and shut down
  • run history network but without storage.

History requires beacon for validating all post capella content

So we also need to run beacon also

@KolbyML
Copy link
Member

KolbyML commented Aug 17, 2024

Considering that in short/mid term we only plan to gossip only first 1'000'000 blocks and the most recent ones (head state), we are already covered with history network (4444 + latest). And for anything else, having history at 100% should be higher priority than having state at 100% (and also much easier).

In addition, I would say it makes sense for state bridge to first gossip block header to history network, then process that block (execute + gossip), and we avoid this problem altogether.

While I think I gave good arguments, I'm also fine with having a startup flag that can disable this check (or at least attempt it, and if we can't fine the header consider that proof is anchored). But I would leave the check enabled by default (at least for now).

In like 4 months when history requires beacon for post capella blocks and latest for validation of headers, if we ever want to test state we would then need to have the beacon and history network running.

Maybe this won't be a issue I guess we will see, def a lot more requirements but I that is by design

In addition, I would say it makes sense for state bridge to first gossip block header to history network, then process that block (execute + gossip), and we avoid this problem altogether.

We can do this

@KolbyML
Copy link
Member

KolbyML commented Aug 17, 2024

One concern is what if the user isn't running the history network, there doesn't appear to be any measures in place to prevent against this error case

This is good argument, but we will always have this issue (now or later). There are few simple ways around it:

  • my preference: print error message at startup and shut down
  • run history network but without storage.

I think the first option should be included in this PR in that case

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:

Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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

LGTM 👍 .

@morph-dev
Copy link
Collaborator Author

I noticed that bridge was one more place where I needed to handle flags correctly.

The "portal_subnetworks" flag was used both for selecting which bridges to run (HistoryBridge, BeaconBridge, StateBridge) and which subnetworks to enable on portal client (trin or fluffy).

With the latest PR, the subnetworks enabled on the portal client would be calculated based on the needs of such bridge (i.e. StateBridge would enable both history and state subnetwork).

@morph-dev
Copy link
Collaborator Author

Final comment. This pr should be merged only after:

@morph-dev
Copy link
Collaborator Author

Rebased to head and cleaned up commit history.

Will most likely merge it tomorrow morning, so I can react if something goes wrong (e.g. hive starts failing).

@morph-dev morph-dev merged commit da113e8 into ethereum:master Aug 21, 2024
8 checks passed
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.

3 participants