Test fixture for sepolia #1097

yrong · 2024-01-04T07:29:57Z

Test fixture from Sepolia testnet
Revamp conditional compile with all-features enabled and improve CI to cover the checks.

yrong · 2024-01-04T07:47:47Z

relayer/relays/beacon/header/syncer/syncer.go

+	// It's possible that beaconState is not available when GetSyncCommitteePeriodUpdate, in this case
+	// there is no blockRootsProof and just ignore it in beacon client
+	if err != nil && err != ErrBeaconStateAvailableYet {
+		return scale.Update{}, fmt.Errorf("fetch block roots: %w", err)
+	}


Allow SyncCommiteeUpdate without blockRootsProof

I'm not sure this change is a good one. Does it even make sense to send the finalized header update without an ancestry proof? That's the main reason we send beacon header updates in the first place.

It's an edge case that I run generateBeaconTestFixture for the SyncCommitteePeriodUpdate from sepolia and beaconState is not available at that moment. Code in

snowbridge/relayer/cmd/generate_beacon_data.go

Lines 189 to 193 in faacc1e

// generate SyncCommitteeUpdate for filling the missing NextSyncCommittee in initial checkpoint

syncCommitteeUpdateScale, err := s.GetSyncCommitteePeriodUpdate(initialSyncPeriod)

if err != nil {

return fmt.Errorf("get sync committee update: %w", err)

}

But it's possible to switch to another time to generate the test fixture so it's not a strong reason to relax the check just for that. Also as you mentioned it could lead to some unexpected behavior so I reverted in ab9ee6f

yrong · 2024-01-04T07:49:23Z

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

+			if !update.block_roots_branch.is_empty() {
+				ensure!(
+					verify_merkle_branch(
+						update.block_roots_root,
+						&update.block_roots_branch,
+						config::BLOCK_ROOTS_SUBTREE_INDEX,
+						config::BLOCK_ROOTS_DEPTH,
+						update.finalized_header.state_root
+					),
+					Error::<T>::InvalidBlockRootsRootMerkleProof
+				);
+			}


For the reason mentioned in #1097 (comment)

Hmmm but how can this merkle proof be conditional? Could it not lead to unexpected behaviour? As an example, this check allows finalized header verification without providing the block roots proof. What could happen is we keep processing these beacon headers without the necessary ancestry proofs (needed for the execution header imports). Then, our execution header imports could fail because there are no ancestry proofs to verify the execution header against a beacon header.

#1097 (comment)

codecov · 2024-01-04T08:05:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (17f7ad0) 58.37% compared to head (87ba289) 80.57%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1097       +/-   ##
===========================================
+ Coverage   58.37%   80.57%   +22.20%     
===========================================
  Files          55       55               
  Lines        2167     2209       +42     
  Branches       72       72               
===========================================
+ Hits         1265     1780      +515     
+ Misses        885      412      -473     
  Partials       17       17

