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

Migrate to weights V2 #1543

Closed
svyatonik opened this issue Aug 8, 2022 · 5 comments · Fixed by #1860
Closed

Migrate to weights V2 #1543

svyatonik opened this issue Aug 8, 2022 · 5 comments · Fixed by #1860
Labels
dependencies Pull requests that update a dependency file

Comments

@svyatonik
Copy link
Contributor

svyatonik commented Aug 8, 2022

We'll need to migrate to this someday (my guess - before Kusama<>Polkadot bridge hub? or even for Rococo<>Wococo bridge hub?).

Right now we can't do anything because: (1) the weights PR is not merged yet (2) we are still referencing XCM v3 branch from our repo - so even if weights will be merged, we can't migrate until XCM is also merged.

paritytech/substrate#10918
paritytech/polkadot-sdk#256

@svyatonik
Copy link
Contributor Author

There's also a limit on max POV size for parachains, which is cumulus_primitives_core::relay_chain::v2::MAX_POV_SIZE (5 * 1024 * 1024). We need to be sure that our size doesn't exceed that (verify in tests?)

@bkontur
Copy link
Contributor

bkontur commented Jan 13, 2023

  • search in code
    TODO: https://github.com/paritytech/parity-bridges-common/issues/1543 - check `set_proof_size` 0 or 64*1024 or 1026?

@svyatonik
Copy link
Contributor Author

svyatonik commented Feb 6, 2023

Remaining things:

  • remove hack with changing max number of GRANDPA authorities in benchmarks - let's always use MaxBridgedAuthorities;
  • we need to respect PoV size when building message delivery transaction - see that huge proof_size in select_delivery_transaction_limits_works test. Some other tests?;
  • ensure that we are rejecting GRANDPA proofs with too-many authorities and decrease maximal number of authorities for Kusama/Polkadot/Rococo/Wococo from 100_000 (const from GRANDPA pallet config) to something much smaller;
  • refund (if that is possible) unused proof_size in all calls where it is possible. Remove blocking operations in SyncProvider substrate#1853;
  • maybe use PovEstimationMode::Estimated mode where appropriate?

@svyatonik
Copy link
Contributor Author

I'm not sure about the last point (using PovEstimationMode::Estimated). E.g. if we rely on call arguments to provide this proof_size, then what if actual proof_size will be larger? We may fail the call by returning error, but the proof size will be larger than "estimated" anyway. Let's keep using MaxEncodedLen as it seems safer. But we need to impl paritytech/substrate#1853 anyway

@svyatonik
Copy link
Contributor Author

Some TODOs are still left in code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants