-
Notifications
You must be signed in to change notification settings - Fork 112
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
Conversation
1e310f8
to
0097de2
Compare
Ideally, this should be merged after ethereum/hive#1152 and ethereum/portal-spec-tests#23 (otherwise, state hive tests will start failing). |
I don't think we should enable this validation until we have live history having 100% coverage |
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 |
If we did want to enable it, we should enable it by block ranges over time |
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).
This is good argument, but we will always have this issue (now or later). There are few simple ways around it:
|
History requires beacon for validating all post capella content So we also need to run beacon also |
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
We can do this |
I think the first option should be included in this PR in that case |
0097de2
to
45fb627
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.
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.
LGTM 👍 .
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 ( 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). |
Final comment. This pr should be merged only after:
|
746d62b
to
22a44ff
Compare
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). |
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