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

feat: store witness safety margins on SC #4260

Merged
merged 8 commits into from
Nov 23, 2023
Merged

Conversation

msgmaxim
Copy link
Contributor

Pull Request

Closes: PRO-983

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

As discussed, used storage itmes instead of constants so we can have different values depending on the network. Everything expect for Perseverance is set to 2 blocks (it is 5 blocks on Perseverance as requested in PRO-983)

@msgmaxim msgmaxim requested a review from kylezs November 20, 2023 07:58
Copy link

linear bot commented Nov 20, 2023

PRO-983 CFE Reads SC Constant for safety margin

We want the safety margin for each chain to be a pallet::constant on the SC, probably in the ingress-egress pallet, such that the CFE can read it to determine the safety margin, and that this can be changed via a runtime upgrade. It also means we can have different margins for different networks rather easily, without having to compile a different engine - as the safety margins are currently stored as engine constants.

  1. SC should store a constant for the witnessing safety margin of each chain.
  2. CFE needs to read this constant and use it as the safety margin

Out of scope:

  • Dynamically adjusting the safety margin while the CFE is running.

The values for these on each public network should be:

Perseverance: 5

Mainnet: 2

(There was a long discussion about this, the tl;dr is that reorgs greater than 2 blocks are very rare on mainnet, and when there is, the chances are that there's a reorg where there's a tx removed entirely is even rarer).

On localnets we can probably bring this down to 2 too - and decrease the ETH margin for localnets while we can too, this should speed up localnets / CI quite a bit.

Copy link
Contributor

@kylezs kylezs left a comment

Choose a reason for hiding this comment

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

This will also need a migration. You can see the state-chain/pallets/cf-ingress-egress/src/migrations/ingress_expiry.rs migration for one way to do this to set different values for the different instances of the instantiable pallets

also note that this PR: #4258 which will probably be merged today, changes the migration process slightly. It doesn't change anything to do with writing the migration file itself along with the pre and post hooks, as in the example above.

state-chain/node/src/chain_spec/berghain.rs Outdated Show resolved Hide resolved
state-chain/node/src/chain_spec/sisyphos.rs Outdated Show resolved Hide resolved
state-chain/node/src/chain_spec/perseverance.rs Outdated Show resolved Hide resolved
state-chain/node/src/chain_spec/devnet.rs Show resolved Hide resolved
state-chain/node/src/chain_spec/testnet.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
engine/src/witness/eth.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (e6ad83e) 72% compared to head (4c290bc) 71%.
Report is 10 commits behind head on main.

❗ Current head 4c290bc differs from pull request most recent head d2d3278. Consider uploading reports for the commit d2d3278 to get more accurate results

Files Patch % Lines
engine/src/witness/btc.rs 0% 9 Missing ⚠️
engine/src/witness/eth.rs 0% 9 Missing ⚠️
state-chain/node/src/chain_spec.rs 0% 8 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #4260   +/-   ##
=====================================
- Coverage     72%     71%   -0%     
=====================================
  Files        385     385           
  Lines      63554   63622   +68     
  Branches   63554   63622   +68     
=====================================
+ Hits       45448   45479   +31     
- Misses     15749   15797   +48     
+ Partials    2357    2346   -11     

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

@msgmaxim msgmaxim requested review from jerryafr and acdibble and removed request for a team November 21, 2023 23:25
@msgmaxim
Copy link
Contributor Author

Added assertions so that the migration can be tested, and checked it with try_runtime_upgrade.ts scirpt. @kylezs

engine/src/witness/btc.rs Outdated Show resolved Hide resolved
@kylezs kylezs merged commit 9ceb03f into main Nov 23, 2023
39 checks passed
@kylezs kylezs deleted the feat/safety-margin-constants branch November 23, 2023 08:04
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.

None yet

2 participants