Flag	Coverage Δ
rust	`80.55% <100.00%> (+27.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yrong · 2024-01-08T01:41:14Z

@claravanstaden
In this PR I've also revamped the condition compile a bit to enable all features which is what Parity does for polkadot-sdk in their CI, the main difference with paritytech/polkadot-sdk#2792 is that when running test with the two conflicting features fast-runtime and runtime-benchmarks enabled at the same time, the minimal spec will get applied and benchmark for beacon client will be ignored. IMO it will make the conditional compile code a bit cleaner.

Please let me know what you think and if anything important missing.

claravanstaden

Hey @yrong! Thanks for these changes. I think resolving the all-features problem in paritytech/polkadot-sdk#2792 in the PR is better than the fixes I attempted. I'm not sure we will be able to convince Parity of the conditional add_benchmarks in the Rococo runtime, we'll have to see.

I am not comfortable with allowing beacon header updates without ancestry proofs. But perhaps I am missing something in my understanding, please let me know.

claravanstaden · 2024-01-08T06:22:40Z

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

+			if !update.block_roots_branch.is_empty() {
+				ensure!(
+					verify_merkle_branch(
+						update.block_roots_root,
+						&update.block_roots_branch,
+						config::BLOCK_ROOTS_SUBTREE_INDEX,
+						config::BLOCK_ROOTS_DEPTH,
+						update.finalized_header.state_root
+					),
+					Error::<T>::InvalidBlockRootsRootMerkleProof
+				);
+			}


Hmmm but how can this merkle proof be conditional? Could it not lead to unexpected behaviour? As an example, this check allows finalized header verification without providing the block roots proof. What could happen is we keep processing these beacon headers without the necessary ancestry proofs (needed for the execution header imports). Then, our execution header imports could fail because there are no ancestry proofs to verify the execution header against a beacon header.

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

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

claravanstaden · 2024-01-08T06:32:02Z

relayer/cmd/generate_beacon_data.go

 		// generate FinalizedUpdate for next epoch
-		log.Info("waiting for a new finalized_update in next epoch and in current sync period,several seconds required...")
+		log.Info("waiting finalized_update in next epoch(6.4 minutes later), be patient and drink a cup of tea...")


claravanstaden · 2024-01-08T06:32:44Z

relayer/cmd/generate_beacon_data.go

-			log.Info("created next sync committee update file")
-		}
-
+		// Generate benchmark fixture for mainnet spec only


How would we regenerate the minimal spec test fixture data? 🤔

Just use snowbridge-relay generate-beacon-data --spec "minimal" for that.

Btw: I think it's fine to also generate benchmark fixtures for minimal spec which we ignore before, then we can avoid the conditional add_benchmarks hack in the Rococo runtime.

63a1bcd

claravanstaden · 2024-01-08T06:35:15Z

relayer/relays/beacon/header/syncer/syncer.go

+	// It's possible that beaconState is not available when GetSyncCommitteePeriodUpdate, in this case
+	// there is no blockRootsProof and just ignore it in beacon client
+	if err != nil && err != ErrBeaconStateAvailableYet {
+		return scale.Update{}, fmt.Errorf("fetch block roots: %w", err)
+	}


I'm not sure this change is a good one. Does it even make sense to send the finalized header update without an ancestry proof? That's the main reason we send beacon header updates in the first place.

web/packages/test/scripts/configure-bridgehub.sh

alistair-singh

LGTM!

claravanstaden · 2024-01-08T09:38:36Z

relayer/relays/beacon/header/syncer/syncer.go

+	finalizedHeaderBlockRoot, err := finalizedHeader.ToSSZ().HashTreeRoot()
 	if err != nil {
-		return scale.Update{}, fmt.Errorf("fetch block roots: %w", err)
+		return scale.Update{}, fmt.Errorf("beacon header hash tree root: %w", err)
 	}

-	finalizedHeaderBlockRoot, err := finalizedHeader.ToSSZ().HashTreeRoot()
+	blockRootsProof, err := s.GetBlockRoots(uint64(finalizedHeader.Slot))
 	if err != nil {
-		return scale.Update{}, fmt.Errorf("beacon header hash tree root: %w", err)
+		return scale.Update{}, fmt.Errorf("fetch block roots: %w", err)


Why is this reordering necessary?

Not necessary and reverted in 87ba289

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

vgeddes

Looks great Ron.

However the conditional add_benchmarks in the Rococo runtime is problematic. I'm sure Parity will reject it.

As you suggest, lets also generate minimal fixtures for benchmarks.

vgeddes

Ignore my previous, seems we no longer need to conditionally compile add_benchmarks.

Approved!

yrong added 2 commits January 4, 2024 15:28

Test fixture for sepolia

2dcc591

Fix clippy

def9742

yrong commented Jan 4, 2024

View reviewed changes

yrong added 5 commits January 7, 2024 09:26

Support all features with CI check

feb78c6

Fix format check

e9305d6

Fix format

b8ee141

Fix

954316a

Fix taplo

faacc1e

yrong marked this pull request as ready for review January 7, 2024 09:33

yrong requested a review from claravanstaden January 8, 2024 01:29

claravanstaden reviewed Jan 8, 2024

View reviewed changes

yrong added 3 commits January 8, 2024 15:24

Revert the change for conditional ancestry proof

ab9ee6f

Fix import

f1c4eb4

Generate benchmark for minimal spec

63a1bcd

alistair-singh approved these changes Jan 8, 2024

View reviewed changes

claravanstaden reviewed Jan 8, 2024

View reviewed changes

yrong added 2 commits January 8, 2024 18:34

Fix clippy for all features

56e12da

Revert change unnecessary

87ba289

claravanstaden approved these changes Jan 8, 2024

View reviewed changes

vgeddes requested changes Jan 8, 2024

View reviewed changes

vgeddes approved these changes Jan 8, 2024

View reviewed changes

yrong merged commit 513fb46 into main Jan 8, 2024
7 checks passed

yrong deleted the ron/test-fixture-for-sepolia branch January 8, 2024 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test fixture for sepolia #1097

Test fixture for sepolia #1097

yrong commented Jan 4, 2024 •

edited

Loading

yrong Jan 4, 2024

claravanstaden Jan 8, 2024

yrong Jan 8, 2024 •

edited

Loading

yrong Jan 4, 2024

claravanstaden Jan 8, 2024

yrong Jan 8, 2024

codecov bot commented Jan 4, 2024 •

edited

Loading

yrong commented Jan 8, 2024

claravanstaden left a comment

claravanstaden Jan 8, 2024

claravanstaden Jan 8, 2024

claravanstaden Jan 8, 2024

yrong Jan 8, 2024 •

edited

Loading

claravanstaden Jan 8, 2024

alistair-singh left a comment

claravanstaden Jan 8, 2024

yrong Jan 8, 2024

vgeddes left a comment

vgeddes left a comment

	// generate SyncCommitteeUpdate for filling the missing NextSyncCommittee in initial checkpoint
	syncCommitteeUpdateScale, err := s.GetSyncCommitteePeriodUpdate(initialSyncPeriod)
	if err != nil {
	return fmt.Errorf("get sync committee update: %w", err)
	}

Test fixture for sepolia #1097

Test fixture for sepolia #1097

Conversation

yrong commented Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 4, 2024 • edited Loading

Codecov Report

yrong commented Jan 8, 2024

claravanstaden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alistair-singh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgeddes left a comment

Choose a reason for hiding this comment

vgeddes left a comment

Choose a reason for hiding this comment

yrong commented Jan 4, 2024 •

edited

Loading

yrong Jan 8, 2024 •

edited

Loading

codecov bot commented Jan 4, 2024 •

edited

Loading

yrong Jan 8, 2024 •

edited

Loading