Snowbridge free consensus updates #5201

claravanstaden · 2024-07-31T13:11:03Z

Allow free Snowbridge consensus updates, if the header interval is larger than the configured value (set to 32, so once a epoch).

This PR also moves the Rococo Snowbridge pallet config into its own module.

Original PR: Snowfork#159

* free finalized updates * update comment * fix check * fixes * tests * fixes * fmt * finishing touches * comments * adds missing impl * adds test * fixes * fix

…e-consensus-updates

claravanstaden · 2024-08-01T13:42:48Z

@yrong @vgeddes Please review. :)

bridges/snowbridge/pallets/ethereum-client/src/lib.rs

yrong

+1

franciscoaguirre · 2024-08-13T08:57:00Z

bridges/snowbridge/pallets/ethereum-client/src/lib.rs

+				return Pays::No;
+			}
+
+			// If free headers are allowed and the latest finalized header is larger than the


This part of the comment, "If free headers are allowed". I don't see it reflected in the code. Should it be another condition apart from the inequality?

Good catch, in an older implementation it was optional. Comment updated here: ab8739c#diff-adbbba03e39a047dd71745d3ed0eaf33b4e982ac58d64f273f1574816ff44940R664

bridges/snowbridge/pallets/ethereum-client/src/lib.rs

franciscoaguirre · 2024-08-13T09:02:36Z

bridges/snowbridge/pallets/ethereum-client/src/tests.rs

