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

Resolve attested/finalized header confusion in rotation flow #44

Closed
wants to merge 3 commits into from

Conversation

nulltea
Copy link
Member

@nulltea nulltea commented Dec 11, 2023

Aims to resolve confusion with finalized_header: update.attested_header that became an issue when integrating with Go node.

My understanding is that the attested state root that contains new sync committee (prev CommitteeRotationArgs.finalized_header.state_root) is going to become finalized in the adjacent step, but since in Spectre.sol both these proofs are expected to be submitted together we use StepInput.finilized_header_root for what is actually CommitteeRotationArgs.attested_header.hash_tree_root() (ie. justified_header_root).

Need help verifying my logic 🙏

In Go node we should, thereby, schedule rotation proof generation when the committee update epoch becomes finalized.

@ec2
Copy link
Member

ec2 commented Dec 14, 2023

Lightclients only concern themselves with finalized objects and not justified objects. Attested headers are not justified headers they are finalized

The logic changes look good otherwise.

@@ -101,8 +101,8 @@ mod tests {
let accumulator = [bn256::Fr::zero(); 12]; // this can be anything.. The test is just checking it gets correctly concatenated to the start of the encoded input

let instance = CommitteeUpdateCircuit::<Minimal, bn256::Fr>::instance(&witness, LIMB_BITS);
let finalized_block_root = witness
.finalized_header
let justified_block_root = witness
Copy link
Member

Choose a reason for hiding this comment

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

This naming is incorrect. We work with finalized and attested objects w.r.t. the light client protocol. There is no justified things in the light client protocol

@willemolding
Copy link
Collaborator

Lightclients only concern themselves with finalized objects and not justified objects. Attested headers are not justified headers they are finalized

The logic changes look good otherwise.

Can second this. We need to avoid using the term "justified" in this context as it isn't the same as justification in Casper and isn't mentioned in the sync protocol spec. Where "justified" is used here we should be using "attested"

@nulltea
Copy link
Member Author

nulltea commented Dec 14, 2023

Closing as CommitteUpdateArgs.finalized_header is said to remain as is.

@nulltea nulltea closed this Dec 14, 2023
@nulltea nulltea deleted the timoftime/fix_attested_header_confusion branch December 15, 2023 15:43
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