Follow consensus-specs sync protocol more closely by yrong · Pull Request #840 · Snowfork/snowbridge

yrong · 2023-05-22T13:48:13Z

strict follows ALC spec only apply_light_client_update after successfully validate_light_client_update

yrong · 2023-05-22T14:18:41Z

More checks added per spec required as following:

snowbridge/parachain/pallets/ethereum-beacon-client/src/lib.rs

Lines 263 to 267 in a948332

    
           ensure!( 
        
           	update.signature_slot > update.attested_header.slot && 
        
           		update.attested_header.slot >= update.finalized_header.slot, 
        
           	Error::<T>::InvalidUpdateSlot 
        
           );

snowbridge/parachain/pallets/ethereum-beacon-client/src/lib.rs

Lines 331 to 337 in a948332

    
           if update_attested_period == store_period && <NextSyncCommittee<T>>::exists() { 
        
           	let next_committee_root = <NextSyncCommittee<T>>::get().sync_committee_root; 
        
           	ensure!( 
        
           		sync_committee_root == next_committee_root, 
        
           		Error::<T>::InvalidSyncCommitteeUpdate 
        
           	); 
        
           }

snowbridge/parachain/pallets/ethereum-beacon-client/src/lib.rs

Lines 375 to 380 in a948332

    
           if !<NextSyncCommittee<T>>::exists() { 
        
           	ensure!( 
        
           		update_finalized_period == store_period, 
        
           		<Error<T>>::InvalidSyncCommitteeUpdate 
        
           	); 
        
           	<NextSyncCommittee<T>>::set(sync_committee_prepared);

vgeddes

Thanks Ron, this is also a good improvement.

Just wanted to say, we don't have to strictly follow the design in ALC consensus-specs. We can also improve on it, and change the algorithm if it makes sense.

parachain/pallets/ethereum-beacon-client/src/lib.rs

parachain/primitives/beacon/src/types.rs

parachain/primitives/beacon/src/merkle_proof.rs

parachain/pallets/ethereum-beacon-client/src/lib.rs

yrong · 2023-05-23T03:55:29Z

Just wanted to say, we don't have to strictly follow the design in ALC consensus-specs. We can also improve on it, and change the algorithm if it makes sense.

IMHO we first follow spec closely which make it easy to find missing checks as above, also make audit/review a bit easier. Then we do optimization based on that and yes we can definitely change as required.

yrong · 2023-05-23T03:57:32Z

parachain/pallets/ethereum-beacon-client/src/lib.rs

 				(update.next_sync_committee_update.is_some() &&
 					update_attested_period == store_period);
 			ensure!(
-				update.attested_header.slot > store_period || update_has_next_sync_committee,


bug fix here. should be update.attested_header.slot > latest_finalized_state.slot

Nice catch!

yrong · 2023-05-23T03:58:05Z

parachain/pallets/ethereum-beacon-client/src/lib.rs

 				.try_into()
 				.map_err(|_| <Error<T>>::BLSPreparePublicKeysFailed)?;
 			<CurrentSyncCommittee<T>>::set(sync_committee_prepared);
+			<NextSyncCommittee<T>>::kill();


Kill NextSyncCommittee here.

yrong · 2023-05-23T04:01:26Z

relayer/cmd/generate_beacon_data.go

 			return fmt.Errorf("get initial sync: %w", err)
 		}
 		initialSync := initialSyncScale.ToJSON()
-		err = writeJSONToFile(initialSync, fmt.Sprintf("initial-checkpoint.%s.json", activeSpec.ToString()))


As mentioned before, we can not use initialSync generated in same command together with other updates.

How can we improve the command so that it generates all the updates, without having to run it twice? Surely this possible to implement.

Resolved in 7fda5f4

vgeddes · 2023-05-23T06:29:46Z

See this comment here #839 (comment). I don't want to prune FinalizedBeaconState right now, because it depends on FinalizedCheckpoints/FinalizedHeaderSlotsCache. I want to remove that storage item.

vgeddes · 2023-05-23T08:43:28Z

relayer/cmd/generate_beacon_data.go

 		log.Info("downloading beacon state, this can take a few minutes...")
-		syncCommitteePeriod := s.ComputeSyncPeriodAtSlot(initialSyncHeaderSlot)
+		// wait for 5 blocks
+		time.Sleep(6 * time.Second * 5)


Is the block time 6s in our E2E stack? Curious if we configured this explicitly somewhere

You should probably make it wait some more time in case the 5th block hasn't been produced yet.

Yes, it's 6s for minimal spec and we did not hack it for this test. Waiting for 5 blocks is just a decent value to make sure FinalizedUpdate change and SyncPeriod does not change in that period. This is requied by recently added check in our light client here.
There is chance at some time we run command but fail to get data meet requirements, in that case we just fail with error as following and rerun it at another time.

snowbridge/relayer/cmd/generate_beacon_data.go

Lines 214 to 219 in 3accd34

if initialSyncPeriod != finalizedUpdatePeriod {

return fmt.Errorf("initialSyncPeriod %d should be consistent with finalizedUpdatePeriod %d", initialSyncPeriod, finalizedUpdatePeriod)

}

if finalizedUpdate.AttestedHeader.Slot <= initialSyncHeaderSlot {

return fmt.Errorf("AttestedHeader slot %d should be greater than initialSyncHeaderSlot %d", finalizedUpdate.AttestedHeader.Slot, initialSyncHeaderSlot)

}

vgeddes · 2023-05-23T08:44:01Z

relayer/cmd/generate_beacon_data.go


 		log.Info("downloading beacon state, this can take a few minutes...")
-		syncCommitteePeriod := s.ComputeSyncPeriodAtSlot(initialSyncHeaderSlot)
+		// wait for 5 blocks


Is 5 special number here?

#840 (comment)

vgeddes

+1

Strict follows ALC

a948332

yrong force-pushed the ron/improve-ancestry-proofs branch from 61fc13b to a948332 Compare May 22, 2023 13:59

yrong requested a review from vgeddes May 22, 2023 14:18

vgeddes reviewed May 22, 2023

View reviewed changes

vgeddes changed the title ~~Strict follows ALC specification~~ Follow consensus-specs sync protocol more closely May 22, 2023

yrong added 5 commits May 23, 2023 09:13

Polish

b9c7afc

Merge recent change

293c2b5

Fix tests

8248576

Polish

0355982

Merge

495d724

yrong commented May 23, 2023

View reviewed changes

yrong mentioned this pull request May 23, 2023

Simplify beacon light client #839

Merged

yrong force-pushed the ron/improve-ancestry-proofs branch from 4606b52 to 495d724 Compare May 23, 2023 06:57

yrong added 2 commits May 23, 2023 16:22

Generate all updates in same command

7fda5f4

Sync version of lodestar

273c1ff

vgeddes reviewed May 23, 2023

View reviewed changes

yrong added 2 commits May 23, 2023 16:56

Clean code

f938e28

Add more checks when generate test data

3accd34

vgeddes approved these changes May 23, 2023

View reviewed changes

yrong merged commit 2156d17 into vincent/improve-ancestry-proofs May 23, 2023

yrong deleted the ron/improve-ancestry-proofs branch May 23, 2023 14:26

	if initialSyncPeriod != finalizedUpdatePeriod {
	return fmt.Errorf("initialSyncPeriod %d should be consistent with finalizedUpdatePeriod %d", initialSyncPeriod, finalizedUpdatePeriod)
	}
	if finalizedUpdate.AttestedHeader.Slot <= initialSyncHeaderSlot {
	return fmt.Errorf("AttestedHeader slot %d should be greater than initialSyncHeaderSlot %d", finalizedUpdate.AttestedHeader.Slot, initialSyncHeaderSlot)
	}

Conversation

yrong commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yrong commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vgeddes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yrong commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vgeddes May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vgeddes commented May 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yrong May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vgeddes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yrong commented May 22, 2023 •

edited

Loading

yrong commented May 22, 2023 •

edited

Loading

yrong commented May 23, 2023 •

edited

Loading

vgeddes May 23, 2023 •

edited

Loading

yrong May 23, 2023 •

edited

Loading