+	new_tester().execute_with(|| {
+		// Not free, smaller than the allowed free header interval
+		assert_eq!(
+			EthereumBeaconClient::check_refundable(&finalized_update.clone(), 8190),


It might be clearer if you use finalized_update.finalized_header.slot + some_offset

Agreed, updated here: ab8739c#diff-ea6025b3cac6719f750d1f0065ba30d70a16d386a5ce374e4a6b79299f48a3eeR136

franciscoaguirre · 2024-08-13T09:05:01Z

cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/bridge_to_ethereum_config.rs

+	type RuntimeEvent = RuntimeEvent;
+	type ForkVersions = ChainForkVersions;
+	// Free consensus update every epoch. Works out to be 225 updates per day.
+	type FreeHeadersInterval = ConstU32<32>;


I guess the epoch will not change often, but probably better to use a constant like EPOCH_LENGTH

ab8739c#diff-f639f7c31fb8e9ee5ce9a5b81b1eb1eba5c9a37d05d1fa425d694cf7eec957ecR168

franciscoaguirre · 2024-08-13T09:06:28Z

prdoc/pr_5201.prdoc

+      Allow free consensus updates to the Snowbridge Ethereum client if the headers are more than a certain
+      number of headers apart.


How does this impact the devs? Is it cheaper to run relayers now?
Does this affect the user somehow? E.g. cheaper to use the bridge?

ab8739c#diff-cfa518fee4956eaf8afc94f3fad7a131b1c1474c2be560798ba1c5cec04f57dcR10

Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>

acatangiu · 2024-08-26T07:50:16Z

bridges/snowbridge/pallets/ethereum-client/src/lib.rs

+			let pays_fee = Self::check_refundable(update, latest_finalized_state.slot);
+			let actual_weight = match update.next_sync_committee_update {
+				None => T::WeightInfo::submit(),
+				Some(_) => T::WeightInfo::submit_with_sync_committee(),
+			};
+
 			if update.finalized_header.slot > latest_finalized_state.slot {
 				Self::store_finalized_header(update.finalized_header, update.block_roots_root)?;
 			}

-			Ok(())
+			Ok(PostDispatchInfo { actual_weight: Some(actual_weight), pays_fee })


Can you double-check and confirm that this doesn't allow free spamming for this call with using the same "valid" update (with update.next_sync_committee_update.is_some() == true) over and over for free, potentially DoS-ing BH blocks?

It looks like there is a compute_period verification for updating sync committee, that would not allow the DoS I described above, but I got a bit lost in the details and would like you to confirm.

@acatangiu that's a valid point, thanks! There is a specific test for that (duplicate_sync_committee_updates_are_not_free): https://github.com/paritytech/polkadot-sdk/pull/5201/files#diff-ea6025b3cac6719f750d1f0065ba30d70a16d386a5ce374e4a6b79299f48a3eeR684

Thanks, didn't yet look at tests 🙈

The test tries to submit the exact same proof twice, which will fail the second time because the attested header in the proof is no longer newer than the stored one on-chain.

But what about the following scenario:

Relayer submits valid update for slot N in period P with sync committee S provided in Some(next_sync_committee_update)

Relayer then provides update for slot N+1 in period P with sync committee S provided in Some(next_sync_committee_update)?

Basically try to "farm" the Some(next_sync_committee_update) tx fee freebie, to spam update one slot at a time.

The above is a valid issue, and is being fixed in Snowfork#172

paritytech-cicd-pr · 2024-08-26T08:34:31Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7125017

acatangiu

Approving this like so, Clara to follow-up with requested test in another PR.

bkchr · 2024-09-02T17:19:29Z

@claravanstaden please merge master to make this mergable.

claravanstaden · 2024-09-02T18:27:20Z

@claravanstaden please merge master to make this mergable.

On it!

# Conflicts: # cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs

Allow free Snowbridge consensus updates, if the header interval is larger than the configured value (set to 32, so once a epoch). This PR also moves the Rococo Snowbridge pallet config into its own module. Original PR: Snowfork#159 --------- Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>

Allow free Snowbridge consensus updates, if the header interval is larger than the configured value (set to 32, so once a epoch). This PR also moves the Rococo Snowbridge pallet config into its own module. Original PR: Snowfork#159 ---------- Original `pr_5201.prdoc` is present but moved to release dir, ergo `R0-Silent` for this backport PR. --------- Co-authored-by: Clara van Staden <claravanstaden64@gmail.com> Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>

Allow free Snowbridge consensus updates, if the header interval is larger than the configured value (set to 32, so once a epoch). This PR also moves the Rococo Snowbridge pallet config into its own module. Original PR: Snowfork#159 --------- Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>

# Description A fix for a border condition introduced with new feature #5201. A malicious relayer could spam the Ethereum client with sync committee updates that have already been imported for the period. This PR adds a storage item to track the last imported sync committee period, so that subsequent irrelevant updates are not free. Original PR: Snowfork#172 ## Integration Downstream projects are not affected. Relayers will not be able to spam the Ethereum client with irrelevant sync committee updates for free. ## Review Notes Adds a storage item to track the last free sync committee update period, so that duplicate imports are not free. --------- Co-authored-by: Adrian Catangiu <adrian@parity.io>

# Description A fix for a border condition introduced with new feature #5201. A malicious relayer could spam the Ethereum client with sync committee updates that have already been imported for the period. This PR adds a storage item to track the last imported sync committee period, so that subsequent irrelevant updates are not free. Original PR: Snowfork#172 ## Integration Downstream projects are not affected. Relayers will not be able to spam the Ethereum client with irrelevant sync committee updates for free. ## Review Notes Adds a storage item to track the last free sync committee update period, so that duplicate imports are not free. --------- Co-authored-by: Adrian Catangiu <adrian@parity.io> (cherry picked from commit 1790e42)

claravanstaden added 8 commits July 31, 2024 15:08

Free finalized Ethereum updates (#159)

5dde91c

* free finalized updates * update comment * fix check * fixes * tests * fixes * fmt * finishing touches * comments * adds missing impl * adds test * fixes * fix

cleanup

e8e0616

more cleanup

8aa9d7e

import fix

ea85bf5

prdoc

3c55dd8

Merge branch 'master' into free-consensus-updates

a74bcc2

prdoc bump corrections

e07082e

Merge remote-tracking branch 'origin/free-consensus-updates' into fre…

622d9e2

…e-consensus-updates

claravanstaden marked this pull request as ready for review August 1, 2024 13:41

paritytech-review-bot bot requested a review from a team August 1, 2024 13:42

vgeddes reviewed Aug 1, 2024

View reviewed changes

bridges/snowbridge/pallets/ethereum-client/src/lib.rs Outdated Show resolved Hide resolved

claravanstaden added 2 commits August 2, 2024 09:41

Merge branch 'master' into free-consensus-updates

4994b52

saturating add

9391a36

claravanstaden requested a review from vgeddes August 2, 2024 07:58

Merge branch 'master' into free-consensus-updates

f768444

yrong reviewed Aug 5, 2024

View reviewed changes

bridges/snowbridge/pallets/ethereum-client/src/lib.rs Show resolved Hide resolved

vgeddes approved these changes Aug 5, 2024

View reviewed changes

yrong approved these changes Aug 6, 2024

View reviewed changes

claravanstaden added 3 commits August 6, 2024 18:52

Merge branch 'master' into free-consensus-updates

6417823

Merge branch 'master' into free-consensus-updates

3321bc0

Merge branch 'master' into free-consensus-updates

6ad199b

franciscoaguirre reviewed Aug 13, 2024

View reviewed changes

serban300 approved these changes Aug 21, 2024

View reviewed changes

serban300 added the T15-bridges This PR/Issue is related to bridges. label Aug 21, 2024

Update bridges/snowbridge/pallets/ethereum-client/src/lib.rs

c0077e1

Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>

acatangiu reviewed Aug 26, 2024

View reviewed changes

claravanstaden added 2 commits August 26, 2024 10:18

pr comments

ab8739c

Merge branch 'master' into free-consensus-updates

88d944d

claravanstaden added 2 commits August 26, 2024 10:37

adds westend free headers conf

34dda83

fix prdoc

ba9560a

claravanstaden requested a review from franciscoaguirre August 26, 2024 10:38

Merge branch 'master' into free-consensus-updates

8521c51

acatangiu enabled auto-merge September 2, 2024 15:56

acatangiu approved these changes Sep 2, 2024

View reviewed changes

ggwpez disabled auto-merge September 2, 2024 17:19

ggwpez enabled auto-merge September 2, 2024 17:19

ggwpez disabled auto-merge September 2, 2024 17:20

claravanstaden added 3 commits September 2, 2024 20:28

Merge branch 'master' into free-consensus-updates

67a120b

# Conflicts: # cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs

fmt

82f05de

clippy

dbea452

bkchr added this pull request to the merge queue Sep 2, 2024

Merged via the queue into paritytech:master with commit c8015b2 Sep 2, 2024
183 of 185 checks passed

claravanstaden deleted the free-consensus-updates branch September 3, 2024 06:27

acatangiu self-assigned this Sep 4, 2024

ggwpez mentioned this pull request Sep 9, 2024

Parent issue for stable2409 LTS release #5583

Closed

claravanstaden mentioned this pull request Sep 10, 2024

Fix border condition in Snowbridge free consensus Updates #5671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snowbridge free consensus updates #5201

Snowbridge free consensus updates #5201

claravanstaden commented Jul 31, 2024

claravanstaden commented Aug 1, 2024

yrong left a comment

franciscoaguirre Aug 13, 2024

claravanstaden Aug 26, 2024

franciscoaguirre Aug 13, 2024

claravanstaden Aug 26, 2024

franciscoaguirre Aug 13, 2024

claravanstaden Aug 26, 2024

franciscoaguirre Aug 13, 2024

claravanstaden Aug 26, 2024

acatangiu Aug 26, 2024

claravanstaden Aug 26, 2024

acatangiu Aug 26, 2024

acatangiu Aug 26, 2024

acatangiu Sep 3, 2024

paritytech-cicd-pr commented Aug 26, 2024

acatangiu left a comment

bkchr commented Sep 2, 2024

claravanstaden commented Sep 2, 2024

		Allow free consensus updates to the Snowbridge Ethereum client if the headers are more than a certain
		number of headers apart.

Snowbridge free consensus updates #5201

Snowbridge free consensus updates #5201

Conversation

claravanstaden commented Jul 31, 2024

claravanstaden commented Aug 1, 2024

yrong 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented Aug 26, 2024

acatangiu left a comment

Choose a reason for hiding this comment

bkchr commented Sep 2, 2024

claravanstaden commented Sep 2, 2024