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

Store prev/current justified root #742

Closed
djrtwo opened this issue Mar 8, 2019 · 10 comments
Closed

Store prev/current justified root #742

djrtwo opened this issue Mar 8, 2019 · 10 comments
Labels
general:bug Something isn't working

Comments

@djrtwo
Copy link
Contributor

djrtwo commented Mar 8, 2019

issue

Attestation processing assumes that the previous/current block root will be available in state.latest_block_roots when processing attestations.

attestation.data.justified_block_root == get_block_root(
        state, get_epoch_start_slot(attestation.data.justified_epoch)
)

The call to get_block_root fails if more than SLOTS_PER_HISTORICAL_ROOT have passed since latest justified block. This is currently only about ~13 hours which requires liveness wrt justification in this period for the chain to continue to function. Oh no!

proposed solution

Add previous_justified_root and current_justified_root directly to state to eliminate this lookup to a potentially unavailable slot.

(Thanks @arnetheduck for running your non-justifying chain for 8192 slots!)

@djrtwo djrtwo changed the title Should store prev/current justified root Store prev/current justified root Mar 8, 2019
@arnetheduck
Copy link
Contributor

It might not necessarily have to be part of the state (duplicate info, kind of).. we can cache it somewhere in the client as well - we already keep a few other book-keeping fields like this around anyway.

@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 8, 2019

State transition should be self contained wrt pre_state, block.
I'm not currently aware of any other book keeping fields that are required in the state transition (that said, I expect clients to cache things in various ways as they decide worthwhile).

@hwwhww hwwhww added the general:bug Something isn't working label Mar 9, 2019
@arnetheduck
Copy link
Contributor

fair enuff, I was still on a version that required the previous block.

@arnetheduck
Copy link
Contributor

actually, there's nuance here. it doesn't look like the fields are needed to apply a block state transition - they're needed to fill out a block when you're proposing one, no?

@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 12, 2019

actually, there's nuance here. it doesn't look like the fields are needed to apply a block state transition - they're needed to fill out a block when you're proposing one, no?

@arnetheduck which fields? access to the root of the justified epoch is needed both when creating an attestation and when validating it.

@arnetheduck
Copy link
Contributor

when validating it.

right, my bad. missed this one.

@djrtwo djrtwo closed this as completed Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants
@arnetheduck @djrtwo @hwwhww and others